s390x/tcg: TEST PROTECTION and memory hotplug

PATCH Applied David Hildenbrand 3 participants 11 messages Dec 18, 2017 – Jan 12, 2018

Share & tools

David Hildenbrand

author

2017-12-18 22:46 UTC #0 lore raw

cover s390x/tcg: TEST PROTECTION and memory hotplug

While trying to fix TCG so I can properly detect memory in kvm-unit-tests ... looks like I accidentally made memory hotplug under TCG work (whoops).

qemu-system-s390x ... -m 2048,maxmem=4096M,slots=4 ...

[root@localhost ~]# cat /proc/meminfo

MemTotal:        4143632 kB
MemFree:         3845248 kB
MemAvailable:    3947932 kB

[root@localhost ~]# lsmem

RANGE                                  SIZE  STATE REMOVABLE BLOCK
0x0000000000000000-0x000000000fffffff  256M online        no     0
0x0000000010000000-0x000000006fffffff  1.5G online       yes   1-6
0x0000000070000000-0x00000000ffffffff  2.3G online        no  7-15

Memory block size:       256M
Total online memory:       4G
Total offline memory:      0B

I am not sure if we want to have memory hotplug in its current form later on (the guest can hotplug memory itself). This is different compared to all other architectures. E.g. when booting Fedora 27, it will simply hotplug and add all memory. This doesn't make any sense in the context of VMs where you want to be able to control from the outside, when and how much more memory is given to a VM. But anyhow, seems to work ...

David Hildenbrand (2):
s390x/tcg: implement TEST PROTECTION
s390x/sclp: fix missing be conversion

 hw/s390x/sclp.c           |  4 ++--
 target/s390x/helper.h     |  2 +-
 target/s390x/mem_helper.c | 41 +++++++++++++++++++++++++++++++++++++++--
 target/s390x/translate.c  |  2 +-
 4 files changed, 43 insertions(+), 6 deletions(-)

Signature · 1 line

2.14.3

David Hildenbrand

author

2017-12-18 22:46 UTC #1 lore raw

PATCH 1/2 s390x/tcg: implement TEST PROTECTION

Linux uses TEST PROTECTION to sense for available memory locations.

Let's implement what we can for now (just as for the other instructions, excluding AR mode and special protection mechanisms).

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/helper.h     |  2 +-
 target/s390x/mem_helper.c | 41 +++++++++++++++++++++++++++++++++++++++--
 target/s390x/translate.c  |  2 +-
 3 files changed, 41 insertions(+), 4 deletions(-)

target/s390x/helper.h +1 -1

index 2f17b62d3d..26c1b07b44 100644
@@ -137,7 +137,7 @@ DEF_HELPER_FLAGS_4(lctlg, TCG_CALL_NO_WG, void, env, i32, i64, i32)
 DEF_HELPER_FLAGS_4(stctl, TCG_CALL_NO_WG, void, env, i32, i64, i32)
 DEF_HELPER_FLAGS_4(stctg, TCG_CALL_NO_WG, void, env, i32, i64, i32)
 DEF_HELPER_FLAGS_2(testblock, TCG_CALL_NO_WG, i32, env, i64)
-DEF_HELPER_FLAGS_2(tprot, TCG_CALL_NO_RWG, i32, i64, i64)
+DEF_HELPER_FLAGS_3(tprot, TCG_CALL_NO_RWG, i32, env, i64, i64)
 DEF_HELPER_FLAGS_2(iske, TCG_CALL_NO_RWG_SE, i64, env, i64)
 DEF_HELPER_FLAGS_3(sske, TCG_CALL_NO_RWG, void, env, i64, i64)
 DEF_HELPER_FLAGS_2(rrbe, TCG_CALL_NO_RWG, i32, env, i64)

target/s390x/mem_helper.c +39 -2

48 lines not on this page · raw message

target/s390x/translate.c +1 -1

index eede2ed157..fdcc2ffb3d 100644
@@ -4534,7 +4534,7 @@ static ExitStatus op_testblock(DisasContext *s, DisasOps *o)
 
 static ExitStatus op_tprot(DisasContext *s, DisasOps *o)
 {
-    gen_helper_tprot(cc_op, o->addr1, o->in2);
+    gen_helper_tprot(cc_op, cpu_env, o->addr1, o->in2);
     set_cc_static(s);
     return NO_EXIT;
 }

Signature · 1 line

2.14.3

Thomas Huth

