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

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

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

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

If someone tells me how to figure out the effective I/O access range
with rep ins/outs in all possible CPU modes in three lines, I'll
complete this patch. For now I had no use and was too lazy.

 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..056bd95 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(get_vmcs12(vcpu), CPU_BASED_UNCOND_IO_EXITING))
+		return 1;
+
+	if (!nested_cpu_has(get_vmcs12(vcpu), 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 / 8;
+
+		if (last_bitmap != bitmap)
+			kvm_read_guest(vcpu->kvm, bitmap, &b, 1);
+		if (b & (1 >> (port & 7)))
+			return 1;
+
+		port++;
+		size--;
+		last_bitmap = bitmap;
+	}
+
+	return 0;
+}
+
 /*
  * Return 1 if we should exit from L2 to L1 to handle an MSR access access,
  * rather than handle it ourselves in L0. I.e., check whether L1 expressed
@@ -6102,8 +6153,7 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
 	case EXIT_REASON_DR_ACCESS:
 		return nested_cpu_has(vmcs12, CPU_BASED_MOV_DR_EXITING);
 	case EXIT_REASON_IO_INSTRUCTION:
-		/* TODO: support IO bitmaps */
-		return 1;
+		return nested_vmx_exit_handled_io(vcpu, vmcs12);
 	case EXIT_REASON_MSR_READ:
 	case EXIT_REASON_MSR_WRITE:
 		return nested_vmx_exit_handled_msr(vcpu, vmcs12, exit_reason);
-- 
1.7.3.4

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

[Nadav Har'El]

On Sun, Feb 10, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Improve I/O exit handling":
> +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(get_vmcs12(vcpu), CPU_BASED_UNCOND_IO_EXITING))
> +		return 1;

Instead of calling get_vmcs12(vcpu), you can just use "vmcs12" variable
which you already have. I see I left the same redundant call also in
nested_vmx_exit_handled_msr :(

> +		if (port < 0x8000)
> +			bitmap = vmcs12->io_bitmap_a;
> +		else
> +			bitmap = vmcs12->io_bitmap_b;
> +		bitmap += port / 8;

In the port >= 0x8000, I believe need to subtract 0x8000 from the port
number before using it as an offset into io_bitmap_b?

Nadav.

-- 
Nadav Har'El                        |         Monday, Feb 11 2013, 1 Adar 5773
nyh@math.technion.ac.il             |-----------------------------------------
Phone +972-523-790466, ICQ 13349191 |Attention: There will be a rain dance
http://nadav.harel.org.il           |Friday night, weather permitting.

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]

On 2013-02-11 11:07, Nadav Har'El wrote:
> On Sun, Feb 10, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Improve I/O exit handling":
>> +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(get_vmcs12(vcpu), CPU_BASED_UNCOND_IO_EXITING))
>> +		return 1;
> 
> Instead of calling get_vmcs12(vcpu), you can just use "vmcs12" variable
> which you already have. I see I left the same redundant call also in
> nested_vmx_exit_handled_msr :(

Indeed, copy&pasted from there...

> 
>> +		if (port < 0x8000)
>> +			bitmap = vmcs12->io_bitmap_a;
>> +		else
>> +			bitmap = vmcs12->io_bitmap_b;
>> +		bitmap += port / 8;
> 
> In the port >= 0x8000, I believe need to subtract 0x8000 from the port
> number before using it as an offset into io_bitmap_b?

Oops. There was already a stupid bug in that path, failed to validate it
again. v2 will follow.

Thanks for reviewing,
Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

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

[Jan Kiszka]

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);
+		if (b & (1 << (port & 7)))
+			return 1;
+
+		port++;
+		size--;
+		last_bitmap = bitmap;
+	}
+
+	return 0;
+}
+
 /*
  * Return 1 if we should exit from L2 to L1 to handle an MSR access access,
  * rather than handle it ourselves in L0. I.e., check whether L1 expressed
@@ -6102,8 +6153,7 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
 	case EXIT_REASON_DR_ACCESS:
 		return nested_cpu_has(vmcs12, CPU_BASED_MOV_DR_EXITING);
 	case EXIT_REASON_IO_INSTRUCTION:
-		/* TODO: support IO bitmaps */
-		return 1;
+		return nested_vmx_exit_handled_io(vcpu, vmcs12);
 	case EXIT_REASON_MSR_READ:
 	case EXIT_REASON_MSR_WRITE:
 		return nested_vmx_exit_handled_msr(vcpu, vmcs12, exit_reason);
-- 
1.7.3.4

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

[Gleb Natapov]

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.

> +		if (b & (1 << (port & 7)))
> +			return 1;
> +
> +		port++;
> +		size--;
> +		last_bitmap = bitmap;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * Return 1 if we should exit from L2 to L1 to handle an MSR access access,
>   * rather than handle it ourselves in L0. I.e., check whether L1 expressed
> @@ -6102,8 +6153,7 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
>  	case EXIT_REASON_DR_ACCESS:
>  		return nested_cpu_has(vmcs12, CPU_BASED_MOV_DR_EXITING);
>  	case EXIT_REASON_IO_INSTRUCTION:
> -		/* TODO: support IO bitmaps */
> -		return 1;
> +		return nested_vmx_exit_handled_io(vcpu, vmcs12);
>  	case EXIT_REASON_MSR_READ:
>  	case EXIT_REASON_MSR_WRITE:
>  		return nested_vmx_exit_handled_msr(vcpu, vmcs12, exit_reason);
> -- 
> 1.7.3.4

--
			Gleb.

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

[Jan Kiszka]

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
simply initialize b to -1 before each call, enforcing an exit on
unaccessible bitmaps.

BTW, nested_vmx_exit_handled_msr needs some improvement in this regard, too.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

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

[Gleb Natapov]

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?

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

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

--
			Gleb.

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

[Jan Kiszka]

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

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

[Gleb Natapov]

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.

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

[Nadav Har'El]

On Thu, Feb 14, 2013, Gleb Natapov wrote about "Re: [PATCH v2] KVM: nVMX: Improve I/O exit handling":
> > >> 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.

On real hardware, note that the MSR-bitmap address specified in the VMCS
is a physical address, so there can never be an error - if I understand
correctly, there is no such thing as a non-existant physical address -
reading from non-existant physical memory returns random garbage (please
correct me if I'm wrong here!). So if I'm correct, using a non-existent
address for an MSR-bitmap will give you an undefined behavior - not any
sort of entry error to special type of exit.

The current code, using a random value on the stack, fits with the
"undefined behavior" definition, but you're right the more logical
behavior is to, indeed, always exit on the MSR access when the bitmap
cannot be read. This will make an unreadable bitmap equivalent to no
bitmap at all - which I think makes most sense.

You're also right that this case is identical in both MSR and I/O bitmap
cases, and should be fixed in both.


-- 
Nadav Har'El                        |       Thursday, Feb 14 2013, 4 Adar 5773
nyh@math.technion.ac.il             |-----------------------------------------
Phone +972-523-790466, ICQ 13349191 |A fanatic is one who can't change his
http://nadav.harel.org.il           |mind and won't change the subject.

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

[Gleb Natapov]

On Thu, Feb 14, 2013 at 03:54:23PM +0200, Nadav Har'El wrote:
> On Thu, Feb 14, 2013, Gleb Natapov wrote about "Re: [PATCH v2] KVM: nVMX: Improve I/O exit handling":
> > > >> 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.
> 
> On real hardware, note that the MSR-bitmap address specified in the VMCS
> is a physical address, so there can never be an error - if I understand
> correctly, there is no such thing as a non-existant physical address -
> reading from non-existant physical memory returns random garbage (please
> correct me if I'm wrong here!). So if I'm correct, using a non-existent
> address for an MSR-bitmap will give you an undefined behavior - not any
> sort of entry error to special type of exit.
> 
That's true for real HW. Reading from physical address outside of
physical memory will either access some random device MMIO or nothing.
Either way result is unpredictable and may hang the machine. I am fine
with killing a guest that tries to do that.

> The current code, using a random value on the stack, fits with the
> "undefined behavior" definition, but you're right the more logical
> behavior is to, indeed, always exit on the MSR access when the bitmap
> cannot be read. This will make an unreadable bitmap equivalent to no
> bitmap at all - which I think makes most sense.
Current case leaks data from host kernel, not just exhibit random
behaviour.

> 
> You're also right that this case is identical in both MSR and I/O bitmap
> cases, and should be fixed in both.
> 
> 
> -- 
> Nadav Har'El                        |       Thursday, Feb 14 2013, 4 Adar 5773
> nyh@math.technion.ac.il             |-----------------------------------------
> Phone +972-523-790466, ICQ 13349191 |A fanatic is one who can't change his
> http://nadav.harel.org.il           |mind and won't change the subject.

--
			Gleb.

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

[Jan Kiszka]

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 v3:
 - trap unconditionally if bitmap access fails

 arch/x86/kvm/vmx.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 6667042..2633199 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5908,6 +5908,58 @@ 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)
+			if (kvm_read_guest(vcpu->kvm, bitmap, &b, 1))
+				return 1;
+		if (b & (1 << (port & 7)))
+			return 1;
+
+		port++;
+		size--;
+		last_bitmap = bitmap;
+	}
+
+	return 0;
+}
+
 /*
  * Return 1 if we should exit from L2 to L1 to handle an MSR access access,
  * rather than handle it ourselves in L0. I.e., check whether L1 expressed
@@ -6097,8 +6149,7 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
 	case EXIT_REASON_DR_ACCESS:
 		return nested_cpu_has(vmcs12, CPU_BASED_MOV_DR_EXITING);
 	case EXIT_REASON_IO_INSTRUCTION:
-		/* TODO: support IO bitmaps */
-		return 1;
+		return nested_vmx_exit_handled_io(vcpu, vmcs12);
 	case EXIT_REASON_MSR_READ:
 	case EXIT_REASON_MSR_WRITE:
 		return nested_vmx_exit_handled_msr(vcpu, vmcs12, exit_reason);
-- 
1.7.3.4

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

[Gleb Natapov]

On Thu, Feb 14, 2013 at 07:46:23PM +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>
Reviewed-by: Gleb Natapov <gleb@redhat.com>

> ---
> 
> Changes in v3:
>  - trap unconditionally if bitmap access fails
> 
>  arch/x86/kvm/vmx.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 6667042..2633199 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5908,6 +5908,58 @@ 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)
> +			if (kvm_read_guest(vcpu->kvm, bitmap, &b, 1))
> +				return 1;
> +		if (b & (1 << (port & 7)))
> +			return 1;
> +
> +		port++;
> +		size--;
> +		last_bitmap = bitmap;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * Return 1 if we should exit from L2 to L1 to handle an MSR access access,
>   * rather than handle it ourselves in L0. I.e., check whether L1 expressed
> @@ -6097,8 +6149,7 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
>  	case EXIT_REASON_DR_ACCESS:
>  		return nested_cpu_has(vmcs12, CPU_BASED_MOV_DR_EXITING);
>  	case EXIT_REASON_IO_INSTRUCTION:
> -		/* TODO: support IO bitmaps */
> -		return 1;
> +		return nested_vmx_exit_handled_io(vcpu, vmcs12);
>  	case EXIT_REASON_MSR_READ:
>  	case EXIT_REASON_MSR_WRITE:
>  		return nested_vmx_exit_handled_msr(vcpu, vmcs12, exit_reason);
> -- 
> 1.7.3.4

--
			Gleb.

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2922 bytes --]

On 2013-02-14 19:46, 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 v3:
>  - trap unconditionally if bitmap access fails
> 
>  arch/x86/kvm/vmx.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 6667042..2633199 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5908,6 +5908,58 @@ 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;

Nonsense, rep ins/outs always works against the same port. We can simply
drop this check and be done with the feature. I'll come up with v4.

Jan

