Re: [RFC] docs: FRED support in Xen

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.01.2025 17:06, Andrew Cooper wrote:
> On 06/01/2025 2:28 pm, Jan Beulich wrote:
>> On 03.01.2025 21:47, Andrew Cooper wrote:
>>> + #. In x86_emulate.c's ``put_fpu()``.  As far as I can tell, this is
>>> +    completely buggy; the values will be poisoned for HVM guests, and stale
>>> +    from the prior context switch for PV guests.
>> For HVM guests the ->read_segment() hook will be populated, so the path isn't
>> taken (leaving aside the odd case of the hook failing). For PV guests I don't
>> see any staleness concern: When the vCPU was switched in, the fields were set
>> (restored), weren't they?
> 
> There is up to 30ms of guest runtime between the last schedule in and
> this emulation, and segment loads don't generally trap for PV guests.

Oh, yes, I see now what you mean. Luckily even pv/emul-priv-op.c sets the hook.
Hence what's affected here are the two uses of the emulator from
pv/ro-page-fault.c. Sadly HVM isn't entirely unaffected, as the shadow mode use
of the emulator doesn't set the hook. Neither of the three cases are likely to
involve FPU insns, yet it's not excluded.

Question is what to do: Simply failing the entire emulation feels too heavy
handed. We could choose to simply store nul selectors instead. That would be
kind of wrong though for (in the hypervisor: hypothetical) cases where the
incoming regs are fully populated.

Regardless of what we're going to do, the underlying issue of callers passing
in partially inapplicable (to avoid calling it uninitialized) state remains,
...

>> For the purpose of FRED this doesn't matter much - wherever the values are to
>> be held, they'll be taken from by put_fpu().
> 
> I maybe wasn't clear.  To support FRED, I need to delete the vm86 fields.
> 
> @@ -54,10 +54,6 @@ struct cpu_user_regs
>      DECL_REG_LO16(flags); /* rflags.IF == !saved_upcall_mask */
>      DECL_REG_LO8(sp);
>      uint16_t ss, _pad2[3];
> -    uint16_t es, _pad3[3];
> -    uint16_t ds, _pad4[3];
> -    uint16_t fs, _pad5[3];
> -    uint16_t gs, _pad6[3];
> +    uint64_t edata, _rsvd;
>  };
>  
>  #undef DECL_REG_HI

... at least until your rework is in place. I did understand that you mean
to remove the struct fields. You made clear though that you'd re-introduce
them elsewhere (struct pv_vcpu) instead. And without (yet) recognizing the
staleness aspect I was implying we could read the values from there.

Jan