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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

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.

Regards
Bertrand

> 
> Thank you!