[PATCH 0/3] KVM: PPC: Minor fixes

[PATCH 0/3] KVM: PPC: Minor fixes

[Fabiano Rosas]

Two fixes for MMIO emulation code that don't really affect anything.

One fix for ioctl return code that is a prerequisite for the selftests
enablement.

Fabiano Rosas (3):
  KVM: PPC: Book3S HV: Stop returning internal values to userspace
  KVM: PPC: Fix vmx/vsx mixup in mmio emulation
  KVM: PPC: Fix mmio length message

 arch/powerpc/kvm/powerpc.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

-- 
2.33.1

[PATCH 1/3] KVM: PPC: Book3S HV: Stop returning internal values to userspace

[Fabiano Rosas]

Our kvm_arch_vcpu_ioctl_run currently returns the RESUME_HOST values
to userspace, against the API of the KVM_RUN ioctl which returns 0 on
success.

Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
This was noticed while enabling the kvm selftests for powerpc. There's
an assert at the _vcpu_run function when we return a value different
from the expected.
---
 arch/powerpc/kvm/powerpc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index a72920f4f221..1e130bb087c4 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -1849,6 +1849,14 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
 #ifdef CONFIG_ALTIVEC
 out:
 #endif
+
+	/*
+	 * We're already returning to userspace, don't pass the
+	 * RESUME_HOST flags along.
+	 */
+	if (r > 0)
+		r = 0;
+
 	vcpu_put(vcpu);
 	return r;
 }
-- 
2.33.1

Re: [PATCH 1/3] KVM: PPC: Book3S HV: Stop returning internal values to userspace

[Nicholas Piggin]

Excerpts from Fabiano Rosas's message of December 24, 2021 7:15 am:
> Our kvm_arch_vcpu_ioctl_run currently returns the RESUME_HOST values
> to userspace, against the API of the KVM_RUN ioctl which returns 0 on
> success.
> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---
> This was noticed while enabling the kvm selftests for powerpc. There's
> an assert at the _vcpu_run function when we return a value different
> from the expected.

That's nasty. Looks like qemu never touches the return value except if
it was < 0, so hopefully should be okay.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> ---
>  arch/powerpc/kvm/powerpc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index a72920f4f221..1e130bb087c4 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -1849,6 +1849,14 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>  #ifdef CONFIG_ALTIVEC
>  out:
>  #endif
> +
> +	/*
> +	 * We're already returning to userspace, don't pass the
> +	 * RESUME_HOST flags along.
> +	 */
> +	if (r > 0)
> +		r = 0;
> +
>  	vcpu_put(vcpu);
>  	return r;
>  }
> -- 
> 2.33.1
> 
> 

[PATCH 2/3] KVM: PPC: Fix vmx/vsx mixup in mmio emulation

[Fabiano Rosas]

The MMIO emulation code for vector instructions is duplicated between
VSX and VMX. When emulating VMX we should check the VMX copy size
instead of the VSX one.

