Re: [PATCH v2] KVM: nVMX: Improve I/O exit handling

[Gleb Natapov <gleb@redhat.com>]

On Thu, Feb 14, 2013 at 01:22:01PM +0100, Jan Kiszka wrote:
> On 2013-02-14 13:11, Gleb Natapov wrote:
> > On Thu, Feb 14, 2013 at 12:19:26PM +0100, Jan Kiszka wrote:
> >> On 2013-02-14 10:32, Gleb Natapov wrote:
> >>> On Mon, Feb 11, 2013 at 12:19:17PM +0100, Jan Kiszka wrote:
> >>>> This prevents trapping L2 I/O exits if L1 has neither unconditional nor
> >>>> bitmap-based exiting enabled. Furthermore, it implements basic I/O
> >>>> bitmap handling. Repeated string accesses are still reported to L1
> >>>> unconditionally for now.
> >>>>
> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>> ---
> >>>>
> >>>> Changes in v2:
> >>>>  - fix two brown-paper-bag bugs (offset for bitmap_b, wrong direction
> >>>>    of mask shift)
> >>>>  - use vmcs12 argument instead of get_vmcs12
> >>>>
> >>>>  arch/x86/kvm/vmx.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >>>>  1 files changed, 52 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> >>>> index fe9a9cf..64e1233 100644
> >>>> --- a/arch/x86/kvm/vmx.c
> >>>> +++ b/arch/x86/kvm/vmx.c
> >>>> @@ -5913,6 +5913,57 @@ static int (*const kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = {
> >>>>  static const int kvm_vmx_max_exit_handlers =
> >>>>  	ARRAY_SIZE(kvm_vmx_exit_handlers);
> >>>>  
> >>>> +static bool nested_vmx_exit_handled_io(struct kvm_vcpu *vcpu,
> >>>> +				       struct vmcs12 *vmcs12)
> >>>> +{
> >>>> +	unsigned long exit_qualification;
> >>>> +	gpa_t bitmap, last_bitmap;
> >>>> +	bool string, rep;
> >>>> +	u16 port;
> >>>> +	int size;
> >>>> +	u8 b;
> >>>> +
> >>>> +	if (nested_cpu_has(vmcs12, CPU_BASED_UNCOND_IO_EXITING))
> >>>> +		return 1;
> >>>> +
> >>>> +	if (!nested_cpu_has(vmcs12, CPU_BASED_USE_IO_BITMAPS))
> >>>> +		return 0;
> >>>> +
> >>>> +	exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
> >>>> +
> >>>> +	string = exit_qualification & 16;
> >>>> +	rep = exit_qualification & 32;
> >>>> +
> >>>> +	/* TODO: interpret instruction and check range against bitmap */
> >>>> +	if (string && rep)
> >>>> +		return 1;
> >>>> +
> >>>> +	port = exit_qualification >> 16;
> >>>> +	size = (exit_qualification & 7) + 1;
> >>>> +
> >>>> +	last_bitmap = (gpa_t)-1;
> >>>> +	b = -1;
> >>>> +
> >>>> +	while (size > 0) {
> >>>> +		if (port < 0x8000)
> >>>> +			bitmap = vmcs12->io_bitmap_a;
> >>>> +		else
> >>>> +			bitmap = vmcs12->io_bitmap_b;
> >>>> +		bitmap += (port & 0x7fff) / 8;
> >>>> +
> >>>> +		if (last_bitmap != bitmap)
> >>>> +			kvm_read_guest(vcpu->kvm, bitmap, &b, 1);
> >>> Return value is ignored.
> >>
> >> Not sure how to map a failure on real HW behaviour. I guess it's best to
> > Exit to L1 with nested_vmx_failValid() may be?
> 
> To my understanding, nested_vmx_failValid/Invalid are related to errors
> directly related to vm instruction execution. This one is triggered by
> the guest later on.
> 
You are right. We need some kind of error vmexit here, but nothing
appropriate is defined by the spec, so lets just assume that exit to L1
is needed if we cannot read permission bitmaps.

> > 
> >> simply initialize b to -1 before each call, enforcing an exit on
> >> unaccessible bitmaps.
> >>
> > I'd just make it explicit:
> >  if (kvm_read_guest())
> >     return 1;
> 
> OK.
> 
> > 
> >> BTW, nested_vmx_exit_handled_msr needs some improvement in this regard, too.
> >>
> > Yes, nested_vmx_exit_handled_msr() use uninitialized 'b' from the stack.
> > We are leaking host kernel data to a guest here. Patch?
> 
> Will follow.
> 
> Jan
> 
> -- 
> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
> Corporate Competence Center Embedded Linux

--
			Gleb.