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

--
Julien Grall

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