> +
> +	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)
> +			if (kvm_read_guest(vcpu->kvm, bitmap, &b, 1))
> +				return 1;
> +		if (b & (1 << (port & 7)))
> +			return 1;
> +
> +		port++;
> +		size--;
> +		last_bitmap = bitmap;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * Return 1 if we should exit from L2 to L1 to handle an MSR access access,
>   * rather than handle it ourselves in L0. I.e., check whether L1 expressed
> @@ -6097,8 +6149,7 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
>  	case EXIT_REASON_DR_ACCESS:
>  		return nested_cpu_has(vmcs12, CPU_BASED_MOV_DR_EXITING);
>  	case EXIT_REASON_IO_INSTRUCTION:
> -		/* TODO: support IO bitmaps */
> -		return 1;
> +		return nested_vmx_exit_handled_io(vcpu, vmcs12);
>  	case EXIT_REASON_MSR_READ:
>  	case EXIT_REASON_MSR_WRITE:
>  		return nested_vmx_exit_handled_msr(vcpu, vmcs12, exit_reason);
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

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

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

This prevents trapping L2 I/O exits if L1 has neither unconditional nor
bitmap-based exiting enabled. Furthermore, it implements I/O bitmap
handling.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Changes in v4:
 - drop pointless check for rep+string - we are already done with this
   feature

 arch/x86/kvm/vmx.c |   47 +++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 6667042..a6c4a32 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5908,6 +5908,50 @@ 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;
+	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);
+
+	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)
+			if (kvm_read_guest(vcpu->kvm, bitmap, &b, 1))
+				return 1;
+		if (b & (1 << (port & 7)))
+			return 1;
+
+		port++;
+		size--;
+		last_bitmap = bitmap;
+	}
+
+	return 0;
+}
+
 /*
  * Return 1 if we should exit from L2 to L1 to handle an MSR access access,
  * rather than handle it ourselves in L0. I.e., check whether L1 expressed
@@ -6097,8 +6141,7 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
 	case EXIT_REASON_DR_ACCESS:
 		return nested_cpu_has(vmcs12, CPU_BASED_MOV_DR_EXITING);
 	case EXIT_REASON_IO_INSTRUCTION:
-		/* TODO: support IO bitmaps */
-		return 1;
+		return nested_vmx_exit_handled_io(vcpu, vmcs12);
 	case EXIT_REASON_MSR_READ:
 	case EXIT_REASON_MSR_WRITE:
 		return nested_vmx_exit_handled_msr(vcpu, vmcs12, exit_reason);
-- 
1.7.3.4

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

[Gleb Natapov]

On Mon, Feb 18, 2013 at 07:32:53AM +0100, Jan Kiszka wrote:
> On 2013-02-14 19:46, 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 v3:
> >  - trap unconditionally if bitmap access fails
> > 
> >  arch/x86/kvm/vmx.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 53 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index 6667042..2633199 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -5908,6 +5908,58 @@ 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;
> 
> Nonsense, rep ins/outs always works against the same port. We can simply
> drop this check and be done with the feature. I'll come up with v4.
> 
Actually this reminds me that we should check range of ports depending
on operand size, not one port. But here is a catch, older cpus do not
provide operand size as part of exit information.

> Jan
> 
> > +
> > +	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)
> > +			if (kvm_read_guest(vcpu->kvm, bitmap, &b, 1))
> > +				return 1;
> > +		if (b & (1 << (port & 7)))
> > +			return 1;
> > +
> > +		port++;
> > +		size--;
> > +		last_bitmap = bitmap;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /*
> >   * Return 1 if we should exit from L2 to L1 to handle an MSR access access,
> >   * rather than handle it ourselves in L0. I.e., check whether L1 expressed
> > @@ -6097,8 +6149,7 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
> >  	case EXIT_REASON_DR_ACCESS:
> >  		return nested_cpu_has(vmcs12, CPU_BASED_MOV_DR_EXITING);
> >  	case EXIT_REASON_IO_INSTRUCTION:
> > -		/* TODO: support IO bitmaps */
> > -		return 1;
> > +		return nested_vmx_exit_handled_io(vcpu, vmcs12);
> >  	case EXIT_REASON_MSR_READ:
> >  	case EXIT_REASON_MSR_WRITE:
> >  		return nested_vmx_exit_handled_msr(vcpu, vmcs12, exit_reason);
> > 
> 
> 



--
			Gleb.

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2242 bytes --]

