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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/08/2025 11:09 am, Jan Beulich wrote:
> 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.

Oh, sorry.  I did specifically look (everywhere in tools, not just
xenctx), and they're not used at all.

Xenalyze uses an error_code, but that's a field name from the EPT/NPT
fault trace record, not from cpu_user_regs.

Finally, the observation about the information leak.  The information
present is often the timer interrupt (end of time-slice), or the event
check IPI (from vcpu_pause()).

gdbsx is the only utility that stands a chance of reliably using
->entry_vector, and even it doesn't because that's not how GDB works.

Overall, I'd say people have been pretty good at following the /*
Private */ note in the public ABI.


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

Code generation.  If you hoist the memset(), it can't be merged with the
assignments.

Although I see now it's not even attempting the mere (it was in the
past), and I don't care enough to argue, so I'll change it.

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

Thanks.

~Andrew