Reviewed-by on v1 1/2

2017-12-19 08:20 UTC #2 lore raw

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 1/2] s390x/tcg: implement TEST PROTECTION

Quoted 34 lines · David Hildenbrand

Linux uses TEST PROTECTION to sense for available memory locations.

Let's implement what we can for now (just as for the other instructions, excluding AR mode and special protection mechanisms).

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/helper.h     |  2 +-
 target/s390x/mem_helper.c | 41 +++++++++++++++++++++++++++++++++++++++--
 target/s390x/translate.c  |  2 +-
 3 files changed, 41 insertions(+), 4 deletions(-)

target/s390x/helper.h +1 -1

index 2f17b62d3d..26c1b07b44 100644
@@ -137,7 +137,7 @@ DEF_HELPER_FLAGS_4(lctlg, TCG_CALL_NO_WG, void, env, i32, i64, i32)
 DEF_HELPER_FLAGS_4(stctl, TCG_CALL_NO_WG, void, env, i32, i64, i32)
 DEF_HELPER_FLAGS_4(stctg, TCG_CALL_NO_WG, void, env, i32, i64, i32)
 DEF_HELPER_FLAGS_2(testblock, TCG_CALL_NO_WG, i32, env, i64)
-DEF_HELPER_FLAGS_2(tprot, TCG_CALL_NO_RWG, i32, i64, i64)
+DEF_HELPER_FLAGS_3(tprot, TCG_CALL_NO_RWG, i32, env, i64, i64)
 DEF_HELPER_FLAGS_2(iske, TCG_CALL_NO_RWG_SE, i64, env, i64)
 DEF_HELPER_FLAGS_3(sske, TCG_CALL_NO_RWG, void, env, i64, i64)
 DEF_HELPER_FLAGS_2(rrbe, TCG_CALL_NO_RWG, i32, env, i64)

target/s390x/mem_helper.c +0 -1

index 2625d843b3..359e446c6f 100644
@@ -1717,9 +1717,46 @@ uint32_t HELPER(testblock)(CPUS390XState *env, uint64_t real_addr)
     return 0;
 }
 
-uint32_t HELPER(tprot)(uint64_t a1, uint64_t a2)

target/s390x/mem_helper.c uint32_t HELPER(testblock)(CPUS390XState *env, uint64_t real_addr) · line 1728

Quoted 10 lines

target/s390x/mem_helper.c +8 -1

+uint32_t HELPER(tprot)(CPUS390XState *env, uint64_t a1, uint64_t a2)
 {
-    /* XXX implement */
+    S390CPU *cpu = s390_env_get_cpu(env);
+    CPUState *cs = CPU(cpu);
+
+    /*
+     * TODO: we currently don't handle all access protection types
+     * (including access-list and key-controlled) as well as AR mode.
+     */

Maybe add some "if (a2 & 0xf0) qemu_log_mask(LOG_UNIMPL, ...)" so that we've got a chance to detect such conditions later?

Quoted 40 lines

target/s390x/mem_helper.c +31 -0

+    if (!s390_cpu_virt_mem_check_write(cpu, a1, 0, 1)) {
+        /* Fetching permitted; storing permitted */
+        return 0;
+    }
+    switch (env->int_pgm_code) {
+    case PGM_PROTECTION:
+        /* Fetching permitted; storing not permitted */
+        cs->exception_index = 0;
+        return 1;
+    case PGM_ADDRESSING:
+        /* Fetching not permitted; storing not permitted */
+        cs->exception_index = 0;
+        return 2;
+    case PGM_ASCE_TYPE:
+    case PGM_REG_FIRST_TRANS:
+    case PGM_REG_SEC_TRANS:
+    case PGM_REG_THIRD_TRANS:
+    case PGM_SEGMENT_TRANS:
+    case PGM_PAGE_TRANS:
+    case PGM_ALET_SPEC:
+    case PGM_ALEN_SPEC:
+    case PGM_ALE_SEQ:
+    case PGM_ASTE_VALID:
+    case PGM_ASTE_SEQ:
+    case PGM_EXT_AUTH:
+        /* Translation not available */
+        cs->exception_index = 0;
+        return 3;
+    }
+    /* any other exception is forwarded to the guest */
+    s390_cpu_virt_mem_handle_exc(cpu, GETPC());
     return 0;
 }

target/s390x/translate.c +0 -0

index eede2ed157..fdcc2ffb3d 100644

target/s390x/translate.c static ExitStatus op_testblock(DisasContext *s, DisasOps *o) · line 4540

Quoted 8 lines

target/s390x/translate.c +1 -1

@@ -4534,7 +4534,7 @@ static ExitStatus op_testblock(DisasContext *s, DisasOps *o)
 
 static ExitStatus op_tprot(DisasContext *s, DisasOps *o)
 {
-    gen_helper_tprot(cc_op, o->addr1, o->in2);
+    gen_helper_tprot(cc_op, cpu_env, o->addr1, o->in2);
     set_cc_static(s);
     return NO_EXIT;
 }

Reviewed-by Thomas Huth <[email protected]>

David Hildenbrand

author

2017-12-19 10:32 UTC #3 lore raw

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 1/2] s390x/tcg: implement TEST PROTECTION

Quoted 34 lines · Thomas Huth

Quoted 33 lines · David Hildenbrand

Linux uses TEST PROTECTION to sense for available memory locations.

Let's implement what we can for now (just as for the other instructions, excluding AR mode and special protection mechanisms).

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/helper.h     |  2 +-
 target/s390x/mem_helper.c | 41 +++++++++++++++++++++++++++++++++++++++--
 target/s390x/translate.c  |  2 +-
 3 files changed, 41 insertions(+), 4 deletions(-)

target/s390x/helper.h +1 -1

index 2f17b62d3d..26c1b07b44 100644
@@ -137,7 +137,7 @@ DEF_HELPER_FLAGS_4(lctlg, TCG_CALL_NO_WG, void, env, i32, i64, i32)
 DEF_HELPER_FLAGS_4(stctl, TCG_CALL_NO_WG, void, env, i32, i64, i32)
 DEF_HELPER_FLAGS_4(stctg, TCG_CALL_NO_WG, void, env, i32, i64, i32)
 DEF_HELPER_FLAGS_2(testblock, TCG_CALL_NO_WG, i32, env, i64)
-DEF_HELPER_FLAGS_2(tprot, TCG_CALL_NO_RWG, i32, i64, i64)
+DEF_HELPER_FLAGS_3(tprot, TCG_CALL_NO_RWG, i32, env, i64, i64)
 DEF_HELPER_FLAGS_2(iske, TCG_CALL_NO_RWG_SE, i64, env, i64)
 DEF_HELPER_FLAGS_3(sske, TCG_CALL_NO_RWG, void, env, i64, i64)
 DEF_HELPER_FLAGS_2(rrbe, TCG_CALL_NO_RWG, i32, env, i64)

target/s390x/mem_helper.c +0 -0

index 2625d843b3..359e446c6f 100644
@@ -1717,9 +1717,46 @@ uint32_t HELPER(testblock)(CPUS390XState *env, uint64_t real_addr)
     return 0;
 }

Quoted 15 lines

target/s390x/mem_helper.c uint32_t HELPER(testblock)(CPUS390XState *env, uint64_t real_addr) · line 1728
Quoted 11 lines
target/s390x/mem_helper.c +8 -2
-uint32_t HELPER(tprot)(uint64_t a1, uint64_t a2)
+uint32_t HELPER(tprot)(CPUS390XState *env, uint64_t a1, uint64_t a2)
 {
-    /* XXX implement */
+    S390CPU *cpu = s390_env_get_cpu(env);
+    CPUState *cs = CPU(cpu);
+
+    /*
+     * TODO: we currently don't handle all access protection types
+     * (including access-list and key-controlled) as well as AR mode.
+     */
Maybe add some "if (a2 & 0xf0) qemu_log_mask(LOG_UNIMPL, ...)" so that we've got a chance to detect such conditions later?

In general I would agree, however as such acess-list + storage key stuff is missing literally all over the place, I don't think it is very helpful.

Once (if ever) we implement it, we will have to go through the whole PoP either way and fixup all functions.

Thanks!

Signature · 3 lines

Thanks,

David / dhildenb

Cornelia Huck

2017-12-21 17:17 UTC #4 lore raw

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 1/2] s390x/tcg: implement TEST PROTECTION

Quoted 12 lines · On Tue, 19 Dec 2017 11:32:16 +0100 David Hildenbrand <[email protected]> wrote:

