In do_ats_write() we construct a PAR value based on the result
of the translation.  A comment says "S2WLK and FSTAGE are always
zero, because we don't implement virtualization".
Since we do in fact now implement virtualization, add the missing
code that sets these bits based on the reported ARMMMUFaultInfo.

(These bits are named PTW and S in ARMv8, so we follow that
convention in the new comments in this patch.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 43afdd082e1..dc849b09893 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2344,10 +2344,12 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
 
             par64 |= 1; /* F */
             par64 |= (fsr & 0x3f) << 1; /* FS */
-            /* Note that S2WLK and FSTAGE are always zero, because we don't
-             * implement virtualization and therefore there can't be a stage 2
-             * fault.
-             */
+            if (fi.stage2) {
+                par64 |= (1 << 9); /* S */
+            }
+            if (fi.s1ptw) {
+                par64 |= (1 << 8); /* PTW */
+            }
         }
     } else {
         /* fsr is a DFSR/IFSR value for the short descriptor
-- 
2.19.0

On Tue, Oct 16, 2018 at 10:37:02AM +0100, Peter Maydell wrote:
> In do_ats_write() we construct a PAR value based on the result
> of the translation.  A comment says "S2WLK and FSTAGE are always
> zero, because we don't implement virtualization".
> Since we do in fact now implement virtualization, add the missing
> code that sets these bits based on the reported ARMMMUFaultInfo.
> 
> (These bits are named PTW and S in ARMv8, so we follow that
> convention in the new comments in this patch.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  target/arm/helper.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 43afdd082e1..dc849b09893 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2344,10 +2344,12 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
>  
>              par64 |= 1; /* F */
>              par64 |= (fsr & 0x3f) << 1; /* FS */
> -            /* Note that S2WLK and FSTAGE are always zero, because we don't
> -             * implement virtualization and therefore there can't be a stage 2
> -             * fault.
> -             */
> +            if (fi.stage2) {
> +                par64 |= (1 << 9); /* S */
> +            }
> +            if (fi.s1ptw) {
> +                par64 |= (1 << 8); /* PTW */
> +            }
>          }
>      } else {
>          /* fsr is a DFSR/IFSR value for the short descriptor
> -- 
> 2.19.0
> 
> 

Peter Maydell <peter.maydell@linaro.org> writes:

> In do_ats_write() we construct a PAR value based on the result
> of the translation.  A comment says "S2WLK and FSTAGE are always
> zero, because we don't implement virtualization".
> Since we do in fact now implement virtualization, add the missing
> code that sets these bits based on the reported ARMMMUFaultInfo.
>
> (These bits are named PTW and S in ARMv8, so we follow that
> convention in the new comments in this patch.)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 43afdd082e1..dc849b09893 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2344,10 +2344,12 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
>
>              par64 |= 1; /* F */

To aid readability, mainly for those not familiar like me, maybe:

  par64 |= 1; /* PAR_EL1.F == 1, failed translation */

>              par64 |= (fsr & 0x3f) << 1; /* FS */
> -            /* Note that S2WLK and FSTAGE are always zero, because we don't
> -             * implement virtualization and therefore there can't be a stage 2
> -             * fault.
> -             */
> +            if (fi.stage2) {
> +                par64 |= (1 << 9); /* S */
> +            }
> +            if (fi.s1ptw) {
> +                par64 |= (1 << 8); /* PTW */
> +            }
>          }
>      } else {
>          /* fsr is a DFSR/IFSR value for the short descriptor

Anyway:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


--
Alex Bennée

On 5 November 2018 at 16:24, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> In do_ats_write() we construct a PAR value based on the result
>> of the translation.  A comment says "S2WLK and FSTAGE are always
>> zero, because we don't implement virtualization".
>> Since we do in fact now implement virtualization, add the missing
>> code that sets these bits based on the reported ARMMMUFaultInfo.
>>
>> (These bits are named PTW and S in ARMv8, so we follow that
>> convention in the new comments in this patch.)
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  target/arm/helper.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index 43afdd082e1..dc849b09893 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -2344,10 +2344,12 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
>>
>>              par64 |= 1; /* F */
>
> To aid readability, mainly for those not familiar like me, maybe:
>
>   par64 |= 1; /* PAR_EL1.F == 1, failed translation */

That's in the existing code...

>>              par64 |= (fsr & 0x3f) << 1; /* FS */
>> -            /* Note that S2WLK and FSTAGE are always zero, because we don't
>> -             * implement virtualization and therefore there can't be a stage 2
>> -             * fault.
>> -             */
>> +            if (fi.stage2) {
>> +                par64 |= (1 << 9); /* S */
>> +            }
>> +            if (fi.s1ptw) {
>> +                par64 |= (1 << 8); /* PTW */
>> +            }
>>          }
>>      } else {
>>          /* fsr is a DFSR/IFSR value for the short descriptor
>
> Anyway:
>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

thanks
-- PMM

Peter Maydell <peter.maydell@linaro.org> writes:

> On 5 November 2018 at 16:24, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> In do_ats_write() we construct a PAR value based on the result
>>> of the translation.  A comment says "S2WLK and FSTAGE are always
>>> zero, because we don't implement virtualization".
>>> Since we do in fact now implement virtualization, add the missing
>>> code that sets these bits based on the reported ARMMMUFaultInfo.
>>>
>>> (These bits are named PTW and S in ARMv8, so we follow that
>>> convention in the new comments in this patch.)
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>>  target/arm/helper.c | 10 ++++++----
>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>>> index 43afdd082e1..dc849b09893 100644
>>> --- a/target/arm/helper.c
>>> +++ b/target/arm/helper.c
>>> @@ -2344,10 +2344,12 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
>>>
>>>              par64 |= 1; /* F */
>>
>> To aid readability, mainly for those not familiar like me, maybe:
>>
>>   par64 |= 1; /* PAR_EL1.F == 1, failed translation */
>
> That's in the existing code...

I know, it was just a suggestion to help make it clearer there are two
forms when you are reading the register definition.

--
Alex Bennée