On 2013-02-18 09:44, Gleb Natapov wrote:
> On Mon, Feb 18, 2013 at 07:32:53AM +0100, Jan Kiszka wrote:
>> On 2013-02-14 19:46, 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 v3:
>>>  - trap unconditionally if bitmap access fails
>>>
>>>  arch/x86/kvm/vmx.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>  1 files changed, 53 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>> index 6667042..2633199 100644
>>> --- a/arch/x86/kvm/vmx.c
>>> +++ b/arch/x86/kvm/vmx.c
>>> @@ -5908,6 +5908,58 @@ 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;
>>
>> Nonsense, rep ins/outs always works against the same port. We can simply
>> drop this check and be done with the feature. I'll come up with v4.
>>
> Actually this reminds me that we should check range of ports depending
> on operand size, not one port. But here is a catch, older cpus do not
> provide operand size as part of exit information.

You mean what bit 54 in VMX_BASIC is telling us? Too bad. OK, will write
v5 which takes this into account.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

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

[Gleb Natapov]

On Mon, Feb 18, 2013 at 09:53:22AM +0100, Jan Kiszka wrote:
> On 2013-02-18 09:44, Gleb Natapov wrote:
> > On Mon, Feb 18, 2013 at 07:32:53AM +0100, Jan Kiszka wrote:
> >> On 2013-02-14 19:46, 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 v3:
> >>>  - trap unconditionally if bitmap access fails
> >>>
> >>>  arch/x86/kvm/vmx.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >>>  1 files changed, 53 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> >>> index 6667042..2633199 100644
> >>> --- a/arch/x86/kvm/vmx.c
> >>> +++ b/arch/x86/kvm/vmx.c
> >>> @@ -5908,6 +5908,58 @@ 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;
> >>
> >> Nonsense, rep ins/outs always works against the same port. We can simply
> >> drop this check and be done with the feature. I'll come up with v4.
> >>
> > Actually this reminds me that we should check range of ports depending
> > on operand size, not one port. But here is a catch, older cpus do not
> > provide operand size as part of exit information.
> 
> You mean what bit 54 in VMX_BASIC is telling us? Too bad. OK, will write
> v5 which takes this into account.
> 
Yes, this one. We can just exit unconditionally on older cpus.

--
			Gleb.

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

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

This prevents trapping L2 I/O exits if L1 has neither unconditional nor
bitmap-based exiting enabled. Furthermore, it implements I/O bitmap
handling. We still exit unconditionally in case the CPU does not provide
information for ins/outs.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Changes in v5:
 - still exit unconditionally if CPU refuses to provide exit
   information on ins/outs

 arch/x86/kvm/vmx.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 6667042..ccc7c17 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -651,6 +651,7 @@ static struct vmcs_config {
 	int size;
 	int order;
 	u32 revision_id;
+	u32 vmx_basic_high;
 	u32 pin_based_exec_ctrl;
 	u32 cpu_based_exec_ctrl;
 	u32 cpu_based_2nd_exec_ctrl;
@@ -752,6 +753,11 @@ static inline bool vm_need_tpr_shadow(struct kvm *kvm)
 	return (cpu_has_vmx_tpr_shadow()) && (irqchip_in_kernel(kvm));
 }
 
+static inline bool cpu_has_stringio_exit_info(void)
+{
+	return vmcs_config.vmx_basic_high & (VMX_BASIC_INOUT >> 32);
+}
+
 static inline bool cpu_has_secondary_exec_ctrls(void)
 {
 	return vmcs_config.cpu_based_exec_ctrl &
@@ -2635,6 +2641,7 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf)
 	vmcs_conf->size = vmx_msr_high & 0x1fff;
 	vmcs_conf->order = get_order(vmcs_config.size);
 	vmcs_conf->revision_id = vmx_msr_low;
+	vmcs_conf->vmx_basic_high = vmx_msr_high;
 
 	vmcs_conf->pin_based_exec_ctrl = _pin_based_exec_control;
 	vmcs_conf->cpu_based_exec_ctrl = _cpu_based_exec_control;
