Re: [PATCH v3 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Jan,

On 05.05.2021 10:00, Jan Beulich wrote:
> On 05.05.2021 09:43, Michal Orzel wrote:
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -267,10 +267,10 @@ struct vcpu_guest_core_regs
>>  
>>      /* Return address and mode */
>>      __DECL_REG(pc64,         pc32);             /* ELR_EL2 */
>> -    uint32_t cpsr;                              /* SPSR_EL2 */
>> +    uint64_t cpsr;                              /* SPSR_EL2 */
>>  
>>      union {
>> -        uint32_t spsr_el1;       /* AArch64 */
>> +        uint64_t spsr_el1;       /* AArch64 */
>>          uint32_t spsr_svc;       /* AArch32 */
>>      };
> 
> This change affects, besides domctl, also default_initialise_vcpu(),
> which Arm's arch_initialise_vcpu() calls. I realize do_arm_vcpu_op()
> only allows two unrelated VCPUOP_* to pass, but then I don't
> understand why arch_initialise_vcpu() doesn't simply return e.g.
> -EOPNOTSUPP. Hence I suspect I'm missing something.
> 
I agree that do_arm_vcpu_op only allows two VCPUOP* to pass and
arch_initialise_vcpu being called in case of VCPUOP_initialise
has no sense as VCPUOP_initialise is not supported on arm.
It makes sense that it should return -EOPNOTSUPP.
However do_arm_vcpu_op will not accept VCPUOP_initialise and will return
-EINVAL. So arch_initialise_vcpu for arm will not be called.
Do you think that changing this behaviour so that arch_initialise_vcpu returns
-EOPNOTSUPP should be part of this patch?

>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -38,7 +38,7 @@
>>  #include "hvm/save.h"
>>  #include "memory.h"
>>  
>> -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000013
>> +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000014
> 
> So this is to cover for the struct vcpu_guest_core_regs change.
> 
>> --- a/xen/include/public/vm_event.h
>> +++ b/xen/include/public/vm_event.h
>> @@ -266,8 +266,7 @@ struct vm_event_regs_arm {
>>      uint64_t ttbr1;
>>      uint64_t ttbcr;
>>      uint64_t pc;
>> -    uint32_t cpsr;
>> -    uint32_t _pad;
>> +    uint64_t cpsr;
>>  };
> 
> Then I wonder why this isn't accompanied by a similar bump of
> VM_EVENT_INTERFACE_VERSION. I don't see you drop any checking /
> filling of the _pad field, so existing callers may pass garbage
> there, and new callers need to be prevented from looking at the
> upper half when running on an older hypervisor.
> 
> Jan
> 
Cheers,
Michal