[Qemu-devel] [PATCH-for-4.2 v1 0/9] s390x: MMU changes and extensions

[Qemu-devel] [PATCH-for-4.2 v1 0/9] s390x: MMU changes and extensions

[David Hildenbrand]

This series rewrites the MMU DAT translation code completely, adds EDAT2
MMU support, and implements/indicates related facilities
(ESOP-1, ESOP-2, IEP, ...) for TCG. The QEMU CPU model is updated.

This series is based on the new 4.2 compat machines from Cornelia.

Cc: Ilya Leoshkevich <iii@linux.ibm.com>

David Hildenbrand (9):
  s390x/mmu: Better ASC selection in s390_cpu_get_phys_page_debug()
  s390x/tcg: Rework MMU selection for instruction fetches
  s390x/mmu: DAT translation rewrite
  s390x/mmu: Add EDAT2 translation support
  s390x/mmu: Implement access-exception-fetch/store-indication facility
  s390x/mmu: Implement enhanced suppression-on-protection facility 2
  s390x/mmu: Implement Instruction-Execution-Protection Facility
  s390x/cpumodel: Prepare for changes of QEMU model
  s390x/cpumodel: Add new TCG features to QEMU cpu model

 hw/s390x/s390-virtio-ccw.c  |   2 +
 target/s390x/cpu.h          |  85 ++++++--
 target/s390x/gen-features.c |  10 +-
 target/s390x/helper.c       |  10 +-
 target/s390x/mem_helper.c   |  13 +-
 target/s390x/mmu_helper.c   | 410 ++++++++++++++++++------------------
 6 files changed, 294 insertions(+), 236 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH-for-4.2 v1 1/9] s390x/mmu: Better ASC selection in s390_cpu_get_phys_page_debug()

[David Hildenbrand]

Let's select the ASC before calling the function and use MMU_DATA_LOAD.
This is a preparation to:
- Remove the ASC magic depending on the access mode from mmu_translate
- Implement IEP support, where we could run into access exceptions
  trying to fetch instructions

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/helper.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index 13ae9909ad..08166558a0 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -58,7 +58,15 @@ hwaddr s390_cpu_get_phys_page_debug(CPUState *cs, vaddr vaddr)
         vaddr &= 0x7fffffff;
     }
 
-    if (mmu_translate(env, vaddr, MMU_INST_FETCH, asc, &raddr, &prot, false)) {
+    /*
+     * We want to read the code, however, not run into access exceptions
+     * (especially, IEP).
+     */
+    if (asc != PSW_ASC_HOME) {
+        asc = PSW_ASC_PRIMARY;
+    }
+
+    if (mmu_translate(env, vaddr, MMU_DATA_LOAD, asc, &raddr, &prot, false)) {
         return -1;
     }
     return raddr;
-- 
2.21.0

Re: [Qemu-devel] [PATCH-for-4.2 v1 1/9] s390x/mmu: Better ASC selection in s390_cpu_get_phys_page_debug()

[Cornelia Huck]

On Mon,  5 Aug 2019 17:29:39 +0200
David Hildenbrand <david@redhat.com> wrote:

> Let's select the ASC before calling the function and use MMU_DATA_LOAD.
> This is a preparation to:
> - Remove the ASC magic depending on the access mode from mmu_translate
> - Implement IEP support, where we could run into access exceptions

'IEP' was instruction execution protection?

>   trying to fetch instructions
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/helper.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
> index 13ae9909ad..08166558a0 100644
> --- a/target/s390x/helper.c
> +++ b/target/s390x/helper.c
> @@ -58,7 +58,15 @@ hwaddr s390_cpu_get_phys_page_debug(CPUState *cs, vaddr vaddr)
>          vaddr &= 0x7fffffff;
>      }
>  
> -    if (mmu_translate(env, vaddr, MMU_INST_FETCH, asc, &raddr, &prot, false)) {
> +    /*
> +     * We want to read the code, however, not run into access exceptions
> +     * (especially, IEP).
> +     */
> +    if (asc != PSW_ASC_HOME) {
> +        asc = PSW_ASC_PRIMARY;
> +    }

Previously, if we'd go in here specifying access register mode, we'd
hw_error() out. Now, that would be rewritten to using primary. Could
that be a problem, or do we filter out access register mode even
earlier?

(As an aside, I'm not sure the guest crashing qemu by specifying access
register mode was a good idea. Or do we get to slap the guest before
that happens?)

> +
> +    if (mmu_translate(env, vaddr, MMU_DATA_LOAD, asc, &raddr, &prot, false)) {
>          return -1;
>      }
>      return raddr;

Re: [Qemu-devel] [PATCH-for-4.2 v1 1/9] s390x/mmu: Better ASC selection in s390_cpu_get_phys_page_debug()

[David Hildenbrand]

On 08.08.19 14:57, Cornelia Huck wrote:
> On Mon,  5 Aug 2019 17:29:39 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Let's select the ASC before calling the function and use MMU_DATA_LOAD.
>> This is a preparation to:
>> - Remove the ASC magic depending on the access mode from mmu_translate
>> - Implement IEP support, where we could run into access exceptions
> 
> 'IEP' was instruction execution protection?
> 
>>   trying to fetch instructions
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  target/s390x/helper.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
>> index 13ae9909ad..08166558a0 100644
>> --- a/target/s390x/helper.c
>> +++ b/target/s390x/helper.c
>> @@ -58,7 +58,15 @@ hwaddr s390_cpu_get_phys_page_debug(CPUState *cs, vaddr vaddr)
>>          vaddr &= 0x7fffffff;
>>      }
>>  
>> -    if (mmu_translate(env, vaddr, MMU_INST_FETCH, asc, &raddr, &prot, false)) {
>> +    /*
>> +     * We want to read the code, however, not run into access exceptions
>> +     * (especially, IEP).
>> +     */
>> +    if (asc != PSW_ASC_HOME) {
>> +        asc = PSW_ASC_PRIMARY;
>> +    }
> 
> Previously, if we'd go in here specifying access register mode, we'd
> hw_error() out. Now, that would be rewritten to using primary. Could
> that be a problem, or do we filter out access register mode even
> earlier?

As this is just a debug interface it's not an issue (it actually makes
sure that we really read code and not data). Any guest that would be
trying to read data while in AR mode would immediately crash.

> 
> (As an aside, I'm not sure the guest crashing qemu by specifying access
> register mode was a good idea. Or do we get to slap the guest before
> that happens?)

I guess there isn't too much we can do - continue running the guest in a
wrong mode would lead to data corruption :/ And as it's core ISA, we
cannot really forbid switching to it. The only solution is to implement
AR mode :D

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH-for-4.2 v1 1/9] s390x/mmu: Better ASC selection in s390_cpu_get_phys_page_debug()

[Thomas Huth]