@@ -5908,6 +5915,54 @@ 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;
+	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;
+
+	/* TODO: for older CPUs, derive access width from instruction */
+	if (!cpu_has_stringio_exit_info())
+		return 1;
+
+	exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
+
+	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)
+			if (kvm_read_guest(vcpu->kvm, bitmap, &b, 1))
+				return 1;
+		if (b & (1 << (port & 7)))
+			return 1;
+
+		port++;
+		size--;
+		last_bitmap = bitmap;
+	}
+
+	return 0;
+}
+
 /*
  * Return 1 if we should exit from L2 to L1 to handle an MSR access access,
  * rather than handle it ourselves in L0. I.e., check whether L1 expressed
@@ -6097,8 +6152,7 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
 	case EXIT_REASON_DR_ACCESS:
 		return nested_cpu_has(vmcs12, CPU_BASED_MOV_DR_EXITING);
 	case EXIT_REASON_IO_INSTRUCTION:
-		/* TODO: support IO bitmaps */
-		return 1;
+		return nested_vmx_exit_handled_io(vcpu, vmcs12);
 	case EXIT_REASON_MSR_READ:
 	case EXIT_REASON_MSR_WRITE:
 		return nested_vmx_exit_handled_msr(vcpu, vmcs12, exit_reason);
-- 
1.7.3.4

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

[Gleb Natapov]

On Mon, Feb 18, 2013 at 10:17:14AM +0100, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> This prevents trapping L2 I/O exits if L1 has neither unconditional nor
> bitmap-based exiting enabled. Furthermore, it implements I/O bitmap
> handling. We still exit unconditionally in case the CPU does not provide
> information for ins/outs.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> Changes in v5:
>  - still exit unconditionally if CPU refuses to provide exit
>    information on ins/outs
> 
>  arch/x86/kvm/vmx.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 56 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 6667042..ccc7c17 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -651,6 +651,7 @@ static struct vmcs_config {
>  	int size;
>  	int order;
>  	u32 revision_id;
> +	u32 vmx_basic_high;
>  	u32 pin_based_exec_ctrl;
>  	u32 cpu_based_exec_ctrl;
>  	u32 cpu_based_2nd_exec_ctrl;
> @@ -752,6 +753,11 @@ static inline bool vm_need_tpr_shadow(struct kvm *kvm)
>  	return (cpu_has_vmx_tpr_shadow()) && (irqchip_in_kernel(kvm));
>  }
>  
> +static inline bool cpu_has_stringio_exit_info(void)
> +{
> +	return vmcs_config.vmx_basic_high & (VMX_BASIC_INOUT >> 32);
> +}
> +
>  static inline bool cpu_has_secondary_exec_ctrls(void)
>  {
>  	return vmcs_config.cpu_based_exec_ctrl &
> @@ -2635,6 +2641,7 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf)
>  	vmcs_conf->size = vmx_msr_high & 0x1fff;
>  	vmcs_conf->order = get_order(vmcs_config.size);
>  	vmcs_conf->revision_id = vmx_msr_low;
> +	vmcs_conf->vmx_basic_high = vmx_msr_high;
>  
>  	vmcs_conf->pin_based_exec_ctrl = _pin_based_exec_control;
>  	vmcs_conf->cpu_based_exec_ctrl = _cpu_based_exec_control;
> @@ -5908,6 +5915,54 @@ 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;
> +	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;
> +
> +	/* TODO: for older CPUs, derive access width from instruction */
> +	if (!cpu_has_stringio_exit_info())
> +		return 1;
> +

Sigh, actually I am stupid :( The information that is not available
on older cpus is address size in "VM-exit instruction information",
not operand size on exit qualification, so your v4 is correct. Except
handling of port wrap around:

  If the “use I/O bitmaps” VM-execution control is 1, the instruction
  causes a VM exit if it attempts to access an I/O port corresponding to a
  bit set to 1 in the appropriate I/O bitmap (see Section 24.6.4). If an
  I/O operation “wraps around” the 16-bit I/O-port space (accesses
  ports FFFFH and 0000H), the I/O instruction causes a VM exit (the
  “unconditional I/O exiting” VM-execution control is ignored if the
  “use I/O bitmaps” VM-execution control is 1).


> +	exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
> +
> +	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)
> +			if (kvm_read_guest(vcpu->kvm, bitmap, &b, 1))
> +				return 1;
> +		if (b & (1 << (port & 7)))
> +			return 1;
> +
> +		port++;
> +		size--;
> +		last_bitmap = bitmap;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * Return 1 if we should exit from L2 to L1 to handle an MSR access access,
>   * rather than handle it ourselves in L0. I.e., check whether L1 expressed
> @@ -6097,8 +6152,7 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
>  	case EXIT_REASON_DR_ACCESS:
>  		return nested_cpu_has(vmcs12, CPU_BASED_MOV_DR_EXITING);
>  	case EXIT_REASON_IO_INSTRUCTION:
> -		/* TODO: support IO bitmaps */
> -		return 1;
> +		return nested_vmx_exit_handled_io(vcpu, vmcs12);
>  	case EXIT_REASON_MSR_READ:
>  	case EXIT_REASON_MSR_WRITE:
>  		return nested_vmx_exit_handled_msr(vcpu, vmcs12, exit_reason);
> -- 
> 1.7.3.4

