Add vsysreg emulation for registers trapped when TID3 bit is activated
in HSR.
The emulation is returning the value stored in cpuinfo_guest structure
for know registers and is handling reserved registers as RAZ.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
Changes in V2: Rebase
Changes in V3:
  Fix commit message
  Fix code style for GENERATE_TID3_INFO declaration
  Add handling of reserved registers as RAZ.

---
 xen/arch/arm/arm64/vsysreg.c | 53 ++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
index 8a85507d9d..ef7a11dbdd 100644
--- a/xen/arch/arm/arm64/vsysreg.c
+++ b/xen/arch/arm/arm64/vsysreg.c
@@ -69,6 +69,14 @@ TVM_REG(CONTEXTIDR_EL1)
         break;                                                          \
     }
 
+/* Macro to generate easily case for ID co-processor emulation */
+#define GENERATE_TID3_INFO(reg, field, offset)                          \
+    case HSR_SYSREG_##reg:                                              \
+    {                                                                   \
+        return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr,   \
+                          1, guest_cpuinfo.field.bits[offset]);         \
+    }
+
 void do_sysreg(struct cpu_user_regs *regs,
                const union hsr hsr)
 {
@@ -259,6 +267,51 @@ void do_sysreg(struct cpu_user_regs *regs,
          */
         return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
 
+    /*
+     * HCR_EL2.TID3
+     *
+     * This is trapping most Identification registers used by a guest
+     * to identify the processor features
+     */
+    GENERATE_TID3_INFO(ID_PFR0_EL1, pfr32, 0)
+    GENERATE_TID3_INFO(ID_PFR1_EL1, pfr32, 1)
+    GENERATE_TID3_INFO(ID_PFR2_EL1, pfr32, 2)
+    GENERATE_TID3_INFO(ID_DFR0_EL1, dbg32, 0)
+    GENERATE_TID3_INFO(ID_DFR1_EL1, dbg32, 1)
+    GENERATE_TID3_INFO(ID_AFR0_EL1, aux32, 0)
+    GENERATE_TID3_INFO(ID_MMFR0_EL1, mm32, 0)
+    GENERATE_TID3_INFO(ID_MMFR1_EL1, mm32, 1)
+    GENERATE_TID3_INFO(ID_MMFR2_EL1, mm32, 2)
+    GENERATE_TID3_INFO(ID_MMFR3_EL1, mm32, 3)
+    GENERATE_TID3_INFO(ID_MMFR4_EL1, mm32, 4)
+    GENERATE_TID3_INFO(ID_MMFR5_EL1, mm32, 5)
+    GENERATE_TID3_INFO(ID_ISAR0_EL1, isa32, 0)
+    GENERATE_TID3_INFO(ID_ISAR1_EL1, isa32, 1)
+    GENERATE_TID3_INFO(ID_ISAR2_EL1, isa32, 2)
+    GENERATE_TID3_INFO(ID_ISAR3_EL1, isa32, 3)
+    GENERATE_TID3_INFO(ID_ISAR4_EL1, isa32, 4)
+    GENERATE_TID3_INFO(ID_ISAR5_EL1, isa32, 5)
+    GENERATE_TID3_INFO(ID_ISAR6_EL1, isa32, 6)
+    GENERATE_TID3_INFO(MVFR0_EL1, mvfr, 0)
+    GENERATE_TID3_INFO(MVFR1_EL1, mvfr, 1)
+    GENERATE_TID3_INFO(MVFR2_EL1, mvfr, 2)
+    GENERATE_TID3_INFO(ID_AA64PFR0_EL1, pfr64, 0)
+    GENERATE_TID3_INFO(ID_AA64PFR1_EL1, pfr64, 1)
+    GENERATE_TID3_INFO(ID_AA64DFR0_EL1, dbg64, 0)
+    GENERATE_TID3_INFO(ID_AA64DFR1_EL1, dbg64, 1)
+    GENERATE_TID3_INFO(ID_AA64ISAR0_EL1, isa64, 0)
+    GENERATE_TID3_INFO(ID_AA64ISAR1_EL1, isa64, 1)
+    GENERATE_TID3_INFO(ID_AA64MMFR0_EL1, mm64, 0)
+    GENERATE_TID3_INFO(ID_AA64MMFR1_EL1, mm64, 1)
+    GENERATE_TID3_INFO(ID_AA64MMFR2_EL1, mm64, 2)
+    GENERATE_TID3_INFO(ID_AA64AFR0_EL1, aux64, 0)
+    GENERATE_TID3_INFO(ID_AA64AFR1_EL1, aux64, 1)
+    GENERATE_TID3_INFO(ID_AA64ZFR0_EL1, zfr64, 0)
+
+    HSR_SYSREG_TID3_RESERVED_CASE:
+        /* Handle all reserved registers as RAZ */
+        return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 1);
+
     /*
      * HCR_EL2.TIDCP
      *
-- 
2.17.1

On 09/12/2020 16:30, Bertrand Marquis wrote:
> Add vsysreg emulation for registers trapped when TID3 bit is activated
> in HSR.
> The emulation is returning the value stored in cpuinfo_guest structure
> for know registers and is handling reserved registers as RAZ.
> 
> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
> ---
> Changes in V2: Rebase
> Changes in V3:
>    Fix commit message
>    Fix code style for GENERATE_TID3_INFO declaration
>    Add handling of reserved registers as RAZ.
> 
> ---
>   xen/arch/arm/arm64/vsysreg.c | 53 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 53 insertions(+)
> 
> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> index 8a85507d9d..ef7a11dbdd 100644
> --- a/xen/arch/arm/arm64/vsysreg.c
> +++ b/xen/arch/arm/arm64/vsysreg.c
> @@ -69,6 +69,14 @@ TVM_REG(CONTEXTIDR_EL1)
>           break;                                                          \
>       }
>   
> +/* Macro to generate easily case for ID co-processor emulation */
> +#define GENERATE_TID3_INFO(reg, field, offset)                          \
> +    case HSR_SYSREG_##reg:                                              \
> +    {                                                                   \
> +        return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr,   \
> +                          1, guest_cpuinfo.field.bits[offset]);         \

The indentation looks wrong here. The "1" should be aligned with "regs".

> +    }
> +
>   void do_sysreg(struct cpu_user_regs *regs,
>                  const union hsr hsr)
>   {
> @@ -259,6 +267,51 @@ void do_sysreg(struct cpu_user_regs *regs,
>            */
>           return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
>   
> +    /*
> +     * HCR_EL2.TID3
> +     *
> +     * This is trapping most Identification registers used by a guest
> +     * to identify the processor features
> +     */
> +    GENERATE_TID3_INFO(ID_PFR0_EL1, pfr32, 0)
> +    GENERATE_TID3_INFO(ID_PFR1_EL1, pfr32, 1)
> +    GENERATE_TID3_INFO(ID_PFR2_EL1, pfr32, 2)
> +    GENERATE_TID3_INFO(ID_DFR0_EL1, dbg32, 0)
> +    GENERATE_TID3_INFO(ID_DFR1_EL1, dbg32, 1)
> +    GENERATE_TID3_INFO(ID_AFR0_EL1, aux32, 0)
> +    GENERATE_TID3_INFO(ID_MMFR0_EL1, mm32, 0)
> +    GENERATE_TID3_INFO(ID_MMFR1_EL1, mm32, 1)
> +    GENERATE_TID3_INFO(ID_MMFR2_EL1, mm32, 2)
> +    GENERATE_TID3_INFO(ID_MMFR3_EL1, mm32, 3)
> +    GENERATE_TID3_INFO(ID_MMFR4_EL1, mm32, 4)
> +    GENERATE_TID3_INFO(ID_MMFR5_EL1, mm32, 5)
> +    GENERATE_TID3_INFO(ID_ISAR0_EL1, isa32, 0)
> +    GENERATE_TID3_INFO(ID_ISAR1_EL1, isa32, 1)
> +    GENERATE_TID3_INFO(ID_ISAR2_EL1, isa32, 2)
> +    GENERATE_TID3_INFO(ID_ISAR3_EL1, isa32, 3)
> +    GENERATE_TID3_INFO(ID_ISAR4_EL1, isa32, 4)
> +    GENERATE_TID3_INFO(ID_ISAR5_EL1, isa32, 5)
> +    GENERATE_TID3_INFO(ID_ISAR6_EL1, isa32, 6)
> +    GENERATE_TID3_INFO(MVFR0_EL1, mvfr, 0)
> +    GENERATE_TID3_INFO(MVFR1_EL1, mvfr, 1)
> +    GENERATE_TID3_INFO(MVFR2_EL1, mvfr, 2)
> +    GENERATE_TID3_INFO(ID_AA64PFR0_EL1, pfr64, 0)
> +    GENERATE_TID3_INFO(ID_AA64PFR1_EL1, pfr64, 1)
> +    GENERATE_TID3_INFO(ID_AA64DFR0_EL1, dbg64, 0)
> +    GENERATE_TID3_INFO(ID_AA64DFR1_EL1, dbg64, 1)
> +    GENERATE_TID3_INFO(ID_AA64ISAR0_EL1, isa64, 0)
> +    GENERATE_TID3_INFO(ID_AA64ISAR1_EL1, isa64, 1)
> +    GENERATE_TID3_INFO(ID_AA64MMFR0_EL1, mm64, 0)
> +    GENERATE_TID3_INFO(ID_AA64MMFR1_EL1, mm64, 1)
> +    GENERATE_TID3_INFO(ID_AA64MMFR2_EL1, mm64, 2)
> +    GENERATE_TID3_INFO(ID_AA64AFR0_EL1, aux64, 0)
> +    GENERATE_TID3_INFO(ID_AA64AFR1_EL1, aux64, 1)
> +    GENERATE_TID3_INFO(ID_AA64ZFR0_EL1, zfr64, 0)
> +
> +    HSR_SYSREG_TID3_RESERVED_CASE:
> +        /* Handle all reserved registers as RAZ */
> +        return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 1);
> +
>       /*
>        * HCR_EL2.TIDCP
>        *
> 

-- 
Julien Grall

Hi Julien,

> On 9 Dec 2020, at 23:13, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 09/12/2020 16:30, Bertrand Marquis wrote:
>> Add vsysreg emulation for registers trapped when TID3 bit is activated
>> in HSR.
>> The emulation is returning the value stored in cpuinfo_guest structure
>> for know registers and is handling reserved registers as RAZ.
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
>> ---
>> Changes in V2: Rebase
>> Changes in V3:
>>   Fix commit message
>>   Fix code style for GENERATE_TID3_INFO declaration
>>   Add handling of reserved registers as RAZ.
>> ---
>>  xen/arch/arm/arm64/vsysreg.c | 53 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 53 insertions(+)
>> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
>> index 8a85507d9d..ef7a11dbdd 100644
>> --- a/xen/arch/arm/arm64/vsysreg.c
>> +++ b/xen/arch/arm/arm64/vsysreg.c
>> @@ -69,6 +69,14 @@ TVM_REG(CONTEXTIDR_EL1)
>>          break;                                                          \
>>      }
>>  +/* Macro to generate easily case for ID co-processor emulation */
>> +#define GENERATE_TID3_INFO(reg, field, offset)                          \
>> +    case HSR_SYSREG_##reg:                                              \
>> +    {                                                                   \
>> +        return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr,   \
>> +                          1, guest_cpuinfo.field.bits[offset]);         \
> 
> The indentation looks wrong here. The "1" should be aligned with "regs".

Right, I will fix that in v4.

Cheers
Bertrand

> 
>> +    }
>> +
>>  void do_sysreg(struct cpu_user_regs *regs,
>>                 const union hsr hsr)
>>  {
>> @@ -259,6 +267,51 @@ void do_sysreg(struct cpu_user_regs *regs,
>>           */
>>          return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
>>  +    /*
>> +     * HCR_EL2.TID3
>> +     *
>> +     * This is trapping most Identification registers used by a guest
>> +     * to identify the processor features
>> +     */
>> +    GENERATE_TID3_INFO(ID_PFR0_EL1, pfr32, 0)
>> +    GENERATE_TID3_INFO(ID_PFR1_EL1, pfr32, 1)
>> +    GENERATE_TID3_INFO(ID_PFR2_EL1, pfr32, 2)
>> +    GENERATE_TID3_INFO(ID_DFR0_EL1, dbg32, 0)
>> +    GENERATE_TID3_INFO(ID_DFR1_EL1, dbg32, 1)
>> +    GENERATE_TID3_INFO(ID_AFR0_EL1, aux32, 0)
>> +    GENERATE_TID3_INFO(ID_MMFR0_EL1, mm32, 0)
>> +    GENERATE_TID3_INFO(ID_MMFR1_EL1, mm32, 1)
>> +    GENERATE_TID3_INFO(ID_MMFR2_EL1, mm32, 2)
>> +    GENERATE_TID3_INFO(ID_MMFR3_EL1, mm32, 3)
>> +    GENERATE_TID3_INFO(ID_MMFR4_EL1, mm32, 4)
>> +    GENERATE_TID3_INFO(ID_MMFR5_EL1, mm32, 5)
>> +    GENERATE_TID3_INFO(ID_ISAR0_EL1, isa32, 0)
>> +    GENERATE_TID3_INFO(ID_ISAR1_EL1, isa32, 1)
>> +    GENERATE_TID3_INFO(ID_ISAR2_EL1, isa32, 2)
>> +    GENERATE_TID3_INFO(ID_ISAR3_EL1, isa32, 3)
>> +    GENERATE_TID3_INFO(ID_ISAR4_EL1, isa32, 4)
>> +    GENERATE_TID3_INFO(ID_ISAR5_EL1, isa32, 5)
>> +    GENERATE_TID3_INFO(ID_ISAR6_EL1, isa32, 6)
>> +    GENERATE_TID3_INFO(MVFR0_EL1, mvfr, 0)
>> +    GENERATE_TID3_INFO(MVFR1_EL1, mvfr, 1)
>> +    GENERATE_TID3_INFO(MVFR2_EL1, mvfr, 2)
>> +    GENERATE_TID3_INFO(ID_AA64PFR0_EL1, pfr64, 0)
>> +    GENERATE_TID3_INFO(ID_AA64PFR1_EL1, pfr64, 1)
>> +    GENERATE_TID3_INFO(ID_AA64DFR0_EL1, dbg64, 0)
>> +    GENERATE_TID3_INFO(ID_AA64DFR1_EL1, dbg64, 1)
>> +    GENERATE_TID3_INFO(ID_AA64ISAR0_EL1, isa64, 0)
>> +    GENERATE_TID3_INFO(ID_AA64ISAR1_EL1, isa64, 1)
>> +    GENERATE_TID3_INFO(ID_AA64MMFR0_EL1, mm64, 0)
>> +    GENERATE_TID3_INFO(ID_AA64MMFR1_EL1, mm64, 1)
>> +    GENERATE_TID3_INFO(ID_AA64MMFR2_EL1, mm64, 2)
>> +    GENERATE_TID3_INFO(ID_AA64AFR0_EL1, aux64, 0)
>> +    GENERATE_TID3_INFO(ID_AA64AFR1_EL1, aux64, 1)
>> +    GENERATE_TID3_INFO(ID_AA64ZFR0_EL1, zfr64, 0)
>> +
>> +    HSR_SYSREG_TID3_RESERVED_CASE:
>> +        /* Handle all reserved registers as RAZ */
>> +        return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 1);
>> +
>>      /*
>>       * HCR_EL2.TIDCP
>>       *
> 
> -- 
> Julien Grall

On Wed, 9 Dec 2020, Bertrand Marquis wrote:
> Add vsysreg emulation for registers trapped when TID3 bit is activated
> in HSR.
> The emulation is returning the value stored in cpuinfo_guest structure
> for know registers and is handling reserved registers as RAZ.
> 
> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
> ---
> Changes in V2: Rebase
> Changes in V3:
>   Fix commit message
>   Fix code style for GENERATE_TID3_INFO declaration
>   Add handling of reserved registers as RAZ.
> 
> ---
>  xen/arch/arm/arm64/vsysreg.c | 53 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> index 8a85507d9d..ef7a11dbdd 100644
> --- a/xen/arch/arm/arm64/vsysreg.c
> +++ b/xen/arch/arm/arm64/vsysreg.c
> @@ -69,6 +69,14 @@ TVM_REG(CONTEXTIDR_EL1)
>          break;                                                          \
>      }
>  
> +/* Macro to generate easily case for ID co-processor emulation */
> +#define GENERATE_TID3_INFO(reg, field, offset)                          \
> +    case HSR_SYSREG_##reg:                                              \
> +    {                                                                   \
> +        return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr,   \
> +                          1, guest_cpuinfo.field.bits[offset]);         \

[...]

> +    HSR_SYSREG_TID3_RESERVED_CASE:
> +        /* Handle all reserved registers as RAZ */
> +        return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 1);


