Re: [PATCH v2 1/4] x86/domctl: Stop using XLAT_cpu_user_regs()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.08.2025 13:16, 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.
> 
> This does cause some minor changes in behaviour for the hypercalls.
> 
> It is specifically not the case that a toolstack could set_info(); get_info();
> and get an identical bit pattern back.  Amongst other things, the
> architectural sticky bits in registers are applied during setting.
> 
> Previously, XLAT_cpu_user_regs() omitted the _pad fields in the compat case
> whereas the non-compat case included them owing to the single memcpy().
> 
> Omit the _pad fields in the non-compat case too; for all but the oldest of
> CPUs, the segment selectors are zero-extended by hardware when pushed onto the
> stack, so non-zero values here get lost naturally.  Furthermore, FRED reuses
> the space above cs and ss for extra state, and a PV guest for now at least
> must not be able to write the control state.
> 
> Omit the error_code and entry_vector fields too.  They're already identified
> as private fields in the public API, and are stale outside of Xen's
> interrupt/exception/syscall handler.  They're also a very minor information
> leak of which event caused the last deschedule of a vCPU.

I think my prior remark towards tools like xenctx wasn't really addressed.
Then again that particular tool doesn't use the fields now, so apparently
no-one ever saw a need.

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1233,7 +1233,24 @@ int arch_set_info_guest(
>  
>      if ( !compat )
>      {
> -        memcpy(&v->arch.user_regs, &c.nat->user_regs, 
> sizeof(c.nat->user_regs));
> +        memset(&v->arch.user_regs, 0, sizeof(v->arch.user_regs));

Any reason to have this and ...

> +        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;
> +        v->arch.user_regs.rsi               = c.nat->user_regs.rsi;
> +        v->arch.user_regs.rdi               = c.nat->user_regs.rdi;
> +        v->arch.user_regs.rbp               = c.nat->user_regs.rbp;
> +        v->arch.user_regs.rax               = c.nat->user_regs.rax;
> +        v->arch.user_regs.rip               = c.nat->user_regs.rip;
> +        v->arch.user_regs.cs                = c.nat->user_regs.cs;
> +        v->arch.user_regs.rflags            = c.nat->user_regs.rflags;
> +        v->arch.user_regs.rsp               = c.nat->user_regs.rsp;
> +        v->arch.user_regs.ss                = c.nat->user_regs.ss;
> +        v->arch.user_regs.es                = c.nat->user_regs.es;
> +        v->arch.user_regs.ds                = c.nat->user_regs.ds;
> +        v->arch.user_regs.fs                = c.nat->user_regs.fs;
> +        v->arch.user_regs.gs                = c.nat->user_regs.gs;
> +
>          if ( is_pv_domain(d) )
>              memcpy(v->arch.pv.trap_ctxt, c.nat->trap_ctxt,
>                     sizeof(c.nat->trap_ctxt));
> @@ -1241,7 +1258,24 @@ int arch_set_info_guest(
>  #ifdef CONFIG_COMPAT
>      else
>      {
> -        XLAT_cpu_user_regs(&v->arch.user_regs, &c.cmp->user_regs);
> +        memset(&v->arch.user_regs, 0, sizeof(v->arch.user_regs));

... this separate, rather than putting just one ahead of the if()?

Preferably with that adjustment:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan