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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