We are implementing both the known and the implementation defined
registers as read-as-zero. On write, we inject an exception.

However, reading the manual, it looks like the implementation defined
registers should be read-as-zero/write-ignore, is that right?

I couldn't easily find in the manual if it is OK to inject an exception
on write to a known register.

Hi Stefano,

> On 9 Dec 2020, at 19:38, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Wed, 9 Dec 2020, Bertrand Marquis wrote:
>> Add vsysreg emulation for registers trapped when TID3 bit is activated
>> in HSR.
>> The emulation is returning the value stored in cpuinfo_guest structure
>> for know registers and is handling reserved registers as RAZ.
>> 
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
>> ---
>> Changes in V2: Rebase
>> Changes in V3:
>>  Fix commit message
>>  Fix code style for GENERATE_TID3_INFO declaration
>>  Add handling of reserved registers as RAZ.
>> 
>> ---
>> xen/arch/arm/arm64/vsysreg.c | 53 ++++++++++++++++++++++++++++++++++++
>> 1 file changed, 53 insertions(+)
>> 
>> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
>> index 8a85507d9d..ef7a11dbdd 100644
>> --- a/xen/arch/arm/arm64/vsysreg.c
>> +++ b/xen/arch/arm/arm64/vsysreg.c
>> @@ -69,6 +69,14 @@ TVM_REG(CONTEXTIDR_EL1)
>>         break;                                                          \
>>     }
>> 
>> +/* Macro to generate easily case for ID co-processor emulation */
>> +#define GENERATE_TID3_INFO(reg, field, offset)                          \
>> +    case HSR_SYSREG_##reg:                                              \
>> +    {                                                                   \
>> +        return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr,   \
>> +                          1, guest_cpuinfo.field.bits[offset]);         \
> 
> [...]
> 
>> +    HSR_SYSREG_TID3_RESERVED_CASE:
>> +        /* Handle all reserved registers as RAZ */
>> +        return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 1);
> 
> 
> We are implementing both the known and the implementation defined
> registers as read-as-zero. On write, we inject an exception.
> 
> However, reading the manual, it looks like the implementation defined
> registers should be read-as-zero/write-ignore, is that right?

