[PATCH] Check use I/O bitmap first before unconditional I/O exit

[PATCH] Check use I/O bitmap first before unconditional I/O exit

[Zhihui Zhang]

According to Table C-1 of Intel SDM 3C, a VM exit happens on an I/O instruction when
"use I/O bitmaps" VM-execution control was 0 _and_ the "unconditional I/O exiting"
VM-execution control was 1. So we can't just check "unconditional I/O exiting" alone.
This patch was improved by suggestion from Jan Kiszka.

Signed-off-by: Zhihui Zhang <zzhsuny@gmail.com>
---
 arch/x86/kvm/vmx.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index b2fe1c2..45617db 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -6460,11 +6460,8 @@ static bool nested_vmx_exit_handled_io(struct kvm_vcpu *vcpu,
 	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;
+		return nested_cpu_has(vmcs12, CPU_BASED_UNCOND_IO_EXITING);
 
 	exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
 
-- 
1.8.1.2

Re: [PATCH] Check use I/O bitmap first before unconditional I/O exit

[Jan Kiszka]

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

On 2013-12-30 21:56, Zhihui Zhang wrote:
> According to Table C-1 of Intel SDM 3C, a VM exit happens on an I/O instruction when
> "use I/O bitmaps" VM-execution control was 0 _and_ the "unconditional I/O exiting"
> VM-execution control was 1. So we can't just check "unconditional I/O exiting" alone.
> This patch was improved by suggestion from Jan Kiszka.
> 
> Signed-off-by: Zhihui Zhang <zzhsuny@gmail.com>
> ---
>  arch/x86/kvm/vmx.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index b2fe1c2..45617db 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -6460,11 +6460,8 @@ static bool nested_vmx_exit_handled_io(struct kvm_vcpu *vcpu,
>  	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;
> +		return nested_cpu_has(vmcs12, CPU_BASED_UNCOND_IO_EXITING);
>  
>  	exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
>  
> 

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



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

Re: [PATCH] Check use I/O bitmap first before unconditional I/O exit

[Marcelo Tosatti]

On Mon, Dec 30, 2013 at 03:56:29PM -0500, Zhihui Zhang wrote:
> According to Table C-1 of Intel SDM 3C, a VM exit happens on an I/O instruction when
> "use I/O bitmaps" VM-execution control was 0 _and_ the "unconditional I/O exiting"
> VM-execution control was 1. So we can't just check "unconditional I/O exiting" alone.
> This patch was improved by suggestion from Jan Kiszka.
> 
> Signed-off-by: Zhihui Zhang <zzhsuny@gmail.com>

Applied, thanks.

[PATCH] Check use I/O bitmap first before unconditional I/O exit

[Zhihui Zhang]

Signed-off-by: Zhihui Zhang <zzhsuny@gmail.com>
---
 arch/x86/kvm/vmx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index b2fe1c2..44d4f829 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -6460,11 +6460,11 @@ static bool nested_vmx_exit_handled_io(struct kvm_vcpu *vcpu,
 	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))
+	if (!nested_cpu_has(vmcs12, CPU_BASED_USE_IO_BITMAPS)) {
+		if (nested_cpu_has(vmcs12, CPU_BASED_UNCOND_IO_EXITING))
+			return 1;
 		return 0;
+	}
 
 	exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
 
-- 
1.8.1.2

Re: [PATCH] Check use I/O bitmap first before unconditional I/O exit

[Jan Kiszka]

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

On 2013-12-30 16:09, Zhihui Zhang wrote:
> Signed-off-by: Zhihui Zhang <zzhsuny@gmail.com>

You should explain the "why" here, e.g. point to the SDM section that
requires this ordering.

> ---
>  arch/x86/kvm/vmx.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index b2fe1c2..44d4f829 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -6460,11 +6460,11 @@ static bool nested_vmx_exit_handled_io(struct kvm_vcpu *vcpu,
>  	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))
> +	if (!nested_cpu_has(vmcs12, CPU_BASED_USE_IO_BITMAPS)) {
> +		if (nested_cpu_has(vmcs12, CPU_BASED_UNCOND_IO_EXITING))
> +			return 1;

Minor nit, but what about

if (!nested_cpu_has(vmcs12, CPU_BASED_USE_IO_BITMAPS))
	return nested_cpu_has(vmcs12, CPU_BASED_UNCOND_IO_EXITING);

instead?

Logic is correct, thanks for addressing this.

Could you also write an according sub-test in kvm-unit-tests [1]? We are
already checking IO bitmap accesses, but not yet unconditional PIO
exiting. Would be great!

Jan

[1] https://git.kernel.org/cgit/virt/kvm/kvm-unit-tests.git

>  		return 0;
> +	}
>  
>  	exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
>  
> 



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