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

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxx>
> > ---
> >   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,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel