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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

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.

Cheers
Bertrand