Re: [PATCH 2/4] x86/xstate: Rework XSAVE/XRSTOR given a newer toolchain baseline

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.01.2026 22:08, Andrew Cooper wrote:
> On 06/01/2026 7:59 am, Jan Beulich wrote:
>>>>> @@ -489,17 +484,17 @@ void xrstor(struct vcpu *v, uint64_t mask)
>>>>>              ptr->xsave_hdr.xcomp_bv = 0;
>>>>>          }
>>>>>          memset(ptr->xsave_hdr.reserved, 0, 
>>>>> sizeof(ptr->xsave_hdr.reserved));
>>>>> -        continue;
>>>>> +        goto retry;
>>>>>  
>>>>>      case 2: /* Stage 2: Reset all state. */
>>>>>          ptr->fpu_sse.mxcsr = MXCSR_DEFAULT;
>>>>>          ptr->xsave_hdr.xstate_bv = 0;
>>>>>          ptr->xsave_hdr.xcomp_bv = v->arch.xcr0_accum & XSTATE_XSAVES_ONLY
>>>>>              ? XSTATE_COMPACTION_ENABLED : 0;
>>>>> -        continue;
>>>>> -    }
>>>>> +        goto retry;
>>>>>  
>>>>> -        domain_crash(current->domain);
>>>>> +    default: /* Stage 3: Nothing else to do. */
>>>>> +        domain_crash(v->domain, "Uncorrectable XRSTOR fault\n");
>>>>>          return;
>>>> There's an unexplained change here as to which domain is being crashed.
>>>> You switch to crashing the subject domain, yet if that's not also the
>>>> requester, it isn't "guilty" in causing the observed fault.
>>> So dom0 should be crashed because there bad data in the migration stream?
>> Well, I'm not saying the behavior needs to stay like this, or that's it's
>> the best of all possible options. But in principle Dom0 could sanitize the
>> migration stream before passing it to Xen. So it is still first and foremost
>> Dom0 which is to blame.
> 
> BNDCFGU contains a pointer which, for PV context, needs access_ok(), not
> just a regular canonical check.  Most supervisor states are in a similar
> position.

Yes, so exposing them to PV would require extra care. Note that MPX isn't
exposed to PV.

> Just because Xen has managed to get away without such checks (by not yet
> supporting a state where it matters), I don't agree that its safe to
> trust dom0 to do this.

Yet the guest itself can't have got in place a non-canonical value, can it?
Its attempts to load it into hardware would have faulted. So it's still
not the target domain which is to be blamed for a fault resulting from
XRSTOR encountering bogus pointers.

> For this case, it's v's xstate buffer which cannot be loaded, so it's v
> which cannot be context switched into, and must be crashed.  More below.

Well, yes, as said - that's one possible way of treating things. My main
request is not so much to undo the change, but to properly justify it in
the description. (Or maybe that really wants to be a separate change, in
particular if you wanted the changed behavior to also be backported.)

>>> v is always curr.
>> Not quite - see xstate_set_init().
> 
> Also more below.
> 
>> And for some of the callers of
>> hvm_update_guest_cr() I also don't think they always act on current. In
>> particular hvm_vcpu_reset_state() never does, I suppose (not the least
>> because of the vcpu_pause() in its sole caller).
> 
> We discussed the need to not be remotely poking register state like
> that.  But I don't see where the connection is between
> hvm_update_guest_cr() and xsave()/xrstor().

At the example of svm_update_guest_cr(): It calls svm_fpu_enter(), which
in turn calls vcpu_restore_fpu_lazy(). But yes, that's explicitly only
when v == current. I fear I didn't look closely enough when writing the
earlier reply, sorry.

> Tangent: hvm_vcpu_reset_state() is terribly named as it's attempting to
> put the vCPU into the INIT state, not the #RESET set.
> 
> But it only operates on the xstate header in memory while the target is
> de-scheduled.  It's not using XSAVE/XRSTOR to load the results into
> registers as far as I can tell.

Iirc I mentioned hvm_vcpu_reset_state() because it calls
hvm_update_guest_cr() several times.

>>>   XRSTOR can't be used correctly outside of the subject context,
>> Then are you suggesting e.g. xstate_set_init() is buggy?
> 
> No, but it switches into enough of v's context to function.  Really its
> neither current nor remote context.
> 
> But, it's single caller is adjust_bnd() in the emulator so it's always
> actually current context with a no-op on xcr0.

That's its single present caller. Who knows what else we might need it for.
It would better be operating correctly in the more general case.

> As said on Matrix, I think it's going to be necessary to remove MPX to
> continue the XSAVE cleanup.

Possibly, yes.

Jan