Re: [PATCH v2 1/3] KVM: nVMX: Don't advertise single context invalidation for invept

[Marcelo Tosatti <mtosatti@redhat.com>]

On Fri, Apr 11, 2014 at 08:53:09PM +0200, Jan Kiszka wrote:
> On 2014-04-11 20:35, Bandan Das wrote:
> > Jan Kiszka <jan.kiszka@siemens.com> writes:
> > 
> >> On 2014-04-11 19:26, Bandan Das wrote:
> >>> Jan Kiszka <jan.kiszka@siemens.com> writes:
> >>>
> >>>> On 2014-04-11 02:27, Bandan Das wrote:
> >>>>> Marcelo Tosatti <mtosatti@redhat.com> writes:
> >>>>>
> >>>>>> On Mon, Mar 31, 2014 at 05:00:23PM -0400, Bandan Das wrote:
> >>>>>>> For single context invalidation, we fall through to global
> >>>>>>> invalidation in handle_invept() except for one case - when
> >>>>>>> the operand supplied by L1 is different from what we have in
> >>>>>>> vmcs12. However, typically hypervisors will only call invept
> >>>>>>> for the currently loaded eptp, so the condition will
> >>>>>>> never be true.
> >>>>>>>
> >>>>>>> Signed-off-by: Bandan Das <bsd@redhat.com>
> >>>>>>
> >>>>>> Bandan,
> >>>>>>
> >>>>>> Why not fix INVEPT single-context rather than removing it entirely?
> >>>>>>
> >>>>>> "Single-context. If the INVEPT type is 1, the logical processor
> >>>>>> invalidates all guest-physical mappings and combined mappings associated
> >>>>>> with the EP4TA specified in the INVEPT descriptor. Combined mappings for
> >>>>>> that EP4TA are invalidated for all VPIDs and all PCIDs. (The instruction
> >>>>>> may invalidate mappings associated with other EP4TAs.)"
> >>>>>>
> >>>>>> So just removing the "if (EPTP != CURRENT.EPTP) BREAK" should be enough.
> >>>>>
> >>>>> The single context invalidation in handle_invept() doesn't do 
> >>>>> anything different. It just falls down to the global case.
> >>>>> And the invept code in Xen and KVM both seemed to fall back
> >>>>> to global invalidation if support for single context wasn't found.
> >>>>> So, it was proposed not to advertise it at all.
> >>>>>
> >>>>> But rethinking this again, I agree with you. If there's a hypervisor
> >>>>> with a  single context invept implmentation that does not fallback,
> >>>>> this will unfortunately not work. Jan, do you agree with this ?
> >>>>
> >>>> A hypervisor that doesn't properly check the HW caps is just broken. And
> >>>> one that mandates single context invalidation support is silly.
> >>>
> >>> Well, but we could make life a little bit easier for the unfortunate user
> >>> using the broken hypervisor :) And advertising single context inavalidation
> >>> doesn't really seem to have any downsides.
> >>
> >> Ok, let's try it this way: single-context invalidation is inherently
> >> tied to VPID support (that's how you address a context). However, KVM
> >> does not expose VPID to its guest. So this discussion is mood: no
> >> hypervisor will make use of this feature as it has no means to fill in
> >> the required parameter.
> > 
> > I thought (from the spec) invept single context invalidation
> > takes the EP4TA as the second argument. invvpid single context
> > however takes the VPID as its descriptor.
> 
> Oops, invept/invvpid mess-up while re-reading the spec - sorry.
> 
> > 
> > The Xen L1 hypervisor was actually calling single context invept
> > multiple times. That's how I hit this bug.
> 
> ...and it's no longer doing it now, I suppose. The question remains,
> which hypervisor we want to cater with a
> "single-context-that-is-current-context" invalidation (that is my
> understanding of Marcelo's proposal). 

My proposal is to implement what is in the spec.

> On the other hand, if some hypervisor actually uses invept to
> invalidate a non-current mapping, we would regress compared to not
> exposing single context invept. Hope I got this conclusion right. ;)

In that case INVEPT global would also be broken.