Re: [PATCH v3 4/7] xen/arm: Add handler for ID registers on arm64

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 10 Dec 2020, Bertrand Marquis wrote:
> Hi Stefano,
> 
> > On 9 Dec 2020, at 19:38, Stefano Stabellini <sstabellini@xxxxxxxxxx> 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@xxxxxxx>
> >> ---
> >> 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!