* [PATCH 0/3] KVM: PPC: Minor fixes
@ 2021-12-23 21:15 ` Fabiano Rosas
0 siblings, 0 replies; 20+ messages in thread
From: Fabiano Rosas @ 2021-12-23 21:15 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, npiggin, aik
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 0/3] KVM: PPC: Minor fixes
@ 2021-12-23 21:15 ` Fabiano Rosas
0 siblings, 0 replies; 20+ messages in thread
From: Fabiano Rosas @ 2021-12-23 21:15 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, npiggin, aik
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/3] KVM: PPC: Book3S HV: Stop returning internal values to userspace
2021-12-23 21:15 ` Fabiano Rosas
@ 2021-12-23 21:15 ` Fabiano Rosas
-1 siblings, 0 replies; 20+ messages in thread
From: Fabiano Rosas @ 2021-12-23 21:15 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, npiggin, aik
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 1/3] KVM: PPC: Book3S HV: Stop returning internal values to userspace
@ 2021-12-23 21:15 ` Fabiano Rosas
0 siblings, 0 replies; 20+ messages in thread
From: Fabiano Rosas @ 2021-12-23 21:15 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, npiggin, aik
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/3] KVM: PPC: Fix vmx/vsx mixup in mmio emulation
2021-12-23 21:15 ` Fabiano Rosas
@ 2021-12-23 21:15 ` Fabiano Rosas
-1 siblings, 0 replies; 20+ messages in thread
From: Fabiano Rosas @ 2021-12-23 21:15 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, npiggin, aik
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/3] KVM: PPC: Fix vmx/vsx mixup in mmio emulation
@ 2021-12-23 21:15 ` Fabiano Rosas
0 siblings, 0 replies; 20+ messages in thread
From: Fabiano Rosas @ 2021-12-23 21:15 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, npiggin, aik
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/3] KVM: PPC: Fix mmio length message
2021-12-23 21:15 ` Fabiano Rosas
@ 2021-12-23 21:15 ` Fabiano Rosas
-1 siblings, 0 replies; 20+ messages in thread
From: Fabiano Rosas @ 2021-12-23 21:15 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, npiggin, aik
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/3] KVM: PPC: Fix mmio length message
@ 2021-12-23 21:15 ` Fabiano Rosas
0 siblings, 0 replies; 20+ messages in thread
From: Fabiano Rosas @ 2021-12-23 21:15 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, npiggin, aik
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] KVM: PPC: Book3S HV: Stop returning internal values to userspace
2021-12-23 21:15 ` Fabiano Rosas
@ 2021-12-25 10:11 ` Nicholas Piggin
-1 siblings, 0 replies; 20+ messages in thread
From: Nicholas Piggin @ 2021-12-25 10:11 UTC (permalink / raw)
To: Fabiano Rosas, kvm-ppc; +Cc: aik, linuxppc-dev
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
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] KVM: PPC: Book3S HV: Stop returning internal values to userspace
@ 2021-12-25 10:11 ` Nicholas Piggin
0 siblings, 0 replies; 20+ messages in thread
From: Nicholas Piggin @ 2021-12-25 10:11 UTC (permalink / raw)
To: Fabiano Rosas, kvm-ppc; +Cc: aik, linuxppc-dev
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
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] KVM: PPC: Fix vmx/vsx mixup in mmio emulation
2021-12-23 21:15 ` Fabiano Rosas
@ 2021-12-25 10:12 ` Nicholas Piggin
-1 siblings, 0 replies; 20+ messages in thread
From: Nicholas Piggin @ 2021-12-25 10:12 UTC (permalink / raw)
To: Fabiano Rosas, kvm-ppc; +Cc: aik, linuxppc-dev
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
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] KVM: PPC: Fix vmx/vsx mixup in mmio emulation
@ 2021-12-25 10:12 ` Nicholas Piggin
0 siblings, 0 replies; 20+ messages in thread
From: Nicholas Piggin @ 2021-12-25 10:12 UTC (permalink / raw)
To: Fabiano Rosas, kvm-ppc; +Cc: aik, linuxppc-dev
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
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] KVM: PPC: Fix mmio length message
2021-12-23 21:15 ` Fabiano Rosas
@ 2021-12-25 10:16 ` Nicholas Piggin
-1 siblings, 0 replies; 20+ messages in thread
From: Nicholas Piggin @ 2021-12-25 10:16 UTC (permalink / raw)
To: Fabiano Rosas, kvm-ppc; +Cc: aik, linuxppc-dev
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
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] KVM: PPC: Fix mmio length message
@ 2021-12-25 10:16 ` Nicholas Piggin
0 siblings, 0 replies; 20+ messages in thread
From: Nicholas Piggin @ 2021-12-25 10:16 UTC (permalink / raw)
To: Fabiano Rosas, kvm-ppc; +Cc: aik, linuxppc-dev
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
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] KVM: PPC: Fix vmx/vsx mixup in mmio emulation
2021-12-25 10:12 ` Nicholas Piggin
@ 2021-12-27 17:28 ` Fabiano Rosas
-1 siblings, 0 replies; 20+ messages in thread
From: Fabiano Rosas @ 2021-12-27 17:28 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: aik, linuxppc-dev
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)"
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] KVM: PPC: Fix vmx/vsx mixup in mmio emulation
@ 2021-12-27 17:28 ` Fabiano Rosas
0 siblings, 0 replies; 20+ messages in thread
From: Fabiano Rosas @ 2021-12-27 17:28 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: aik, linuxppc-dev
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)"
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] KVM: PPC: Fix mmio length message
2021-12-25 10:16 ` Nicholas Piggin
@ 2021-12-30 18:24 ` Fabiano Rosas
-1 siblings, 0 replies; 20+ messages in thread
From: Fabiano Rosas @ 2021-12-30 18:24 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: aik, linuxppc-dev
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] KVM: PPC: Fix mmio length message
@ 2021-12-30 18:24 ` Fabiano Rosas
0 siblings, 0 replies; 20+ messages in thread
From: Fabiano Rosas @ 2021-12-30 18:24 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: aik, linuxppc-dev
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] KVM: PPC: Fix vmx/vsx mixup in mmio emulation
2021-12-27 17:28 ` Fabiano Rosas
@ 2022-01-04 9:01 ` Alexey Kardashevskiy
-1 siblings, 0 replies; 20+ messages in thread
From: Alexey Kardashevskiy @ 2022-01-04 9:01 UTC (permalink / raw)
To: Fabiano Rosas, Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] KVM: PPC: Fix vmx/vsx mixup in mmio emulation
@ 2022-01-04 9:01 ` Alexey Kardashevskiy
0 siblings, 0 replies; 20+ messages in thread
From: Alexey Kardashevskiy @ 2022-01-04 9:01 UTC (permalink / raw)
To: Fabiano Rosas, Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev
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
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2022-01-04 9:02 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-23 21:15 [PATCH 0/3] KVM: PPC: Minor fixes Fabiano Rosas
2021-12-23 21:15 ` Fabiano Rosas
2021-12-23 21:15 ` [PATCH 1/3] KVM: PPC: Book3S HV: Stop returning internal values to userspace Fabiano Rosas
2021-12-23 21:15 ` Fabiano Rosas
2021-12-25 10:11 ` Nicholas Piggin
2021-12-25 10:11 ` Nicholas Piggin
2021-12-23 21:15 ` [PATCH 2/3] KVM: PPC: Fix vmx/vsx mixup in mmio emulation Fabiano Rosas
2021-12-23 21:15 ` Fabiano Rosas
2021-12-25 10:12 ` Nicholas Piggin
2021-12-25 10:12 ` Nicholas Piggin
2021-12-27 17:28 ` Fabiano Rosas
2021-12-27 17:28 ` Fabiano Rosas
2022-01-04 9:01 ` Alexey Kardashevskiy
2022-01-04 9:01 ` Alexey Kardashevskiy
2021-12-23 21:15 ` [PATCH 3/3] KVM: PPC: Fix mmio length message Fabiano Rosas
2021-12-23 21:15 ` Fabiano Rosas
2021-12-25 10:16 ` Nicholas Piggin
2021-12-25 10:16 ` Nicholas Piggin
2021-12-30 18:24 ` Fabiano Rosas
2021-12-30 18:24 ` Fabiano Rosas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.