[PATCH v4] target/s390x: Implement the MVPG condition-code-option bit

[PATCH v4] target/s390x: Implement the MVPG condition-code-option bit

[Thomas Huth]

From: Richard Henderson <richard.henderson@linaro.org>

If the CCO bit is set, MVPG should not generate an exception but
report page translation faults via a CC code.

Create a new helper, access_prepare_nf, which can use probe_access_flags
in non-faulting mode, and then handle watchpoints.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
[thuth: Added logic to still inject protection exceptions]
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 v4: Add logic to inject protection exceptions if necessary

 target/s390x/cpu.h         |  3 ++
 target/s390x/excp_helper.c |  3 ++
 target/s390x/mem_helper.c  | 93 ++++++++++++++++++++++++++++----------
 3 files changed, 76 insertions(+), 23 deletions(-)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 60d434d5ed..825503c6c0 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -114,6 +114,9 @@ struct CPUS390XState {
 
     uint64_t diag318_info;
 
+    uint64_t tlb_fill_tec;   /* translation exception code during tlb_fill */
+    int tlb_fill_exc;        /* exception number seen during tlb_fill */
+
     /* Fields up to this point are cleared by a CPU reset */
     struct {} end_reset_fields;
 
diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
index ce16af394b..c48cd6b46f 100644
--- a/target/s390x/excp_helper.c
+++ b/target/s390x/excp_helper.c
@@ -164,6 +164,9 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
         tec = 0; /* unused */
     }
 
+    env->tlb_fill_exc = excp;
+    env->tlb_fill_tec = tec;
+
     if (!excp) {
         qemu_log_mask(CPU_LOG_MMU,
                       "%s: set tlb %" PRIx64 " -> %" PRIx64 " (%x)\n",
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 25cfede806..cf741541d3 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -130,28 +130,62 @@ typedef struct S390Access {
     int mmu_idx;
 } S390Access;
 
+static bool access_prepare_nf(S390Access *access, CPUS390XState *env,
+                              bool nofault, vaddr vaddr1, int size,
+                              MMUAccessType access_type,
+                              int mmu_idx, uintptr_t ra)
+{
+    void *haddr1, *haddr2 = NULL;
+    int size1, size2;
+    vaddr vaddr2 = 0;
+    int flags;
+
+    assert(size > 0 && size <= 4096);
+
+    size1 = MIN(size, -(vaddr1 | TARGET_PAGE_MASK)),
+    size2 = size - size1;
+
+    flags = probe_access_flags(env, vaddr1, access_type, mmu_idx,
+                               nofault, &haddr1, ra);
+    if (unlikely(size2)) {
+        /* The access crosses page boundaries. */
+        vaddr2 = wrap_address(env, vaddr1 + size1);
+        flags |= probe_access_flags(env, vaddr2, access_type, mmu_idx,
+                                    nofault, &haddr2, ra);
+    }
+
+    if (unlikely(flags & TLB_INVALID_MASK)) {
+        return false;
+    }
+    if (unlikely(flags & TLB_WATCHPOINT)) {
+        /* S390 does not presently use transaction attributes. */
+        cpu_check_watchpoint(env_cpu(env), vaddr1, size,
+                             MEMTXATTRS_UNSPECIFIED,
+                             (access_type == MMU_DATA_STORE
+                              ? BP_MEM_WRITE : BP_MEM_READ), ra);
+    }
+
+    *access = (S390Access) {
+        .vaddr1 = vaddr1,
+        .vaddr2 = vaddr2,
+        .haddr1 = haddr1,
+        .haddr2 = haddr2,
+        .size1 = size1,
+        .size2 = size2,
+        .mmu_idx = mmu_idx
+    };
+    return true;
+}
+
 static S390Access access_prepare(CPUS390XState *env, vaddr vaddr, int size,
                                  MMUAccessType access_type, int mmu_idx,
                                  uintptr_t ra)
 {
-    S390Access access = {
-        .vaddr1 = vaddr,
-        .size1 = MIN(size, -(vaddr | TARGET_PAGE_MASK)),
-        .mmu_idx = mmu_idx,
-    };
-
-    g_assert(size > 0 && size <= 4096);
-    access.haddr1 = probe_access(env, access.vaddr1, access.size1, access_type,
-                                 mmu_idx, ra);
-
-    if (unlikely(access.size1 != size)) {
-        /* The access crosses page boundaries. */
-        access.vaddr2 = wrap_address(env, vaddr + access.size1);
-        access.size2 = size - access.size1;
-        access.haddr2 = probe_access(env, access.vaddr2, access.size2,
-                                     access_type, mmu_idx, ra);
-    }
-    return access;
+    S390Access ret;
+    bool ok = access_prepare_nf(&ret, env, false, vaddr, size,
+                                access_type, mmu_idx, ra);
+    assert(ok);
+    return ret;
 }
 
 /* Helper to handle memset on a single page. */
@@ -845,8 +879,10 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2)
     const int mmu_idx = cpu_mmu_index(env, false);
     const bool f = extract64(r0, 11, 1);
     const bool s = extract64(r0, 10, 1);
+    const bool cco = extract64(r0, 8, 1);
     uintptr_t ra = GETPC();
     S390Access srca, desta;
+    bool ok;
 
     if ((f && s) || extract64(r0, 12, 4)) {
         tcg_s390_program_interrupt(env, PGM_SPECIFICATION, GETPC());
@@ -858,13 +894,24 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2)
     /*
      * TODO:
      * - Access key handling
-     * - CC-option with surpression of page-translation exceptions
      * - Store r1/r2 register identifiers at real location 162
      */
-    srca = access_prepare(env, r2, TARGET_PAGE_SIZE, MMU_DATA_LOAD, mmu_idx,
-                          ra);
-    desta = access_prepare(env, r1, TARGET_PAGE_SIZE, MMU_DATA_STORE, mmu_idx,
-                           ra);
+    ok = access_prepare_nf(&srca, env, cco, r2, TARGET_PAGE_SIZE,
+                           MMU_DATA_LOAD, mmu_idx, ra);
+    if (!ok) {
+        return 2;
+    }
+    ok = access_prepare_nf(&desta, env, cco, r1, TARGET_PAGE_SIZE,
+                           MMU_DATA_STORE, mmu_idx, ra);
+    if (!ok) {
+        if (env->tlb_fill_exc == PGM_PROTECTION) {
+            stq_phys(env_cpu(env)->as,
+                     env->psa + offsetof(LowCore, trans_exc_code),
+                     env->tlb_fill_tec);
+            tcg_s390_program_interrupt(env, PGM_PROTECTION, ra);
+        }
+        return 1;
+    }
     access_memmove(env, &desta, &srca, ra);
     return 0; /* data moved */
 }