--
			Gleb.

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3558 bytes --]

On 2013-02-18 10:36, Gleb Natapov wrote:
> On Mon, Feb 18, 2013 at 10:17:14AM +0100, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> This prevents trapping L2 I/O exits if L1 has neither unconditional nor
>> bitmap-based exiting enabled. Furthermore, it implements I/O bitmap
>> handling. We still exit unconditionally in case the CPU does not provide
>> information for ins/outs.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> Changes in v5:
>>  - still exit unconditionally if CPU refuses to provide exit
>>    information on ins/outs
>>
>>  arch/x86/kvm/vmx.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 files changed, 56 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 6667042..ccc7c17 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -651,6 +651,7 @@ static struct vmcs_config {
>>  	int size;
>>  	int order;
>>  	u32 revision_id;
>> +	u32 vmx_basic_high;
>>  	u32 pin_based_exec_ctrl;
>>  	u32 cpu_based_exec_ctrl;
>>  	u32 cpu_based_2nd_exec_ctrl;
>> @@ -752,6 +753,11 @@ static inline bool vm_need_tpr_shadow(struct kvm *kvm)
>>  	return (cpu_has_vmx_tpr_shadow()) && (irqchip_in_kernel(kvm));
>>  }
>>  
>> +static inline bool cpu_has_stringio_exit_info(void)
>> +{
>> +	return vmcs_config.vmx_basic_high & (VMX_BASIC_INOUT >> 32);
>> +}
>> +
>>  static inline bool cpu_has_secondary_exec_ctrls(void)
>>  {
>>  	return vmcs_config.cpu_based_exec_ctrl &
>> @@ -2635,6 +2641,7 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf)
>>  	vmcs_conf->size = vmx_msr_high & 0x1fff;
>>  	vmcs_conf->order = get_order(vmcs_config.size);
>>  	vmcs_conf->revision_id = vmx_msr_low;
>> +	vmcs_conf->vmx_basic_high = vmx_msr_high;
>>  
>>  	vmcs_conf->pin_based_exec_ctrl = _pin_based_exec_control;
>>  	vmcs_conf->cpu_based_exec_ctrl = _cpu_based_exec_control;
>> @@ -5908,6 +5915,54 @@ 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;
>> +	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;
>> +
>> +	/* TODO: for older CPUs, derive access width from instruction */
>> +	if (!cpu_has_stringio_exit_info())
>> +		return 1;
>> +
> 
> Sigh, actually I am stupid :( The information that is not available
> on older cpus is address size in "VM-exit instruction information",
> not operand size on exit qualification, so your v4 is correct. Except
> handling of port wrap around:
> 
>   If the “use I/O bitmaps” VM-execution control is 1, the instruction
>   causes a VM exit if it attempts to access an I/O port corresponding to a
>   bit set to 1 in the appropriate I/O bitmap (see Section 24.6.4). If an
>   I/O operation “wraps around” the 16-bit I/O-port space (accesses
>   ports FFFFH and 0000H), the I/O instruction causes a VM exit (the
>   “unconditional I/O exiting” VM-execution control is ignored if the
>   “use I/O bitmaps” VM-execution control is 1).

Ah, indeed.

OK, let's see if this "simple" patch manages to reach two-digit revision
numbers...

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

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

[Jan Kiszka]

This prevents trapping L2 I/O exits if L1 has neither unconditional nor
bitmap-based exiting enabled. Furthermore, it implements I/O bitmap
handling.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Changes in v6:
 - drop the bogus check of vmx_basix.54 again
 - exit unconditionally on I/O address wrap around

 arch/x86/kvm/vmx.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 6667042..b4ce43c 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5908,6 +5908,52 @@ 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;
+	unsigned int 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);
+
+	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 if (port < 0x10000)
+			bitmap = vmcs12->io_bitmap_b;
+		else
+			return 1;
+		bitmap += (port & 0x7fff) / 8;
+
+		if (last_bitmap != bitmap)
+			if (kvm_read_guest(vcpu->kvm, bitmap, &b, 1))
+				return 1;
+		if (b & (1 << (port & 7)))
+			return 1;
+
+		port++;
+		size--;
+		last_bitmap = bitmap;
+	}
+
+	return 0;
+}
+
 /*
  * Return 1 if we should exit from L2 to L1 to handle an MSR access access,
  * rather than handle it ourselves in L0. I.e., check whether L1 expressed
@@ -6097,8 +6143,7 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
 	case EXIT_REASON_DR_ACCESS:
 		return nested_cpu_has(vmcs12, CPU_BASED_MOV_DR_EXITING);
 	case EXIT_REASON_IO_INSTRUCTION:
-		/* TODO: support IO bitmaps */
-		return 1;
+		return nested_vmx_exit_handled_io(vcpu, vmcs12);
 	case EXIT_REASON_MSR_READ:
 	case EXIT_REASON_MSR_WRITE:
 		return nested_vmx_exit_handled_msr(vcpu, vmcs12, exit_reason);
