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

Reply via email to