On Fri, Aug 21, 2020 at 9:08 AM Alexander Graf <graf@xxxxxxxxxx> wrote: > > > > On 21.08.20 16:27, Aaron Lewis wrote: > > > > On Thu, Aug 20, 2020 at 3:35 PM Jim Mattson <jmattson@xxxxxxxxxx> wrote: > >> > >> On Thu, Aug 20, 2020 at 3:04 PM Alexander Graf <graf@xxxxxxxxxx> wrote: > >>> > >>> > >>> > >>> On 20.08.20 02:18, Aaron Lewis wrote: > >>>> > >>>> On Wed, Aug 19, 2020 at 8:26 AM Alexander Graf <graf@xxxxxxxxxx> wrote: > >>>>> > >>>>> > >>>>> > >>>>> On 18.08.20 23:15, Aaron Lewis wrote: > >>>>>> > >>>>>> SDM volume 3: 24.6.9 "MSR-Bitmap Address" and APM volume 2: 15.11 "MS > >>>>>> intercepts" describe MSR permission bitmaps. Permission bitmaps are > >>>>>> used to control whether an execution of rdmsr or wrmsr will cause a > >>>>>> vm exit. For userspace tracked MSRs it is required they cause a vm > >>>>>> exit, so the host is able to forward the MSR to userspace. This change > >>>>>> adds vmx/svm support to ensure the permission bitmap is properly set to > >>>>>> cause a vm_exit to the host when rdmsr or wrmsr is used by one of the > >>>>>> userspace tracked MSRs. Also, to avoid repeatedly setting them, > >>>>>> kvm_make_request() is used to coalesce these into a single call. > >>>>>> > >>>>>> Signed-off-by: Aaron Lewis <aaronlewis@xxxxxxxxxx> > >>>>>> Reviewed-by: Oliver Upton <oupton@xxxxxxxxxx> > >>>>> > >>>>> This is incomplete, as it doesn't cover all of the x2apic registers. > >>>>> There are also a few MSRs that IIRC are handled differently from this > >>>>> logic, such as EFER. > >>>>> > >>>>> I'm really curious if this is worth the effort? I would be inclined to > >>>>> say that MSRs that KVM has direct access for need special handling one > >>>>> way or another. > >>>>> > >>>> > >>>> Can you please elaborate on this? It was my understanding that the > >>>> permission bitmap covers the x2apic registers. Also, I’m not sure how > >>> > >>> So x2apic MSR passthrough is configured specially: > >>> > >>> > >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kvm/vmx/vmx.c#n3796 > >>> > >>> and I think not handled by this patch? > >> > >> By happenstance only, I think, since there is also a call there to > >> vmx_disable_intercept_for_msr() for the TPR when x2APIC is enabled. > > > > If we want to be more explicit about it we could add > > kvm_make_request(KVM_REQ_USER_MSR_UPDATE, vcpu) After the bitmap is > > modified, but that doesn't seem to be necessary as Jim pointed out as > > there are calls to vmx_disable_intercept_for_msr() there which will > > set the update request for us. And we only have to worry about that > > if the bitmap is cleared which means MSR_BITMAP_MODE_X2APIC_APICV is > > set, and that flag can only be set if MSR_BITMAP_MODE_X2APIC is set. > > So, AFAICT that is covered by my changes. > > > > I don't understand - for most x2APIC MSRs, > vmx_{en,dis}able_intercept_for_msr() never gets called, no? > Sorry, to be clear. We just need it to be called once for us to be covered. When it's invoked we set a flag, so the next time vm enter is called we run over the list of MSRs userspace cares about and ensure they are all set correctly. So, if the x2APIC permission bitmaps are modified directly and cleared, we just need for vmx_disable_intercept_for_msr() to be called at least once to tell us to run over the list and set the bits for all MSRs userspace is tracking. > > Alex > > > > Amazon Development Center Germany GmbH > Krausenstr. 38 > 10117 Berlin > Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss > Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B > Sitz: Berlin > Ust-ID: DE 289 237 879 > >