Quoted 10 lines · Thomas Huth
target/s390x/mem_helper.c uint32_t HELPER(testblock)(CPUS390XState *env, uint64_t real_addr) · line 1728
Quoted 4 lines · David Hildenbrand
target/s390x/mem_helper.c +4 -0
+    /*
+     * TODO: we currently don't handle all access protection types
+     * (including access-list and key-controlled) as well as AR mode.
+     */  
Maybe add some "if (a2 & 0xf0) qemu_log_mask(LOG_UNIMPL, ...)" so that we've got a chance to detect such conditions later?

Quoted 5 lines

In general I would agree, however as such acess-list + storage key stuff is missing literally all over the place, I don't think it is very helpful.
Once (if ever) we implement it, we will have to go through the whole PoP either way and fixup all functions.

The TODO is fine with me.

Out of curiousity, do you already have an idea how much work that would be? (Not asking for patches right now, mind you :)

David Hildenbrand

author

2017-12-18 22:46 UTC #5 lore raw

PATCH 2/2 s390x/sclp: fix missing be conversion

In reply to David Hildenbrand

Linux crashes right now if maxmem > mem is specified on the command line.

On s390x, the guest can hotplug memory itself right now - very weird - and e.g. Fedora 27 will simply add all memory it can when booting.

So now, we have at least the same behavior on TCG and KVM.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/sclp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/s390x/sclp.c +2 -2

index 9be0cb80ad..829c13bcf2 100644
@@ -233,7 +233,7 @@ static void assign_storage(SCLPDevice *sclp, SCCB *sccb)
         sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
         return;
     }
-    assign_addr = (assign_info->rn - 1) * mhd->rzm;
+    assign_addr = (be16_to_cpu(assign_info->rn) - 1) * mhd->rzm;
 
     if ((assign_addr % MEM_SECTION_SIZE == 0) &&
         (assign_addr >= mhd->padded_ram_size)) {

@@ -292,7 +292,7 @@ static void unassign_storage(SCLPDevice *sclp, SCCB *sccb)
         sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
         return;
     }
-    unassign_addr = (assign_info->rn - 1) * mhd->rzm;
+    unassign_addr = (be16_to_cpu(assign_info->rn) - 1) * mhd->rzm;
 
     /* if the addr is a multiple of 256 MB */
     if ((unassign_addr % MEM_SECTION_SIZE == 0) &&

Signature · 1 line

2.14.3

Thomas Huth

Reviewed-by on v1 2/2

2017-12-19 08:25 UTC #6 lore raw

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 2/2] s390x/sclp: fix missing be conversion

Quoted 27 lines · David Hildenbrand

Linux crashes right now if maxmem > mem is specified on the command line.
On s390x, the guest can hotplug memory itself right now - very weird - and e.g. Fedora 27 will simply add all memory it can when booting.
So now, we have at least the same behavior on TCG and KVM.
Signed-off-by David Hildenbrand <[email protected]>
---
 hw/s390x/sclp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
hw/s390x/sclp.c +1 -1
index 9be0cb80ad..829c13bcf2 100644
@@ -233,7 +233,7 @@ static void assign_storage(SCLPDevice *sclp, SCCB *sccb)
         sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
         return;
     }
-    assign_addr = (assign_info->rn - 1) * mhd->rzm;
+    assign_addr = (be16_to_cpu(assign_info->rn) - 1) * mhd->rzm;
 
     if ((assign_addr % MEM_SECTION_SIZE == 0) &&
         (assign_addr >= mhd->padded_ram_size)) {

hw/s390x/sclp.c static void unassign_storage(SCLPDevice *sclp, SCCB *sccb) · line 298

Quoted 8 lines

hw/s390x/sclp.c +1 -1

@@ -292,7 +292,7 @@ static void unassign_storage(SCLPDevice *sclp, SCCB *sccb)
         sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
         return;
     }