-- 
2.27.0

Re: [PATCH v4] target/s390x: Implement the MVPG condition-code-option bit

[Richard Henderson]

On 3/3/21 5:28 AM, Thomas Huth wrote:
> From: Richard Henderson<richard.henderson@linaro.org>
> 
> If the CCO bit is set, MVPG should not generate an exception but
> report page translation faults via a CC code.
> 
> Create a new helper, access_prepare_nf, which can use probe_access_flags
> in non-faulting mode, and then handle watchpoints.
> 
> Signed-off-by: Richard Henderson<richard.henderson@linaro.org>
> [thuth: Added logic to still inject protection exceptions]
> Signed-off-by: Thomas Huth<thuth@redhat.com>
> ---
>   v4: Add logic to inject protection exceptions if necessary

Works for me.  I was considering adding yet another cputlb interface to help 
with this, but this is sufficiently clean that I don't think it's required.


r~

Re: [PATCH v4] target/s390x: Implement the MVPG condition-code-option bit

[David Hildenbrand]

On 03.03.21 14:28, Thomas Huth wrote:
> From: Richard Henderson <richard.henderson@linaro.org>
> 
> If the CCO bit is set, MVPG should not generate an exception but
> report page translation faults via a CC code.
> 
> Create a new helper, access_prepare_nf, which can use probe_access_flags
> in non-faulting mode, and then handle watchpoints.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> [thuth: Added logic to still inject protection exceptions]
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   v4: Add logic to inject protection exceptions if necessary
> 
>   target/s390x/cpu.h         |  3 ++
>   target/s390x/excp_helper.c |  3 ++
>   target/s390x/mem_helper.c  | 93 ++++++++++++++++++++++++++++----------
>   3 files changed, 76 insertions(+), 23 deletions(-)
> 
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 60d434d5ed..825503c6c0 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -114,6 +114,9 @@ struct CPUS390XState {
>   
>       uint64_t diag318_info;
>   

Should we start wrapping that stuff into #ifdef CONFIG_TCG ?

> +    uint64_t tlb_fill_tec;   /* translation exception code during tlb_fill */
> +    int tlb_fill_exc;        /* exception number seen during tlb_fill */
> +
>       /* Fields up to this point are cleared by a CPU reset */
>       struct {} end_reset_fields;
>   
> diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
> index ce16af394b..c48cd6b46f 100644
> --- a/target/s390x/excp_helper.c
> +++ b/target/s390x/excp_helper.c
> @@ -164,6 +164,9 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>           tec = 0; /* unused */
>       }
>   
> +    env->tlb_fill_exc = excp;
> +    env->tlb_fill_tec = tec;
> +

Just what I had in mind.