On 8/5/19 5:29 PM, David Hildenbrand wrote:
> Let's select the ASC before calling the function and use MMU_DATA_LOAD.
> This is a preparation to:
> - Remove the ASC magic depending on the access mode from mmu_translate
> - Implement IEP support, where we could run into access exceptions
>   trying to fetch instructions
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/helper.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
> index 13ae9909ad..08166558a0 100644
> --- a/target/s390x/helper.c
> +++ b/target/s390x/helper.c
> @@ -58,7 +58,15 @@ hwaddr s390_cpu_get_phys_page_debug(CPUState *cs, vaddr vaddr)
>          vaddr &= 0x7fffffff;
>      }
>  
> -    if (mmu_translate(env, vaddr, MMU_INST_FETCH, asc, &raddr, &prot, false)) {
> +    /*
> +     * We want to read the code, however, not run into access exceptions

Is this really a safe assumption here that we always use this to
translate code addresses and not data addresses? ... I don't think so.
For example with the "gva2gpa" HMP command, I'd rather expect that it
also works with the secondary space mode...?

So maybe we need a proper MemTxAttrs bit or something similar for
distinguishing instruction accesses from data accesses here?

 Thomas

Re: [Qemu-devel] [PATCH-for-4.2 v1 1/9] s390x/mmu: Better ASC selection in s390_cpu_get_phys_page_debug()

[David Hildenbrand]

On 12.08.19 09:12, Thomas Huth wrote:
> On 8/5/19 5:29 PM, David Hildenbrand wrote:
>> Let's select the ASC before calling the function and use MMU_DATA_LOAD.
>> This is a preparation to:
>> - Remove the ASC magic depending on the access mode from mmu_translate
>> - Implement IEP support, where we could run into access exceptions
>>   trying to fetch instructions
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  target/s390x/helper.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
>> index 13ae9909ad..08166558a0 100644
>> --- a/target/s390x/helper.c
>> +++ b/target/s390x/helper.c
>> @@ -58,7 +58,15 @@ hwaddr s390_cpu_get_phys_page_debug(CPUState *cs, vaddr vaddr)
>>          vaddr &= 0x7fffffff;
>>      }
>>  
>> -    if (mmu_translate(env, vaddr, MMU_INST_FETCH, asc, &raddr, &prot, false)) {
>> +    /*
>> +     * We want to read the code, however, not run into access exceptions
> 
> Is this really a safe assumption here that we always use this to
> translate code addresses and not data addresses? ... I don't think so.
> For example with the "gva2gpa" HMP command, I'd rather expect that it
> also works with the secondary space mode...?

Well, it's what current code does. I am not changing that behavior.

While it is in general broken to have a single interface to debug
code+data (which is only a problem on s390x), it makes a lot of sense if
you think about single-stepping through disassembled code using the
gdbstub. Or dumping code where you crashed.

In Linux, code+data will luckily usually have the same virtual->physical
tables, so it's not a real issue.

> 
> So maybe we need a proper MemTxAttrs bit or something similar for
> distinguishing instruction accesses from data accesses here?

There would first have to be a way to ask "get_phys_page_debug" to get
code or data for this to make sense. Right now we used it to get code.

Thanks.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH-for-4.2 v1 1/9] s390x/mmu: Better ASC selection in s390_cpu_get_phys_page_debug()

[Cornelia Huck]

On Mon, 12 Aug 2019 09:52:56 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 12.08.19 09:12, Thomas Huth wrote:
> > On 8/5/19 5:29 PM, David Hildenbrand wrote:  
> >> Let's select the ASC before calling the function and use MMU_DATA_LOAD.
> >> This is a preparation to:
> >> - Remove the ASC magic depending on the access mode from mmu_translate
> >> - Implement IEP support, where we could run into access exceptions
> >>   trying to fetch instructions
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  target/s390x/helper.c | 10 +++++++++-
> >>  1 file changed, 9 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
> >> index 13ae9909ad..08166558a0 100644
> >> --- a/target/s390x/helper.c
> >> +++ b/target/s390x/helper.c
> >> @@ -58,7 +58,15 @@ hwaddr s390_cpu_get_phys_page_debug(CPUState *cs, vaddr vaddr)
> >>          vaddr &= 0x7fffffff;
> >>      }
> >>  
> >> -    if (mmu_translate(env, vaddr, MMU_INST_FETCH, asc, &raddr, &prot, false)) {
> >> +    /*
> >> +     * We want to read the code, however, not run into access exceptions  
> > 
> > Is this really a safe assumption here that we always use this to
> > translate code addresses and not data addresses? ... I don't think so.
> > For example with the "gva2gpa" HMP command, I'd rather expect that it
> > also works with the secondary space mode...?  
> 
> Well, it's what current code does. I am not changing that behavior.

Agreed, that is not actively breaking something.

> 
> While it is in general broken to have a single interface to debug
> code+data (which is only a problem on s390x), it makes a lot of sense if
> you think about single-stepping through disassembled code using the
> gdbstub. Or dumping code where you crashed.

What about the memsave interface?

> 
> In Linux, code+data will luckily usually have the same virtual->physical
> tables, so it's not a real issue.
> 
> > 
> > So maybe we need a proper MemTxAttrs bit or something similar for
> > distinguishing instruction accesses from data accesses here?  
> 
> There would first have to be a way to ask "get_phys_page_debug" to get
> code or data for this to make sense. Right now we used it to get code.

I'm wondering if we're able to do better; but if the code/data
distinction is not considered in architecture independent code,
probably not easily.

Re: [Qemu-devel] [PATCH-for-4.2 v1 1/9] s390x/mmu: Better ASC selection in s390_cpu_get_phys_page_debug()

[David Hildenbrand]

On 12.08.19 15:40, Cornelia Huck wrote:
> On Mon, 12 Aug 2019 09:52:56 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 12.08.19 09:12, Thomas Huth wrote:
>>> On 8/5/19 5:29 PM, David Hildenbrand wrote:  
>>>> Let's select the ASC before calling the function and use MMU_DATA_LOAD.
>>>> This is a preparation to:
>>>> - Remove the ASC magic depending on the access mode from mmu_translate
>>>> - Implement IEP support, where we could run into access exceptions
>>>>   trying to fetch instructions
>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>  target/s390x/helper.c | 10 +++++++++-
>>>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
>>>> index 13ae9909ad..08166558a0 100644
>>>> --- a/target/s390x/helper.c
>>>> +++ b/target/s390x/helper.c
>>>> @@ -58,7 +58,15 @@ hwaddr s390_cpu_get_phys_page_debug(CPUState *cs, vaddr vaddr)
>>>>          vaddr &= 0x7fffffff;
>>>>      }
>>>>  
>>>> -    if (mmu_translate(env, vaddr, MMU_INST_FETCH, asc, &raddr, &prot, false)) {
>>>> +    /*
>>>> +     * We want to read the code, however, not run into access exceptions  
>>>
>>> Is this really a safe assumption here that we always use this to
>>> translate code addresses and not data addresses? ... I don't think so.
>>> For example with the "gva2gpa" HMP command, I'd rather expect that it
>>> also works with the secondary space mode...?  
>>
>> Well, it's what current code does. I am not changing that behavior.
> 
> Agreed, that is not actively breaking something.
> 
>>
>> While it is in general broken to have a single interface to debug
>> code+data (which is only a problem on s390x), it makes a lot of sense if
>> you think about single-stepping through disassembled code using the
>> gdbstub. Or dumping code where you crashed.
> 
> What about the memsave interface?

I guess the same problem:

"save to disk virtual memory dump starting at @var{addr} of size
@var{size}" -  which virtual memory (code vs. data)? These old interface
are really x86 specific (meaning: it made sense this way for x86)

I'd like to note that if our KVM guest is in AR mode, we would now no
longer be able to crash it :) (well, a nice side-effect of instruction
fetches not going via AR mode).

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH-for-4.2 v1 1/9] s390x/mmu: Better ASC selection in s390_cpu_get_phys_page_debug()

[Cornelia Huck]

On Mon, 12 Aug 2019 15:45:25 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 12.08.19 15:40, Cornelia Huck wrote:
> > On Mon, 12 Aug 2019 09:52:56 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 12.08.19 09:12, Thomas Huth wrote:  
> >>> On 8/5/19 5:29 PM, David Hildenbrand wrote:    
> >>>> Let's select the ASC before calling the function and use MMU_DATA_LOAD.
> >>>> This is a preparation to:
> >>>> - Remove the ASC magic depending on the access mode from mmu_translate
> >>>> - Implement IEP support, where we could run into access exceptions
> >>>>   trying to fetch instructions
> >>>>
> >>>> Signed-off-by: David Hildenbrand <david@redhat.com>
> >>>> ---
> >>>>  target/s390x/helper.c | 10 +++++++++-
> >>>>  1 file changed, 9 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
> >>>> index 13ae9909ad..08166558a0 100644
> >>>> --- a/target/s390x/helper.c
> >>>> +++ b/target/s390x/helper.c
> >>>> @@ -58,7 +58,15 @@ hwaddr s390_cpu_get_phys_page_debug(CPUState *cs, vaddr vaddr)
> >>>>          vaddr &= 0x7fffffff;
> >>>>      }
> >>>>  
> >>>> -    if (mmu_translate(env, vaddr, MMU_INST_FETCH, asc, &raddr, &prot, false)) {
> >>>> +    /*
> >>>> +     * We want to read the code, however, not run into access exceptions    
> >>>
> >>> Is this really a safe assumption here that we always use this to
> >>> translate code addresses and not data addresses? ... I don't think so.
> >>> For example with the "gva2gpa" HMP command, I'd rather expect that it
> >>> also works with the secondary space mode...?    
> >>
> >> Well, it's what current code does. I am not changing that behavior.  
> > 
> > Agreed, that is not actively breaking something.
> >   
> >>
> >> While it is in general broken to have a single interface to debug
> >> code+data (which is only a problem on s390x), it makes a lot of sense if
> >> you think about single-stepping through disassembled code using the
> >> gdbstub. Or dumping code where you crashed.  
> > 
> > What about the memsave interface?  
> 
> I guess the same problem:
> 
> "save to disk virtual memory dump starting at @var{addr} of size
> @var{size}" -  which virtual memory (code vs. data)? These old interface
> are really x86 specific (meaning: it made sense this way for x86)

So, the general verdict is "gnarly interface, but at least not broken
for Linux guests", I guess.

> 
> I'd like to note that if our KVM guest is in AR mode, we would now no
> longer be able to crash it :) (well, a nice side-effect of instruction
> fetches not going via AR mode).

Heh :)

Q: What do we need to consider beyond Linux guests? Probably not much,
given that they would be broken already...

Re: [Qemu-devel] [PATCH-for-4.2 v1 1/9] s390x/mmu: Better ASC selection in s390_cpu_get_phys_page_debug()

[David Hildenbrand]

On 12.08.19 15:58, Cornelia Huck wrote:
> On Mon, 12 Aug 2019 15:45:25 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 12.08.19 15:40, Cornelia Huck wrote:
>>> On Mon, 12 Aug 2019 09:52:56 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> On 12.08.19 09:12, Thomas Huth wrote:  
>>>>> On 8/5/19 5:29 PM, David Hildenbrand wrote:    
>>>>>> Let's select the ASC before calling the function and use MMU_DATA_LOAD.
>>>>>> This is a preparation to:
>>>>>> - Remove the ASC magic depending on the access mode from mmu_translate
>>>>>> - Implement IEP support, where we could run into access exceptions
>>>>>>   trying to fetch instructions
>>>>>>
>>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>>> ---
>>>>>>  target/s390x/helper.c | 10 +++++++++-
>>>>>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
>>>>>> index 13ae9909ad..08166558a0 100644
>>>>>> --- a/target/s390x/helper.c
>>>>>> +++ b/target/s390x/helper.c
>>>>>> @@ -58,7 +58,15 @@ hwaddr s390_cpu_get_phys_page_debug(CPUState *cs, vaddr vaddr)
>>>>>>          vaddr &= 0x7fffffff;
>>>>>>      }
>>>>>>  
>>>>>> -    if (mmu_translate(env, vaddr, MMU_INST_FETCH, asc, &raddr, &prot, false)) {
>>>>>> +    /*
>>>>>> +     * We want to read the code, however, not run into access exceptions    
>>>>>
>>>>> Is this really a safe assumption here that we always use this to
>>>>> translate code addresses and not data addresses? ... I don't think so.
>>>>> For example with the "gva2gpa" HMP command, I'd rather expect that it
>>>>> also works with the secondary space mode...?    
>>>>
>>>> Well, it's what current code does. I am not changing that behavior.  
>>>
>>> Agreed, that is not actively breaking something.
>>>   
>>>>
>>>> While it is in general broken to have a single interface to debug
>>>> code+data (which is only a problem on s390x), it makes a lot of sense if
>>>> you think about single-stepping through disassembled code using the
>>>> gdbstub. Or dumping code where you crashed.  
>>>
>>> What about the memsave interface?  
>>
>> I guess the same problem:
>>
>> "save to disk virtual memory dump starting at @var{addr} of size
>> @var{size}" -  which virtual memory (code vs. data)? These old interface
>> are really x86 specific (meaning: it made sense this way for x86)
> 
> So, the general verdict is "gnarly interface, but at least not broken
> for Linux guests", I guess.
> 
>>
>> I'd like to note that if our KVM guest is in AR mode, we would now no
>> longer be able to crash it :) (well, a nice side-effect of instruction
>> fetches not going via AR mode).
> 
> Heh :)
> 
> Q: What do we need to consider beyond Linux guests? Probably not much,
> given that they would be broken already...
> 

I think Linux guests only use AR mode for some instances of vDSO, but I
don't recall the details (and why it never was an issue for TCG, where
we fail hard early). Apart from that, I don't think we have to consider
other guests (kvm-unit-tests ...).

-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH-for-4.2 v1 2/9] s390x/tcg: Rework MMU selection for instruction fetches

[David Hildenbrand]

Instructions are always fetched from primary address space, except when
in home address mode. Perform the selection directly in cpu_mmu_index().

get_mem_index() is only used to perform data access, instructions are
fetched via cpu_lduw_code(), which translates to cpu_mmu_index(env, true).

We don't care about restricting the access permissions of the TLB
entries anymore, as we no longer enter PRIMARY entries into the
SECONDARY MMU.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu.h        |  7 +++++++
 target/s390x/mmu_helper.c | 15 ++-------------
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index a606547b4d..c34992bb2e 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -332,6 +332,13 @@ static inline int cpu_mmu_index(CPUS390XState *env, bool ifetch)
         return MMU_REAL_IDX;
     }
 
+    if (ifetch) {
+        if ((env->psw.mask & PSW_MASK_ASC) == PSW_ASC_HOME) {
+            return MMU_HOME_IDX;
+        }
+        return MMU_PRIMARY_IDX;
+    }
+
     switch (env->psw.mask & PSW_MASK_ASC) {
     case PSW_ASC_PRIMARY:
         return MMU_PRIMARY_IDX;
diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index 6e9c4d6151..9c0d9b5c5f 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -397,19 +397,8 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
         break;
     case PSW_ASC_SECONDARY:
         PTE_DPRINTF("%s: asc=secondary\n", __func__);
-        /*
-         * Instruction: Primary
-         * Data: Secondary
-         */
-        if (rw == MMU_INST_FETCH) {
-            r = mmu_translate_asce(env, vaddr, PSW_ASC_PRIMARY, env->cregs[1],
-                                   raddr, flags, rw, exc);
-            *flags &= ~(PAGE_READ | PAGE_WRITE);
-        } else {
-            r = mmu_translate_asce(env, vaddr, PSW_ASC_SECONDARY, env->cregs[7],
-                                   raddr, flags, rw, exc);
-            *flags &= ~(PAGE_EXEC);
-        }
+        r = mmu_translate_asce(env, vaddr, PSW_ASC_SECONDARY, env->cregs[7],
+                               raddr, flags, rw, exc);
         break;
     case PSW_ASC_ACCREG:
     default:
-- 
2.21.0

[Qemu-devel] [PATCH-for-4.2 v1 3/9] s390x/mmu: DAT translation rewrite

[David Hildenbrand]

Let's rewrite the DAT translation in a non-recursive way, similar to
arch/s390/kvm/gaccess.c:guest_translate() in KVM. This makes the
code much easier to read, compare and maintain.

Use better names for the region/section/page table entries and for the
macros to extract relevant parts from virtual address. Introduce defines
for all defined bits, this will come in handy soon.

All access exceptions now directly go via trigger_access_exception(),
at a central point. DAT protection checks are performed at a central
place.

Also, we now catch and indicate invalid addresses of page tables. All
table entries are accessed via read_table_entry().

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu.h        |  77 +++++---
 target/s390x/mem_helper.c |  13 +-
 target/s390x/mmu_helper.c | 360 +++++++++++++++++---------------------
 3 files changed, 229 insertions(+), 221 deletions(-)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index c34992bb2e..1ff14250bd 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -554,26 +554,63 @@ QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
 #define ASCE_TYPE_SEGMENT     0x00        /* segment table type               */
 #define ASCE_TABLE_LENGTH     0x03        /* region table length              */
 
-#define REGION_ENTRY_ORIGIN   (~0xfffULL) /* region/segment table origin    */
-#define REGION_ENTRY_RO       0x200       /* region/segment protection bit  */
-#define REGION_ENTRY_TF       0xc0        /* region/segment table offset    */
-#define REGION_ENTRY_INV      0x20        /* invalid region table entry     */
-#define REGION_ENTRY_TYPE_MASK 0x0c       /* region/segment table type mask */
-#define REGION_ENTRY_TYPE_R1  0x0c        /* region first table type        */
-#define REGION_ENTRY_TYPE_R2  0x08        /* region second table type       */
-#define REGION_ENTRY_TYPE_R3  0x04        /* region third table type        */
-#define REGION_ENTRY_LENGTH   0x03        /* region third length            */
-
-#define SEGMENT_ENTRY_ORIGIN  (~0x7ffULL) /* segment table origin        */
-#define SEGMENT_ENTRY_FC      0x400       /* format control              */
-#define SEGMENT_ENTRY_RO      0x200       /* page protection bit         */
-#define SEGMENT_ENTRY_INV     0x20        /* invalid segment table entry */
-
-#define VADDR_PX              0xff000     /* page index bits   */
-
-#define PAGE_RO               0x200       /* HW read-only bit  */
-#define PAGE_INVALID          0x400       /* HW invalid bit    */
-#define PAGE_RES0             0x800       /* bit must be zero  */
+#define REGION_ENTRY_ORIGIN         0xfffffffffffff000ULL
+#define REGION_ENTRY_P              0x0000000000000200ULL
+#define REGION_ENTRY_TF             0x00000000000000c0ULL
+#define REGION_ENTRY_I              0x0000000000000020ULL
+#define REGION_ENTRY_TT             0x000000000000000cULL
+#define REGION_ENTRY_TL             0x0000000000000003ULL
+
+#define REGION_ENTRY_TT_REGION1     0x000000000000000cULL
+#define REGION_ENTRY_TT_REGION2     0x0000000000000008ULL
+#define REGION_ENTRY_TT_REGION3     0x0000000000000004ULL
+
+#define REGION3_ENTRY_RFAA          0xffffffff80000000ULL
+#define REGION3_ENTRY_AV            0x0000000000010000ULL
+#define REGION3_ENTRY_ACC           0x000000000000f000ULL
+#define REGION3_ENTRY_F             0x0000000000000800ULL
+#define REGION3_ENTRY_FC            0x0000000000000400ULL
+#define REGION3_ENTRY_IEP           0x0000000000000100ULL
+#define REGION3_ENTRY_CR            0x0000000000000010ULL
+
+#define SEGMENT_ENTRY_ORIGIN        0xfffffffffffff800ULL
+#define SEGMENT_ENTRY_SFAA          0xfffffffffff80000ULL
+#define SEGMENT_ENTRY_AV            0x0000000000010000ULL
+#define SEGMENT_ENTRY_ACC           0x000000000000f000ULL
+#define SEGMENT_ENTRY_F             0x0000000000000800ULL
+#define SEGMENT_ENTRY_FC            0x0000000000000400ULL
+#define SEGMENT_ENTRY_P             0x0000000000000200ULL
+#define SEGMENT_ENTRY_IEP           0x0000000000000100ULL
+#define SEGMENT_ENTRY_I             0x0000000000000020ULL
+#define SEGMENT_ENTRY_CS            0x0000000000000010ULL
+#define SEGMENT_ENTRY_TT            0x000000000000000cULL
+
+#define SEGMENT_ENTRY_TT_REGION1    0x000000000000000cULL
+#define SEGMENT_ENTRY_TT_REGION2    0x0000000000000008ULL
+#define SEGMENT_ENTRY_TT_REGION3    0x0000000000000004ULL
+#define SEGMENT_ENTRY_TT_SEGMENT    0x0000000000000000ULL
+
+#define PAGE_ENTRY_0                0x0000000000000800ULL
+#define PAGE_ENTRY_I                0x0000000000000400ULL
+#define PAGE_ENTRY_P                0x0000000000000200ULL
+#define PAGE_ENTRY_IEP              0x0000000000000100ULL
+
+#define VADDR_REGION1_TX_MASK       0xffe0000000000000ULL
+#define VADDR_REGION2_TX_MASK       0x001ffc0000000000ULL
+#define VADDR_REGION3_TX_MASK       0x000003ff80000000ULL
+#define VADDR_SEGMENT_TX_MASK       0x000000007ff00000ULL
+#define VADDR_PAGE_TX_MASK          0x00000000000ff000ULL
+
+#define VADDR_REGION1_TX(vaddr)     (((vaddr) & VADDR_REGION1_TX_MASK) >> 53)
+#define VADDR_REGION2_TX(vaddr)     (((vaddr) & VADDR_REGION2_TX_MASK) >> 42)
+#define VADDR_REGION3_TX(vaddr)     (((vaddr) & VADDR_REGION3_TX_MASK) >> 31)
+#define VADDR_SEGMENT_TX(vaddr)     (((vaddr) & VADDR_SEGMENT_TX_MASK) >> 20)
+#define VADDR_PAGE_TX(vaddr)        (((vaddr) & VADDR_PAGE_TX_MASK) >> 12)
+
+#define VADDR_REGION1_TL(vaddr)     (((vaddr) & 0xc000000000000000ULL) >> 62)
+#define VADDR_REGION2_TL(vaddr)     (((vaddr) & 0x0018000000000000ULL) >> 51)
+#define VADDR_REGION3_TL(vaddr)     (((vaddr) & 0x0000030000000000ULL) >> 40)
+#define VADDR_SEGMENT_TL(vaddr)     (((vaddr) & 0x0000000060000000ULL) >> 29)
 
 #define SK_C                    (0x1 << 1)
 #define SK_R                    (0x1 << 2)
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 29d9eaa5b7..46cc0d66f7 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -1936,9 +1936,9 @@ void HELPER(idte)(CPUS390XState *env, uint64_t r1, uint64_t r2, uint32_t m4)
             /* addresses are not wrapped in 24/31bit mode but table index is */
             raddr = table + ((index + i) & 0x7ff) * sizeof(entry);
             entry = cpu_ldq_real_ra(env, raddr, ra);
-            if (!(entry & REGION_ENTRY_INV)) {
+            if (!(entry & REGION_ENTRY_I)) {
                 /* we are allowed to not store if already invalid */
-                entry |= REGION_ENTRY_INV;
+                entry |= REGION_ENTRY_I;
                 cpu_stq_real_ra(env, raddr, entry, ra);
             }
         }
@@ -1963,17 +1963,18 @@ void HELPER(ipte)(CPUS390XState *env, uint64_t pto, uint64_t vaddr,
 
     /* Compute the page table entry address */
     pte_addr = (pto & SEGMENT_ENTRY_ORIGIN);
-    pte_addr += (vaddr & VADDR_PX) >> 9;
+    pte_addr += VADDR_PAGE_TX(vaddr) * 8;
 
     /* Mark the page table entry as invalid */
     pte = cpu_ldq_real_ra(env, pte_addr, ra);
-    pte |= PAGE_INVALID;
+    pte |= PAGE_ENTRY_I;
+
     cpu_stq_real_ra(env, pte_addr, pte, ra);
 
     /* XXX we exploit the fact that Linux passes the exact virtual
        address here - it's not obliged to! */
     if (m4 & 1) {
-        if (vaddr & ~VADDR_PX) {
+        if (vaddr & ~VADDR_PAGE_TX_MASK) {
             tlb_flush_page(cs, page);
             /* XXX 31-bit hack */
             tlb_flush_page(cs, page ^ 0x80000000);
@@ -1982,7 +1983,7 @@ void HELPER(ipte)(CPUS390XState *env, uint64_t pto, uint64_t vaddr,
             tlb_flush(cs);
         }
     } else {
-        if (vaddr & ~VADDR_PX) {
+        if (vaddr & ~VADDR_PAGE_TX_MASK) {
             tlb_flush_page_all_cpus_synced(cs, page);
             /* XXX 31-bit hack */
             tlb_flush_page_all_cpus_synced(cs, page ^ 0x80000000);
diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index 9c0d9b5c5f..de7798284d 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -72,44 +72,6 @@ static void trigger_access_exception(CPUS390XState *env, uint32_t type,
     }
 }
 
-static void trigger_prot_fault(CPUS390XState *env, target_ulong vaddr,
-                               uint64_t asc, int rw, bool exc)
-{
-    uint64_t tec;
-
-    tec = vaddr | (rw == MMU_DATA_STORE ? FS_WRITE : FS_READ) | 4 | asc >> 46;
-
-    DPRINTF("%s: trans_exc_code=%016" PRIx64 "\n", __func__, tec);
-
-    if (!exc) {
-        return;
-    }
-
-    trigger_access_exception(env, PGM_PROTECTION, ILEN_AUTO, tec);
-}
-
-static void trigger_page_fault(CPUS390XState *env, target_ulong vaddr,
-                               uint32_t type, uint64_t asc, int rw, bool exc)
-{
-    int ilen = ILEN_AUTO;
-    uint64_t tec;
-
-    tec = vaddr | (rw == MMU_DATA_STORE ? FS_WRITE : FS_READ) | asc >> 46;
-
-    DPRINTF("%s: trans_exc_code=%016" PRIx64 "\n", __func__, tec);
-
-    if (!exc) {
-        return;
-    }
-
-    /* Code accesses have an undefined ilc.  */
-    if (rw == MMU_INST_FETCH) {
-        ilen = 2;
-    }
-
-    trigger_access_exception(env, type, ilen, tec);
-}
-
 /* check whether the address would be proteted by Low-Address Protection */
 static bool is_low_address(uint64_t addr)
 {
@@ -155,183 +117,171 @@ target_ulong mmu_real2abs(CPUS390XState *env, target_ulong raddr)
     return raddr;
 }
 
-/* Decode page table entry (normal 4KB page) */
-static int mmu_translate_pte(CPUS390XState *env, target_ulong vaddr,
-                             uint64_t asc, uint64_t pt_entry,
-                             target_ulong *raddr, int *flags, int rw, bool exc)
+static inline int read_table_entry(hwaddr gaddr, uint64_t *entry)
 {
-    if (pt_entry & PAGE_INVALID) {
-        DPRINTF("%s: PTE=0x%" PRIx64 " invalid\n", __func__, pt_entry);
-        trigger_page_fault(env, vaddr, PGM_PAGE_TRANS, asc, rw, exc);
-        return -1;
+    /*
+     * According to the PoP, these table addresses are "unpredictably real
+     * or absolute". Also, "it is unpredictable whether the address wraps
+     * or an addressing exception is recognized".
+     *
+     * We treat them as absolute addresses and don't wrap them.
+     */
+    if (address_space_read(&address_space_memory, gaddr, MEMTXATTRS_UNSPECIFIED,
+                           (uint8_t *)entry, sizeof(*entry)) != MEMTX_OK) {
+        return -EFAULT;
     }
-    if (pt_entry & PAGE_RES0) {
-        trigger_page_fault(env, vaddr, PGM_TRANS_SPEC, asc, rw, exc);
-        return -1;
-    }
-    if (pt_entry & PAGE_RO) {
-        *flags &= ~PAGE_WRITE;
-    }
-
-    *raddr = pt_entry & ASCE_ORIGIN;
-
-    PTE_DPRINTF("%s: PTE=0x%" PRIx64 "\n", __func__, pt_entry);
-
+    *entry = be64_to_cpu(*entry);
     return 0;
 }
 
-/* Decode segment table entry */
-static int mmu_translate_segment(CPUS390XState *env, target_ulong vaddr,
-                                 uint64_t asc, uint64_t st_entry,
-                                 target_ulong *raddr, int *flags, int rw,
-                                 bool exc)
-{
-    CPUState *cs = env_cpu(env);
-    uint64_t origin, offs, pt_entry;
-
-    if (st_entry & SEGMENT_ENTRY_RO) {
-        *flags &= ~PAGE_WRITE;
-    }
-
-    if ((st_entry & SEGMENT_ENTRY_FC) && (env->cregs[0] & CR0_EDAT)) {
-        /* Decode EDAT1 segment frame absolute address (1MB page) */
-        *raddr = (st_entry & 0xfffffffffff00000ULL) | (vaddr & 0xfffff);
-        PTE_DPRINTF("%s: SEG=0x%" PRIx64 "\n", __func__, st_entry);
-        return 0;
-    }
-
-    /* Look up 4KB page entry */
-    origin = st_entry & SEGMENT_ENTRY_ORIGIN;
-    offs  = (vaddr & VADDR_PX) >> 9;
-    pt_entry = ldq_phys(cs->as, origin + offs);
-    PTE_DPRINTF("%s: 0x%" PRIx64 " + 0x%" PRIx64 " => 0x%016" PRIx64 "\n",
-                __func__, origin, offs, pt_entry);
-    return mmu_translate_pte(env, vaddr, asc, pt_entry, raddr, flags, rw, exc);
-}
-
-/* Decode region table entries */
-static int mmu_translate_region(CPUS390XState *env, target_ulong vaddr,
-                                uint64_t asc, uint64_t entry, int level,
-                                target_ulong *raddr, int *flags, int rw,
-                                bool exc)
-{
-    CPUState *cs = env_cpu(env);
-    uint64_t origin, offs, new_entry;
-    const int pchks[4] = {
-        PGM_SEGMENT_TRANS, PGM_REG_THIRD_TRANS,
-        PGM_REG_SEC_TRANS, PGM_REG_FIRST_TRANS
-    };
-
-    PTE_DPRINTF("%s: 0x%" PRIx64 "\n", __func__, entry);
-
-    origin = entry & REGION_ENTRY_ORIGIN;
-    offs = (vaddr >> (17 + 11 * level / 4)) & 0x3ff8;
-
-    new_entry = ldq_phys(cs->as, origin + offs);
-    PTE_DPRINTF("%s: 0x%" PRIx64 " + 0x%" PRIx64 " => 0x%016" PRIx64 "\n",
-                __func__, origin, offs, new_entry);
-
-    if ((new_entry & REGION_ENTRY_INV) != 0) {
-        DPRINTF("%s: invalid region\n", __func__);
-        trigger_page_fault(env, vaddr, pchks[level / 4], asc, rw, exc);
-        return -1;
-    }
-
-    if ((new_entry & REGION_ENTRY_TYPE_MASK) != level) {
-        trigger_page_fault(env, vaddr, PGM_TRANS_SPEC, asc, rw, exc);
-        return -1;
-    }
-
-    if (level == ASCE_TYPE_SEGMENT) {
-        return mmu_translate_segment(env, vaddr, asc, new_entry, raddr, flags,
-                                     rw, exc);
-    }
-
-    /* Check region table offset and length */
-    offs = (vaddr >> (28 + 11 * (level - 4) / 4)) & 3;
-    if (offs < ((new_entry & REGION_ENTRY_TF) >> 6)
-        || offs > (new_entry & REGION_ENTRY_LENGTH)) {
-        DPRINTF("%s: invalid offset or len (%lx)\n", __func__, new_entry);
-        trigger_page_fault(env, vaddr, pchks[level / 4 - 1], asc, rw, exc);
-        return -1;
-    }
-
-    if ((env->cregs[0] & CR0_EDAT) && (new_entry & REGION_ENTRY_RO)) {
-        *flags &= ~PAGE_WRITE;
-    }
-
-    /* yet another region */
-    return mmu_translate_region(env, vaddr, asc, new_entry, level - 4,
-                                raddr, flags, rw, exc);
-}
-
 static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
-                              uint64_t asc, uint64_t asce, target_ulong *raddr,
-                              int *flags, int rw, bool exc)
+                              uint64_t asce, target_ulong *raddr, int *flags)
 {
-    int level;
-    int r;
+    const bool edat1 = (env->cregs[0] & CR0_EDAT) &&
+                       s390_has_feat(S390_FEAT_EDAT);
+    const int asce_tl = asce & ASCE_TABLE_LENGTH;
+    const int asce_p = asce & ASCE_PRIVATE_SPACE;
+    hwaddr gaddr = asce & ASCE_ORIGIN;
+    uint64_t entry;
 
     if (asce & ASCE_REAL_SPACE) {
-        /* direct mapping */
         *raddr = vaddr;
         return 0;
     }
 
-    level = asce & ASCE_TYPE_MASK;
-    switch (level) {
+    switch (asce & ASCE_TYPE_MASK) {
     case ASCE_TYPE_REGION1:
-        if ((vaddr >> 62) > (asce & ASCE_TABLE_LENGTH)) {
-            trigger_page_fault(env, vaddr, PGM_REG_FIRST_TRANS, asc, rw, exc);
-            return -1;
+        if (VADDR_REGION1_TL(vaddr) > asce_tl) {
+            return PGM_REG_FIRST_TRANS;
         }
+        gaddr += VADDR_REGION1_TX(vaddr) * 8;
         break;
     case ASCE_TYPE_REGION2:
-        if (vaddr & 0xffe0000000000000ULL) {
-            DPRINTF("%s: vaddr doesn't fit 0x%16" PRIx64
-                    " 0xffe0000000000000ULL\n", __func__, vaddr);
-            trigger_page_fault(env, vaddr, PGM_ASCE_TYPE, asc, rw, exc);
-            return -1;
+        if (VADDR_REGION1_TX(vaddr)) {
+            return PGM_ASCE_TYPE;
         }
-        if ((vaddr >> 51 & 3) > (asce & ASCE_TABLE_LENGTH)) {
-            trigger_page_fault(env, vaddr, PGM_REG_SEC_TRANS, asc, rw, exc);
-            return -1;
+        if (VADDR_REGION2_TL(vaddr) > asce_tl) {
+            return PGM_REG_SEC_TRANS;
         }
+        gaddr += VADDR_REGION2_TX(vaddr) * 8;
         break;
     case ASCE_TYPE_REGION3:
-        if (vaddr & 0xfffffc0000000000ULL) {
-            DPRINTF("%s: vaddr doesn't fit 0x%16" PRIx64
-                    " 0xfffffc0000000000ULL\n", __func__, vaddr);
-            trigger_page_fault(env, vaddr, PGM_ASCE_TYPE, asc, rw, exc);
-            return -1;
+        if (VADDR_REGION1_TX(vaddr) || VADDR_REGION2_TX(vaddr)) {
+            return PGM_ASCE_TYPE;
         }
-        if ((vaddr >> 40 & 3) > (asce & ASCE_TABLE_LENGTH)) {
-            trigger_page_fault(env, vaddr, PGM_REG_THIRD_TRANS, asc, rw, exc);
-            return -1;
+        if (VADDR_REGION3_TL(vaddr) > asce_tl) {
+            return PGM_REG_THIRD_TRANS;
         }
+        gaddr += VADDR_REGION3_TX(vaddr) * 8;
         break;
     case ASCE_TYPE_SEGMENT:
-        if (vaddr & 0xffffffff80000000ULL) {
-            DPRINTF("%s: vaddr doesn't fit 0x%16" PRIx64
-                    " 0xffffffff80000000ULL\n", __func__, vaddr);
-            trigger_page_fault(env, vaddr, PGM_ASCE_TYPE, asc, rw, exc);
-            return -1;
+        if (VADDR_REGION1_TX(vaddr) || VADDR_REGION2_TX(vaddr) ||
+            VADDR_REGION3_TX(vaddr)) {
+            return PGM_ASCE_TYPE;
         }
-        if ((vaddr >> 29 & 3) > (asce & ASCE_TABLE_LENGTH)) {
-            trigger_page_fault(env, vaddr, PGM_SEGMENT_TRANS, asc, rw, exc);
-            return -1;
+        if (VADDR_SEGMENT_TL(vaddr) > asce_tl) {
+            return PGM_SEGMENT_TRANS;
         }
+        gaddr += VADDR_SEGMENT_TX(vaddr) * 8;
+        break;
+    }
+    switch (asce & ASCE_TYPE_MASK) {
+    case ASCE_TYPE_REGION1:
+        if (read_table_entry(gaddr, &entry)) {
+            return PGM_ADDRESSING;
+        }
+        if (entry & REGION_ENTRY_I) {
+            return PGM_REG_FIRST_TRANS;
+        }
+        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION1) {
+            return PGM_TRANS_SPEC;
+        }
+        if (VADDR_REGION2_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
+            VADDR_REGION2_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
+            return PGM_REG_SEC_TRANS;
+        }
+        if (edat1 && (entry & REGION_ENTRY_P)) {
+            *flags &= ~PAGE_WRITE;
+        }
+        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_REGION2_TX(vaddr) * 8;
+        /* FALL THROUGH */
+    case ASCE_TYPE_REGION2:
+        if (read_table_entry(gaddr, &entry)) {
+            return PGM_ADDRESSING;
+        }
+        if (entry & REGION_ENTRY_I) {
+            return PGM_REG_SEC_TRANS;
+        }
+        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION2) {
+            return PGM_TRANS_SPEC;
+        }
+        if (VADDR_REGION3_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
+            VADDR_REGION3_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
+            return PGM_REG_THIRD_TRANS;
+        }
+        if (edat1 && (entry & REGION_ENTRY_P)) {
+            *flags &= ~PAGE_WRITE;
+        }
+        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_REGION3_TX(vaddr) * 8;
+        /* FALL THROUGH */
+    case ASCE_TYPE_REGION3:
+        if (read_table_entry(gaddr, &entry)) {
+            return PGM_ADDRESSING;
+        }
+        if (entry & REGION_ENTRY_I) {
+            return PGM_REG_THIRD_TRANS;
+        }
+        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION3) {
+            return PGM_TRANS_SPEC;
+        }
+        if (edat1 && (entry & REGION_ENTRY_P)) {
+            *flags &= ~PAGE_WRITE;
+        }
+        if (VADDR_SEGMENT_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
+            VADDR_SEGMENT_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
+            return PGM_SEGMENT_TRANS;
+        }
+        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_SEGMENT_TX(vaddr) * 8;
+        /* FALL THROUGH */
+    case ASCE_TYPE_SEGMENT:
+        if (read_table_entry(gaddr, &entry)) {
+            return PGM_ADDRESSING;
+        }
+        if (entry & SEGMENT_ENTRY_I) {
+            return PGM_SEGMENT_TRANS;
+        }
+        if ((entry & SEGMENT_ENTRY_TT) != SEGMENT_ENTRY_TT_SEGMENT) {
+            return PGM_TRANS_SPEC;
+        }
+        if ((entry & SEGMENT_ENTRY_CS) && asce_p) {
+            return PGM_TRANS_SPEC;
+        }
+        if (entry & SEGMENT_ENTRY_P) {
+            *flags &= ~PAGE_WRITE;
+        }
+        if (edat1 && (entry & SEGMENT_ENTRY_FC)) {
+            *raddr = entry & SEGMENT_ENTRY_SFAA;
+            return 0;
+        }
+        gaddr = (entry & SEGMENT_ENTRY_ORIGIN) + VADDR_PAGE_TX(vaddr) * 8;
         break;
     }
 
-    r = mmu_translate_region(env, vaddr, asc, asce, level, raddr, flags, rw,
-                             exc);
-    if (!r && rw == MMU_DATA_STORE && !(*flags & PAGE_WRITE)) {
-        trigger_prot_fault(env, vaddr, asc, rw, exc);
-        return -1;
+    if (read_table_entry(gaddr, &entry)) {
+        return PGM_ADDRESSING;
+    }
+    if (entry & PAGE_ENTRY_I) {
+        return PGM_PAGE_TRANS;
+    }
+    if (entry & PAGE_ENTRY_0) {
+        return PGM_TRANS_SPEC;
+    }
+    if (entry & PAGE_ENTRY_P) {
+        *flags &= ~PAGE_WRITE;
     }
 
-    return r;
+    *raddr = entry & TARGET_PAGE_MASK;
+    return 0;
 }
 
 /**
@@ -347,9 +297,14 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
 int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
                   target_ulong *raddr, int *flags, bool exc)
 {
+    /* Code accesses have an undefined ilc, let's use 2 bytes. */
+    const int ilen = (rw == MMU_INST_FETCH) ? 2 : ILEN_AUTO;
+    uint64_t tec = (vaddr & TARGET_PAGE_MASK) | (asc >> 46) |
+                   (rw == MMU_DATA_STORE ? FS_WRITE : FS_READ);
+    uint64_t asce;
     static S390SKeysState *ss;
     static S390SKeysClass *skeyclass;
-    int r = -1;
+    int r;
     uint8_t key;
 
     if (unlikely(!ss)) {
@@ -380,25 +335,21 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
 
     if (!(env->psw.mask & PSW_MASK_DAT)) {
         *raddr = vaddr;
-        r = 0;
-        goto out;
+        goto nodat;
     }
 
     switch (asc) {
     case PSW_ASC_PRIMARY:
         PTE_DPRINTF("%s: asc=primary\n", __func__);
-        r = mmu_translate_asce(env, vaddr, asc, env->cregs[1], raddr, flags,
-                               rw, exc);
+        asce = env->cregs[1];
         break;
     case PSW_ASC_HOME:
         PTE_DPRINTF("%s: asc=home\n", __func__);
-        r = mmu_translate_asce(env, vaddr, asc, env->cregs[13], raddr, flags,
-                               rw, exc);
+        asce = env->cregs[13];
         break;
     case PSW_ASC_SECONDARY:
         PTE_DPRINTF("%s: asc=secondary\n", __func__);
-        r = mmu_translate_asce(env, vaddr, PSW_ASC_SECONDARY, env->cregs[7],
-                               raddr, flags, rw, exc);
+        asce = env->cregs[7];
         break;
     case PSW_ASC_ACCREG:
     default:
@@ -406,11 +357,30 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
         break;
     }
 
- out:
+    /* perform the translation */
+    r = mmu_translate_asce(env, vaddr, asce, raddr, flags);
+    if (r) {
+        if (exc) {
+            trigger_access_exception(env, r, ilen, tec);
+        }
+        return -1;
+    }
+
+    /* check for DAT protection */
+    if (rw == MMU_DATA_STORE && !(*flags & PAGE_WRITE)) {
+        if (exc) {
+            /* DAT sets bit 61 only */
+            tec |= 0x4;
+            trigger_access_exception(env, PGM_PROTECTION, ilen, tec);
+        }
+        return -1;
+    }
+
+nodat:
     /* Convert real address -> absolute address */
     *raddr = mmu_real2abs(env, *raddr);
 
-    if (r == 0 && *raddr < ram_size) {
+    if (*raddr < ram_size) {
         if (skeyclass->get_skeys(ss, *raddr / TARGET_PAGE_SIZE, 1, &key)) {
             trace_get_skeys_nonzero(r);
             return 0;
@@ -430,7 +400,7 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
         }
     }
 
-    return r;
+    return 0;
 }
 
 /**
-- 
2.21.0

Re: [Qemu-devel] [PATCH-for-4.2 v1 3/9] s390x/mmu: DAT translation rewrite

[Thomas Huth]

On 8/5/19 5:29 PM, David Hildenbrand wrote:
> Let's rewrite the DAT translation in a non-recursive way, similar to
> arch/s390/kvm/gaccess.c:guest_translate() in KVM. This makes the
> code much easier to read, compare and maintain.
> 
> Use better names for the region/section/page table entries and for the
> macros to extract relevant parts from virtual address. Introduce defines
> for all defined bits, this will come in handy soon.
> 
> All access exceptions now directly go via trigger_access_exception(),
> at a central point. DAT protection checks are performed at a central
> place.
> 
> Also, we now catch and indicate invalid addresses of page tables. All
> table entries are accessed via read_table_entry().
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/cpu.h        |  77 +++++---
>  target/s390x/mem_helper.c |  13 +-
>  target/s390x/mmu_helper.c | 360 +++++++++++++++++---------------------
>  3 files changed, 229 insertions(+), 221 deletions(-)
> 
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index c34992bb2e..1ff14250bd 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -554,26 +554,63 @@ QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
>  #define ASCE_TYPE_SEGMENT     0x00        /* segment table type               */
>  #define ASCE_TABLE_LENGTH     0x03        /* region table length              */
>  
> -#define REGION_ENTRY_ORIGIN   (~0xfffULL) /* region/segment table origin    */
> -#define REGION_ENTRY_RO       0x200       /* region/segment protection bit  */
> -#define REGION_ENTRY_TF       0xc0        /* region/segment table offset    */
> -#define REGION_ENTRY_INV      0x20        /* invalid region table entry     */
> -#define REGION_ENTRY_TYPE_MASK 0x0c       /* region/segment table type mask */
> -#define REGION_ENTRY_TYPE_R1  0x0c        /* region first table type        */
> -#define REGION_ENTRY_TYPE_R2  0x08        /* region second table type       */
> -#define REGION_ENTRY_TYPE_R3  0x04        /* region third table type        */
> -#define REGION_ENTRY_LENGTH   0x03        /* region third length            */
> -
> -#define SEGMENT_ENTRY_ORIGIN  (~0x7ffULL) /* segment table origin        */
> -#define SEGMENT_ENTRY_FC      0x400       /* format control              */
> -#define SEGMENT_ENTRY_RO      0x200       /* page protection bit         */
> -#define SEGMENT_ENTRY_INV     0x20        /* invalid segment table entry */
> -
> -#define VADDR_PX              0xff000     /* page index bits   */
> -
> -#define PAGE_RO               0x200       /* HW read-only bit  */
> -#define PAGE_INVALID          0x400       /* HW invalid bit    */
> -#define PAGE_RES0             0x800       /* bit must be zero  */
> +#define REGION_ENTRY_ORIGIN         0xfffffffffffff000ULL
> +#define REGION_ENTRY_P              0x0000000000000200ULL
> +#define REGION_ENTRY_TF             0x00000000000000c0ULL
> +#define REGION_ENTRY_I              0x0000000000000020ULL
> +#define REGION_ENTRY_TT             0x000000000000000cULL
> +#define REGION_ENTRY_TL             0x0000000000000003ULL
> +
> +#define REGION_ENTRY_TT_REGION1     0x000000000000000cULL
> +#define REGION_ENTRY_TT_REGION2     0x0000000000000008ULL
> +#define REGION_ENTRY_TT_REGION3     0x0000000000000004ULL
> +
> +#define REGION3_ENTRY_RFAA          0xffffffff80000000ULL
> +#define REGION3_ENTRY_AV            0x0000000000010000ULL
> +#define REGION3_ENTRY_ACC           0x000000000000f000ULL
> +#define REGION3_ENTRY_F             0x0000000000000800ULL
> +#define REGION3_ENTRY_FC            0x0000000000000400ULL
> +#define REGION3_ENTRY_IEP           0x0000000000000100ULL
> +#define REGION3_ENTRY_CR            0x0000000000000010ULL
> +
> +#define SEGMENT_ENTRY_ORIGIN        0xfffffffffffff800ULL
> +#define SEGMENT_ENTRY_SFAA          0xfffffffffff80000ULL
> +#define SEGMENT_ENTRY_AV            0x0000000000010000ULL
> +#define SEGMENT_ENTRY_ACC           0x000000000000f000ULL
> +#define SEGMENT_ENTRY_F             0x0000000000000800ULL
> +#define SEGMENT_ENTRY_FC            0x0000000000000400ULL
> +#define SEGMENT_ENTRY_P             0x0000000000000200ULL
> +#define SEGMENT_ENTRY_IEP           0x0000000000000100ULL
> +#define SEGMENT_ENTRY_I             0x0000000000000020ULL
> +#define SEGMENT_ENTRY_CS            0x0000000000000010ULL
> +#define SEGMENT_ENTRY_TT            0x000000000000000cULL
> +
> +#define SEGMENT_ENTRY_TT_REGION1    0x000000000000000cULL
> +#define SEGMENT_ENTRY_TT_REGION2    0x0000000000000008ULL
> +#define SEGMENT_ENTRY_TT_REGION3    0x0000000000000004ULL
> +#define SEGMENT_ENTRY_TT_SEGMENT    0x0000000000000000ULL
> +
> +#define PAGE_ENTRY_0                0x0000000000000800ULL
> +#define PAGE_ENTRY_I                0x0000000000000400ULL
> +#define PAGE_ENTRY_P                0x0000000000000200ULL
> +#define PAGE_ENTRY_IEP              0x0000000000000100ULL
> +
> +#define VADDR_REGION1_TX_MASK       0xffe0000000000000ULL
> +#define VADDR_REGION2_TX_MASK       0x001ffc0000000000ULL
> +#define VADDR_REGION3_TX_MASK       0x000003ff80000000ULL
> +#define VADDR_SEGMENT_TX_MASK       0x000000007ff00000ULL
> +#define VADDR_PAGE_TX_MASK          0x00000000000ff000ULL
> +
> +#define VADDR_REGION1_TX(vaddr)     (((vaddr) & VADDR_REGION1_TX_MASK) >> 53)
> +#define VADDR_REGION2_TX(vaddr)     (((vaddr) & VADDR_REGION2_TX_MASK) >> 42)
> +#define VADDR_REGION3_TX(vaddr)     (((vaddr) & VADDR_REGION3_TX_MASK) >> 31)
> +#define VADDR_SEGMENT_TX(vaddr)     (((vaddr) & VADDR_SEGMENT_TX_MASK) >> 20)
> +#define VADDR_PAGE_TX(vaddr)        (((vaddr) & VADDR_PAGE_TX_MASK) >> 12)
> +
> +#define VADDR_REGION1_TL(vaddr)     (((vaddr) & 0xc000000000000000ULL) >> 62)
> +#define VADDR_REGION2_TL(vaddr)     (((vaddr) & 0x0018000000000000ULL) >> 51)
> +#define VADDR_REGION3_TL(vaddr)     (((vaddr) & 0x0000030000000000ULL) >> 40)
> +#define VADDR_SEGMENT_TL(vaddr)     (((vaddr) & 0x0000000060000000ULL) >> 29)

Ugh, this patch is quite big, and you're doing multiple things at once
here, e.g. renaming macros from PAGE_INVALID to PAGE_ENTRY_I ... could
you maybe split this up in multiple patches instead? Also, is this
complete rewrite really necessary, just to match your personal taste? I
think it would be easier to review if you'd just fix the current code
instead if necessary...

 Thomas

Re: [Qemu-devel] [PATCH-for-4.2 v1 3/9] s390x/mmu: DAT translation rewrite

[David Hildenbrand]

On 12.08.19 09:20, Thomas Huth wrote:
> On 8/5/19 5:29 PM, David Hildenbrand wrote:
>> Let's rewrite the DAT translation in a non-recursive way, similar to
>> arch/s390/kvm/gaccess.c:guest_translate() in KVM. This makes the
>> code much easier to read, compare and maintain.
>>
>> Use better names for the region/section/page table entries and for the
>> macros to extract relevant parts from virtual address. Introduce defines
>> for all defined bits, this will come in handy soon.
>>
>> All access exceptions now directly go via trigger_access_exception(),
>> at a central point. DAT protection checks are performed at a central
>> place.
>>
>> Also, we now catch and indicate invalid addresses of page tables. All
>> table entries are accessed via read_table_entry().
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  target/s390x/cpu.h        |  77 +++++---
>>  target/s390x/mem_helper.c |  13 +-
>>  target/s390x/mmu_helper.c | 360 +++++++++++++++++---------------------
>>  3 files changed, 229 insertions(+), 221 deletions(-)
>>
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index c34992bb2e..1ff14250bd 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -554,26 +554,63 @@ QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
>>  #define ASCE_TYPE_SEGMENT     0x00        /* segment table type               */
>>  #define ASCE_TABLE_LENGTH     0x03        /* region table length              */
>>  
>> -#define REGION_ENTRY_ORIGIN   (~0xfffULL) /* region/segment table origin    */
>> -#define REGION_ENTRY_RO       0x200       /* region/segment protection bit  */
>> -#define REGION_ENTRY_TF       0xc0        /* region/segment table offset    */
>> -#define REGION_ENTRY_INV      0x20        /* invalid region table entry     */
>> -#define REGION_ENTRY_TYPE_MASK 0x0c       /* region/segment table type mask */
>> -#define REGION_ENTRY_TYPE_R1  0x0c        /* region first table type        */
>> -#define REGION_ENTRY_TYPE_R2  0x08        /* region second table type       */
>> -#define REGION_ENTRY_TYPE_R3  0x04        /* region third table type        */
>> -#define REGION_ENTRY_LENGTH   0x03        /* region third length            */
>> -
>> -#define SEGMENT_ENTRY_ORIGIN  (~0x7ffULL) /* segment table origin        */
>> -#define SEGMENT_ENTRY_FC      0x400       /* format control              */
>> -#define SEGMENT_ENTRY_RO      0x200       /* page protection bit         */
>> -#define SEGMENT_ENTRY_INV     0x20        /* invalid segment table entry */
>> -
>> -#define VADDR_PX              0xff000     /* page index bits   */
>> -
>> -#define PAGE_RO               0x200       /* HW read-only bit  */
>> -#define PAGE_INVALID          0x400       /* HW invalid bit    */
>> -#define PAGE_RES0             0x800       /* bit must be zero  */
>> +#define REGION_ENTRY_ORIGIN         0xfffffffffffff000ULL
>> +#define REGION_ENTRY_P              0x0000000000000200ULL
>> +#define REGION_ENTRY_TF             0x00000000000000c0ULL
>> +#define REGION_ENTRY_I              0x0000000000000020ULL
>> +#define REGION_ENTRY_TT             0x000000000000000cULL
>> +#define REGION_ENTRY_TL             0x0000000000000003ULL
>> +
>> +#define REGION_ENTRY_TT_REGION1     0x000000000000000cULL
>> +#define REGION_ENTRY_TT_REGION2     0x0000000000000008ULL
>> +#define REGION_ENTRY_TT_REGION3     0x0000000000000004ULL
>> +
>> +#define REGION3_ENTRY_RFAA          0xffffffff80000000ULL
>> +#define REGION3_ENTRY_AV            0x0000000000010000ULL
>> +#define REGION3_ENTRY_ACC           0x000000000000f000ULL
>> +#define REGION3_ENTRY_F             0x0000000000000800ULL
>> +#define REGION3_ENTRY_FC            0x0000000000000400ULL
>> +#define REGION3_ENTRY_IEP           0x0000000000000100ULL
>> +#define REGION3_ENTRY_CR            0x0000000000000010ULL
>> +
>> +#define SEGMENT_ENTRY_ORIGIN        0xfffffffffffff800ULL
>> +#define SEGMENT_ENTRY_SFAA          0xfffffffffff80000ULL
>> +#define SEGMENT_ENTRY_AV            0x0000000000010000ULL
>> +#define SEGMENT_ENTRY_ACC           0x000000000000f000ULL
>> +#define SEGMENT_ENTRY_F             0x0000000000000800ULL
>> +#define SEGMENT_ENTRY_FC            0x0000000000000400ULL
>> +#define SEGMENT_ENTRY_P             0x0000000000000200ULL
>> +#define SEGMENT_ENTRY_IEP           0x0000000000000100ULL
>> +#define SEGMENT_ENTRY_I             0x0000000000000020ULL
>> +#define SEGMENT_ENTRY_CS            0x0000000000000010ULL
>> +#define SEGMENT_ENTRY_TT            0x000000000000000cULL
>> +
>> +#define SEGMENT_ENTRY_TT_REGION1    0x000000000000000cULL
>> +#define SEGMENT_ENTRY_TT_REGION2    0x0000000000000008ULL
>> +#define SEGMENT_ENTRY_TT_REGION3    0x0000000000000004ULL
>> +#define SEGMENT_ENTRY_TT_SEGMENT    0x0000000000000000ULL
>> +
>> +#define PAGE_ENTRY_0                0x0000000000000800ULL
>> +#define PAGE_ENTRY_I                0x0000000000000400ULL
>> +#define PAGE_ENTRY_P                0x0000000000000200ULL
>> +#define PAGE_ENTRY_IEP              0x0000000000000100ULL
>> +
>> +#define VADDR_REGION1_TX_MASK       0xffe0000000000000ULL
>> +#define VADDR_REGION2_TX_MASK       0x001ffc0000000000ULL
>> +#define VADDR_REGION3_TX_MASK       0x000003ff80000000ULL
>> +#define VADDR_SEGMENT_TX_MASK       0x000000007ff00000ULL
>> +#define VADDR_PAGE_TX_MASK          0x00000000000ff000ULL
>> +
>> +#define VADDR_REGION1_TX(vaddr)     (((vaddr) & VADDR_REGION1_TX_MASK) >> 53)
>> +#define VADDR_REGION2_TX(vaddr)     (((vaddr) & VADDR_REGION2_TX_MASK) >> 42)
>> +#define VADDR_REGION3_TX(vaddr)     (((vaddr) & VADDR_REGION3_TX_MASK) >> 31)
>> +#define VADDR_SEGMENT_TX(vaddr)     (((vaddr) & VADDR_SEGMENT_TX_MASK) >> 20)
>> +#define VADDR_PAGE_TX(vaddr)        (((vaddr) & VADDR_PAGE_TX_MASK) >> 12)
>> +
>> +#define VADDR_REGION1_TL(vaddr)     (((vaddr) & 0xc000000000000000ULL) >> 62)
>> +#define VADDR_REGION2_TL(vaddr)     (((vaddr) & 0x0018000000000000ULL) >> 51)
>> +#define VADDR_REGION3_TL(vaddr)     (((vaddr) & 0x0000030000000000ULL) >> 40)
>> +#define VADDR_SEGMENT_TL(vaddr)     (((vaddr) & 0x0000000060000000ULL) >> 29)
> 
> Ugh, this patch is quite big, and you're doing multiple things at once
> here, e.g. renaming macros from PAGE_INVALID to PAGE_ENTRY_I ... could

I could split out renaming the macros, however, besides a lot of work on
my side this won't really make a huge difference here.

> you maybe split this up in multiple patches instead? Also, is this
> complete rewrite really necessary, just to match your personal taste? I

I'm not gonna use lipstick on a pig ;) No honestly, the recursion is
just nasty and we can now easily compare the current code with
arch/s390/kvm/gaccess.c:guest_translate()

> think it would be easier to review if you'd just fix the current code
> instead if necessary...

No, won't do that.

Still thanks for having a look.


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH-for-4.2 v1 3/9] s390x/mmu: DAT translation rewrite

[David Hildenbrand]

On 12.08.19 09:43, David Hildenbrand wrote:
> On 12.08.19 09:20, Thomas Huth wrote:
>> On 8/5/19 5:29 PM, David Hildenbrand wrote:
>>> Let's rewrite the DAT translation in a non-recursive way, similar to
>>> arch/s390/kvm/gaccess.c:guest_translate() in KVM. This makes the
>>> code much easier to read, compare and maintain.
>>>
>>> Use better names for the region/section/page table entries and for the
>>> macros to extract relevant parts from virtual address. Introduce defines
>>> for all defined bits, this will come in handy soon.
>>>
>>> All access exceptions now directly go via trigger_access_exception(),
>>> at a central point. DAT protection checks are performed at a central
>>> place.
>>>
>>> Also, we now catch and indicate invalid addresses of page tables. All
>>> table entries are accessed via read_table_entry().
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>  target/s390x/cpu.h        |  77 +++++---
>>>  target/s390x/mem_helper.c |  13 +-
>>>  target/s390x/mmu_helper.c | 360 +++++++++++++++++---------------------
>>>  3 files changed, 229 insertions(+), 221 deletions(-)
>>>
>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>>> index c34992bb2e..1ff14250bd 100644
>>> --- a/target/s390x/cpu.h
>>> +++ b/target/s390x/cpu.h
>>> @@ -554,26 +554,63 @@ QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
>>>  #define ASCE_TYPE_SEGMENT     0x00        /* segment table type               */
>>>  #define ASCE_TABLE_LENGTH     0x03        /* region table length              */
>>>  
>>> -#define REGION_ENTRY_ORIGIN   (~0xfffULL) /* region/segment table origin    */
>>> -#define REGION_ENTRY_RO       0x200       /* region/segment protection bit  */
>>> -#define REGION_ENTRY_TF       0xc0        /* region/segment table offset    */
>>> -#define REGION_ENTRY_INV      0x20        /* invalid region table entry     */
>>> -#define REGION_ENTRY_TYPE_MASK 0x0c       /* region/segment table type mask */
>>> -#define REGION_ENTRY_TYPE_R1  0x0c        /* region first table type        */
>>> -#define REGION_ENTRY_TYPE_R2  0x08        /* region second table type       */
>>> -#define REGION_ENTRY_TYPE_R3  0x04        /* region third table type        */
>>> -#define REGION_ENTRY_LENGTH   0x03        /* region third length            */
>>> -
>>> -#define SEGMENT_ENTRY_ORIGIN  (~0x7ffULL) /* segment table origin        */
>>> -#define SEGMENT_ENTRY_FC      0x400       /* format control              */
>>> -#define SEGMENT_ENTRY_RO      0x200       /* page protection bit         */
>>> -#define SEGMENT_ENTRY_INV     0x20        /* invalid segment table entry */
>>> -
>>> -#define VADDR_PX              0xff000     /* page index bits   */
>>> -
>>> -#define PAGE_RO               0x200       /* HW read-only bit  */
>>> -#define PAGE_INVALID          0x400       /* HW invalid bit    */
>>> -#define PAGE_RES0             0x800       /* bit must be zero  */
>>> +#define REGION_ENTRY_ORIGIN         0xfffffffffffff000ULL
>>> +#define REGION_ENTRY_P              0x0000000000000200ULL
>>> +#define REGION_ENTRY_TF             0x00000000000000c0ULL
>>> +#define REGION_ENTRY_I              0x0000000000000020ULL
>>> +#define REGION_ENTRY_TT             0x000000000000000cULL
>>> +#define REGION_ENTRY_TL             0x0000000000000003ULL
>>> +
>>> +#define REGION_ENTRY_TT_REGION1     0x000000000000000cULL
>>> +#define REGION_ENTRY_TT_REGION2     0x0000000000000008ULL
>>> +#define REGION_ENTRY_TT_REGION3     0x0000000000000004ULL
>>> +
>>> +#define REGION3_ENTRY_RFAA          0xffffffff80000000ULL
>>> +#define REGION3_ENTRY_AV            0x0000000000010000ULL
>>> +#define REGION3_ENTRY_ACC           0x000000000000f000ULL
>>> +#define REGION3_ENTRY_F             0x0000000000000800ULL
>>> +#define REGION3_ENTRY_FC            0x0000000000000400ULL
>>> +#define REGION3_ENTRY_IEP           0x0000000000000100ULL
>>> +#define REGION3_ENTRY_CR            0x0000000000000010ULL
>>> +
>>> +#define SEGMENT_ENTRY_ORIGIN        0xfffffffffffff800ULL
>>> +#define SEGMENT_ENTRY_SFAA          0xfffffffffff80000ULL
>>> +#define SEGMENT_ENTRY_AV            0x0000000000010000ULL
>>> +#define SEGMENT_ENTRY_ACC           0x000000000000f000ULL
>>> +#define SEGMENT_ENTRY_F             0x0000000000000800ULL
>>> +#define SEGMENT_ENTRY_FC            0x0000000000000400ULL
>>> +#define SEGMENT_ENTRY_P             0x0000000000000200ULL
>>> +#define SEGMENT_ENTRY_IEP           0x0000000000000100ULL
>>> +#define SEGMENT_ENTRY_I             0x0000000000000020ULL
>>> +#define SEGMENT_ENTRY_CS            0x0000000000000010ULL
>>> +#define SEGMENT_ENTRY_TT            0x000000000000000cULL
>>> +
>>> +#define SEGMENT_ENTRY_TT_REGION1    0x000000000000000cULL
>>> +#define SEGMENT_ENTRY_TT_REGION2    0x0000000000000008ULL
>>> +#define SEGMENT_ENTRY_TT_REGION3    0x0000000000000004ULL
>>> +#define SEGMENT_ENTRY_TT_SEGMENT    0x0000000000000000ULL
>>> +
>>> +#define PAGE_ENTRY_0                0x0000000000000800ULL
>>> +#define PAGE_ENTRY_I                0x0000000000000400ULL
>>> +#define PAGE_ENTRY_P                0x0000000000000200ULL
>>> +#define PAGE_ENTRY_IEP              0x0000000000000100ULL
>>> +
>>> +#define VADDR_REGION1_TX_MASK       0xffe0000000000000ULL
>>> +#define VADDR_REGION2_TX_MASK       0x001ffc0000000000ULL
>>> +#define VADDR_REGION3_TX_MASK       0x000003ff80000000ULL
>>> +#define VADDR_SEGMENT_TX_MASK       0x000000007ff00000ULL
>>> +#define VADDR_PAGE_TX_MASK          0x00000000000ff000ULL
>>> +
>>> +#define VADDR_REGION1_TX(vaddr)     (((vaddr) & VADDR_REGION1_TX_MASK) >> 53)
>>> +#define VADDR_REGION2_TX(vaddr)     (((vaddr) & VADDR_REGION2_TX_MASK) >> 42)
>>> +#define VADDR_REGION3_TX(vaddr)     (((vaddr) & VADDR_REGION3_TX_MASK) >> 31)
>>> +#define VADDR_SEGMENT_TX(vaddr)     (((vaddr) & VADDR_SEGMENT_TX_MASK) >> 20)
>>> +#define VADDR_PAGE_TX(vaddr)        (((vaddr) & VADDR_PAGE_TX_MASK) >> 12)
>>> +
>>> +#define VADDR_REGION1_TL(vaddr)     (((vaddr) & 0xc000000000000000ULL) >> 62)
>>> +#define VADDR_REGION2_TL(vaddr)     (((vaddr) & 0x0018000000000000ULL) >> 51)
>>> +#define VADDR_REGION3_TL(vaddr)     (((vaddr) & 0x0000030000000000ULL) >> 40)
>>> +#define VADDR_SEGMENT_TL(vaddr)     (((vaddr) & 0x0000000060000000ULL) >> 29)
>>
>> Ugh, this patch is quite big, and you're doing multiple things at once
>> here, e.g. renaming macros from PAGE_INVALID to PAGE_ENTRY_I ... could
> 
> I could split out renaming the macros, however, besides a lot of work on
> my side this won't really make a huge difference here.

FWIW, I can try to perform some changes on the old code and then perform
the switch from recursion->single function in one step. Will try to see
how that turns out.


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH-for-4.2 v1 3/9] s390x/mmu: DAT translation rewrite

[Thomas Huth]

On 8/5/19 5:29 PM, David Hildenbrand wrote:
> Let's rewrite the DAT translation in a non-recursive way, similar to
> arch/s390/kvm/gaccess.c:guest_translate() in KVM. This makes the
> code much easier to read, compare and maintain.

Ok, I just had another look at this patch, and even if I don't like the
complete rewrite just for the sake of it, the new code looks ok to me.

[...]
> +    switch (asce & ASCE_TYPE_MASK) {
> +    case ASCE_TYPE_REGION1:
> +        if (read_table_entry(gaddr, &entry)) {
> +            return PGM_ADDRESSING;
> +        }
> +        if (entry & REGION_ENTRY_I) {
> +            return PGM_REG_FIRST_TRANS;
> +        }
> +        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION1) {
> +            return PGM_TRANS_SPEC;
> +        }
> +        if (VADDR_REGION2_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
> +            VADDR_REGION2_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
> +            return PGM_REG_SEC_TRANS;
> +        }
> +        if (edat1 && (entry & REGION_ENTRY_P)) {
> +            *flags &= ~PAGE_WRITE;
> +        }
> +        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_REGION2_TX(vaddr) * 8;
> +        /* FALL THROUGH */
> +    case ASCE_TYPE_REGION2:
> +        if (read_table_entry(gaddr, &entry)) {
> +            return PGM_ADDRESSING;
> +        }
> +        if (entry & REGION_ENTRY_I) {
> +            return PGM_REG_SEC_TRANS;
> +        }
> +        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION2) {
> +            return PGM_TRANS_SPEC;
> +        }
> +        if (VADDR_REGION3_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
> +            VADDR_REGION3_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
> +            return PGM_REG_THIRD_TRANS;
> +        }
> +        if (edat1 && (entry & REGION_ENTRY_P)) {
> +            *flags &= ~PAGE_WRITE;
> +        }
> +        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_REGION3_TX(vaddr) * 8;
> +        /* FALL THROUGH */
> +    case ASCE_TYPE_REGION3:
> +        if (read_table_entry(gaddr, &entry)) {
> +            return PGM_ADDRESSING;
> +        }
> +        if (entry & REGION_ENTRY_I) {
> +            return PGM_REG_THIRD_TRANS;
> +        }
> +        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION3) {
> +            return PGM_TRANS_SPEC;
> +        }
> +        if (edat1 && (entry & REGION_ENTRY_P)) {
> +            *flags &= ~PAGE_WRITE;
> +        }
> +        if (VADDR_SEGMENT_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
> +            VADDR_SEGMENT_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
> +            return PGM_SEGMENT_TRANS;
> +        }
> +        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_SEGMENT_TX(vaddr) * 8;
> +        /* FALL THROUGH */

If you don't like recursion, maybe you could at least use a for-loop for
the region tables? ... the code is really quite repetitive here... just
my 0.02 €.

 Thomas

Re: [Qemu-devel] [qemu-s390x] [PATCH-for-4.2 v1 3/9] s390x/mmu: DAT translation rewrite

[David Hildenbrand]

On 19.08.19 13:40, Thomas Huth wrote:
> On 8/5/19 5:29 PM, David Hildenbrand wrote:
>> Let's rewrite the DAT translation in a non-recursive way, similar to
>> arch/s390/kvm/gaccess.c:guest_translate() in KVM. This makes the
>> code much easier to read, compare and maintain.
> 
> Ok, I just had another look at this patch, and even if I don't like the
> complete rewrite just for the sake of it, the new code looks ok to me.
> 
> [...]
>> +    switch (asce & ASCE_TYPE_MASK) {
>> +    case ASCE_TYPE_REGION1:
>> +        if (read_table_entry(gaddr, &entry)) {
>> +            return PGM_ADDRESSING;
>> +        }
>> +        if (entry & REGION_ENTRY_I) {
>> +            return PGM_REG_FIRST_TRANS;
>> +        }
>> +        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION1) {
>> +            return PGM_TRANS_SPEC;
>> +        }
>> +        if (VADDR_REGION2_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
>> +            VADDR_REGION2_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
>> +            return PGM_REG_SEC_TRANS;
>> +        }
>> +        if (edat1 && (entry & REGION_ENTRY_P)) {
>> +            *flags &= ~PAGE_WRITE;
>> +        }
>> +        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_REGION2_TX(vaddr) * 8;
>> +        /* FALL THROUGH */
>> +    case ASCE_TYPE_REGION2:
>> +        if (read_table_entry(gaddr, &entry)) {
>> +            return PGM_ADDRESSING;
>> +        }
>> +        if (entry & REGION_ENTRY_I) {
>> +            return PGM_REG_SEC_TRANS;
>> +        }
>> +        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION2) {
>> +            return PGM_TRANS_SPEC;
>> +        }
>> +        if (VADDR_REGION3_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
>> +            VADDR_REGION3_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
>> +            return PGM_REG_THIRD_TRANS;
>> +        }
>> +        if (edat1 && (entry & REGION_ENTRY_P)) {
>> +            *flags &= ~PAGE_WRITE;
>> +        }
>> +        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_REGION3_TX(vaddr) * 8;
>> +        /* FALL THROUGH */
>> +    case ASCE_TYPE_REGION3:
>> +        if (read_table_entry(gaddr, &entry)) {
>> +            return PGM_ADDRESSING;
>> +        }
>> +        if (entry & REGION_ENTRY_I) {
>> +            return PGM_REG_THIRD_TRANS;
>> +        }
>> +        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION3) {
>> +            return PGM_TRANS_SPEC;
>> +        }
>> +        if (edat1 && (entry & REGION_ENTRY_P)) {
>> +            *flags &= ~PAGE_WRITE;
>> +        }
>> +        if (VADDR_SEGMENT_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
>> +            VADDR_SEGMENT_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
>> +            return PGM_SEGMENT_TRANS;
>> +        }
>> +        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_SEGMENT_TX(vaddr) * 8;
>> +        /* FALL THROUGH */
> 
> If you don't like recursion, maybe you could at least use a for-loop for
> the region tables? ... the code is really quite repetitive here... just
> my 0.02 €.

I'll split this patch further up once I have time. With the unrolling I
am not yet quite sure - the tables tend to get more different with every
new HW release (especially, see the other patches in this series, like
edat2 and IEP) and we want our branch predictor to produce sane
predictions (especially when processing consecutive pages).

> 
>  Thomas
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH-for-4.2 v1 3/9] s390x/mmu: DAT translation rewrite

[Thomas Huth]

On 8/19/19 1:58 PM, David Hildenbrand wrote:
> On 19.08.19 13:40, Thomas Huth wrote:
>> On 8/5/19 5:29 PM, David Hildenbrand wrote:
>>> Let's rewrite the DAT translation in a non-recursive way, similar to
>>> arch/s390/kvm/gaccess.c:guest_translate() in KVM. This makes the
>>> code much easier to read, compare and maintain.
>>
>> Ok, I just had another look at this patch, and even if I don't like the
>> complete rewrite just for the sake of it, the new code looks ok to me.
>>
>> [...]
>>> +    switch (asce & ASCE_TYPE_MASK) {
>>> +    case ASCE_TYPE_REGION1:
>>> +        if (read_table_entry(gaddr, &entry)) {
>>> +            return PGM_ADDRESSING;
>>> +        }
>>> +        if (entry & REGION_ENTRY_I) {
>>> +            return PGM_REG_FIRST_TRANS;
>>> +        }
>>> +        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION1) {
>>> +            return PGM_TRANS_SPEC;
>>> +        }
>>> +        if (VADDR_REGION2_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
>>> +            VADDR_REGION2_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
>>> +            return PGM_REG_SEC_TRANS;
>>> +        }
>>> +        if (edat1 && (entry & REGION_ENTRY_P)) {
>>> +            *flags &= ~PAGE_WRITE;
>>> +        }
>>> +        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_REGION2_TX(vaddr) * 8;
>>> +        /* FALL THROUGH */
>>> +    case ASCE_TYPE_REGION2:
>>> +        if (read_table_entry(gaddr, &entry)) {
>>> +            return PGM_ADDRESSING;
>>> +        }
>>> +        if (entry & REGION_ENTRY_I) {
>>> +            return PGM_REG_SEC_TRANS;
>>> +        }
>>> +        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION2) {
>>> +            return PGM_TRANS_SPEC;
>>> +        }
>>> +        if (VADDR_REGION3_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
>>> +            VADDR_REGION3_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
>>> +            return PGM_REG_THIRD_TRANS;
>>> +        }
>>> +        if (edat1 && (entry & REGION_ENTRY_P)) {
>>> +            *flags &= ~PAGE_WRITE;
>>> +        }
>>> +        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_REGION3_TX(vaddr) * 8;
>>> +        /* FALL THROUGH */
>>> +    case ASCE_TYPE_REGION3:
>>> +        if (read_table_entry(gaddr, &entry)) {
>>> +            return PGM_ADDRESSING;
>>> +        }
>>> +        if (entry & REGION_ENTRY_I) {
>>> +            return PGM_REG_THIRD_TRANS;
>>> +        }
>>> +        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION3) {
>>> +            return PGM_TRANS_SPEC;
>>> +        }
>>> +        if (edat1 && (entry & REGION_ENTRY_P)) {
>>> +            *flags &= ~PAGE_WRITE;
>>> +        }
>>> +        if (VADDR_SEGMENT_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
>>> +            VADDR_SEGMENT_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
>>> +            return PGM_SEGMENT_TRANS;
>>> +        }
>>> +        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_SEGMENT_TX(vaddr) * 8;
>>> +        /* FALL THROUGH */
>>
>> If you don't like recursion, maybe you could at least use a for-loop for
>> the region tables? ... the code is really quite repetitive here... just
>> my 0.02 €.
> 
> I'll split this patch further up once I have time. With the unrolling I
> am not yet quite sure - the tables tend to get more different with every
> new HW release (especially, see the other patches in this series, like
> edat2 and IEP) and we want our branch predictor to produce sane
> predictions (especially when processing consecutive pages).

Ok, I just also saw in the next patch that region 3 already needs some
special handling for EDAT-2, so never mind, it's likely best to keep it
unrolled.

 Thomas

[Qemu-devel] [PATCH-for-4.2 v1 4/9] s390x/mmu: Add EDAT2 translation support

[David Hildenbrand]

This only adds basic support to the DAT translation, but no EDAT2 support
for TCG. E.g., the gdbstub under kvm uses this function, too, to
translate virtual addresses.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/mmu_helper.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index de7798284d..5c9c7d385d 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -139,6 +139,7 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
 {
     const bool edat1 = (env->cregs[0] & CR0_EDAT) &&
                        s390_has_feat(S390_FEAT_EDAT);
+    const bool edat2 = edat1 && s390_has_feat(S390_FEAT_EDAT_2);
     const int asce_tl = asce & ASCE_TABLE_LENGTH;
     const int asce_p = asce & ASCE_PRIVATE_SPACE;
     hwaddr gaddr = asce & ASCE_ORIGIN;
@@ -234,9 +235,16 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
         if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION3) {
             return PGM_TRANS_SPEC;
         }
+        if (edat2 && (entry & REGION3_ENTRY_CR) && asce_p) {
+            return PGM_TRANS_SPEC;
+        }
         if (edat1 && (entry & REGION_ENTRY_P)) {
             *flags &= ~PAGE_WRITE;
         }
+        if (edat2 && (entry & REGION3_ENTRY_FC)) {
+            *raddr = entry & REGION3_ENTRY_RFAA;
+            return 0;
+        }
         if (VADDR_SEGMENT_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
             VADDR_SEGMENT_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
             return PGM_SEGMENT_TRANS;
-- 
2.21.0

Re: [Qemu-devel] [qemu-s390x] [PATCH-for-4.2 v1 4/9] s390x/mmu: Add EDAT2 translation support

[Thomas Huth]

On 8/5/19 5:29 PM, David Hildenbrand wrote:
> This only adds basic support to the DAT translation, but no EDAT2 support
> for TCG. E.g., the gdbstub under kvm uses this function, too, to
> translate virtual addresses.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/mmu_helper.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
> index de7798284d..5c9c7d385d 100644
> --- a/target/s390x/mmu_helper.c
> +++ b/target/s390x/mmu_helper.c
> @@ -139,6 +139,7 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
>  {
>      const bool edat1 = (env->cregs[0] & CR0_EDAT) &&
>                         s390_has_feat(S390_FEAT_EDAT);
> +    const bool edat2 = edat1 && s390_has_feat(S390_FEAT_EDAT_2);
>      const int asce_tl = asce & ASCE_TABLE_LENGTH;
>      const int asce_p = asce & ASCE_PRIVATE_SPACE;
>      hwaddr gaddr = asce & ASCE_ORIGIN;
> @@ -234,9 +235,16 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
>          if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION3) {
>              return PGM_TRANS_SPEC;
>          }
> +        if (edat2 && (entry & REGION3_ENTRY_CR) && asce_p) {
> +            return PGM_TRANS_SPEC;
> +        }
>          if (edat1 && (entry & REGION_ENTRY_P)) {
>              *flags &= ~PAGE_WRITE;
>          }
> +        if (edat2 && (entry & REGION3_ENTRY_FC)) {
> +            *raddr = entry & REGION3_ENTRY_RFAA;
> +            return 0;
> +        }
>          if (VADDR_SEGMENT_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
>              VADDR_SEGMENT_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
>              return PGM_SEGMENT_TRANS;

Reviewed-by: Thomas Huth <thuth@redhat.com>

[Qemu-devel] [PATCH-for-4.2 v1 5/9] s390x/mmu: Implement access-exception-fetch/store-indication facility

[David Hildenbrand]

We always have to indicate whether it is a fetch or a store for all access
exceptions. This is only missing for LAP exceptions.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/mmu_helper.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index 5c9c7d385d..f3e988e4fd 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -333,7 +333,7 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
         *flags |= PAGE_WRITE_INV;
         if (is_low_address(vaddr) && rw == MMU_DATA_STORE) {
             if (exc) {
-                trigger_access_exception(env, PGM_PROTECTION, ILEN_AUTO, 0);
+                trigger_access_exception(env, PGM_PROTECTION, ilen, tec);
             }
             return -EACCES;
         }
@@ -511,6 +511,8 @@ void s390_cpu_virt_mem_handle_exc(S390CPU *cpu, uintptr_t ra)
 int mmu_translate_real(CPUS390XState *env, target_ulong raddr, int rw,
                        target_ulong *addr, int *flags)
 {
+    uint64_t tec = (raddr & TARGET_PAGE_MASK) |
+                   (rw == MMU_DATA_STORE ? FS_WRITE : FS_READ);
     const bool lowprot_enabled = env->cregs[0] & CR0_LOWPROT;
 
     *flags = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
@@ -518,7 +520,7 @@ int mmu_translate_real(CPUS390XState *env, target_ulong raddr, int rw,
         /* see comment in mmu_translate() how this works */
         *flags |= PAGE_WRITE_INV;
         if (is_low_address(raddr) && rw == MMU_DATA_STORE) {
-            trigger_access_exception(env, PGM_PROTECTION, ILEN_AUTO, 0);
+            trigger_access_exception(env, PGM_PROTECTION, ILEN_AUTO, tec);
             return -EACCES;
         }
     }
-- 
2.21.0

Re: [Qemu-devel] [qemu-s390x] [PATCH-for-4.2 v1 5/9] s390x/mmu: Implement access-exception-fetch/store-indication facility

[Thomas Huth]

On 8/5/19 5:29 PM, David Hildenbrand wrote:
> We always have to indicate whether it is a fetch or a store for all access
> exceptions. This is only missing for LAP exceptions.

Do we really need this for LAP, too? If I get figure 3-5 "Enhanced
Suppression-on-Protection Results" right, these bits are not set for LAP
exceptions...? Do I miss something?

 Thomas

Re: [Qemu-devel] [qemu-s390x] [PATCH-for-4.2 v1 5/9] s390x/mmu: Implement access-exception-fetch/store-indication facility

[Thomas Huth]

On 8/19/19 2:16 PM, Thomas Huth wrote:
> On 8/5/19 5:29 PM, David Hildenbrand wrote:
>> We always have to indicate whether it is a fetch or a store for all access
>> exceptions. This is only missing for LAP exceptions.
> 
> Do we really need this for LAP, too? If I get figure 3-5 "Enhanced
> Suppression-on-Protection Results" right, these bits are not set for LAP
> exceptions...? Do I miss something?

I was looking at an older version of the PoP ... the table that I mean
is "Figure 3-8. Enhanced Suppression-on-Protection Facility 2 Results"
in SA22-7832-11.

 Thomas

Re: [Qemu-devel] [qemu-s390x] [PATCH-for-4.2 v1 5/9] s390x/mmu: Implement access-exception-fetch/store-indication facility

[David Hildenbrand]

On 19.08.19 14:22, Thomas Huth wrote:
> On 8/19/19 2:16 PM, Thomas Huth wrote:
>> On 8/5/19 5:29 PM, David Hildenbrand wrote:
>>> We always have to indicate whether it is a fetch or a store for all access
>>> exceptions. This is only missing for LAP exceptions.
>>
>> Do we really need this for LAP, too? If I get figure 3-5 "Enhanced
>> Suppression-on-Protection Results" right, these bits are not set for LAP
>> exceptions...? Do I miss something?
> 
> I was looking at an older version of the PoP ... the table that I mean
> is "Figure 3-8. Enhanced Suppression-on-Protection Facility 2 Results"
> in SA22-7832-11.
> 
>  Thomas
> 

I think that table only states that if 56==60==61==0, then we might have
either KCP or LAP ("Presented if TEID details are not available" - but
as we have TEID information available, we can just set 56=1 and 60=61=0
(== LAP), or am I missing something?

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH-for-4.2 v1 5/9] s390x/mmu: Implement access-exception-fetch/store-indication facility

[Thomas Huth]

On 8/19/19 2:26 PM, David Hildenbrand wrote:
> On 19.08.19 14:22, Thomas Huth wrote:
>> On 8/19/19 2:16 PM, Thomas Huth wrote:
>>> On 8/5/19 5:29 PM, David Hildenbrand wrote:
>>>> We always have to indicate whether it is a fetch or a store for all access
>>>> exceptions. This is only missing for LAP exceptions.
>>>
>>> Do we really need this for LAP, too? If I get figure 3-5 "Enhanced
>>> Suppression-on-Protection Results" right, these bits are not set for LAP
>>> exceptions...? Do I miss something?
>>
>> I was looking at an older version of the PoP ... the table that I mean
>> is "Figure 3-8. Enhanced Suppression-on-Protection Facility 2 Results"
>> in SA22-7832-11.
>>
>>  Thomas
>>
> 
> I think that table only states that if 56==60==61==0, then we might have
> either KCP or LAP ("Presented if TEID details are not available" - but
> as we have TEID information available, we can just set 56=1 and 60=61=0
> (== LAP), or am I missing something?

Oh, well, I was looking at the older version of the PoP first, and it
was not specified there yet, and when I started looking the the new
version, I only saw the first LAP line and stopped reading properly
afterwards... of course you're right, there is another LAP line in the
table where they say that the address is correclty specified.

Please mentioned the "Enhanced Suppression-on-Protection
Facility 2" (which introduced this new behavior) in the patch
description to make this clear, then your patch is fine.

 Thomas

Re: [Qemu-devel] [qemu-s390x] [PATCH-for-4.2 v1 5/9] s390x/mmu: Implement access-exception-fetch/store-indication facility

[David Hildenbrand]

On 19.08.19 14:30, Thomas Huth wrote:
> On 8/19/19 2:26 PM, David Hildenbrand wrote:
>> On 19.08.19 14:22, Thomas Huth wrote:
>>> On 8/19/19 2:16 PM, Thomas Huth wrote:
>>>> On 8/5/19 5:29 PM, David Hildenbrand wrote:
>>>>> We always have to indicate whether it is a fetch or a store for all access
>>>>> exceptions. This is only missing for LAP exceptions.
>>>>
>>>> Do we really need this for LAP, too? If I get figure 3-5 "Enhanced
>>>> Suppression-on-Protection Results" right, these bits are not set for LAP
>>>> exceptions...? Do I miss something?
>>>
>>> I was looking at an older version of the PoP ... the table that I mean
>>> is "Figure 3-8. Enhanced Suppression-on-Protection Facility 2 Results"
>>> in SA22-7832-11.
>>>
>>>  Thomas
>>>
>>
>> I think that table only states that if 56==60==61==0, then we might have
>> either KCP or LAP ("Presented if TEID details are not available" - but
>> as we have TEID information available, we can just set 56=1 and 60=61=0
>> (== LAP), or am I missing something?
> 
> Oh, well, I was looking at the older version of the PoP first, and it
> was not specified there yet, and when I started looking the the new
> version, I only saw the first LAP line and stopped reading properly
> afterwards... of course you're right, there is another LAP line in the
> table where they say that the address is correclty specified.
> 
> Please mentioned the "Enhanced Suppression-on-Protection
> Facility 2" (which introduced this new behavior) in the patch
> description to make this clear, then your patch is fine.
> 

Ah, right, that comes in the next patch. Might make sense to reshuffle
both patches. Will have a look.

Thanks!

>  Thomas
> 


-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH-for-4.2 v1 6/9] s390x/mmu: Implement enhanced suppression-on-protection facility 2

[David Hildenbrand]

We already implement ESOP-1. For ESOP-2, we only have to indicate all
protection exceptions properly. Due to EDAT-1, we already indicate DAT
exceptions properly. We don't trigger KCP/ALCP/IEP exceptions yet.

So all we have to do is set the TEID (TEC) to the right values
(bit 56, 60, 61) in case of LAP.

We don't have any side-effects (e.g., no guarded-storage facility),
therefore, bit 64 of the TEID (TEC) is always 0.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/mmu_helper.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index f3e988e4fd..631cc29c28 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -333,6 +333,8 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
         *flags |= PAGE_WRITE_INV;
         if (is_low_address(vaddr) && rw == MMU_DATA_STORE) {
             if (exc) {
+                /* LAP sets bit 56 */
+                tec |= 0x80;
                 trigger_access_exception(env, PGM_PROTECTION, ilen, tec);
             }
             return -EACCES;
@@ -520,6 +522,8 @@ int mmu_translate_real(CPUS390XState *env, target_ulong raddr, int rw,
         /* see comment in mmu_translate() how this works */
         *flags |= PAGE_WRITE_INV;
         if (is_low_address(raddr) && rw == MMU_DATA_STORE) {
+            /* LAP sets bit 56 */
+            tec |= 0x80;
             trigger_access_exception(env, PGM_PROTECTION, ILEN_AUTO, tec);
             return -EACCES;
         }
-- 
2.21.0

Re: [Qemu-devel] [qemu-s390x] [PATCH-for-4.2 v1 6/9] s390x/mmu: Implement enhanced suppression-on-protection facility 2

[Thomas Huth]

On 8/5/19 5:29 PM, David Hildenbrand wrote:
> We already implement ESOP-1. For ESOP-2, we only have to indicate all
> protection exceptions properly. Due to EDAT-1, we already indicate DAT
> exceptions properly. We don't trigger KCP/ALCP/IEP exceptions yet.
> 
> So all we have to do is set the TEID (TEC) to the right values
> (bit 56, 60, 61) in case of LAP.
> 
> We don't have any side-effects (e.g., no guarded-storage facility),
> therefore, bit 64 of the TEID (TEC) is always 0.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/mmu_helper.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
> index f3e988e4fd..631cc29c28 100644
> --- a/target/s390x/mmu_helper.c
> +++ b/target/s390x/mmu_helper.c
> @@ -333,6 +333,8 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
>          *flags |= PAGE_WRITE_INV;
>          if (is_low_address(vaddr) && rw == MMU_DATA_STORE) {
>              if (exc) {
> +                /* LAP sets bit 56 */
> +                tec |= 0x80;
>                  trigger_access_exception(env, PGM_PROTECTION, ilen, tec);
>              }
>              return -EACCES;
> @@ -520,6 +522,8 @@ int mmu_translate_real(CPUS390XState *env, target_ulong raddr, int rw,
>          /* see comment in mmu_translate() how this works */
>          *flags |= PAGE_WRITE_INV;
>          if (is_low_address(raddr) && rw == MMU_DATA_STORE) {
> +            /* LAP sets bit 56 */
> +            tec |= 0x80;
>              trigger_access_exception(env, PGM_PROTECTION, ILEN_AUTO, tec);
>              return -EACCES;
>          }

I'd suggest to merge this with the previous patch since the other bits
are only valid if bit 56 is enabled.

 Thomas

[Qemu-devel] [PATCH-for-4.2 v1 7/9] s390x/mmu: Implement Instruction-Execution-Protection Facility

[David Hildenbrand]

IEP support in the mmu is fairly easy. Set the right permissions for TLB
entries and properly report an exception.

Make sure to handle EDAT-2 by setting bit 56/60/61 of the TEID (TEC) to
the right values.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu.h        |  1 +
 target/s390x/mmu_helper.c | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 1ff14250bd..9a8318b3aa 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -311,6 +311,7 @@ extern const struct VMStateDescription vmstate_s390_cpu;
 #define CR0_EDAT                0x0000000000800000ULL
 #define CR0_AFP                 0x0000000000040000ULL
 #define CR0_VECTOR              0x0000000000020000ULL
+#define CR0_IEP                 0x0000000000100000ULL
 #define CR0_EMERGENCY_SIGNAL_SC 0x0000000000004000ULL
 #define CR0_EXTERNAL_CALL_SC    0x0000000000002000ULL
 #define CR0_CKC_SC              0x0000000000000800ULL
diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index 631cc29c28..83e241c430 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -140,6 +140,8 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
     const bool edat1 = (env->cregs[0] & CR0_EDAT) &&
                        s390_has_feat(S390_FEAT_EDAT);
     const bool edat2 = edat1 && s390_has_feat(S390_FEAT_EDAT_2);
+    const bool iep = (env->cregs[0] & CR0_IEP) &&
+                     s390_has_feat(S390_FEAT_INSTRUCTION_EXEC_PROT);
     const int asce_tl = asce & ASCE_TABLE_LENGTH;
     const int asce_p = asce & ASCE_PRIVATE_SPACE;
     hwaddr gaddr = asce & ASCE_ORIGIN;
@@ -242,6 +244,9 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
             *flags &= ~PAGE_WRITE;
         }
         if (edat2 && (entry & REGION3_ENTRY_FC)) {
+            if (iep && (entry & REGION3_ENTRY_IEP)) {
+                *flags &= ~PAGE_EXEC;
+            }
             *raddr = entry & REGION3_ENTRY_RFAA;
             return 0;
         }
@@ -268,6 +273,9 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
             *flags &= ~PAGE_WRITE;
         }
         if (edat1 && (entry & SEGMENT_ENTRY_FC)) {
+            if (iep && (entry & SEGMENT_ENTRY_IEP)) {
+                *flags &= ~PAGE_EXEC;
+            }
             *raddr = entry & SEGMENT_ENTRY_SFAA;
             return 0;
         }
@@ -287,6 +295,9 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
     if (entry & PAGE_ENTRY_P) {
         *flags &= ~PAGE_WRITE;
     }
+    if (iep && (entry & PAGE_ENTRY_IEP)) {
+        *flags &= ~PAGE_EXEC;
+    }
 
     *raddr = entry & TARGET_PAGE_MASK;
     return 0;
@@ -386,6 +397,16 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
         return -1;
     }
 
+    /* check for Instruction-Execution-Protection */
+    if (rw == MMU_INST_FETCH && !(*flags & PAGE_EXEC)) {
+        if (exc) {
+            /* IEP sets bit 56 and 61 */
+            tec |= 0x84;
+            trigger_access_exception(env, PGM_PROTECTION, ilen, tec);
+        }
+        return -1;
+    }
+
 nodat:
     /* Convert real address -> absolute address */
     *raddr = mmu_real2abs(env, *raddr);
-- 
2.21.0

Re: [Qemu-devel] [qemu-s390x] [PATCH-for-4.2 v1 7/9] s390x/mmu: Implement Instruction-Execution-Protection Facility

[Thomas Huth]

On 8/5/19 5:29 PM, David Hildenbrand wrote:
> IEP support in the mmu is fairly easy. Set the right permissions for TLB
> entries and properly report an exception.
> 
> Make sure to handle EDAT-2 by setting bit 56/60/61 of the TEID (TEC) to
> the right values.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/cpu.h        |  1 +
>  target/s390x/mmu_helper.c | 21 +++++++++++++++++++++
>  2 files changed, 22 insertions(+)

LGTM!

Reviewed-by: Thomas Huth <thuth@redhat.com>

[Qemu-devel] [PATCH-for-4.2 v1 8/9] s390x/cpumodel: Prepare for changes of QEMU model

[David Hildenbrand]

Setup the 4.1 compatibility model so we can add new features to the
LATEST model.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c  | 2 ++
 target/s390x/gen-features.c | 6 +++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 593b34e0e2..c815a65ee9 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -671,7 +671,9 @@ DEFINE_CCW_MACHINE(4_2, "4.2", true);
 
 static void ccw_machine_4_1_instance_options(MachineState *machine)
 {
+    static const S390FeatInit qemu_cpu_feat = { S390_FEAT_LIST_QEMU_V4_1 };
     ccw_machine_4_2_instance_options(machine);
+    s390_set_qemu_cpu_model(0x2964, 13, 2, qemu_cpu_feat);
 }
 
 static void ccw_machine_4_1_class_options(MachineClass *mc)
diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
index 49a650ac52..7e82f2f004 100644
--- a/target/s390x/gen-features.c
+++ b/target/s390x/gen-features.c
@@ -698,11 +698,14 @@ static uint16_t qemu_V4_0[] = {
     S390_FEAT_ZPCI,
 };
 
-static uint16_t qemu_LATEST[] = {
+static uint16_t qemu_V4_1[] = {
     S390_FEAT_STFLE_53,
     S390_FEAT_VECTOR,
 };
 
+static uint16_t qemu_LATEST[] = {
+};
+
 /* add all new definitions before this point */
 static uint16_t qemu_MAX[] = {
     /* generates a dependency warning, leave it out for now */
@@ -824,6 +827,7 @@ static FeatGroupDefSpec QemuFeatDef[] = {
     QEMU_FEAT_INITIALIZER(V2_11),
     QEMU_FEAT_INITIALIZER(V3_1),
     QEMU_FEAT_INITIALIZER(V4_0),
+    QEMU_FEAT_INITIALIZER(V4_1),
     QEMU_FEAT_INITIALIZER(LATEST),
     QEMU_FEAT_INITIALIZER(MAX),
 };
-- 
2.21.0

Re: [Qemu-devel] [PATCH-for-4.2 v1 8/9] s390x/cpumodel: Prepare for changes of QEMU model

[Cornelia Huck]

On Mon,  5 Aug 2019 17:29:46 +0200
David Hildenbrand <david@redhat.com> wrote:

> Setup the 4.1 compatibility model so we can add new features to the
> LATEST model.

Basically a nop for now from an outside view.

> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-virtio-ccw.c  | 2 ++
>  target/s390x/gen-features.c | 6 +++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [Qemu-devel] [qemu-s390x] [PATCH-for-4.2 v1 8/9] s390x/cpumodel: Prepare for changes of QEMU model

[Thomas Huth]

On 8/5/19 5:29 PM, David Hildenbrand wrote:
> Setup the 4.1 compatibility model so we can add new features to the
> LATEST model.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-virtio-ccw.c  | 2 ++
>  target/s390x/gen-features.c | 6 +++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 593b34e0e2..c815a65ee9 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -671,7 +671,9 @@ DEFINE_CCW_MACHINE(4_2, "4.2", true);
>  
>  static void ccw_machine_4_1_instance_options(MachineState *machine)
>  {
> +    static const S390FeatInit qemu_cpu_feat = { S390_FEAT_LIST_QEMU_V4_1 };
>      ccw_machine_4_2_instance_options(machine);
> +    s390_set_qemu_cpu_model(0x2964, 13, 2, qemu_cpu_feat);
>  }
>  
>  static void ccw_machine_4_1_class_options(MachineClass *mc)
> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
> index 49a650ac52..7e82f2f004 100644
> --- a/target/s390x/gen-features.c
> +++ b/target/s390x/gen-features.c
> @@ -698,11 +698,14 @@ static uint16_t qemu_V4_0[] = {
>      S390_FEAT_ZPCI,
>  };
>  
> -static uint16_t qemu_LATEST[] = {
> +static uint16_t qemu_V4_1[] = {
>      S390_FEAT_STFLE_53,
>      S390_FEAT_VECTOR,
>  };
>  
> +static uint16_t qemu_LATEST[] = {
> +};
> +
>  /* add all new definitions before this point */
>  static uint16_t qemu_MAX[] = {
>      /* generates a dependency warning, leave it out for now */
> @@ -824,6 +827,7 @@ static FeatGroupDefSpec QemuFeatDef[] = {
>      QEMU_FEAT_INITIALIZER(V2_11),
>      QEMU_FEAT_INITIALIZER(V3_1),
>      QEMU_FEAT_INITIALIZER(V4_0),
> +    QEMU_FEAT_INITIALIZER(V4_1),
>      QEMU_FEAT_INITIALIZER(LATEST),
>      QEMU_FEAT_INITIALIZER(MAX),
>  };
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

[Qemu-devel] [PATCH-for-4.2 v1 9/9] s390x/cpumodel: Add new TCG features to QEMU cpu model

[David Hildenbrand]

We now implement a bunch of new facilities we can properly indicate.

ESOP-1/ESOP-2 handling is discussed in the PoP Chafter 3-15
("Suppression on Protection"). The "Basic suppression-on-protection (SOP)
facility" is a core part of z/Architecture without a facility
indication. ESOP-2 is indicated by ESOP-1 + Side-effect facility
("ESOP-2"). Besides ESOP-2, the side-effect facility is only relevant for
the guarded-storage facility (we don't implement).

S390_ESOP:
- We indicate DAT exeptions by setting bit 61 of the TEID (TEC) to 1 and
  bit 60 to zero. We don't trigger ALCP exceptions yet. Also, we set
  bit 0-51 and bit 62/63 to the right values.
S390_ACCESS_EXCEPTION_FS_INDICATION:
- The TEID (TEC) properly indicates in bit 52/53 on any access if it was
  a fetch or a store
S390_SIDE_EFFECT_ACCESS_ESOP2:
- We have no side-effect accesses (esp., we don't implement the
  guarded-storage faciliy), we correctly set bit 64 of the TEID (TEC) to
  0 (no side-effect).
- ESOP2: We properly set bit 56, 60, 61 in the TEID (TEC) to indicate the
  type of protection. We don't trigger KCP/ALCP exceptions yet.
S390_INSTRUCTION_EXEC_PROT:
- The MMU properly detects and indicates the exception on instruction fetches
- Protected TLB entries will never get PAGE_EXEC set.

There is no need to fake the abscence of any of the facilities - without
the facilities, some bits of the TEID (TEC) are simply unpredictable.

As IEP was added with z14 and we currently implement a z13, add it to
the MAX model instead.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/gen-features.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
index 7e82f2f004..6e78d40d9a 100644
--- a/target/s390x/gen-features.c
+++ b/target/s390x/gen-features.c
@@ -704,12 +704,16 @@ static uint16_t qemu_V4_1[] = {
 };
 
 static uint16_t qemu_LATEST[] = {
+    S390_FEAT_ACCESS_EXCEPTION_FS_INDICATION,
+    S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2,
+    S390_FEAT_ESOP,
 };
 
 /* add all new definitions before this point */
 static uint16_t qemu_MAX[] = {
     /* generates a dependency warning, leave it out for now */
     S390_FEAT_MSA_EXT_5,
+    S390_FEAT_INSTRUCTION_EXEC_PROT,
 };
 
 /****** END FEATURE DEFS ******/
-- 
2.21.0

Re: [Qemu-devel] [PATCH-for-4.2 v1 9/9] s390x/cpumodel: Add new TCG features to QEMU cpu model

[Cornelia Huck]

On Mon,  5 Aug 2019 17:29:47 +0200
David Hildenbrand <david@redhat.com> wrote:

> We now implement a bunch of new facilities we can properly indicate.
> 
> ESOP-1/ESOP-2 handling is discussed in the PoP Chafter 3-15
> ("Suppression on Protection"). The "Basic suppression-on-protection (SOP)
> facility" is a core part of z/Architecture without a facility
> indication. ESOP-2 is indicated by ESOP-1 + Side-effect facility
> ("ESOP-2"). Besides ESOP-2, the side-effect facility is only relevant for
> the guarded-storage facility (we don't implement).
> 
> S390_ESOP:
> - We indicate DAT exeptions by setting bit 61 of the TEID (TEC) to 1 and
>   bit 60 to zero. We don't trigger ALCP exceptions yet. Also, we set
>   bit 0-51 and bit 62/63 to the right values.
> S390_ACCESS_EXCEPTION_FS_INDICATION:
> - The TEID (TEC) properly indicates in bit 52/53 on any access if it was
>   a fetch or a store
> S390_SIDE_EFFECT_ACCESS_ESOP2:
> - We have no side-effect accesses (esp., we don't implement the
>   guarded-storage faciliy), we correctly set bit 64 of the TEID (TEC) to
>   0 (no side-effect).
> - ESOP2: We properly set bit 56, 60, 61 in the TEID (TEC) to indicate the
>   type of protection. We don't trigger KCP/ALCP exceptions yet.
> S390_INSTRUCTION_EXEC_PROT:
> - The MMU properly detects and indicates the exception on instruction fetches
> - Protected TLB entries will never get PAGE_EXEC set.
> 
> There is no need to fake the abscence of any of the facilities - without
> the facilities, some bits of the TEID (TEC) are simply unpredictable.
> 
> As IEP was added with z14 and we currently implement a z13, add it to
> the MAX model instead.

Looks sane, once we get those features supported.

> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/gen-features.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
> index 7e82f2f004..6e78d40d9a 100644
> --- a/target/s390x/gen-features.c
> +++ b/target/s390x/gen-features.c
> @@ -704,12 +704,16 @@ static uint16_t qemu_V4_1[] = {
>  };
>  
>  static uint16_t qemu_LATEST[] = {
> +    S390_FEAT_ACCESS_EXCEPTION_FS_INDICATION,
> +    S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2,
> +    S390_FEAT_ESOP,
>  };
>  
>  /* add all new definitions before this point */
>  static uint16_t qemu_MAX[] = {
>      /* generates a dependency warning, leave it out for now */
>      S390_FEAT_MSA_EXT_5,
> +    S390_FEAT_INSTRUCTION_EXEC_PROT,

This 'dependency warning' only refers to msa_ext_5, no? Can we make
that more obvious?

>  };
>  
>  /****** END FEATURE DEFS ******/