-    unassign_addr = (assign_info->rn - 1) * mhd->rzm;
+    unassign_addr = (be16_to_cpu(assign_info->rn) - 1) * mhd->rzm;
 
     /* if the addr is a multiple of 256 MB */
     if ((unassign_addr % MEM_SECTION_SIZE == 0) &&

Reviewed-by Thomas Huth <[email protected]>

Cornelia Huck

Applied

2017-12-22 10:57 UTC #7 lore raw

In reply to David Hildenbrand

Quoted 2 lines · On Mon, 18 Dec 2017 23:46:14 +0100 David Hildenbrand <[email protected]> wrote:

While trying to fix TCG so I can properly detect memory in kvm-unit-tests ... looks like I accidentally made memory hotplug under TCG work (whoops).

Quoted 18 lines

qemu-system-s390x ... -m 2048,maxmem=4096M,slots=4 ...

[root@localhost ~]# cat /proc/meminfo

MemTotal:        4143632 kB
MemFree:         3845248 kB
MemAvailable:    3947932 kB

[root@localhost ~]# lsmem

RANGE                                  SIZE  STATE REMOVABLE BLOCK
0x0000000000000000-0x000000000fffffff  256M online        no     0
0x0000000010000000-0x000000006fffffff  1.5G online       yes   1-6
0x0000000070000000-0x00000000ffffffff  2.3G online        no  7-15

Memory block size:       256M
Total online memory:       4G
Total offline memory:      0B

Quoted 6 lines

I am not sure if we want to have memory hotplug in its current form later on (the guest can hotplug memory itself). This is different compared to all other architectures. E.g. when booting Fedora 27, it will simply hotplug and add all memory. This doesn't make any sense in the context of VMs where you want to be able to control from the outside, when and how much more memory is given to a VM. But anyhow, seems to work ...

Yes, s390x is different from anyone else in that respect.

I played with it for a bit and it behaves exactly the same under kvm and under tcg.

Quoted 6 lines

David Hildenbrand (2):
s390x/tcg: implement TEST PROTECTION
s390x/sclp: fix missing be conversion

Quoted 6 lines

 hw/s390x/sclp.c           |  4 ++--
 target/s390x/helper.h     |  2 +-
 target/s390x/mem_helper.c | 41 +++++++++++++++++++++++++++++++++++++++--
 target/s390x/translate.c  |  2 +-
 4 files changed, 43 insertions(+), 6 deletions(-)

Thanks, applied.

David Hildenbrand

author

2018-01-12 10:43 UTC #8 lore raw

Quoted 5 lines

Quoted 5 lines
David Hildenbrand (2):
s390x/tcg: implement TEST PROTECTION
s390x/sclp: fix missing be conversion

Quoted 9 lines

Quoted 6 lines

 hw/s390x/sclp.c           |  4 ++--
 target/s390x/helper.h     |  2 +-
 target/s390x/mem_helper.c | 41 +++++++++++++++++++++++++++++++++++++++--
 target/s390x/translate.c  |  2 +-
 4 files changed, 43 insertions(+), 6 deletions(-)

Thanks, applied.

FWIW, just found out I interpreted the PGM_ADRESSING case wrong, will resend once I fixed it.

Signature · 3 lines

Thanks,

David / dhildenb

Cornelia Huck

2018-01-12 10:51 UTC #9 lore raw

Quoted 11 lines · On Fri, 12 Jan 2018 11:43:00 +0100 David Hildenbrand <[email protected]> wrote:

Quoted 11 lines
Quoted 10 lines
David Hildenbrand (2):
s390x/tcg: implement TEST PROTECTION
s390x/sclp: fix missing be conversion
 hw/s390x/sclp.c           |  4 ++--
 target/s390x/helper.h     |  2 +-
 target/s390x/mem_helper.c | 41 +++++++++++++++++++++++++++++++++++++++--
 target/s390x/translate.c  |  2 +-
 4 files changed, 43 insertions(+), 6 deletions(-)

Quoted 6 lines

Quoted 2 lines
Thanks, applied.
FWIW, just found out I interpreted the PGM_ADRESSING case wrong, will resend once I fixed it.

I'd prefer a fixup patch on top, as I have already queued stuff on top of this.

David Hildenbrand

author

2018-01-12 10:52 UTC #10 lore raw

Quoted 17 lines · Cornelia Huck

Quoted 14 lines · On Fri, 12 Jan 2018 11:43:00 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 13 lines
Quoted 10 lines
David Hildenbrand (2):
s390x/tcg: implement TEST PROTECTION
s390x/sclp: fix missing be conversion
 hw/s390x/sclp.c           |  4 ++--
 target/s390x/helper.h     |  2 +-
 target/s390x/mem_helper.c | 41 +++++++++++++++++++++++++++++++++++++++--
 target/s390x/translate.c  |  2 +-
 4 files changed, 43 insertions(+), 6 deletions(-)
Thanks, applied.

Quoted 7 lines

Quoted 3 lines
FWIW, just found out I interpreted the PGM_ADRESSING case wrong, will resend once I fixed it.
I'd prefer a fixup patch on top, as I have already queued stuff on top of this.

Can do!

Signature · 3 lines

Thanks,

David / dhildenb