>       if (!excp) {
>           qemu_log_mask(CPU_LOG_MMU,
>                         "%s: set tlb %" PRIx64 " -> %" PRIx64 " (%x)\n",
> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> index 25cfede806..cf741541d3 100644
> --- a/target/s390x/mem_helper.c
> +++ b/target/s390x/mem_helper.c
> @@ -130,28 +130,62 @@ typedef struct S390Access {
>       int mmu_idx;
>   } S390Access;
>   
> +static bool access_prepare_nf(S390Access *access, CPUS390XState *env,
> +                              bool nofault, vaddr vaddr1, int size,
> +                              MMUAccessType access_type,
> +                              int mmu_idx, uintptr_t ra)
> +{
> +    void *haddr1, *haddr2 = NULL;
> +    int size1, size2;
> +    vaddr vaddr2 = 0;
> +    int flags;
> +
> +    assert(size > 0 && size <= 4096);
> +
> +    size1 = MIN(size, -(vaddr1 | TARGET_PAGE_MASK)),
> +    size2 = size - size1;
> +
> +    flags = probe_access_flags(env, vaddr1, access_type, mmu_idx,
> +                               nofault, &haddr1, ra);
> +    if (unlikely(size2)) {
> +        /* The access crosses page boundaries. */
> +        vaddr2 = wrap_address(env, vaddr1 + size1);
> +        flags |= probe_access_flags(env, vaddr2, access_type, mmu_idx,
> +                                    nofault, &haddr2, ra);
> +    }
> +
> +    if (unlikely(flags & TLB_INVALID_MASK)) {
> +        return false;

^ I recall PAGE_WRITE_INV handling where we immediately set 
TLB_INVALID_MASK again on write access (to handle low-address protection 
cleanly). I suspect that TLB_INVALID_MASK will be set in that case (I 
could be wrong, though).

What certainly would work is checking for "haddr != NULL".

/* Don't rely on TLB_INVALID_MASK - see PAGE_WRITE_INV handling. */
if (unlikely(!haddr1)) {
	return false;
}

> +    }
> +    if (unlikely(flags & TLB_WATCHPOINT)) {
> +        /* S390 does not presently use transaction attributes. */
> +        cpu_check_watchpoint(env_cpu(env), vaddr1, size,
> +                             MEMTXATTRS_UNSPECIFIED,
> +                             (access_type == MMU_DATA_STORE
> +                              ? BP_MEM_WRITE : BP_MEM_READ), ra);
> +    }
> +

[...]

>   /* Helper to handle memset on a single page. */
> @@ -845,8 +879,10 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2)
>       const int mmu_idx = cpu_mmu_index(env, false);
>       const bool f = extract64(r0, 11, 1);
>       const bool s = extract64(r0, 10, 1);
> +    const bool cco = extract64(r0, 8, 1);
>       uintptr_t ra = GETPC();
>       S390Access srca, desta;
> +    bool ok;
>   
>       if ((f && s) || extract64(r0, 12, 4)) {
>           tcg_s390_program_interrupt(env, PGM_SPECIFICATION, GETPC());
> @@ -858,13 +894,24 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2)
>       /*
>        * TODO:
>        * - Access key handling
> -     * - CC-option with surpression of page-translation exceptions
>        * - Store r1/r2 register identifiers at real location 162
>        */
> -    srca = access_prepare(env, r2, TARGET_PAGE_SIZE, MMU_DATA_LOAD, mmu_idx,
> -                          ra);
> -    desta = access_prepare(env, r1, TARGET_PAGE_SIZE, MMU_DATA_STORE, mmu_idx,
> -                           ra);
> +    ok = access_prepare_nf(&srca, env, cco, r2, TARGET_PAGE_SIZE,
> +                           MMU_DATA_LOAD, mmu_idx, ra);
> +    if (!ok) {
> +        return 2;
> +    }
> +    ok = access_prepare_nf(&desta, env, cco, r1, TARGET_PAGE_SIZE,
> +                           MMU_DATA_STORE, mmu_idx, ra);
> +    if (!ok) {
> +        if (env->tlb_fill_exc == PGM_PROTECTION) {
> +            stq_phys(env_cpu(env)->as,
> +                     env->psa + offsetof(LowCore, trans_exc_code),
> +                     env->tlb_fill_tec);
> +            tcg_s390_program_interrupt(env, PGM_PROTECTION, ra);
> +        }
> +        return 1;
> +    }
>       access_memmove(env, &desta, &srca, ra);
>       return 0; /* data moved */
>   }
> 

Apart from that, looks good to me.

-- 
Thanks,

David / dhildenb

Re: [PATCH v4] target/s390x: Implement the MVPG condition-code-option bit

[Richard Henderson]

On 3/3/21 11:39 AM, David Hildenbrand wrote:
> Should we start wrapping that stuff into #ifdef CONFIG_TCG ?
> 
>> +    uint64_t tlb_fill_tec;   /* translation exception code during tlb_fill */
>> +    int tlb_fill_exc;        /* exception number seen during tlb_fill */

Eh, probably not.  At least not until we elide the softmmu tlb, which is 
fantastically larger.

>> +    if (unlikely(flags & TLB_INVALID_MASK)) {
>> +        return false;
> 
> ^ I recall PAGE_WRITE_INV handling where we immediately set TLB_INVALID_MASK 
> again on write access (to handle low-address protection cleanly). I suspect 
> that TLB_INVALID_MASK will be set in that case (I could be wrong, though).
> 
> What certainly would work is checking for "haddr != NULL".
> 
> /* Don't rely on TLB_INVALID_MASK - see PAGE_WRITE_INV handling. */
> if (unlikely(!haddr1)) {
>      return false;
> }

Ah, right.  I consider TLB_INVALID_MASK being set in the return from 
probe_access_flags for PAGE_WRITE_INV a bug.  I'm not sure how to fix that 
right away.

Well, !haddr1 is also false for TLB_MMIO, so you'd need to check for that as well.


r~

Re: [PATCH v4] target/s390x: Implement the MVPG condition-code-option bit

[David Hildenbrand]

> Am 03.03.2021 um 22:05 schrieb Richard Henderson <richard.henderson@linaro.org>:
> 
> On 3/3/21 11:39 AM, David Hildenbrand wrote:
>> Should we start wrapping that stuff into #ifdef CONFIG_TCG ?
>>> +    uint64_t tlb_fill_tec;   /* translation exception code during tlb_fill */
>>> +    int tlb_fill_exc;        /* exception number seen during tlb_fill */
> 
> Eh, probably not.  At least not until we elide the softmmu tlb, which is fantastically larger.
> 
>>> +    if (unlikely(flags & TLB_INVALID_MASK)) {
>>> +        return false;
>> ^ I recall PAGE_WRITE_INV handling where we immediately set TLB_INVALID_MASK again on write access (to handle low-address protection cleanly). I suspect that TLB_INVALID_MASK will be set in that case (I could be wrong, though).
>> What certainly would work is checking for "haddr != NULL".
>> /* Don't rely on TLB_INVALID_MASK - see PAGE_WRITE_INV handling. */
>> if (unlikely(!haddr1)) {
>>     return false;
>> }
> 
> Ah, right.  I consider TLB_INVALID_MASK being set in the return from probe_access_flags for PAGE_WRITE_INV a bug.  I'm not sure how to fix that right away.
> 
> Well, !haddr1 is also false for TLB_MMIO, so you'd need to check for that as well.

MMIO on s390x? :)

Re: [PATCH v4] target/s390x: Implement the MVPG condition-code-option bit

[Richard Henderson]

On 3/3/21 1:11 PM, David Hildenbrand wrote:
> MMIO on s390x? :)

hw/s390x/s390-pci-bus.c, memory_region_init_io*().


r~

Re: [PATCH v4] target/s390x: Implement the MVPG condition-code-option bit

[David Hildenbrand]

> Am 03.03.2021 um 22:19 schrieb Richard Henderson <richard.henderson@linaro.org>:
> 
> On 3/3/21 1:11 PM, David Hildenbrand wrote:
>> MMIO on s390x? :)
> 
> hw/s390x/s390-pci-bus.c, memory_region_init_io*().
> 

