Re: [PATCH 4/8] x86/traps: Demand-populate PASID MSR via #GP

[Luck, Tony]

On Thu, Sep 23, 2021 at 04:17:05PM -0700, Andy Lutomirski wrote:
> > +	fpregs_lock();
> > +
> > +	/*
> > +	 * If the task's FPU doesn't need to be loaded or is valid, directly
> > +	 * write the IA32_PASID MSR. Otherwise, write the PASID state and
> > +	 * the MSR will be loaded from the PASID state before returning to
> > +	 * the user.
> > +	 */
> > +	if (!test_thread_flag(TIF_NEED_FPU_LOAD) ||
> > +	    fpregs_state_valid(fpu, smp_processor_id())) {
> > +		wrmsrl(MSR_IA32_PASID, msr_val);
> 
> Let me try to decode this.
> 
> If the current task's FPU state is live or if the state is in memory but the CPU regs just happen to match the copy in memory, then write the MSR.  Else write the value to memory.
> 
> This is wrong.  If !TIF_NEED_FPU_LOAD && fpregs_state_valid, you MUST NOT MODIFY FPU STATE.  This is not negotiable -- you will break coherence between CPU regs and the memory image.  The way you modify the current task's state is either you modify it in CPU regs (if the kernel knows that the CPU regs are the one and only source of truth) OR you modify it in memory and invalidate any preserved copies (by zapping last_cpu). 
> 
> In any event, that particular bit of logic really doesn't belong in here -- it belongs in some helper that gets it right, once.

Andy,

A helper sounds like a good idea. Can you flesh out what
you would like that to look like?

Is it just the "where is the live register state?" so the
above could be written:

	if (xsave_state_in_memory(args ...))
		update pasid bit of xsave state in memory
	else
		wrmsrl(MSR_IA32_PASID, msr_val);

Or are you thinking of a helper that does both the check
and the update ... so the code here could be:

	update_one_xsave_feature(XFEATURE_PASID, &msr_val, sizeof(msr_val));

With the helper being something like:

void update_one_xsave_feature(enum xfeature xfeature, void *data, size_t size)
{
	if (xsave_state_in_memory(args ...)) {
		addr = get_xsave_addr(xsave, xfeature);
		memcpy(addr, data, size);
		xsave->header.xfeatures |= (1 << xfeature);
		return;
	}

	switch (xfeature) {
	case XFEATURE_PASID:
		wrmsrl(MSR_IA32_PASID, *(u64 *)data);
		break;

	case each_of_the_other_XFEATURE_enums:
		code to update registers for that XFEATURE
	}
}

either way needs the definitive correct coding for xsave_state_in_memory()

-Tony