Re: [PATCH 14/22] x86/traps: Extend struct cpu_user_regs/cpu_info with FRED fields

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/08/2025 2:12 pm, Jan Beulich wrote:
> On 08.08.2025 22:23, Andrew Cooper wrote:
>> @@ -42,17 +46,76 @@ struct cpu_user_regs
>>       */
>>  
>>      union { uint64_t rip;    uint32_t eip;    uint16_t ip; };
>> -    uint16_t cs, _pad0[1];
>> -    uint8_t  saved_upcall_mask; /* PV (v)rflags.IF == !saved_upcall_mask */
>> -    uint8_t  _pad1[3];
>> +    union {
>> +        struct {
>> +            uint16_t      cs;
>> +            unsigned long :16;
>> +            uint8_t       saved_upcall_mask; /* PV (v)rflags.IF == 
>> !saved_upcall_mask */
> Would this better be reproduced ...
>
>> +        };
>> +        unsigned long     csx;
>> +        struct {
>> +            /*
>> +             * Bits 0 thru 31 control ERET{U,S} behaviour, and is state of 
>> the
>> +             * interrupted context.
>> +             */
>> +            uint16_t      cs;
>> +            unsigned int  sl:2;      /* Stack Level */
>> +            bool          wfe:1;     /* Wait-for-ENDBRANCH state */
> ... here as well, just like you reproduce "cs"?

saved_upcall_mask is a property of an in-guest IRET frame only.  It is
only produced in create_bounce_frame, and never consumed by Xen.

It needs to exist in this structure so asm-offsets.c can generate a
constant.

Also, be aware that there are new features being planned which rely on FRED.

>
>> +        } fred_cs;
>> +    };
>>      union { uint64_t rflags; uint32_t eflags; uint16_t flags; };
>>      union { uint64_t rsp;    uint32_t esp;    uint16_t sp;    uint8_t spl; 
>> };
>> -    uint16_t ss, _pad2[3];
>> +    union {
>> +        uint16_t          ss;
>> +        unsigned long     ssx;
> What use do you foresee for this and "csx"?

That also came from Linux.  I'm using it to zero the control metadata so
ERETU behaves more like IRET.

>
>> +        struct {
>> +            /*
>> +             * Bits 0 thru 31 control ERET{U,S} behaviour, and is state 
>> about
>> +             * the event which occured.
>> +             */
>> +            uint16_t      ss;
>> +            bool          sti:1;     /* Was blocked-by-STI, and not 
>> cancelled */
>> +            bool          swint:1;   /* Was a SYSCALL/SYSENTER/INT $N */
>> +            bool          nmi:1;     /* Was an NMI. */
>> +            unsigned long :13;
>> +
>> +            /*
>> +             * Bits 32 thru 63 are ignored by ERET{U,S} and are informative
>> +             * only.
>> +             */
>> +            uint8_t       vector;
>> +            unsigned long :8;
>> +            unsigned int  type:4;    /* X86_ET_* */
>> +            unsigned long :4;
>> +            bool          enclave:1; /* Event taken in SGX mode */
>> +            bool          lm:1;      /* Was in Long Mode */
> The bit indicates 64-bit mode aiui, not long mode (without which FRED isn't 
> even
> available).

Oh, yes.  This is something that changed across revisions, and I wrote
this patch to an older spec.

It's %cs.l of the interrupted context, so I probably should just drop the m.

>
>> --- a/xen/arch/x86/include/asm/current.h
>> +++ b/xen/arch/x86/include/asm/current.h
>> @@ -38,6 +38,8 @@ struct vcpu;
>>  
>>  struct cpu_info {
>>      struct cpu_user_regs guest_cpu_user_regs;
>> +    struct fred_info _fred; /* Only used when FRED is active. */
> Any particular need for the leading underscore?

Somewhat, yes.  It's not safe to reference this field, except for
loading MSR_PL0_RSP.

Everyone else should use cpu_regs_fred_info() to get the fred_info,
which has a safety ASSERT().

~Andrew