-- 
1.7.3.4

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

[Gleb Natapov]

On Mon, Feb 18, 2013 at 11:21:16AM +0100, Jan Kiszka wrote:
> This prevents trapping L2 I/O exits if L1 has neither unconditional nor
> bitmap-based exiting enabled. Furthermore, it implements I/O bitmap
> handling.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Reviewed-by: Gleb Natapov <gleb@redhat.com>

> ---
> 
> Changes in v6:
>  - drop the bogus check of vmx_basix.54 again
>  - exit unconditionally on I/O address wrap around
> 
>  arch/x86/kvm/vmx.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 6667042..b4ce43c 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5908,6 +5908,52 @@ 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;
> +	unsigned int 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);
> +
> +	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 if (port < 0x10000)
> +			bitmap = vmcs12->io_bitmap_b;
> +		else
> +			return 1;
> +		bitmap += (port & 0x7fff) / 8;
> +
> +		if (last_bitmap != bitmap)
> +			if (kvm_read_guest(vcpu->kvm, bitmap, &b, 1))
> +				return 1;
> +		if (b & (1 << (port & 7)))
> +			return 1;
> +
> +		port++;
> +		size--;
> +		last_bitmap = bitmap;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * Return 1 if we should exit from L2 to L1 to handle an MSR access access,
>   * rather than handle it ourselves in L0. I.e., check whether L1 expressed
> @@ -6097,8 +6143,7 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
>  	case EXIT_REASON_DR_ACCESS:
>  		return nested_cpu_has(vmcs12, CPU_BASED_MOV_DR_EXITING);
>  	case EXIT_REASON_IO_INSTRUCTION:
> -		/* TODO: support IO bitmaps */
> -		return 1;
> +		return nested_vmx_exit_handled_io(vcpu, vmcs12);
>  	case EXIT_REASON_MSR_READ:
>  	case EXIT_REASON_MSR_WRITE:
>  		return nested_vmx_exit_handled_msr(vcpu, vmcs12, exit_reason);
> -- 
> 1.7.3.4

--
			Gleb.

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

[Marcelo Tosatti]

On Mon, Feb 18, 2013 at 12:32:37PM +0200, Gleb Natapov wrote:
> On Mon, Feb 18, 2013 at 11:21:16AM +0100, Jan Kiszka wrote:
> > This prevents trapping L2 I/O exits if L1 has neither unconditional nor
> > bitmap-based exiting enabled. Furthermore, it implements I/O bitmap
> > handling.
> > 
> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> Reviewed-by: Gleb Natapov <gleb@redhat.com>

Applied, thanks.