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

[Jan Kiszka <jan.kiszka@siemens.com>]

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.

> 
>> 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