Re: [4.22] Re: [PATCH] x86/domctl: restore all registers in arch_{get,set}_info_guest()

On 7/20/26 9:19 AM, Jan Beulich wrote:
> On 20.07.2026 02:12, Marek Marczykowski-Górecki wrote:
>> Commit 9f892f84c279 ("x86/domctl: Stop using XLAT_cpu_user_regs()")
>> converted memcpy() of the cpu_user_regs structure to explicit copy of
>> its fields. In the compat case, it intentionally missed few of them,
>> named in the commit message. But the 64bit case missed also r8-r15
>> registers, which was not intentional. This, at least, caused Linux
>> 6.18.x crash when resuming PVH domU.
> 
> Oh, wow, what a bad mistake (including by me as the reviewer).
> 
>> Fix it by adding missing assignments.
>>
>> Fixes: 9f892f84c279 ("x86/domctl: Stop using XLAT_cpu_user_regs()")
>> Signed-off-by: Marek Marczykowski-Górecki <[email protected]>
> 
> Reviewed-by: Jan Beulich <[email protected]>
> 
> One remark though (equally applying to the original change):
> 
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1255,6 +1255,14 @@ int arch_set_info_guest(
>>   
>>       if ( !compat )
>>       {
>> +        v->arch.user_regs.r15               = c.nat->user_regs.r15;
>> +        v->arch.user_regs.r14               = c.nat->user_regs.r14;
>> +        v->arch.user_regs.r13               = c.nat->user_regs.r13;
>> +        v->arch.user_regs.r12               = c.nat->user_regs.r12;
>> +        v->arch.user_regs.r11               = c.nat->user_regs.r11;
>> +        v->arch.user_regs.r10               = c.nat->user_regs.r10;
>> +        v->arch.user_regs.r9                = c.nat->user_regs.r9;
>> +        v->arch.user_regs.r8                = c.nat->user_regs.r8;
>>           v->arch.user_regs.rbx               = c.nat->user_regs.rbx;
>>           v->arch.user_regs.rcx               = c.nat->user_regs.rcx;
>>           v->arch.user_regs.rdx               = c.nat->user_regs.rdx;
> 
> Neither here nor ...
> 
>> --- a/xen/arch/x86/domctl.c
>> +++ b/xen/arch/x86/domctl.c
>> @@ -1481,6 +1481,14 @@ void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
>>       if ( !compat )
>>       {
>>           /* Backing memory is pre-zeroed. */
>> +        c.nat->user_regs.r15               = v->arch.user_regs.r15;
>> +        c.nat->user_regs.r14               = v->arch.user_regs.r14;
>> +        c.nat->user_regs.r13               = v->arch.user_regs.r13;
>> +        c.nat->user_regs.r12               = v->arch.user_regs.r12;
>> +        c.nat->user_regs.r11               = v->arch.user_regs.r11;
>> +        c.nat->user_regs.r10               = v->arch.user_regs.r10;
>> +        c.nat->user_regs.r9                = v->arch.user_regs.r9;
>> +        c.nat->user_regs.r8                = v->arch.user_regs.r8;
>>           c.nat->user_regs.rbx               = v->arch.user_regs.rbx;
>>           c.nat->user_regs.rcx               = v->arch.user_regs.rcx;
>>           c.nat->user_regs.rdx               = v->arch.user_regs.rdx;
> 
> ... here it becomes clear what has determined the order in which fields
> are copied. It's neither by register number nor by field order nor
> alphabetically. We might do ourselves a (however minor) favor if we used
> a clear criteria; likely field order would be best. The more that the
> new internal struct cpu_user_regs mirrors field order from the original
> external one. Andrew (in particular)?
> 
> Also, despite this being an issue in 4.21 already, I think this definitely
> wants considering for 4.22.

Considering that it leads to crash of Linux I agree that we want to have 
this in 4.22:
  Reviewed-by: Oleksii Kurochko <[email protected]>

Thanks.

~ Oleksii