... part of system address space where a CPU could stumble over it?

> r~
> 

Re: [PATCH v4] target/s390x: Implement the MVPG condition-code-option bit

[Richard Henderson]

On 3/3/21 1:22 PM, David Hildenbrand wrote:
> 
>> Am 03.03.2021 um 22:19 schrieb Richard Henderson <richard.henderson@linaro.org>:
>>
>> On 3/3/21 1:11 PM, David Hildenbrand wrote:
>>> MMIO on s390x? :)
>>
>> hw/s390x/s390-pci-bus.c, memory_region_init_io*().
>>
> 
> ... part of system address space where a CPU could stumble over it?

Impossible to tell within 3 layers of object wrappers.  :-(
I suppose I have no idea how "pci" was hacked onto s390x.

r~

Re: [PATCH v4] target/s390x: Implement the MVPG condition-code-option bit

[David Hildenbrand]

On 03.03.21 22:36, Richard Henderson wrote:
> On 3/3/21 1:22 PM, David Hildenbrand wrote:
>>
>>> Am 03.03.2021 um 22:19 schrieb Richard Henderson <richard.henderson@linaro.org>:
>>>
>>> On 3/3/21 1:11 PM, David Hildenbrand wrote:
>>>> MMIO on s390x? :)
>>>
>>> hw/s390x/s390-pci-bus.c, memory_region_init_io*().
>>>
>>
>> ... part of system address space where a CPU could stumble over it?
> 
> Impossible to tell within 3 layers of object wrappers.  :-(
> I suppose I have no idea how "pci" was hacked onto s390x.

You've used the right words to describe "pci" (!) on s390x.

IIRC, there is no MMIO: configuration space accesses etc. are performed 
using special access instructions - which will "emulate" the MMIO access 
performed on other archs via simple read/write instructions.

Ordinary instructions (e.g., mvpg) that operate on the system address 
space should never stumble over MMIO regions - because that concept does 
not exist on s390x.

-- 
Thanks,

David / dhildenb

Re: [PATCH v4] target/s390x: Implement the MVPG condition-code-option bit

[Cornelia Huck]

On Thu, 4 Mar 2021 09:10:44 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 03.03.21 22:36, Richard Henderson wrote:
> > On 3/3/21 1:22 PM, David Hildenbrand wrote:  
> >>  
> >>> Am 03.03.2021 um 22:19 schrieb Richard Henderson <richard.henderson@linaro.org>:
> >>>
> >>> On 3/3/21 1:11 PM, David Hildenbrand wrote:  
> >>>> MMIO on s390x? :)  
> >>>
> >>> hw/s390x/s390-pci-bus.c, memory_region_init_io*().
> >>>  
> >>
> >> ... part of system address space where a CPU could stumble over it?  
> > 
> > Impossible to tell within 3 layers of object wrappers.  :-(
> > I suppose I have no idea how "pci" was hacked onto s390x.  
> 
> You've used the right words to describe "pci" (!) on s390x.
> 
> IIRC, there is no MMIO: configuration space accesses etc. are performed 
> using special access instructions - which will "emulate" the MMIO access 
> performed on other archs via simple read/write instructions.

Yes, it's all specialized instructions. (QEMU emulates one set, there's
a newer one supported by the Linux kernel that is a bit more mmio-like
in semantics, but I don't know the details.)

> 
> Ordinary instructions (e.g., mvpg) that operate on the system address 
> space should never stumble over MMIO regions - because that concept does 
> not exist on s390x.
> 

That's my understanding as well.

Re: [PATCH v4] target/s390x: Implement the MVPG condition-code-option bit

[Thomas Huth]

On 04/03/2021 09.10, David Hildenbrand wrote:
> On 03.03.21 22:36, Richard Henderson wrote:
>> On 3/3/21 1:22 PM, David Hildenbrand wrote:
>>>
>>>> Am 03.03.2021 um 22:19 schrieb Richard Henderson 
>>>> <richard.henderson@linaro.org>:
>>>>
>>>> On 3/3/21 1:11 PM, David Hildenbrand wrote:
>>>>> MMIO on s390x? :)
>>>>
>>>> hw/s390x/s390-pci-bus.c, memory_region_init_io*().
>>>>
>>>
>>> ... part of system address space where a CPU could stumble over it?
>>
>> Impossible to tell within 3 layers of object wrappers.  :-(
>> I suppose I have no idea how "pci" was hacked onto s390x.
> 
> You've used the right words to describe "pci" (!) on s390x.
> 
> IIRC, there is no MMIO: configuration space accesses etc. are performed 
> using special access instructions - which will "emulate" the MMIO access 
> performed on other archs via simple read/write instructions.
> 
> Ordinary instructions (e.g., mvpg) that operate on the system address space 
> should never stumble over MMIO regions - because that concept does not exist 
> on s390x.

Sorry, guys, you've lost me here ... is there now something left to do for 
this patch or is it good as it is?

  Thomas

Re: [PATCH v4] target/s390x: Implement the MVPG condition-code-option bit

[David Hildenbrand]

> Am 09.03.2021 um 22:05 schrieb Thomas Huth <thuth@redhat.com>:
> 
> On 04/03/2021 09.10, David Hildenbrand wrote:
>>> On 03.03.21 22:36, Richard Henderson wrote:
>>> On 3/3/21 1:22 PM, David Hildenbrand wrote:
>>>> 
>>>>> Am 03.03.2021 um 22:19 schrieb Richard Henderson <richard.henderson@linaro.org>:
>>>>> 
>>>>> On 3/3/21 1:11 PM, David Hildenbrand wrote:
>>>>>> MMIO on s390x? :)
>>>>> 
>>>>> hw/s390x/s390-pci-bus.c, memory_region_init_io*().
>>>>> 
>>>> 
>>>> ... part of system address space where a CPU could stumble over it?
>>> 
>>> Impossible to tell within 3 layers of object wrappers.  :-(
>>> I suppose I have no idea how "pci" was hacked onto s390x.
>> You've used the right words to describe "pci" (!) on s390x.
>> IIRC, there is no MMIO: configuration space accesses etc. are performed using special access instructions - which will "emulate" the MMIO access performed on other archs via simple read/write instructions.
>> Ordinary instructions (e.g., mvpg) that operate on the system address space should never stumble over MMIO regions - because that concept does not exist on s390x.
> 
> Sorry, guys, you've lost me here ... is there now something left to do for this patch or is it good as it is?

I think that check for invalid TLB should be replaced by a check against host == NULL.

Re: [PATCH v4] target/s390x: Implement the MVPG condition-code-option bit

[Cornelia Huck]

On Wed, 10 Mar 2021 21:49:22 +0100
David Hildenbrand <david@redhat.com> wrote:

> > Am 09.03.2021 um 22:05 schrieb Thomas Huth <thuth@redhat.com>:
> > 
> > On 04/03/2021 09.10, David Hildenbrand wrote:  
> >>> On 03.03.21 22:36, Richard Henderson wrote:
> >>> On 3/3/21 1:22 PM, David Hildenbrand wrote:  
> >>>>   
> >>>>> Am 03.03.2021 um 22:19 schrieb Richard Henderson <richard.henderson@linaro.org>:
> >>>>> 
> >>>>> On 3/3/21 1:11 PM, David Hildenbrand wrote:  
> >>>>>> MMIO on s390x? :)  
> >>>>> 
> >>>>> hw/s390x/s390-pci-bus.c, memory_region_init_io*().
> >>>>>   
> >>>> 
> >>>> ... part of system address space where a CPU could stumble over it?  
> >>> 
> >>> Impossible to tell within 3 layers of object wrappers.  :-(
> >>> I suppose I have no idea how "pci" was hacked onto s390x.  
> >> You've used the right words to describe "pci" (!) on s390x.
> >> IIRC, there is no MMIO: configuration space accesses etc. are performed using special access instructions - which will "emulate" the MMIO access performed on other archs via simple read/write instructions.
> >> Ordinary instructions (e.g., mvpg) that operate on the system address space should never stumble over MMIO regions - because that concept does not exist on s390x.  
> > 
> > Sorry, guys, you've lost me here ... is there now something left to do for this patch or is it good as it is?  
> 
> I think that check for invalid TLB should be replaced by a check against host == NULL.
> 

Just a reminder that softfreeze is on Tuesday next week, and I'd like
to send a pull request by Monday. So a v5 should arrive soon to make it
:)