Fixes: acc9eb9305fe ("KVM: PPC: Reimplement LOAD_VMX/STORE_VMX instruction ...")
Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
 arch/powerpc/kvm/powerpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 1e130bb087c4..793d42bd6c8f 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -1507,7 +1507,7 @@ int kvmppc_handle_vmx_load(struct kvm_vcpu *vcpu,
 {
 	enum emulation_result emulated = EMULATE_DONE;
 
-	if (vcpu->arch.mmio_vsx_copy_nums > 2)
+	if (vcpu->arch.mmio_vmx_copy_nums > 2)
 		return EMULATE_FAIL;
 
 	while (vcpu->arch.mmio_vmx_copy_nums) {
-- 
2.33.1

Re: [PATCH 2/3] KVM: PPC: Fix vmx/vsx mixup in mmio emulation

[Nicholas Piggin]

Excerpts from Fabiano Rosas's message of December 24, 2021 7:15 am:
> The MMIO emulation code for vector instructions is duplicated between
> VSX and VMX. When emulating VMX we should check the VMX copy size
> instead of the VSX one.
> 
> Fixes: acc9eb9305fe ("KVM: PPC: Reimplement LOAD_VMX/STORE_VMX instruction ...")
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>

Good catch. AFAIKS handle_vmx_store needs the same treatment? If you
agree then

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> ---
>  arch/powerpc/kvm/powerpc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 1e130bb087c4..793d42bd6c8f 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -1507,7 +1507,7 @@ int kvmppc_handle_vmx_load(struct kvm_vcpu *vcpu,
>  {
>  	enum emulation_result emulated = EMULATE_DONE;
>  
> -	if (vcpu->arch.mmio_vsx_copy_nums > 2)
> +	if (vcpu->arch.mmio_vmx_copy_nums > 2)
>  		return EMULATE_FAIL;
>  
>  	while (vcpu->arch.mmio_vmx_copy_nums) {
> -- 
> 2.33.1
> 
> 

Re: [PATCH 2/3] KVM: PPC: Fix vmx/vsx mixup in mmio emulation

[Fabiano Rosas]

Nicholas Piggin <npiggin@gmail.com> writes:

> Excerpts from Fabiano Rosas's message of December 24, 2021 7:15 am:
>> The MMIO emulation code for vector instructions is duplicated between
>> VSX and VMX. When emulating VMX we should check the VMX copy size
>> instead of the VSX one.
>> 
>> Fixes: acc9eb9305fe ("KVM: PPC: Reimplement LOAD_VMX/STORE_VMX instruction ...")
>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
>
> Good catch. AFAIKS handle_vmx_store needs the same treatment? If you
> agree then

Half the bug now, half the bug next year... haha I'll send a v2.

aside:
All this duplication is kind of annoying. I'm looking into what it would
take to have quadword instruction emulation here as well (Alexey caught
a bug with syskaller) and the code would be really similar. I see that
x86 has a more generic implementation that maybe we could take advantage
of. See "f78146b0f923 (KVM: Fix page-crossing MMIO)"

Re: [PATCH 2/3] KVM: PPC: Fix vmx/vsx mixup in mmio emulation

[Alexey Kardashevskiy]

On 28/12/2021 04:28, Fabiano Rosas wrote:
> Nicholas Piggin <npiggin@gmail.com> writes:
> 
>> Excerpts from Fabiano Rosas's message of December 24, 2021 7:15 am:
>>> The MMIO emulation code for vector instructions is duplicated between
>>> VSX and VMX. When emulating VMX we should check the VMX copy size
>>> instead of the VSX one.
>>>
>>> Fixes: acc9eb9305fe ("KVM: PPC: Reimplement LOAD_VMX/STORE_VMX instruction ...")
>>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
>>
>> Good catch. AFAIKS handle_vmx_store needs the same treatment? If you
>> agree then
> 
> Half the bug now, half the bug next year... haha I'll send a v2.
> 
> aside:
> All this duplication is kind of annoying. I'm looking into what it would
> take to have quadword instruction emulation here as well (Alexey caught
> a bug with syskaller) and the code would be really similar. I see that
> x86 has a more generic implementation that maybe we could take advantage
> of. See "f78146b0f923 (KVM: Fix page-crossing MMIO)"

Uff. My head exploded with vsx/vmx/vec :)
But this seems to have fixed "lvx" (which is vmx, right?).

Tested with: https://github.com/aik/linux/commits/my_kvm_tests



-- 
Alexey

[PATCH 3/3] KVM: PPC: Fix mmio length message

[Fabiano Rosas]

We check against 'bytes' but print 'run->mmio.len' which at that point
has an old value.

e.g. 16-byte load:

before:
__kvmppc_handle_load: bad MMIO length: 8

now:
__kvmppc_handle_load: bad MMIO length: 16

Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
 arch/powerpc/kvm/powerpc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 793d42bd6c8f..7823207eb8f1 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -1246,7 +1246,7 @@ static int __kvmppc_handle_load(struct kvm_vcpu *vcpu,
 
 	if (bytes > sizeof(run->mmio.data)) {
 		printk(KERN_ERR "%s: bad MMIO length: %d\n", __func__,
-		       run->mmio.len);
+		       bytes);
 	}
 
 	run->mmio.phys_addr = vcpu->arch.paddr_accessed;
@@ -1335,7 +1335,7 @@ int kvmppc_handle_store(struct kvm_vcpu *vcpu,
 
 	if (bytes > sizeof(run->mmio.data)) {
 		printk(KERN_ERR "%s: bad MMIO length: %d\n", __func__,
-		       run->mmio.len);
+		       bytes);
 	}
 
 	run->mmio.phys_addr = vcpu->arch.paddr_accessed;
-- 
2.33.1

Re: [PATCH 3/3] KVM: PPC: Fix mmio length message

[Nicholas Piggin]

Excerpts from Fabiano Rosas's message of December 24, 2021 7:15 am:
> We check against 'bytes' but print 'run->mmio.len' which at that point
> has an old value.
> 
> e.g. 16-byte load:
> 
> before:
> __kvmppc_handle_load: bad MMIO length: 8
> 
> now:
> __kvmppc_handle_load: bad MMIO length: 16
> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>

This patch fine, but in the case of overflow we continue anyway here.
Can that overwrite some other memory in the kvm_run struct?

This is familiar, maybe something Alexey has noticed in the past too?
What was the consensus on fixing it? (at least it should have a comment
if it's not a problem IMO)

Thanks,
Nick

> ---
>  arch/powerpc/kvm/powerpc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 793d42bd6c8f..7823207eb8f1 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -1246,7 +1246,7 @@ static int __kvmppc_handle_load(struct kvm_vcpu *vcpu,
>  
>  	if (bytes > sizeof(run->mmio.data)) {
>  		printk(KERN_ERR "%s: bad MMIO length: %d\n", __func__,
> -		       run->mmio.len);
> +		       bytes);
>  	}
>  
>  	run->mmio.phys_addr = vcpu->arch.paddr_accessed;
> @@ -1335,7 +1335,7 @@ int kvmppc_handle_store(struct kvm_vcpu *vcpu,
>  
>  	if (bytes > sizeof(run->mmio.data)) {
>  		printk(KERN_ERR "%s: bad MMIO length: %d\n", __func__,
> -		       run->mmio.len);
> +		       bytes);
>  	}
>  
>  	run->mmio.phys_addr = vcpu->arch.paddr_accessed;
> -- 
> 2.33.1
> 
> 

Re: [PATCH 3/3] KVM: PPC: Fix mmio length message

[Fabiano Rosas]

Nicholas Piggin <npiggin@gmail.com> writes:

> Excerpts from Fabiano Rosas's message of December 24, 2021 7:15 am:
>> We check against 'bytes' but print 'run->mmio.len' which at that point
>> has an old value.
>> 
>> e.g. 16-byte load:
>> 
>> before:
>> __kvmppc_handle_load: bad MMIO length: 8
>> 
>> now:
>> __kvmppc_handle_load: bad MMIO length: 16
>> 
>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
>
> This patch fine, but in the case of overflow we continue anyway here.
> Can that overwrite some other memory in the kvm_run struct?

I tested this and QEMU will indeed overwrite the subsequent fields of
kvm_run. A `lq` on this data:

mmio_test_data:
	.long	0xdeadbeef
	.long	0x8badf00d
	.long	0x1337c0de
	.long	0x01abcdef

produces:

__kvmppc_handle_load: bad MMIO length: 16
kvmppc_complete_mmio_load data: 0x8badf00ddeadbeef
bad MMIO length: 322420958          <-- mmio.len got nuked

But then we return from kvmppc_complete_mmio_load without writing to the
registers.

>
> This is familiar, maybe something Alexey has noticed in the past too?
> What was the consensus on fixing it? (at least it should have a comment
> if it's not a problem IMO)

My plan was to just add quadword support. And whatever else might
missing. But I got sidetracked with how to test this so I'm just now
coming back to it.

Perhaps a more immediate fix is needed before that? We could block loads
and stores larger than 8 bytes earlier at kvmppc_emulate_loadstore for
instance.