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 <jbeul...@suse.com>
Thanks.
~Andrew