Re: [PATCH v4] target/s390x: Implement the MVPG condition-code-option bit

[David Hildenbrand]

On 03.03.21 14:28, Thomas Huth wrote:
> From: Richard Henderson <richard.henderson@linaro.org>
> 
> If the CCO bit is set, MVPG should not generate an exception but
> report page translation faults via a CC code.
> 
> Create a new helper, access_prepare_nf, which can use probe_access_flags
> in non-faulting mode, and then handle watchpoints.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> [thuth: Added logic to still inject protection exceptions]
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   v4: Add logic to inject protection exceptions if necessary
> 
>   target/s390x/cpu.h         |  3 ++
>   target/s390x/excp_helper.c |  3 ++
>   target/s390x/mem_helper.c  | 93 ++++++++++++++++++++++++++++----------
>   3 files changed, 76 insertions(+), 23 deletions(-)
> 
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 60d434d5ed..825503c6c0 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -114,6 +114,9 @@ struct CPUS390XState {
>   
>       uint64_t diag318_info;
>   
> +    uint64_t tlb_fill_tec;   /* translation exception code during tlb_fill */
> +    int tlb_fill_exc;        /* exception number seen during tlb_fill */
> +
>       /* Fields up to this point are cleared by a CPU reset */
>       struct {} end_reset_fields;
>   
> diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
> index ce16af394b..c48cd6b46f 100644
> --- a/target/s390x/excp_helper.c
> +++ b/target/s390x/excp_helper.c
> @@ -164,6 +164,9 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>           tec = 0; /* unused */
>       }
>   
> +    env->tlb_fill_exc = excp;
> +    env->tlb_fill_tec = tec;
> +
>       if (!excp) {
>           qemu_log_mask(CPU_LOG_MMU,
>                         "%s: set tlb %" PRIx64 " -> %" PRIx64 " (%x)\n",
> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> index 25cfede806..cf741541d3 100644
> --- a/target/s390x/mem_helper.c
> +++ b/target/s390x/mem_helper.c
> @@ -130,28 +130,62 @@ typedef struct S390Access {
>       int mmu_idx;
>   } S390Access;
>   
> +static bool access_prepare_nf(S390Access *access, CPUS390XState *env,
> +                              bool nofault, vaddr vaddr1, int size,
> +                              MMUAccessType access_type,
> +                              int mmu_idx, uintptr_t ra)
> +{
> +    void *haddr1, *haddr2 = NULL;
> +    int size1, size2;
> +    vaddr vaddr2 = 0;
> +    int flags;
> +
> +    assert(size > 0 && size <= 4096);
> +
> +    size1 = MIN(size, -(vaddr1 | TARGET_PAGE_MASK)),
> +    size2 = size - size1;
> +
> +    flags = probe_access_flags(env, vaddr1, access_type, mmu_idx,
> +                               nofault, &haddr1, ra);
> +    if (unlikely(size2)) {
> +        /* The access crosses page boundaries. */
> +        vaddr2 = wrap_address(env, vaddr1 + size1);
> +        flags |= probe_access_flags(env, vaddr2, access_type, mmu_idx,
> +                                    nofault, &haddr2, ra);
> +    }
> +
> +    if (unlikely(flags & TLB_INVALID_MASK)) {
> +        return false;
> +    }
> +    if (unlikely(flags & TLB_WATCHPOINT)) {
> +        /* S390 does not presently use transaction attributes. */
> +        cpu_check_watchpoint(env_cpu(env), vaddr1, size,
> +                             MEMTXATTRS_UNSPECIFIED,
> +                             (access_type == MMU_DATA_STORE
> +                              ? BP_MEM_WRITE : BP_MEM_READ), ra);
> +    }
> +
> +    *access = (S390Access) {
> +        .vaddr1 = vaddr1,
> +        .vaddr2 = vaddr2,
> +        .haddr1 = haddr1,
> +        .haddr2 = haddr2,
> +        .size1 = size1,
> +        .size2 = size2,
> +        .mmu_idx = mmu_idx
> +    };
> +    return true;
> +}
> +
>   static S390Access access_prepare(CPUS390XState *env, vaddr vaddr, int size,
>                                    MMUAccessType access_type, int mmu_idx,
>                                    uintptr_t ra)
>   {
> -    S390Access access = {
> -        .vaddr1 = vaddr,
> -        .size1 = MIN(size, -(vaddr | TARGET_PAGE_MASK)),
> -        .mmu_idx = mmu_idx,
> -    };
> -
> -    g_assert(size > 0 && size <= 4096);
> -    access.haddr1 = probe_access(env, access.vaddr1, access.size1, access_type,
> -                                 mmu_idx, ra);
> -
> -    if (unlikely(access.size1 != size)) {
> -        /* The access crosses page boundaries. */
> -        access.vaddr2 = wrap_address(env, vaddr + access.size1);
> -        access.size2 = size - access.size1;
> -        access.haddr2 = probe_access(env, access.vaddr2, access.size2,
> -                                     access_type, mmu_idx, ra);
> -    }
> -    return access;
> +    S390Access ret;
> +    bool ok = access_prepare_nf(&ret, env, false, vaddr, size,
> +                                access_type, mmu_idx, ra);
> +    assert(ok);
> +    return ret;
>   }
>   
>   /* Helper to handle memset on a single page. */
> @@ -845,8 +879,10 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2)
>       const int mmu_idx = cpu_mmu_index(env, false);
>       const bool f = extract64(r0, 11, 1);
>       const bool s = extract64(r0, 10, 1);
> +    const bool cco = extract64(r0, 8, 1);
>       uintptr_t ra = GETPC();
>       S390Access srca, desta;
> +    bool ok;
>   
>       if ((f && s) || extract64(r0, 12, 4)) {
>           tcg_s390_program_interrupt(env, PGM_SPECIFICATION, GETPC());
> @@ -858,13 +894,24 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2)
>       /*
>        * TODO:
>        * - Access key handling
> -     * - CC-option with surpression of page-translation exceptions
>        * - Store r1/r2 register identifiers at real location 162
>        */
> -    srca = access_prepare(env, r2, TARGET_PAGE_SIZE, MMU_DATA_LOAD, mmu_idx,
> -                          ra);
> -    desta = access_prepare(env, r1, TARGET_PAGE_SIZE, MMU_DATA_STORE, mmu_idx,
> -                           ra);
> +    ok = access_prepare_nf(&srca, env, cco, r2, TARGET_PAGE_SIZE,
> +                           MMU_DATA_LOAD, mmu_idx, ra);
> +    if (!ok) {
> +        return 2;
> +    }
> +    ok = access_prepare_nf(&desta, env, cco, r1, TARGET_PAGE_SIZE,
> +                           MMU_DATA_STORE, mmu_idx, ra);
> +    if (!ok) {
> +        if (env->tlb_fill_exc == PGM_PROTECTION) {
> +            stq_phys(env_cpu(env)->as,
> +                     env->psa + offsetof(LowCore, trans_exc_code),
> +                     env->tlb_fill_tec);
> +            tcg_s390_program_interrupt(env, PGM_PROTECTION, ra);
> +        }
> +        return 1;
> +    }
>       access_memmove(env, &desta, &srca, ra);
>       return 0; /* data moved */
>   }
> 

