[Xen-devel,RFC,10/16] xen/arm: vcpreg: Add wrappers to handle co-proc access trapped by HCR_EL2.TVM

Message ID 20181008183352.16291-11-julien.grall@arm.com
State Accepted
Commit fb1da213f33396a28e933d3facc7b5b78eff1a0f
Headers show
Series
  • xen/arm: Implement Set/Way operations
Related show

Commit Message

Julien Grall Oct. 8, 2018, 6:33 p.m.
A follow-up patch will require to emulate some accesses to some
co-processors registers trapped by HCR_EL2.TVM. When set, all NS EL1 writes
to the virtual memory control registers will be trapped to the hypervisor.

This patch adds the infrastructure to passthrough the access to host
registers. For convenience a bunch of helpers have been added to
generate the different helpers.

Note that HCR_EL2.TVM will be set in a follow-up patch dynamically.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/vcpreg.c        | 144 +++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/cpregs.h |   1 +
 2 files changed, 145 insertions(+)

Comments

Stefano Stabellini Nov. 5, 2018, 7:47 p.m. | #1
On Mon, 8 Oct 2018, Julien Grall wrote:
> A follow-up patch will require to emulate some accesses to some
> co-processors registers trapped by HCR_EL2.TVM. When set, all NS EL1 writes
> to the virtual memory control registers will be trapped to the hypervisor.
> 
> This patch adds the infrastructure to passthrough the access to host
> registers. For convenience a bunch of helpers have been added to
> generate the different helpers.
> 
> Note that HCR_EL2.TVM will be set in a follow-up patch dynamically.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/vcpreg.c        | 144 +++++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/cpregs.h |   1 +
>  2 files changed, 145 insertions(+)
> 
> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
> index b04d996fd3..49529b97cd 100644
> --- a/xen/arch/arm/vcpreg.c
> +++ b/xen/arch/arm/vcpreg.c
> @@ -24,6 +24,122 @@
>  #include <asm/traps.h>
>  #include <asm/vtimer.h>
>  
> +/*
> + * Macros to help generating helpers for registers trapped when
> + * HCR_EL2.TVM is set.
> + *
> + * Note that it only traps NS write access from EL1.
> + *
> + *  - TVM_REG() should not be used outside of the macros. It is there to
> + *    help defining TVM_REG32() and TVM_REG64()
> + *  - TVM_REG32(regname, xreg) and TVM_REG64(regname, xreg) are used to
> + *    resp. generate helper accessing 32-bit and 64-bit register. "regname"
> + *    been the Arm32 name and "xreg" the Arm64 name.
         ^ is

Please add that we use the Arm64 reg name to call WRITE_SYSREG in the
Xen source code even on Arm32 in general


> + *  - UPDATE_REG32_COMBINED(lowreg, hireg, xreg) are used to generate a

TVM_REG32_COMBINED


> + *  pair of registers share the same Arm32 registers. "lowreg" and
> + *  "higreg" been resp. the Arm32 name and "xreg" the Arm64 name. "lowreg"
> + *  will use xreg[31:0] and "hireg" will use xreg[63:32].

Please add that xreg is unused in the Arm32 case.


> + */
> +
> +/* The name is passed from the upper macro to workaround macro expansion. */
> +#define TVM_REG(sz, func, reg...)                                           \
> +static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read)    \
> +{                                                                           \
> +    GUEST_BUG_ON(read);                                                     \
> +    WRITE_SYSREG##sz(*r, reg);                                              \
> +                                                                            \
> +    return true;                                                            \
> +}
> +
> +#define TVM_REG32(regname, xreg) TVM_REG(32, vreg_emulate_##regname, xreg)
> +#define TVM_REG64(regname, xreg) TVM_REG(64, vreg_emulate_##regname, xreg)
> +
> +#ifdef CONFIG_ARM_32
> +#define TVM_REG32_COMBINED(lowreg, hireg, xreg)                     \
> +    /* Use TVM_REG directly to workaround macro expansion. */       \
> +    TVM_REG(32, vreg_emulate_##lowreg, lowreg)                      \
> +    TVM_REG(32, vreg_emulate_##hireg, hireg)
> +
> +#else /* CONFIG_ARM_64 */
> +#define TVM_REG32_COMBINED(lowreg, hireg, xreg)                             \
> +static bool vreg_emulate_##xreg(struct cpu_user_regs *regs, uint32_t *r,    \
> +                                bool read, bool hi)                         \
> +{                                                                           \
> +    register_t reg = READ_SYSREG(xreg);                                     \
> +                                                                            \
> +    GUEST_BUG_ON(read);                                                     \
> +    if ( hi ) /* reg[63:32] is AArch32 register hireg */                    \
> +    {                                                                       \
> +        reg &= GENMASK(31, 0);                                              \

Move GENMASK before the if? It's the same regardless


> +        reg |= ((uint64_t)*r) << 32;                                        \
> +    }                                                                       \
> +    else /* reg[31:0] is AArch32 register lowreg. */                        \
> +    {                                                                       \
> +        reg &= GENMASK(31, 0);                                              \
> +        reg |= *r;                                                          \
> +    }                                                                       \
> +    WRITE_SYSREG(reg, xreg);                                                \
> +                                                                            \
> +    return true;                                                            \
> +}                                                                           \
> +                                                                            \
> +static bool vreg_emulate_##lowreg(struct cpu_user_regs *regs, uint32_t *r,  \
> +                                  bool read)                                \
> +{                                                                           \
> +    return vreg_emulate_##xreg(regs, r, read, false);                       \
> +}                                                                           \
> +                                                                            \
> +static bool vreg_emulate_##hireg(struct cpu_user_regs *regs, uint32_t *r,   \
> +                                 bool read)                                 \
> +{                                                                           \
> +    return vreg_emulate_##xreg(regs, r, read, true);                        \
> +}
> +#endif
> +
> +/* Defining helpers for emulating co-processor registers. */
> +TVM_REG32(SCTLR, SCTLR_EL1)
> +/*
> + * AArch32 provides two way to access TTBR* depending on the access
> + * size, whilst AArch64 provides one way.
> + *
> + * When using AArch32, for simplicity, use the same access size as the
> + * guest.
> + */
> +#ifdef CONFIG_ARM_32
> +TVM_REG32(TTBR0_32, TTBR0_32)
> +TVM_REG32(TTBR1_32, TTBR1_32)
> +#else
> +TVM_REG32(TTBR0_32, TTBR0_EL1)
> +TVM_REG32(TTBR1_32, TTBR1_EL1)
> +#endif
> +TVM_REG64(TTBR0, TTBR0_EL1)
> +TVM_REG64(TTBR1, TTBR1_EL1)
> +/* AArch32 registers TTBCR and TTBCR2 share AArch64 register TCR_EL1. */
> +TVM_REG32_COMBINED(TTBCR, TTBCR2, TCR_EL1)
> +TVM_REG32(DACR, DACR32_EL2)
> +TVM_REG32(DFSR, ESR_EL1)
> +TVM_REG32(IFSR, IFSR32_EL2)
> +/* AArch32 registers DFAR and IFAR shares AArch64 register FAR_EL1. */
> +TVM_REG32_COMBINED(DFAR, IFAR, FAR_EL1)
> +TVM_REG32(ADFSR, AFSR0_EL1)
> +TVM_REG32(AIFSR, AFSR1_EL1)
> +/* AArch32 registers MAIR0 and MAIR1 share AArch64 register MAIR_EL1. */
> +TVM_REG32_COMBINED(MAIR0, MAIR1, MAIR_EL1)
> +/* AArch32 registers AMAIR0 and AMAIR1 share AArch64 register AMAIR_EL1. */
> +TVM_REG32_COMBINED(AMAIR0, AMAIR1, AMAIR_EL1)
> +TVM_REG32(CONTEXTIDR, CONTEXTIDR_EL1)
> +
> +/* Macro to generate easily case for co-processor emulation. */
> +#define GENERATE_CASE(reg, sz)                                      \
> +    case HSR_CPREG##sz(reg):                                        \
> +    {                                                               \
> +        bool res;                                                   \
> +                                                                    \
> +        res = vreg_emulate_cp##sz(regs, hsr, vreg_emulate_##reg);   \
> +        ASSERT(res);                                                \
> +        break;                                                      \
> +    }
> +
>  void do_cp15_32(struct cpu_user_regs *regs, const union hsr hsr)
>  {
>      const struct hsr_cp32 cp32 = hsr.cp32;
> @@ -64,6 +180,31 @@ void do_cp15_32(struct cpu_user_regs *regs, const union hsr hsr)
>          break;
>  
>      /*
> +     * HCR_EL2.TVM
> +     *
> +     * ARMv8 (DDI 0487B.b): Table D1-37

In 0487D.a is D1-99


> +     */
> +    GENERATE_CASE(SCTLR, 32)
> +    GENERATE_CASE(TTBR0_32, 32)
> +    GENERATE_CASE(TTBR1_32, 32)
> +    GENERATE_CASE(TTBCR, 32)
> +    GENERATE_CASE(TTBCR2, 32)
> +    GENERATE_CASE(DACR, 32)
> +    GENERATE_CASE(DFSR, 32)
> +    GENERATE_CASE(IFSR, 32)
> +    GENERATE_CASE(DFAR, 32)
> +    GENERATE_CASE(IFAR, 32)
> +    GENERATE_CASE(ADFSR, 32)
> +    GENERATE_CASE(AIFSR, 32)
> +    /* AKA PRRR */
> +    GENERATE_CASE(MAIR0, 32)
> +    /* AKA NMRR */
> +    GENERATE_CASE(MAIR1, 32)
> +    GENERATE_CASE(AMAIR0, 32)
> +    GENERATE_CASE(AMAIR1, 32)
> +    GENERATE_CASE(CONTEXTIDR, 32)
> +
> +    /*
>       * MDCR_EL2.TPM
>       *
>       * ARMv7 (DDI 0406C.b): B1.14.17
> @@ -192,6 +333,9 @@ void do_cp15_64(struct cpu_user_regs *regs, const union hsr hsr)
>              return inject_undef_exception(regs, hsr);
>          break;
>  
> +    GENERATE_CASE(TTBR0, 64)
> +    GENERATE_CASE(TTBR1, 64)
> +
>      /*
>       * CPTR_EL2.T{0..9,12..13}
>       *
> diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
> index 07e5791983..f1cbac5e5d 100644
> --- a/xen/include/asm-arm/cpregs.h
> +++ b/xen/include/asm-arm/cpregs.h
> @@ -142,6 +142,7 @@
>  
>  /* CP15 CR2: Translation Table Base and Control Registers */
>  #define TTBCR           p15,0,c2,c0,2   /* Translation Table Base Control Register */
> +#define TTBCR2          p15,0,c2,c0,3   /* Translation Table Base Control Register 2 */
>  #define TTBR0           p15,0,c2        /* Translation Table Base Reg. 0 */
>  #define TTBR1           p15,1,c2        /* Translation Table Base Reg. 1 */
>  #define HTTBR           p15,4,c2        /* Hyp. Translation Table Base Register */
> -- 
> 2.11.0
>
Julien Grall Nov. 5, 2018, 11:21 p.m. | #2
Hi Stefano,

On 11/5/18 7:47 PM, Stefano Stabellini wrote:
> On Mon, 8 Oct 2018, Julien Grall wrote:
>> A follow-up patch will require to emulate some accesses to some
>> co-processors registers trapped by HCR_EL2.TVM. When set, all NS EL1 writes
>> to the virtual memory control registers will be trapped to the hypervisor.
>>
>> This patch adds the infrastructure to passthrough the access to host
>> registers. For convenience a bunch of helpers have been added to
>> generate the different helpers.
>>
>> Note that HCR_EL2.TVM will be set in a follow-up patch dynamically.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/vcpreg.c        | 144 +++++++++++++++++++++++++++++++++++++++++++
>>   xen/include/asm-arm/cpregs.h |   1 +
>>   2 files changed, 145 insertions(+)
>>
>> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
>> index b04d996fd3..49529b97cd 100644
>> --- a/xen/arch/arm/vcpreg.c
>> +++ b/xen/arch/arm/vcpreg.c
>> @@ -24,6 +24,122 @@
>>   #include <asm/traps.h>
>>   #include <asm/vtimer.h>
>>   
>> +/*
>> + * Macros to help generating helpers for registers trapped when
>> + * HCR_EL2.TVM is set.
>> + *
>> + * Note that it only traps NS write access from EL1.
>> + *
>> + *  - TVM_REG() should not be used outside of the macros. It is there to
>> + *    help defining TVM_REG32() and TVM_REG64()
>> + *  - TVM_REG32(regname, xreg) and TVM_REG64(regname, xreg) are used to
>> + *    resp. generate helper accessing 32-bit and 64-bit register. "regname"
>> + *    been the Arm32 name and "xreg" the Arm64 name.
>           ^ is
> 
> Please add that we use the Arm64 reg name to call WRITE_SYSREG in the
> Xen source code even on Arm32 in general

I am not sure to understand this. It is common use in Xen to use arm64 
name when code is for both architecture. So why would I need a specific 
comment here?

> 
>> + *  - UPDATE_REG32_COMBINED(lowreg, hireg, xreg) are used to generate a
> 
> TVM_REG32_COMBINED
> 
> 
>> + *  pair of registers share the same Arm32 registers. "lowreg" and
>> + *  "higreg" been resp. the Arm32 name and "xreg" the Arm64 name. "lowreg"
>> + *  will use xreg[31:0] and "hireg" will use xreg[63:32].
> 
> Please add that xreg is unused in the Arm32 case.

Why do you think that? xreg is actually used. It will get expanded to 
whatever is the co-processor encoding and caught by reg... in TVM_REG().

> 
> 
>> + */
>> +
>> +/* The name is passed from the upper macro to workaround macro expansion. */
>> +#define TVM_REG(sz, func, reg...)                                           \
>> +static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read)    \
>> +{                                                                           \
>> +    GUEST_BUG_ON(read);                                                     \
>> +    WRITE_SYSREG##sz(*r, reg);                                              \
>> +                                                                            \
>> +    return true;                                                            \
>> +}
>> +
>> +#define TVM_REG32(regname, xreg) TVM_REG(32, vreg_emulate_##regname, xreg)
>> +#define TVM_REG64(regname, xreg) TVM_REG(64, vreg_emulate_##regname, xreg)
>> +
>> +#ifdef CONFIG_ARM_32
>> +#define TVM_REG32_COMBINED(lowreg, hireg, xreg)                     \
>> +    /* Use TVM_REG directly to workaround macro expansion. */       \
>> +    TVM_REG(32, vreg_emulate_##lowreg, lowreg)                      \
>> +    TVM_REG(32, vreg_emulate_##hireg, hireg)
>> +
>> +#else /* CONFIG_ARM_64 */
>> +#define TVM_REG32_COMBINED(lowreg, hireg, xreg)                             \
>> +static bool vreg_emulate_##xreg(struct cpu_user_regs *regs, uint32_t *r,    \
>> +                                bool read, bool hi)                         \
>> +{                                                                           \
>> +    register_t reg = READ_SYSREG(xreg);                                     \
>> +                                                                            \
>> +    GUEST_BUG_ON(read);                                                     \
>> +    if ( hi ) /* reg[63:32] is AArch32 register hireg */                    \
>> +    {                                                                       \
>> +        reg &= GENMASK(31, 0);                                              \
> 
> Move GENMASK before the if? It's the same regardless

Actually, the second GENMASK is incorrect. It should have been 
GENMASK(63, 32) as we want to update only the lowreg.

So I will fix the mask instead.

>>       /*
>> +     * HCR_EL2.TVM
>> +     *
>> +     * ARMv8 (DDI 0487B.b): Table D1-37
> 
> In 0487D.a is D1-99

I haven't had the chance to download the latest spec (it was released 
last week). I will update to the new spec.

Cheers,
Stefano Stabellini Nov. 6, 2018, 5:36 p.m. | #3
On Mon, 5 Nov 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 11/5/18 7:47 PM, Stefano Stabellini wrote:
> > On Mon, 8 Oct 2018, Julien Grall wrote:
> > > A follow-up patch will require to emulate some accesses to some
> > > co-processors registers trapped by HCR_EL2.TVM. When set, all NS EL1
> > > writes
> > > to the virtual memory control registers will be trapped to the hypervisor.
> > > 
> > > This patch adds the infrastructure to passthrough the access to host
> > > registers. For convenience a bunch of helpers have been added to
> > > generate the different helpers.
> > > 
> > > Note that HCR_EL2.TVM will be set in a follow-up patch dynamically.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > ---
> > >   xen/arch/arm/vcpreg.c        | 144
> > > +++++++++++++++++++++++++++++++++++++++++++
> > >   xen/include/asm-arm/cpregs.h |   1 +
> > >   2 files changed, 145 insertions(+)
> > > 
> > > diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
> > > index b04d996fd3..49529b97cd 100644
> > > --- a/xen/arch/arm/vcpreg.c
> > > +++ b/xen/arch/arm/vcpreg.c
> > > @@ -24,6 +24,122 @@
> > >   #include <asm/traps.h>
> > >   #include <asm/vtimer.h>
> > >   +/*
> > > + * Macros to help generating helpers for registers trapped when
> > > + * HCR_EL2.TVM is set.
> > > + *
> > > + * Note that it only traps NS write access from EL1.
> > > + *
> > > + *  - TVM_REG() should not be used outside of the macros. It is there to
> > > + *    help defining TVM_REG32() and TVM_REG64()
> > > + *  - TVM_REG32(regname, xreg) and TVM_REG64(regname, xreg) are used to
> > > + *    resp. generate helper accessing 32-bit and 64-bit register.
> > > "regname"
> > > + *    been the Arm32 name and "xreg" the Arm64 name.
> >           ^ is
> > 
> > Please add that we use the Arm64 reg name to call WRITE_SYSREG in the
> > Xen source code even on Arm32 in general
> 
> I am not sure to understand this. It is common use in Xen to use arm64 name
> when code is for both architecture. So why would I need a specific comment
> here?

Yes, that's our convention, but as I was looking through the code, I
couldn't quickly find any places where we wrote the convention down. Is
there? I thought it would be good to start somewhere, this could be a
good place as any, also given that it directly affects this code.


> > > + *  - UPDATE_REG32_COMBINED(lowreg, hireg, xreg) are used to generate a
> > 
> > TVM_REG32_COMBINED
> > 
> > 
> > > + *  pair of registers share the same Arm32 registers. "lowreg" and
> > > + *  "higreg" been resp. the Arm32 name and "xreg" the Arm64 name.
> > > "lowreg"
> > > + *  will use xreg[31:0] and "hireg" will use xreg[63:32].
> > 
> > Please add that xreg is unused in the Arm32 case.
> 
> Why do you think that? xreg is actually used. It will get expanded to whatever
> is the co-processor encoding and caught by reg... in TVM_REG().

It is unused in the TVM_REG32_COMBINED case, which is the comment part I
was replying about. This is the code:

  #define TVM_REG32_COMBINED(lowreg, hireg, xreg)                     \
      /* Use TVM_REG directly to workaround macro expansion. */       \
      TVM_REG(32, vreg_emulate_##lowreg, lowreg)                      \
      TVM_REG(32, vreg_emulate_##hireg, hireg)

xreg is not used?
Julien Grall Nov. 6, 2018, 5:52 p.m. | #4
Hi Stefano,

On 06/11/2018 17:36, Stefano Stabellini wrote:
> On Mon, 5 Nov 2018, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 11/5/18 7:47 PM, Stefano Stabellini wrote:
>>> On Mon, 8 Oct 2018, Julien Grall wrote:
>>>> A follow-up patch will require to emulate some accesses to some
>>>> co-processors registers trapped by HCR_EL2.TVM. When set, all NS EL1
>>>> writes
>>>> to the virtual memory control registers will be trapped to the hypervisor.
>>>>
>>>> This patch adds the infrastructure to passthrough the access to host
>>>> registers. For convenience a bunch of helpers have been added to
>>>> generate the different helpers.
>>>>
>>>> Note that HCR_EL2.TVM will be set in a follow-up patch dynamically.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>> ---
>>>>    xen/arch/arm/vcpreg.c        | 144
>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>    xen/include/asm-arm/cpregs.h |   1 +
>>>>    2 files changed, 145 insertions(+)
>>>>
>>>> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
>>>> index b04d996fd3..49529b97cd 100644
>>>> --- a/xen/arch/arm/vcpreg.c
>>>> +++ b/xen/arch/arm/vcpreg.c
>>>> @@ -24,6 +24,122 @@
>>>>    #include <asm/traps.h>
>>>>    #include <asm/vtimer.h>
>>>>    +/*
>>>> + * Macros to help generating helpers for registers trapped when
>>>> + * HCR_EL2.TVM is set.
>>>> + *
>>>> + * Note that it only traps NS write access from EL1.
>>>> + *
>>>> + *  - TVM_REG() should not be used outside of the macros. It is there to
>>>> + *    help defining TVM_REG32() and TVM_REG64()
>>>> + *  - TVM_REG32(regname, xreg) and TVM_REG64(regname, xreg) are used to
>>>> + *    resp. generate helper accessing 32-bit and 64-bit register.
>>>> "regname"
>>>> + *    been the Arm32 name and "xreg" the Arm64 name.
>>>            ^ is
>>>
>>> Please add that we use the Arm64 reg name to call WRITE_SYSREG in the
>>> Xen source code even on Arm32 in general
>>
>> I am not sure to understand this. It is common use in Xen to use arm64 name
>> when code is for both architecture. So why would I need a specific comment
>> here?
> 
> Yes, that's our convention, but as I was looking through the code, I
> couldn't quickly find any places where we wrote the convention down. Is
> there? I thought it would be good to start somewhere, this could be a
> good place as any, also given that it directly affects this code.

include/asm-arm/cpregs.h:

/* Aliases of AArch64 names for use in common code when building for AArch32 */

> 
> 
>>>> + *  - UPDATE_REG32_COMBINED(lowreg, hireg, xreg) are used to generate a
>>>
>>> TVM_REG32_COMBINED
>>>
>>>
>>>> + *  pair of registers share the same Arm32 registers. "lowreg" and
>>>> + *  "higreg" been resp. the Arm32 name and "xreg" the Arm64 name.
>>>> "lowreg"
>>>> + *  will use xreg[31:0] and "hireg" will use xreg[63:32].
>>>
>>> Please add that xreg is unused in the Arm32 case.
>>
>> Why do you think that? xreg is actually used. It will get expanded to whatever
>> is the co-processor encoding and caught by reg... in TVM_REG().
> 
> It is unused in the TVM_REG32_COMBINED case, which is the comment part I
> was replying about. This is the code:
> 
>    #define TVM_REG32_COMBINED(lowreg, hireg, xreg)                     \
>        /* Use TVM_REG directly to workaround macro expansion. */       \
>        TVM_REG(32, vreg_emulate_##lowreg, lowreg)                      \
>        TVM_REG(32, vreg_emulate_##hireg, hireg)
> 
> xreg is not used?

Hrm it is used in that case. I am got confused. How about the following:

TVM_REG32_COMBINED(lowreg, hireg, xreg) are used to generate a
pair of register sharing the same Arm64 register, but are 2 distinct Arm32 
registers. "lowreg" and "hireg" contains the name for on Arm32 registers,
"xreg" contains the name for the combined register on Arm64. The definition of 
"lowreg" and "higreg" match the Armv8 specification, this means "lowreg" is an 
alias to xreg[31:0] and "high" is an alias to xreg[63:32].

Cheers,
Stefano Stabellini Nov. 6, 2018, 5:56 p.m. | #5
On Tue, 6 Nov 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 06/11/2018 17:36, Stefano Stabellini wrote:
> > On Mon, 5 Nov 2018, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > On 11/5/18 7:47 PM, Stefano Stabellini wrote:
> > > > On Mon, 8 Oct 2018, Julien Grall wrote:
> > > > > A follow-up patch will require to emulate some accesses to some
> > > > > co-processors registers trapped by HCR_EL2.TVM. When set, all NS EL1
> > > > > writes
> > > > > to the virtual memory control registers will be trapped to the
> > > > > hypervisor.
> > > > > 
> > > > > This patch adds the infrastructure to passthrough the access to host
> > > > > registers. For convenience a bunch of helpers have been added to
> > > > > generate the different helpers.
> > > > > 
> > > > > Note that HCR_EL2.TVM will be set in a follow-up patch dynamically.
> > > > > 
> > > > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > > > ---
> > > > >    xen/arch/arm/vcpreg.c        | 144
> > > > > +++++++++++++++++++++++++++++++++++++++++++
> > > > >    xen/include/asm-arm/cpregs.h |   1 +
> > > > >    2 files changed, 145 insertions(+)
> > > > > 
> > > > > diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
> > > > > index b04d996fd3..49529b97cd 100644
> > > > > --- a/xen/arch/arm/vcpreg.c
> > > > > +++ b/xen/arch/arm/vcpreg.c
> > > > > @@ -24,6 +24,122 @@
> > > > >    #include <asm/traps.h>
> > > > >    #include <asm/vtimer.h>
> > > > >    +/*
> > > > > + * Macros to help generating helpers for registers trapped when
> > > > > + * HCR_EL2.TVM is set.
> > > > > + *
> > > > > + * Note that it only traps NS write access from EL1.
> > > > > + *
> > > > > + *  - TVM_REG() should not be used outside of the macros. It is there
> > > > > to
> > > > > + *    help defining TVM_REG32() and TVM_REG64()
> > > > > + *  - TVM_REG32(regname, xreg) and TVM_REG64(regname, xreg) are used
> > > > > to
> > > > > + *    resp. generate helper accessing 32-bit and 64-bit register.
> > > > > "regname"
> > > > > + *    been the Arm32 name and "xreg" the Arm64 name.
> > > >            ^ is
> > > > 
> > > > Please add that we use the Arm64 reg name to call WRITE_SYSREG in the
> > > > Xen source code even on Arm32 in general
> > > 
> > > I am not sure to understand this. It is common use in Xen to use arm64
> > > name
> > > when code is for both architecture. So why would I need a specific comment
> > > here?
> > 
> > Yes, that's our convention, but as I was looking through the code, I
> > couldn't quickly find any places where we wrote the convention down. Is
> > there? I thought it would be good to start somewhere, this could be a
> > good place as any, also given that it directly affects this code.
> 
> include/asm-arm/cpregs.h:
> 
> /* Aliases of AArch64 names for use in common code when building for AArch32
> */

Ops X-)
Maybe add reference to it? Fine either way.


> > 
> > 
> > > > > + *  - UPDATE_REG32_COMBINED(lowreg, hireg, xreg) are used to generate
> > > > > a
> > > > 
> > > > TVM_REG32_COMBINED
> > > > 
> > > > 
> > > > > + *  pair of registers share the same Arm32 registers. "lowreg" and
> > > > > + *  "higreg" been resp. the Arm32 name and "xreg" the Arm64 name.
> > > > > "lowreg"
> > > > > + *  will use xreg[31:0] and "hireg" will use xreg[63:32].
> > > > 
> > > > Please add that xreg is unused in the Arm32 case.
> > > 
> > > Why do you think that? xreg is actually used. It will get expanded to
> > > whatever
> > > is the co-processor encoding and caught by reg... in TVM_REG().
> > 
> > It is unused in the TVM_REG32_COMBINED case, which is the comment part I
> > was replying about. This is the code:
> > 
> >    #define TVM_REG32_COMBINED(lowreg, hireg, xreg)                     \
> >        /* Use TVM_REG directly to workaround macro expansion. */       \
> >        TVM_REG(32, vreg_emulate_##lowreg, lowreg)                      \
> >        TVM_REG(32, vreg_emulate_##hireg, hireg)
> > 
> > xreg is not used?
> 
> Hrm it is used in that case. I am got confused. How about the following:
> 
> TVM_REG32_COMBINED(lowreg, hireg, xreg) are used to generate a
> pair of register sharing the same Arm64 register, but are 2 distinct Arm32
> registers. "lowreg" and "hireg" contains the name for on Arm32 registers,
> "xreg" contains the name for the combined register on Arm64. The definition of
> "lowreg" and "higreg" match the Armv8 specification, this means "lowreg" is an
> alias to xreg[31:0] and "high" is an alias to xreg[63:32].

Sounds good
Julien Grall Dec. 4, 2018, 4:24 p.m. | #6
On 11/5/18 11:21 PM, Julien Grall wrote:
> On 11/5/18 7:47 PM, Stefano Stabellini wrote:
>> On Mon, 8 Oct 2018, Julien Grall wrote:
>>>       /*
>>> +     * HCR_EL2.TVM
>>> +     *
>>> +     * ARMv8 (DDI 0487B.b): Table D1-37
>>
>> In 0487D.a is D1-99
> 
> I haven't had the chance to download the latest spec (it was released 
> last week). I will update to the new spec.

Actually, the table you point does not correspond to D1-37 in the 
version B.b. The table is D1-38.

Cheers,

Patch

diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
index b04d996fd3..49529b97cd 100644
--- a/xen/arch/arm/vcpreg.c
+++ b/xen/arch/arm/vcpreg.c
@@ -24,6 +24,122 @@ 
 #include <asm/traps.h>
 #include <asm/vtimer.h>
 
+/*
+ * Macros to help generating helpers for registers trapped when
+ * HCR_EL2.TVM is set.
+ *
+ * Note that it only traps NS write access from EL1.
+ *
+ *  - TVM_REG() should not be used outside of the macros. It is there to
+ *    help defining TVM_REG32() and TVM_REG64()
+ *  - TVM_REG32(regname, xreg) and TVM_REG64(regname, xreg) are used to
+ *    resp. generate helper accessing 32-bit and 64-bit register. "regname"
+ *    been the Arm32 name and "xreg" the Arm64 name.
+ *  - UPDATE_REG32_COMBINED(lowreg, hireg, xreg) are used to generate a
+ *  pair of registers share the same Arm32 registers. "lowreg" and
+ *  "higreg" been resp. the Arm32 name and "xreg" the Arm64 name. "lowreg"
+ *  will use xreg[31:0] and "hireg" will use xreg[63:32].
+ */
+
+/* The name is passed from the upper macro to workaround macro expansion. */
+#define TVM_REG(sz, func, reg...)                                           \
+static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read)    \
+{                                                                           \
+    GUEST_BUG_ON(read);                                                     \
+    WRITE_SYSREG##sz(*r, reg);                                              \
+                                                                            \
+    return true;                                                            \
+}
+
+#define TVM_REG32(regname, xreg) TVM_REG(32, vreg_emulate_##regname, xreg)
+#define TVM_REG64(regname, xreg) TVM_REG(64, vreg_emulate_##regname, xreg)
+
+#ifdef CONFIG_ARM_32
+#define TVM_REG32_COMBINED(lowreg, hireg, xreg)                     \
+    /* Use TVM_REG directly to workaround macro expansion. */       \
+    TVM_REG(32, vreg_emulate_##lowreg, lowreg)                      \
+    TVM_REG(32, vreg_emulate_##hireg, hireg)
+
+#else /* CONFIG_ARM_64 */
+#define TVM_REG32_COMBINED(lowreg, hireg, xreg)                             \
+static bool vreg_emulate_##xreg(struct cpu_user_regs *regs, uint32_t *r,    \
+                                bool read, bool hi)                         \
+{                                                                           \
+    register_t reg = READ_SYSREG(xreg);                                     \
+                                                                            \
+    GUEST_BUG_ON(read);                                                     \
+    if ( hi ) /* reg[63:32] is AArch32 register hireg */                    \
+    {                                                                       \
+        reg &= GENMASK(31, 0);                                              \
+        reg |= ((uint64_t)*r) << 32;                                        \
+    }                                                                       \
+    else /* reg[31:0] is AArch32 register lowreg. */                        \
+    {                                                                       \
+        reg &= GENMASK(31, 0);                                              \
+        reg |= *r;                                                          \
+    }                                                                       \
+    WRITE_SYSREG(reg, xreg);                                                \
+                                                                            \
+    return true;                                                            \
+}                                                                           \
+                                                                            \
+static bool vreg_emulate_##lowreg(struct cpu_user_regs *regs, uint32_t *r,  \
+                                  bool read)                                \
+{                                                                           \
+    return vreg_emulate_##xreg(regs, r, read, false);                       \
+}                                                                           \
+                                                                            \
+static bool vreg_emulate_##hireg(struct cpu_user_regs *regs, uint32_t *r,   \
+                                 bool read)                                 \
+{                                                                           \
+    return vreg_emulate_##xreg(regs, r, read, true);                        \
+}
+#endif
+
+/* Defining helpers for emulating co-processor registers. */
+TVM_REG32(SCTLR, SCTLR_EL1)
+/*
+ * AArch32 provides two way to access TTBR* depending on the access
+ * size, whilst AArch64 provides one way.
+ *
+ * When using AArch32, for simplicity, use the same access size as the
+ * guest.
+ */
+#ifdef CONFIG_ARM_32
+TVM_REG32(TTBR0_32, TTBR0_32)
+TVM_REG32(TTBR1_32, TTBR1_32)
+#else
+TVM_REG32(TTBR0_32, TTBR0_EL1)
+TVM_REG32(TTBR1_32, TTBR1_EL1)
+#endif
+TVM_REG64(TTBR0, TTBR0_EL1)
+TVM_REG64(TTBR1, TTBR1_EL1)
+/* AArch32 registers TTBCR and TTBCR2 share AArch64 register TCR_EL1. */
+TVM_REG32_COMBINED(TTBCR, TTBCR2, TCR_EL1)
+TVM_REG32(DACR, DACR32_EL2)
+TVM_REG32(DFSR, ESR_EL1)
+TVM_REG32(IFSR, IFSR32_EL2)
+/* AArch32 registers DFAR and IFAR shares AArch64 register FAR_EL1. */
+TVM_REG32_COMBINED(DFAR, IFAR, FAR_EL1)
+TVM_REG32(ADFSR, AFSR0_EL1)
+TVM_REG32(AIFSR, AFSR1_EL1)
+/* AArch32 registers MAIR0 and MAIR1 share AArch64 register MAIR_EL1. */
+TVM_REG32_COMBINED(MAIR0, MAIR1, MAIR_EL1)
+/* AArch32 registers AMAIR0 and AMAIR1 share AArch64 register AMAIR_EL1. */
+TVM_REG32_COMBINED(AMAIR0, AMAIR1, AMAIR_EL1)
+TVM_REG32(CONTEXTIDR, CONTEXTIDR_EL1)
+
+/* Macro to generate easily case for co-processor emulation. */
+#define GENERATE_CASE(reg, sz)                                      \
+    case HSR_CPREG##sz(reg):                                        \
+    {                                                               \
+        bool res;                                                   \
+                                                                    \
+        res = vreg_emulate_cp##sz(regs, hsr, vreg_emulate_##reg);   \
+        ASSERT(res);                                                \
+        break;                                                      \
+    }
+
 void do_cp15_32(struct cpu_user_regs *regs, const union hsr hsr)
 {
     const struct hsr_cp32 cp32 = hsr.cp32;
@@ -64,6 +180,31 @@  void do_cp15_32(struct cpu_user_regs *regs, const union hsr hsr)
         break;
 
     /*
+     * HCR_EL2.TVM
+     *
+     * ARMv8 (DDI 0487B.b): Table D1-37
+     */
+    GENERATE_CASE(SCTLR, 32)
+    GENERATE_CASE(TTBR0_32, 32)
+    GENERATE_CASE(TTBR1_32, 32)
+    GENERATE_CASE(TTBCR, 32)
+    GENERATE_CASE(TTBCR2, 32)
+    GENERATE_CASE(DACR, 32)
+    GENERATE_CASE(DFSR, 32)
+    GENERATE_CASE(IFSR, 32)
+    GENERATE_CASE(DFAR, 32)
+    GENERATE_CASE(IFAR, 32)
+    GENERATE_CASE(ADFSR, 32)
+    GENERATE_CASE(AIFSR, 32)
+    /* AKA PRRR */
+    GENERATE_CASE(MAIR0, 32)
+    /* AKA NMRR */
+    GENERATE_CASE(MAIR1, 32)
+    GENERATE_CASE(AMAIR0, 32)
+    GENERATE_CASE(AMAIR1, 32)
+    GENERATE_CASE(CONTEXTIDR, 32)
+
+    /*
      * MDCR_EL2.TPM
      *
      * ARMv7 (DDI 0406C.b): B1.14.17
@@ -192,6 +333,9 @@  void do_cp15_64(struct cpu_user_regs *regs, const union hsr hsr)
             return inject_undef_exception(regs, hsr);
         break;
 
+    GENERATE_CASE(TTBR0, 64)
+    GENERATE_CASE(TTBR1, 64)
+
     /*
      * CPTR_EL2.T{0..9,12..13}
      *
diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
index 07e5791983..f1cbac5e5d 100644
--- a/xen/include/asm-arm/cpregs.h
+++ b/xen/include/asm-arm/cpregs.h
@@ -142,6 +142,7 @@ 
 
 /* CP15 CR2: Translation Table Base and Control Registers */
 #define TTBCR           p15,0,c2,c0,2   /* Translation Table Base Control Register */
+#define TTBCR2          p15,0,c2,c0,3   /* Translation Table Base Control Register 2 */
 #define TTBR0           p15,0,c2        /* Translation Table Base Reg. 0 */
 #define TTBR1           p15,1,c2        /* Translation Table Base Reg. 1 */
 #define HTTBR           p15,4,c2        /* Hyp. Translation Table Base Register */