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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 11 Dec 2020, Bertrand Marquis wrote:
> Hi Stefano,
> 
> > On 10 Dec 2020, at 22:29, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> > 
> > 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.
> 
> I did a test with the following:
> - WRITE_SYSREG64(0xf, S3_0_C0_C3_3)
> - WRITE_SYSREG64(0xf, ID_MMFR0_EL1)
> - WRITE_SYSREG64(0xf, ID_AA64MMFR0_EL1)
> 
> All generate exceptions like:
> Hypervisor Trap. HSR=0x2000000 EC=0x0 IL=1 Syndrome=0x0
> 
> So I think it is right to generate an exception if one of them is accessed.

Great, thanks for checking. In that case the patch is fine as is.