As talked with Thomas off-list, there is no trusting on host==NULL
as well (see comment in struct S390Access). host==NULL simply
means we have to do individual ld/st.

The following on top should work. Not perfect, but seems to get
the job done.


 From 056b3c9f2ffd43b10d8293e7143cf7af5d1d5022 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@redhat.com>
Date: Thu, 11 Mar 2021 14:44:45 +0100
Subject: [PATCH] fixup

Signed-off-by: David Hildenbrand <david@redhat.com>
---
  target/s390x/mem_helper.c | 48 ++++++++++++++++++++++++---------------
  1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index cf741541d3..0a9b15ea90 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -130,10 +130,15 @@ typedef struct S390Access {
      int mmu_idx;
  } S390Access;
  
-static bool access_prepare_nf(S390Access *access, CPUS390XState *env,
-                              bool nofault, vaddr vaddr1, int size,
-                              MMUAccessType access_type,
-                              int mmu_idx, uintptr_t ra)
+/*
+ * With nofault=1, return the generated PGM_ exception that would have
+ * been injected into the guest (tec stored in env->tlb_fill_tec);
+ * return 0 if no exception was detected.
+ */
+static int access_prepare_nf(S390Access *access, CPUS390XState *env,
+                             bool nofault, vaddr vaddr1, int size,
+                             MMUAccessType access_type,
+                             int mmu_idx, uintptr_t ra)
  {
      void *haddr1, *haddr2 = NULL;
      int size1, size2;
@@ -145,18 +150,24 @@ static bool access_prepare_nf(S390Access *access, CPUS390XState *env,
      size1 = MIN(size, -(vaddr1 | TARGET_PAGE_MASK)),
      size2 = size - size1;
  
+    env->tlb_fill_exc = 0;
      flags = probe_access_flags(env, vaddr1, access_type, mmu_idx,
                                 nofault, &haddr1, ra);
+    if (env->tlb_fill_exc) {
+        /* We cannot rely on TLB_INVALID_MASK or haddr being NULL. */
+        return env->tlb_fill_exc;
+    }
      if (unlikely(size2)) {
          /* The access crosses page boundaries. */
          vaddr2 = wrap_address(env, vaddr1 + size1);
          flags |= probe_access_flags(env, vaddr2, access_type, mmu_idx,
                                      nofault, &haddr2, ra);
+        if (env->tlb_fill_exc) {
+            /* We cannot rely on TLB_INVALID_MASK or haddr being NULL. */
+            return env->tlb_fill_exc;
+        }
      }
  
-    if (unlikely(flags & TLB_INVALID_MASK)) {
-        return false;
-    }
      if (unlikely(flags & TLB_WATCHPOINT)) {
          /* S390 does not presently use transaction attributes. */
          cpu_check_watchpoint(env_cpu(env), vaddr1, size,
@@ -174,7 +185,7 @@ static bool access_prepare_nf(S390Access *access, CPUS390XState *env,
          .size2 = size2,
          .mmu_idx = mmu_idx
      };
-    return true;
+    return 0;
  }
  
  static S390Access access_prepare(CPUS390XState *env, vaddr vaddr, int size,
@@ -182,9 +193,9 @@ static S390Access access_prepare(CPUS390XState *env, vaddr vaddr, int size,
                                   uintptr_t ra)
  {
      S390Access ret;
-    bool ok = access_prepare_nf(&ret, env, false, vaddr, size,
+    int exc = access_prepare_nf(&ret, env, false, vaddr, size,
                                  access_type, mmu_idx, ra);
-    assert(ok);
+    assert(!exc);
      return ret;
  }
  
@@ -882,7 +893,7 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2)
      const bool cco = extract64(r0, 8, 1);
      uintptr_t ra = GETPC();
      S390Access srca, desta;
-    bool ok;
+    int exc;
  
      if ((f && s) || extract64(r0, 12, 4)) {
          tcg_s390_program_interrupt(env, PGM_SPECIFICATION, GETPC());
@@ -896,15 +907,16 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2)
       * - Access key handling
       * - Store r1/r2 register identifiers at real location 162
       */
-    ok = access_prepare_nf(&srca, env, cco, r2, TARGET_PAGE_SIZE,
-                           MMU_DATA_LOAD, mmu_idx, ra);
-    if (!ok) {
+    exc = access_prepare_nf(&srca, env, cco, r2, TARGET_PAGE_SIZE,
+                            MMU_DATA_LOAD, mmu_idx, ra);
+    if (exc) {
          return 2;
      }
-    ok = access_prepare_nf(&desta, env, cco, r1, TARGET_PAGE_SIZE,
-                           MMU_DATA_STORE, mmu_idx, ra);
-    if (!ok) {
-        if (env->tlb_fill_exc == PGM_PROTECTION) {
+    exc = access_prepare_nf(&desta, env, cco, r1, TARGET_PAGE_SIZE,
+                            MMU_DATA_STORE, mmu_idx, ra);
+    if (exc) {
+        fprintf(stderr, "Exception: %d\n", exc);
+        if (exc == PGM_PROTECTION) {
              stq_phys(env_cpu(env)->as,
                       env->psa + offsetof(LowCore, trans_exc_code),
                       env->tlb_fill_tec);
-- 
2.29.2



-- 
Thanks,

David / dhildenb

Re: [PATCH v4] target/s390x: Implement the MVPG condition-code-option bit

[Richard Henderson]

On 3/11/21 8:03 AM, David Hildenbrand wrote:
> As talked with Thomas off-list, there is no trusting on host==NULL
> as well (see comment in struct S390Access). host==NULL simply
> means we have to do individual ld/st.

I think that comment is stale with the use of probe_access instead of 
tlb_vaddr_to_host -- TLB_DIRTY is in fact handled now.


> +    env->tlb_fill_exc = 0;
>       flags = probe_access_flags(env, vaddr1, access_type, mmu_idx,
>                                  nofault, &haddr1, ra);
> +    if (env->tlb_fill_exc) {
> +        /* We cannot rely on TLB_INVALID_MASK or haddr being NULL. */
> +        return env->tlb_fill_exc;
> +    }
>       if (unlikely(size2)) {
>           /* The access crosses page boundaries. */
>           vaddr2 = wrap_address(env, vaddr1 + size1);
>           flags |= probe_access_flags(env, vaddr2, access_type, mmu_idx,
>                                       nofault, &haddr2, ra);
> +        if (env->tlb_fill_exc) {
> +            /* We cannot rely on TLB_INVALID_MASK or haddr being NULL. */
> +            return env->tlb_fill_exc;

But this is pretty clean, and definitely works.


r~

Re: [PATCH v4] target/s390x: Implement the MVPG condition-code-option bit

[David Hildenbrand]

On 11.03.21 16:58, Richard Henderson wrote:
> On 3/11/21 8:03 AM, David Hildenbrand wrote:
>> As talked with Thomas off-list, there is no trusting on host==NULL
>> as well (see comment in struct S390Access). host==NULL simply
>> means we have to do individual ld/st.
> 
> I think that comment is stale with the use of probe_access instead of
> tlb_vaddr_to_host -- TLB_DIRTY is in fact handled now.

Yes, it might be worth exploring in which cases we will still have 
issues and updating the comment. LAP is certainly one.

>> +    env->tlb_fill_exc = 0;
>>        flags = probe_access_flags(env, vaddr1, access_type, mmu_idx,
>>                                   nofault, &haddr1, ra);
>> +    if (env->tlb_fill_exc) {
>> +        /* We cannot rely on TLB_INVALID_MASK or haddr being NULL. */
>> +        return env->tlb_fill_exc;
>> +    }
>>        if (unlikely(size2)) {
>>            /* The access crosses page boundaries. */
>>            vaddr2 = wrap_address(env, vaddr1 + size1);
>>            flags |= probe_access_flags(env, vaddr2, access_type, mmu_idx,
>>                                        nofault, &haddr2, ra);
>> +        if (env->tlb_fill_exc) {
>> +            /* We cannot rely on TLB_INVALID_MASK or haddr being NULL. */
>> +            return env->tlb_fill_exc;
> 
> But this is pretty clean, and definitely works.

Except that I need to special case CONFIG_USER_ONLY .... I'll send a v5 
later, then we can discuss when looking at the full diff (including an 
addon patch to handle r1/r2).

-- 
Thanks,

David / dhildenb