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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@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
> */

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

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