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

[Julien Grall <julien@xen.org>]

Hi Jan,

On 17/05/2021 08:01, Jan Beulich wrote:
> On 12.05.2021 19:59, Julien Grall wrote:
>> Hi,
>>
>> On 11/05/2021 07:37, Michal Orzel wrote:
>>> 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 think it is just an overlooked when reviewing the following commit:
>>
>> commit 192df6f9122ddebc21d0a632c10da3453aeee1c2
>> Author: Roger Pau Monné <roger.pau@citrix.com>
>> Date:   Tue Dec 15 14:12:32 2015 +0100
>>
>>       x86: allow HVM guests to use hypercalls to bring up vCPUs
>>
>>       Allow the usage of the VCPUOP_initialise, VCPUOP_up, VCPUOP_down,
>>       VCPUOP_is_up, VCPUOP_get_physid and VCPUOP_send_nmi hypercalls from HVM
>>       guests.
>>
>>       This patch introduces a new structure (vcpu_hvm_context) that
>> should be used
>>       in conjuction with the VCPUOP_initialise hypercall in order to
>> initialize
>>       vCPUs for HVM guests.
>>
>>       Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>       Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>       Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>       Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>
>> On Arm, the structure vcpu_guest_context is not exposed outside of Xen
>> and the tools. Interestingly vcpu_guest_core_regs is but it should only
>> be used within vcpu_guest_context.
>>
>> So as this is not used by stable ABI, it is fine to break it.
>>
>>>>
>>> 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?
>>
>> I think this change is unrelated. So it should be handled in a follow-up
>> patch.
> 
> My only difference in viewing this is that I'd say the adjustment
> would better be a prereq patch to this one, such that the one here
> ends up being more obviously correct.

The function is already not reachable so I felt it was unfair to require 
the clean-up for merging this code.

> Also, if the function is
> indeed not meant to be reachable, besides making it return
> -EOPNOTSUPP (or alike) it should probably also have
> ASSERT_UNREACHABLE() added.

+1 on the idea.

Cheers,

-- 
Julien Grall