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