Re: [PATCH 5/8] x86/domctl: Stop using XLAT_cpu_user_regs()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.03.2025 22:10, Andrew Cooper wrote:
> In order to support FRED, we're going to have to remove the {ds..gs} fields
> from struct cpu_user_regs, meaning that it is going to have to become a
> different type to the structure embedded in vcpu_guest_context_u.
> 
> In both arch_{get,set}_info_guest(), expand the memcpy()/XLAT_cpu_user_regs()
> to copy the fields individually.  This will allow us to eventually make them
> different types.
> 
> No practical change.  The compat cases are identical, while the non-compat
> cases no longer copy _pad fields.

That's fine for "set", but potentially not for "get": Someone simply doing
memcmp() on two pieces of output might then break.

> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> Should we really be copying error_code/entry_vector?  They're already listed
> as explicitly private fields, and I don't think anything good can come of
> providing/consuming them.

I don't see a reason why we'd need to copy them in arch_set_info_guest();
arch_set_info_hvm_guest() doesn't copy them either. For
arch_get_info_guest() it's less clear - toolstack components may have
grown a dependency on them (e.g. introspection?), so I'd err on the side
of retaining prior behavior. (Of course there's then the corner case of
someone calling "get" right after "set", expecting the two fields to come
back unchanged.)

> @@ -1204,7 +1223,26 @@ int arch_set_info_guest(
>  #ifdef CONFIG_COMPAT
>      else
>      {
> -        XLAT_cpu_user_regs(&v->arch.user_regs, &c.cmp->user_regs);
> +        v->arch.user_regs.ebx               = c.cmp->user_regs.ebx;
> +        v->arch.user_regs.ecx               = c.cmp->user_regs.ecx;
> +        v->arch.user_regs.edx               = c.cmp->user_regs.edx;
> +        v->arch.user_regs.esi               = c.cmp->user_regs.esi;
> +        v->arch.user_regs.edi               = c.cmp->user_regs.edi;
> +        v->arch.user_regs.ebp               = c.cmp->user_regs.ebp;
> +        v->arch.user_regs.eax               = c.cmp->user_regs.eax;
> +        v->arch.user_regs.error_code        = c.cmp->user_regs.error_code;
> +        v->arch.user_regs.entry_vector      = c.cmp->user_regs.entry_vector;
> +        v->arch.user_regs.eip               = c.cmp->user_regs.eip;
> +        v->arch.user_regs.cs                = c.cmp->user_regs.cs;
> +        v->arch.user_regs.saved_upcall_mask = 
> c.cmp->user_regs.saved_upcall_mask;
> +        v->arch.user_regs.eflags            = c.cmp->user_regs.eflags;
> +        v->arch.user_regs.esp               = c.cmp->user_regs.esp;
> +        v->arch.user_regs.ss                = c.cmp->user_regs.ss;
> +        v->arch.user_regs.es                = c.cmp->user_regs.es;
> +        v->arch.user_regs.ds                = c.cmp->user_regs.ds;
> +        v->arch.user_regs.fs                = c.cmp->user_regs.fs;
> +        v->arch.user_regs.gs                = c.cmp->user_regs.gs;

Just to mention it (there's no change in behavior here afaict): Us writing
only half of the register fields looks like a latent (but perhaps only
theoretical) problem to me. A dis-aggregated toolstack may set 64-bit PV
context, then toggle address size, then set 32-bit context. That'll leave
the high halves of respective fields non-zero. I didn't check whether any
badness could result from that, as for the time being
XEN_DOMCTL_set_address_size isn't marked dis-aggregation-safe, and hence
this at least isn't of immediate concern.

Jan