In the documentation, I did find all those defined as RO (Arm Architecture
reference manual, chapter D12.3.1). Do you think we should handle Read
only register as write ignore ? now i think of it RO does not explicitely mean
if writes are ignored or should generate an exception.

> 
> I couldn't easily find in the manual if it is OK to inject an exception
> on write to a known register.

I am actually unsure if it should or not.
I will try to run a test to check what is happening if this is done on the
real hardware and come back to you on this one.

Cheers
Bertrand

On Thu, 10 Dec 2020, Bertrand Marquis wrote:
> Hi Stefano,
> 
> > On 9 Dec 2020, at 19:38, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > 
> > On Wed, 9 Dec 2020, Bertrand Marquis wrote:
> >> Add vsysreg emulation for registers trapped when TID3 bit is activated
> >> in HSR.
> >> The emulation is returning the value stored in cpuinfo_guest structure
> >> for know registers and is handling reserved registers as RAZ.
> >> 
> >> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
> >> ---
> >> Changes in V2: Rebase
> >> Changes in V3:
> >>  Fix commit message
> >>  Fix code style for GENERATE_TID3_INFO declaration
> >>  Add handling of reserved registers as RAZ.
> >> 
> >> ---
> >> xen/arch/arm/arm64/vsysreg.c | 53 ++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 53 insertions(+)
> >> 
> >> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> >> index 8a85507d9d..ef7a11dbdd 100644
> >> --- a/xen/arch/arm/arm64/vsysreg.c
> >> +++ b/xen/arch/arm/arm64/vsysreg.c
> >> @@ -69,6 +69,14 @@ TVM_REG(CONTEXTIDR_EL1)
> >>         break;                                                          \
> >>     }
> >> 
> >> +/* Macro to generate easily case for ID co-processor emulation */
> >> +#define GENERATE_TID3_INFO(reg, field, offset)                          \
> >> +    case HSR_SYSREG_##reg:                                              \
> >> +    {                                                                   \
> >> +        return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr,   \
> >> +                          1, guest_cpuinfo.field.bits[offset]);         \
> > 
> > [...]
> > 
> >> +    HSR_SYSREG_TID3_RESERVED_CASE:
> >> +        /* Handle all reserved registers as RAZ */
> >> +        return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 1);
> > 
> > 
> > We are implementing both the known and the implementation defined
> > registers as read-as-zero. On write, we inject an exception.
> > 
> > However, reading the manual, it looks like the implementation defined
> > registers should be read-as-zero/write-ignore, is that right?
> 
> In the documentation, I did find all those defined as RO (Arm Architecture
> reference manual, chapter D12.3.1). Do you think we should handle Read
> only register as write ignore ? now i think of it RO does not explicitely mean
> if writes are ignored or should generate an exception.
> 
> > 
> > I couldn't easily find in the manual if it is OK to inject an exception
> > on write to a known register.
> 
> I am actually unsure if it should or not.
> I will try to run a test to check what is happening if this is done on the
> real hardware and come back to you on this one.

Yeah, that's the best way to do it: if writes are ignored on real
hardware, let's turn this into read-only/write-ignore, otherwise if they
generate an exception then let's keep the code as is.

Also you might want to do that both for a known register and also for an
unknown register to see if it makes a difference.

Thank you!

Hi Stefano,

> On 10 Dec 2020, at 22:29, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Thu, 10 Dec 2020, Bertrand Marquis wrote:
>> Hi Stefano,
>> 
>>> On 9 Dec 2020, at 19:38, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>> 
>>> On Wed, 9 Dec 2020, Bertrand Marquis wrote:
>>>> Add vsysreg emulation for registers trapped when TID3 bit is activated
>>>> in HSR.
>>>> The emulation is returning the value stored in cpuinfo_guest structure
>>>> for know registers and is handling reserved registers as RAZ.
>>>> 
>>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>>> ---
>>>> Changes in V2: Rebase
>>>> Changes in V3:
>>>> Fix commit message
>>>> Fix code style for GENERATE_TID3_INFO declaration
>>>> Add handling of reserved registers as RAZ.
>>>> 
>>>> ---
>>>> xen/arch/arm/arm64/vsysreg.c | 53 ++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 53 insertions(+)
>>>> 
>>>> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
>>>> index 8a85507d9d..ef7a11dbdd 100644
>>>> --- a/xen/arch/arm/arm64/vsysreg.c
>>>> +++ b/xen/arch/arm/arm64/vsysreg.c
>>>> @@ -69,6 +69,14 @@ TVM_REG(CONTEXTIDR_EL1)
>>>>        break;                                                          \
>>>>    }
>>>> 
>>>> +/* Macro to generate easily case for ID co-processor emulation */
>>>> +#define GENERATE_TID3_INFO(reg, field, offset)                          \
>>>> +    case HSR_SYSREG_##reg:                                              \
>>>> +    {                                                                   \
>>>> +        return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr,   \
>>>> +                          1, guest_cpuinfo.field.bits[offset]);         \
>>> 
>>> [...]
>>> 
>>>> +    HSR_SYSREG_TID3_RESERVED_CASE:
>>>> +        /* Handle all reserved registers as RAZ */
>>>> +        return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 1);
>>> 
>>> 
>>> We are implementing both the known and the implementation defined
>>> registers as read-as-zero. On write, we inject an exception.
>>> 
>>> However, reading the manual, it looks like the implementation defined
>>> registers should be read-as-zero/write-ignore, is that right?
>> 
>> In the documentation, I did find all those defined as RO (Arm Architecture
>> reference manual, chapter D12.3.1). Do you think we should handle Read
>> only register as write ignore ? now i think of it RO does not explicitely mean
>> if writes are ignored or should generate an exception.
>> 
>>> 
>>> I couldn't easily find in the manual if it is OK to inject an exception
>>> on write to a known register.
>> 
>> I am actually unsure if it should or not.
>> I will try to run a test to check what is happening if this is done on the
>> real hardware and come back to you on this one.
> 
> Yeah, that's the best way to do it: if writes are ignored on real
> hardware, let's turn this into read-only/write-ignore, otherwise if they
> generate an exception then let's keep the code as is.
> 
> Also you might want to do that both for a known register and also for an
> unknown register to see if it makes a difference.

