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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 20/07/2026 8: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).

Yes, I'm very embarrassed by this mistake.  It also highlights a serious
gap in testing which we need to address.  We've managed a release and a
half with PV migration plain broken before figuring out why.

>
>> Fix it by adding missing assignments.
>>
>> Fixes: 9f892f84c279 ("x86/domctl: Stop using XLAT_cpu_user_regs()")
>> Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

>
> 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)?

They are the order that XLAT_cpu_user_regs() expanded in; that is -
x86_32's cpu_user_regs field order.

... which now I think about it is correct for one of the two uses of
XLAT_cpu_user_regs() but suboptimal for the other.

They all want to be field-order of the loading side, because that way
we're working with the prefetcher rather than against it.

I'm making a follow-up, because I think it's more complicated than just
interleaving these lines, and I suggest we take Marek's patch as-is.

~Andrew