I did a test with the following:
- WRITE_SYSREG64(0xf, S3_0_C0_C3_3)
- WRITE_SYSREG64(0xf, ID_MMFR0_EL1)
- WRITE_SYSREG64(0xf, ID_AA64MMFR0_EL1)

All generate exceptions like:
Hypervisor Trap. HSR=0x2000000 EC=0x0 IL=1 Syndrome=0x0

So I think it is right to generate an exception if one of them is accessed.

Regards
Bertrand

> 
> Thank you!

On Fri, 11 Dec 2020, Bertrand Marquis wrote:
> Hi Stefano,
> 
> > On 10 Dec 2020, at 22:29, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > 
> > On Thu, 10 Dec 2020, Bertrand Marquis wrote:
> >> Hi Stefano,
> >> 
> >>> On 9 Dec 2020, at 19:38, Stefano Stabellini <sstabellini@kernel.org> wrote:
> >>> 
> >>> On Wed, 9 Dec 2020, Bertrand Marquis wrote:
> >>>> Add vsysreg emulation for registers trapped when TID3 bit is activated
> >>>> in HSR.
> >>>> The emulation is returning the value stored in cpuinfo_guest structure
> >>>> for know registers and is handling reserved registers as RAZ.
> >>>> 
> >>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
> >>>> ---
> >>>> Changes in V2: Rebase
> >>>> Changes in V3:
> >>>> Fix commit message
> >>>> Fix code style for GENERATE_TID3_INFO declaration
> >>>> Add handling of reserved registers as RAZ.
> >>>> 
> >>>> ---
> >>>> xen/arch/arm/arm64/vsysreg.c | 53 ++++++++++++++++++++++++++++++++++++
> >>>> 1 file changed, 53 insertions(+)
> >>>> 
> >>>> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> >>>> index 8a85507d9d..ef7a11dbdd 100644
> >>>> --- a/xen/arch/arm/arm64/vsysreg.c
> >>>> +++ b/xen/arch/arm/arm64/vsysreg.c
> >>>> @@ -69,6 +69,14 @@ TVM_REG(CONTEXTIDR_EL1)
> >>>>        break;                                                          \
> >>>>    }
> >>>> 
> >>>> +/* Macro to generate easily case for ID co-processor emulation */
> >>>> +#define GENERATE_TID3_INFO(reg, field, offset)                          \
> >>>> +    case HSR_SYSREG_##reg:                                              \
> >>>> +    {                                                                   \
> >>>> +        return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr,   \
> >>>> +                          1, guest_cpuinfo.field.bits[offset]);         \
> >>> 
> >>> [...]
> >>> 
> >>>> +    HSR_SYSREG_TID3_RESERVED_CASE:
> >>>> +        /* Handle all reserved registers as RAZ */
> >>>> +        return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 1);
> >>> 
> >>> 
> >>> We are implementing both the known and the implementation defined
> >>> registers as read-as-zero. On write, we inject an exception.
> >>> 
> >>> However, reading the manual, it looks like the implementation defined
> >>> registers should be read-as-zero/write-ignore, is that right?
> >> 
> >> In the documentation, I did find all those defined as RO (Arm Architecture
> >> reference manual, chapter D12.3.1). Do you think we should handle Read
> >> only register as write ignore ? now i think of it RO does not explicitely mean
> >> if writes are ignored or should generate an exception.
> >> 
> >>> 
> >>> I couldn't easily find in the manual if it is OK to inject an exception
> >>> on write to a known register.
> >> 
> >> I am actually unsure if it should or not.
> >> I will try to run a test to check what is happening if this is done on the
> >> real hardware and come back to you on this one.
> > 
> > Yeah, that's the best way to do it: if writes are ignored on real
> > hardware, let's turn this into read-only/write-ignore, otherwise if they
> > generate an exception then let's keep the code as is.
> > 
> > Also you might want to do that both for a known register and also for an
> > unknown register to see if it makes a difference.
> 
> I did a test with the following:
> - WRITE_SYSREG64(0xf, S3_0_C0_C3_3)
> - WRITE_SYSREG64(0xf, ID_MMFR0_EL1)
> - WRITE_SYSREG64(0xf, ID_AA64MMFR0_EL1)
> 
> All generate exceptions like:
> Hypervisor Trap. HSR=0x2000000 EC=0x0 IL=1 Syndrome=0x0
> 
> So I think it is right to generate an exception if one of them is accessed.

Great, thanks for checking. In that case the patch is fine as is.