* [PATCH v3 1/6] KVM: PPC: Book3S HV: Stop returning internal values to userspace
2022-01-07 21:00 [PATCH v3 0/6] KVM: PPC: MMIO fixes Fabiano Rosas
@ 2022-01-07 21:00 ` Fabiano Rosas
2022-01-07 21:00 ` [PATCH v3 2/6] KVM: PPC: Fix vmx/vsx mixup in mmio emulation Fabiano Rosas
` (4 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: Fabiano Rosas @ 2022-01-07 21:00 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>
Reviewed-by: Nicholas Piggin <npiggin@gmail.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] 17+ messages in thread* [PATCH v3 2/6] KVM: PPC: Fix vmx/vsx mixup in mmio emulation
2022-01-07 21:00 [PATCH v3 0/6] KVM: PPC: MMIO fixes Fabiano Rosas
2022-01-07 21:00 ` [PATCH v3 1/6] KVM: PPC: Book3S HV: Stop returning internal values to userspace Fabiano Rosas
@ 2022-01-07 21:00 ` Fabiano Rosas
2022-01-07 21:00 ` [PATCH v3 3/6] KVM: PPC: Don't use pr_emerg when mmio emulation fails Fabiano Rosas
` (3 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: Fabiano Rosas @ 2022-01-07 21:00 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>
Reviewed-by: Nicholas Piggin <npiggin@gmail.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 1e130bb087c4..92e552ab5a77 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) {
@@ -1604,7 +1604,7 @@ int kvmppc_handle_vmx_store(struct kvm_vcpu *vcpu,
unsigned int index = rs & KVM_MMIO_REG_MASK;
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;
vcpu->arch.io_gpr = rs;
--
2.33.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v3 3/6] KVM: PPC: Don't use pr_emerg when mmio emulation fails
2022-01-07 21:00 [PATCH v3 0/6] KVM: PPC: MMIO fixes Fabiano Rosas
2022-01-07 21:00 ` [PATCH v3 1/6] KVM: PPC: Book3S HV: Stop returning internal values to userspace Fabiano Rosas
2022-01-07 21:00 ` [PATCH v3 2/6] KVM: PPC: Fix vmx/vsx mixup in mmio emulation Fabiano Rosas
@ 2022-01-07 21:00 ` Fabiano Rosas
2022-01-10 5:22 ` Nicholas Piggin
2022-01-07 21:00 ` [PATCH v3 4/6] KVM: PPC: mmio: Queue interrupt at kvmppc_emulate_mmio Fabiano Rosas
` (2 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Fabiano Rosas @ 2022-01-07 21:00 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, npiggin, aik
If MMIO emulation fails we deliver a Program interrupt to the
guest. This is a normal event for the host, so use pr_info.
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 92e552ab5a77..4d7d0d080232 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -308,7 +308,7 @@ int kvmppc_emulate_mmio(struct kvm_vcpu *vcpu)
kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst);
/* XXX Deliver Program interrupt to guest. */
- pr_emerg("%s: emulation failed (%08x)\n", __func__, last_inst);
+ pr_info("%s: emulation failed (%08x)\n", __func__, last_inst);
r = RESUME_HOST;
break;
}
--
2.33.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v3 3/6] KVM: PPC: Don't use pr_emerg when mmio emulation fails
2022-01-07 21:00 ` [PATCH v3 3/6] KVM: PPC: Don't use pr_emerg when mmio emulation fails Fabiano Rosas
@ 2022-01-10 5:22 ` Nicholas Piggin
2022-01-11 14:39 ` Fabiano Rosas
0 siblings, 1 reply; 17+ messages in thread
From: Nicholas Piggin @ 2022-01-10 5:22 UTC (permalink / raw)
To: Fabiano Rosas, kvm-ppc; +Cc: aik, linuxppc-dev
Excerpts from Fabiano Rosas's message of January 8, 2022 7:00 am:
> If MMIO emulation fails we deliver a Program interrupt to the
> guest. This is a normal event for the host, so use pr_info.
>
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---
Should it be rate limited to prevent guest spamming host log? In the
case of informational messages it's always good if it gives the
administrator some idea of what to do with it. If it's normal
for the host does it even need a message? If yes, then identifying which
guest and adding something like "(this might becaused by a bug in guest
driver)" would set the poor admin's mind at rest.
Thanks,
Nick
> 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 92e552ab5a77..4d7d0d080232 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -308,7 +308,7 @@ int kvmppc_emulate_mmio(struct kvm_vcpu *vcpu)
>
> kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst);
> /* XXX Deliver Program interrupt to guest. */
> - pr_emerg("%s: emulation failed (%08x)\n", __func__, last_inst);
> + pr_info("%s: emulation failed (%08x)\n", __func__, last_inst);
> r = RESUME_HOST;
> break;
> }
> --
> 2.33.1
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3 3/6] KVM: PPC: Don't use pr_emerg when mmio emulation fails
2022-01-10 5:22 ` Nicholas Piggin
@ 2022-01-11 14:39 ` Fabiano Rosas
0 siblings, 0 replies; 17+ messages in thread
From: Fabiano Rosas @ 2022-01-11 14:39 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 January 8, 2022 7:00 am:
>> If MMIO emulation fails we deliver a Program interrupt to the
>> guest. This is a normal event for the host, so use pr_info.
>>
>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
>> ---
>
> Should it be rate limited to prevent guest spamming host log? In the
> case of informational messages it's always good if it gives the
> administrator some idea of what to do with it. If it's normal
> for the host does it even need a message? If yes, then identifying which
> guest and adding something like "(this might becaused by a bug in guest
> driver)" would set the poor admin's mind at rest.
I'll drop this message then and improve the other one that is emitted
earlier at the emulation code.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 4/6] KVM: PPC: mmio: Queue interrupt at kvmppc_emulate_mmio
2022-01-07 21:00 [PATCH v3 0/6] KVM: PPC: MMIO fixes Fabiano Rosas
` (2 preceding siblings ...)
2022-01-07 21:00 ` [PATCH v3 3/6] KVM: PPC: Don't use pr_emerg when mmio emulation fails Fabiano Rosas
@ 2022-01-07 21:00 ` Fabiano Rosas
2022-01-10 3:20 ` Alexey Kardashevskiy
2022-01-10 5:29 ` Nicholas Piggin
2022-01-07 21:00 ` [PATCH v3 5/6] KVM: PPC: mmio: Return to guest after emulation failure Fabiano Rosas
2022-01-07 21:00 ` [PATCH v3 6/6] KVM: PPC: mmio: Reject instructions that access more than mmio.data size Fabiano Rosas
5 siblings, 2 replies; 17+ messages in thread
From: Fabiano Rosas @ 2022-01-07 21:00 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, npiggin, aik
If MMIO emulation fails, we queue a Program interrupt to the
guest. Move that line up into kvmppc_emulate_mmio, which is where we
set RESUME_GUEST/HOST. This allows the removal of the 'advance'
variable.
No functional change, just separation of responsibilities.
Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
arch/powerpc/kvm/emulate_loadstore.c | 8 +-------
arch/powerpc/kvm/powerpc.c | 2 +-
2 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/kvm/emulate_loadstore.c b/arch/powerpc/kvm/emulate_loadstore.c
index 48272a9b9c30..4dec920fe4c9 100644
--- a/arch/powerpc/kvm/emulate_loadstore.c
+++ b/arch/powerpc/kvm/emulate_loadstore.c
@@ -73,7 +73,6 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
{
u32 inst;
enum emulation_result emulated = EMULATE_FAIL;
- int advance = 1;
struct instruction_op op;
/* this default type might be overwritten by subcategories */
@@ -355,15 +354,10 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
}
}
- if (emulated == EMULATE_FAIL) {
- advance = 0;
- kvmppc_core_queue_program(vcpu, 0);
- }
-
trace_kvm_ppc_instr(inst, kvmppc_get_pc(vcpu), emulated);
/* Advance past emulated instruction. */
- if (advance)
+ if (emulated != EMULATE_FAIL)
kvmppc_set_pc(vcpu, kvmppc_get_pc(vcpu) + 4);
return emulated;
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 4d7d0d080232..6daeea4a7de1 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -307,7 +307,7 @@ int kvmppc_emulate_mmio(struct kvm_vcpu *vcpu)
u32 last_inst;
kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst);
- /* XXX Deliver Program interrupt to guest. */
+ kvmppc_core_queue_program(vcpu, 0);
pr_info("%s: emulation failed (%08x)\n", __func__, last_inst);
r = RESUME_HOST;
break;
--
2.33.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v3 4/6] KVM: PPC: mmio: Queue interrupt at kvmppc_emulate_mmio
2022-01-07 21:00 ` [PATCH v3 4/6] KVM: PPC: mmio: Queue interrupt at kvmppc_emulate_mmio Fabiano Rosas
@ 2022-01-10 3:20 ` Alexey Kardashevskiy
2022-01-10 5:29 ` Nicholas Piggin
1 sibling, 0 replies; 17+ messages in thread
From: Alexey Kardashevskiy @ 2022-01-10 3:20 UTC (permalink / raw)
To: Fabiano Rosas, kvm-ppc; +Cc: linuxppc-dev, npiggin
On 08/01/2022 08:00, Fabiano Rosas wrote:
> If MMIO emulation fails, we queue a Program interrupt to the
> guest. Move that line up into kvmppc_emulate_mmio, which is where we
> set RESUME_GUEST/HOST. This allows the removal of the 'advance'
> variable.
>
> No functional change, just separation of responsibilities.
>
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> arch/powerpc/kvm/emulate_loadstore.c | 8 +-------
> arch/powerpc/kvm/powerpc.c | 2 +-
> 2 files changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/arch/powerpc/kvm/emulate_loadstore.c b/arch/powerpc/kvm/emulate_loadstore.c
> index 48272a9b9c30..4dec920fe4c9 100644
> --- a/arch/powerpc/kvm/emulate_loadstore.c
> +++ b/arch/powerpc/kvm/emulate_loadstore.c
> @@ -73,7 +73,6 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
> {
> u32 inst;
> enum emulation_result emulated = EMULATE_FAIL;
> - int advance = 1;
> struct instruction_op op;
>
> /* this default type might be overwritten by subcategories */
> @@ -355,15 +354,10 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
> }
> }
>
> - if (emulated == EMULATE_FAIL) {
> - advance = 0;
> - kvmppc_core_queue_program(vcpu, 0);
> - }
> -
> trace_kvm_ppc_instr(inst, kvmppc_get_pc(vcpu), emulated);
>
> /* Advance past emulated instruction. */
> - if (advance)
> + if (emulated != EMULATE_FAIL)
> kvmppc_set_pc(vcpu, kvmppc_get_pc(vcpu) + 4);
>
> return emulated;
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 4d7d0d080232..6daeea4a7de1 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -307,7 +307,7 @@ int kvmppc_emulate_mmio(struct kvm_vcpu *vcpu)
> u32 last_inst;
>
> kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst);
> - /* XXX Deliver Program interrupt to guest. */
> + kvmppc_core_queue_program(vcpu, 0);
> pr_info("%s: emulation failed (%08x)\n", __func__, last_inst);
> r = RESUME_HOST;
> break;
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3 4/6] KVM: PPC: mmio: Queue interrupt at kvmppc_emulate_mmio
2022-01-07 21:00 ` [PATCH v3 4/6] KVM: PPC: mmio: Queue interrupt at kvmppc_emulate_mmio Fabiano Rosas
2022-01-10 3:20 ` Alexey Kardashevskiy
@ 2022-01-10 5:29 ` Nicholas Piggin
1 sibling, 0 replies; 17+ messages in thread
From: Nicholas Piggin @ 2022-01-10 5:29 UTC (permalink / raw)
To: Fabiano Rosas, kvm-ppc; +Cc: aik, linuxppc-dev
Excerpts from Fabiano Rosas's message of January 8, 2022 7:00 am:
> If MMIO emulation fails, we queue a Program interrupt to the
> guest. Move that line up into kvmppc_emulate_mmio, which is where we
> set RESUME_GUEST/HOST. This allows the removal of the 'advance'
> variable.
>
> No functional change, just separation of responsibilities.
Looks cleaner.
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---
> arch/powerpc/kvm/emulate_loadstore.c | 8 +-------
> arch/powerpc/kvm/powerpc.c | 2 +-
> 2 files changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/arch/powerpc/kvm/emulate_loadstore.c b/arch/powerpc/kvm/emulate_loadstore.c
> index 48272a9b9c30..4dec920fe4c9 100644
> --- a/arch/powerpc/kvm/emulate_loadstore.c
> +++ b/arch/powerpc/kvm/emulate_loadstore.c
> @@ -73,7 +73,6 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
> {
> u32 inst;
> enum emulation_result emulated = EMULATE_FAIL;
> - int advance = 1;
> struct instruction_op op;
>
> /* this default type might be overwritten by subcategories */
> @@ -355,15 +354,10 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
> }
> }
>
> - if (emulated == EMULATE_FAIL) {
> - advance = 0;
> - kvmppc_core_queue_program(vcpu, 0);
> - }
> -
> trace_kvm_ppc_instr(inst, kvmppc_get_pc(vcpu), emulated);
>
> /* Advance past emulated instruction. */
> - if (advance)
> + if (emulated != EMULATE_FAIL)
> kvmppc_set_pc(vcpu, kvmppc_get_pc(vcpu) + 4);
>
> return emulated;
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 4d7d0d080232..6daeea4a7de1 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -307,7 +307,7 @@ int kvmppc_emulate_mmio(struct kvm_vcpu *vcpu)
> u32 last_inst;
>
> kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst);
> - /* XXX Deliver Program interrupt to guest. */
> + kvmppc_core_queue_program(vcpu, 0);
> pr_info("%s: emulation failed (%08x)\n", __func__, last_inst);
> r = RESUME_HOST;
> break;
> --
> 2.33.1
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 5/6] KVM: PPC: mmio: Return to guest after emulation failure
2022-01-07 21:00 [PATCH v3 0/6] KVM: PPC: MMIO fixes Fabiano Rosas
` (3 preceding siblings ...)
2022-01-07 21:00 ` [PATCH v3 4/6] KVM: PPC: mmio: Queue interrupt at kvmppc_emulate_mmio Fabiano Rosas
@ 2022-01-07 21:00 ` Fabiano Rosas
2022-01-10 7:36 ` Nicholas Piggin
2022-01-07 21:00 ` [PATCH v3 6/6] KVM: PPC: mmio: Reject instructions that access more than mmio.data size Fabiano Rosas
5 siblings, 1 reply; 17+ messages in thread
From: Fabiano Rosas @ 2022-01-07 21:00 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, npiggin, aik
If MMIO emulation fails we don't want to crash the whole guest by
returning to userspace.
The original commit bbf45ba57eae ("KVM: ppc: PowerPC 440 KVM
implementation") added a todo:
/* XXX Deliver Program interrupt to guest. */
and later the commit d69614a295ae ("KVM: PPC: Separate loadstore
emulation from priv emulation") added the Program interrupt injection
but in another file, so I'm assuming it was missed that this block
needed to be altered.
Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
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 6daeea4a7de1..56b0faab7a5f 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -309,7 +309,7 @@ int kvmppc_emulate_mmio(struct kvm_vcpu *vcpu)
kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst);
kvmppc_core_queue_program(vcpu, 0);
pr_info("%s: emulation failed (%08x)\n", __func__, last_inst);
- r = RESUME_HOST;
+ r = RESUME_GUEST;
break;
}
default:
--
2.33.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v3 5/6] KVM: PPC: mmio: Return to guest after emulation failure
2022-01-07 21:00 ` [PATCH v3 5/6] KVM: PPC: mmio: Return to guest after emulation failure Fabiano Rosas
@ 2022-01-10 7:36 ` Nicholas Piggin
2022-01-10 23:51 ` Alexey Kardashevskiy
0 siblings, 1 reply; 17+ messages in thread
From: Nicholas Piggin @ 2022-01-10 7:36 UTC (permalink / raw)
To: Fabiano Rosas, kvm-ppc; +Cc: aik, linuxppc-dev
Excerpts from Fabiano Rosas's message of January 8, 2022 7:00 am:
> If MMIO emulation fails we don't want to crash the whole guest by
> returning to userspace.
>
> The original commit bbf45ba57eae ("KVM: ppc: PowerPC 440 KVM
> implementation") added a todo:
>
> /* XXX Deliver Program interrupt to guest. */
>
> and later the commit d69614a295ae ("KVM: PPC: Separate loadstore
> emulation from priv emulation") added the Program interrupt injection
> but in another file, so I'm assuming it was missed that this block
> needed to be altered.
>
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> 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 6daeea4a7de1..56b0faab7a5f 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -309,7 +309,7 @@ int kvmppc_emulate_mmio(struct kvm_vcpu *vcpu)
> kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst);
> kvmppc_core_queue_program(vcpu, 0);
> pr_info("%s: emulation failed (%08x)\n", __func__, last_inst);
> - r = RESUME_HOST;
> + r = RESUME_GUEST;
So at this point can the pr_info just go away?
I wonder if this shouldn't be a DSI rather than a program check.
DSI with DSISR[37] looks a bit more expected. Not that Linux
probably does much with it but at least it would give a SIGBUS
rather than SIGILL.
Thanks,
Nick
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3 5/6] KVM: PPC: mmio: Return to guest after emulation failure
2022-01-10 7:36 ` Nicholas Piggin
@ 2022-01-10 23:51 ` Alexey Kardashevskiy
2022-01-11 3:23 ` Nicholas Piggin
0 siblings, 1 reply; 17+ messages in thread
From: Alexey Kardashevskiy @ 2022-01-10 23:51 UTC (permalink / raw)
To: Nicholas Piggin, Fabiano Rosas, kvm-ppc; +Cc: linuxppc-dev
On 1/10/22 18:36, Nicholas Piggin wrote:
> Excerpts from Fabiano Rosas's message of January 8, 2022 7:00 am:
>> If MMIO emulation fails we don't want to crash the whole guest by
>> returning to userspace.
>>
>> The original commit bbf45ba57eae ("KVM: ppc: PowerPC 440 KVM
>> implementation") added a todo:
>>
>> /* XXX Deliver Program interrupt to guest. */
>>
>> and later the commit d69614a295ae ("KVM: PPC: Separate loadstore
>> emulation from priv emulation") added the Program interrupt injection
>> but in another file, so I'm assuming it was missed that this block
>> needed to be altered.
>>
>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
>> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> 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 6daeea4a7de1..56b0faab7a5f 100644
>> --- a/arch/powerpc/kvm/powerpc.c
>> +++ b/arch/powerpc/kvm/powerpc.c
>> @@ -309,7 +309,7 @@ int kvmppc_emulate_mmio(struct kvm_vcpu *vcpu)
>> kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst);
>> kvmppc_core_queue_program(vcpu, 0);
>> pr_info("%s: emulation failed (%08x)\n", __func__, last_inst);
>> - r = RESUME_HOST;
>> + r = RESUME_GUEST;
>
> So at this point can the pr_info just go away?
>
> I wonder if this shouldn't be a DSI rather than a program check.
> DSI with DSISR[37] looks a bit more expected. Not that Linux
> probably does much with it but at least it would give a SIGBUS
> rather than SIGILL.
It does not like it is more expected to me, it is not about wrong memory
attributes, it is the instruction itself which cannot execute.
DSISR[37]:
Set to 1 if the access is due to a lq, stq, lwat, ldat, lbarx, lharx,
lwarx, ldarx, lqarx, stwat,
stdat, stbcx., sthcx., stwcx., stdcx., or stqcx. instruction that
addresses storage that is Write
Through Required or Caching Inhibited; or if the access is due to a copy
or paste. instruction
that addresses storage that is Caching Inhibited; or if the access is
due to a lwat, ldat, stwat, or
stdat instruction that addresses storage that is Guarded; otherwise set
to 0.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3 5/6] KVM: PPC: mmio: Return to guest after emulation failure
2022-01-10 23:51 ` Alexey Kardashevskiy
@ 2022-01-11 3:23 ` Nicholas Piggin
2022-01-11 14:39 ` Fabiano Rosas
0 siblings, 1 reply; 17+ messages in thread
From: Nicholas Piggin @ 2022-01-11 3:23 UTC (permalink / raw)
To: Alexey Kardashevskiy, Fabiano Rosas, kvm-ppc; +Cc: linuxppc-dev
Excerpts from Alexey Kardashevskiy's message of January 11, 2022 9:51 am:
>
>
> On 1/10/22 18:36, Nicholas Piggin wrote:
>> Excerpts from Fabiano Rosas's message of January 8, 2022 7:00 am:
>>> If MMIO emulation fails we don't want to crash the whole guest by
>>> returning to userspace.
>>>
>>> The original commit bbf45ba57eae ("KVM: ppc: PowerPC 440 KVM
>>> implementation") added a todo:
>>>
>>> /* XXX Deliver Program interrupt to guest. */
>>>
>>> and later the commit d69614a295ae ("KVM: PPC: Separate loadstore
>>> emulation from priv emulation") added the Program interrupt injection
>>> but in another file, so I'm assuming it was missed that this block
>>> needed to be altered.
>>>
>>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
>>> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>> 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 6daeea4a7de1..56b0faab7a5f 100644
>>> --- a/arch/powerpc/kvm/powerpc.c
>>> +++ b/arch/powerpc/kvm/powerpc.c
>>> @@ -309,7 +309,7 @@ int kvmppc_emulate_mmio(struct kvm_vcpu *vcpu)
>>> kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst);
>>> kvmppc_core_queue_program(vcpu, 0);
>>> pr_info("%s: emulation failed (%08x)\n", __func__, last_inst);
>>> - r = RESUME_HOST;
>>> + r = RESUME_GUEST;
>>
>> So at this point can the pr_info just go away?
>>
>> I wonder if this shouldn't be a DSI rather than a program check.
>> DSI with DSISR[37] looks a bit more expected. Not that Linux
>> probably does much with it but at least it would give a SIGBUS
>> rather than SIGILL.
>
> It does not like it is more expected to me, it is not about wrong memory
> attributes, it is the instruction itself which cannot execute.
It's not an illegal instruction though, it can't execute because of the
nature of the data / address it is operating on. That says d-side to me.
DSISR[37] isn't perfect but if you squint it's not terrible. It's about
certain instructions that have restrictions operating on other than
normal cacheable mappings.
Thanks,
Nick
>
> DSISR[37]:
> Set to 1 if the access is due to a lq, stq, lwat, ldat, lbarx, lharx,
> lwarx, ldarx, lqarx, stwat,
> stdat, stbcx., sthcx., stwcx., stdcx., or stqcx. instruction that
> addresses storage that is Write
> Through Required or Caching Inhibited; or if the access is due to a copy
> or paste. instruction
> that addresses storage that is Caching Inhibited; or if the access is
> due to a lwat, ldat, stwat, or
> stdat instruction that addresses storage that is Guarded; otherwise set
> to 0.
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3 5/6] KVM: PPC: mmio: Return to guest after emulation failure
2022-01-11 3:23 ` Nicholas Piggin
@ 2022-01-11 14:39 ` Fabiano Rosas
0 siblings, 0 replies; 17+ messages in thread
From: Fabiano Rosas @ 2022-01-11 14:39 UTC (permalink / raw)
To: Nicholas Piggin, Alexey Kardashevskiy, kvm-ppc; +Cc: linuxppc-dev
Nicholas Piggin <npiggin@gmail.com> writes:
> Excerpts from Alexey Kardashevskiy's message of January 11, 2022 9:51 am:
>>
>>
>> On 1/10/22 18:36, Nicholas Piggin wrote:
>>> Excerpts from Fabiano Rosas's message of January 8, 2022 7:00 am:
>>>> If MMIO emulation fails we don't want to crash the whole guest by
>>>> returning to userspace.
>>>>
>>>> The original commit bbf45ba57eae ("KVM: ppc: PowerPC 440 KVM
>>>> implementation") added a todo:
>>>>
>>>> /* XXX Deliver Program interrupt to guest. */
>>>>
>>>> and later the commit d69614a295ae ("KVM: PPC: Separate loadstore
>>>> emulation from priv emulation") added the Program interrupt injection
>>>> but in another file, so I'm assuming it was missed that this block
>>>> needed to be altered.
>>>>
>>>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
>>>> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>> 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 6daeea4a7de1..56b0faab7a5f 100644
>>>> --- a/arch/powerpc/kvm/powerpc.c
>>>> +++ b/arch/powerpc/kvm/powerpc.c
>>>> @@ -309,7 +309,7 @@ int kvmppc_emulate_mmio(struct kvm_vcpu *vcpu)
>>>> kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst);
>>>> kvmppc_core_queue_program(vcpu, 0);
>>>> pr_info("%s: emulation failed (%08x)\n", __func__, last_inst);
>>>> - r = RESUME_HOST;
>>>> + r = RESUME_GUEST;
>>>
>>> So at this point can the pr_info just go away?
>>>
>>> I wonder if this shouldn't be a DSI rather than a program check.
>>> DSI with DSISR[37] looks a bit more expected. Not that Linux
>>> probably does much with it but at least it would give a SIGBUS
>>> rather than SIGILL.
>>
>> It does not like it is more expected to me, it is not about wrong memory
>> attributes, it is the instruction itself which cannot execute.
>
> It's not an illegal instruction though, it can't execute because of the
> nature of the data / address it is operating on. That says d-side to me.
>
> DSISR[37] isn't perfect but if you squint it's not terrible. It's about
> certain instructions that have restrictions operating on other than
> normal cacheable mappings.
I think I agree with Nick on this one. At least the DSISR gives _some_
information while the Program is maybe too generic. I would probably be
staring at the opcode wondering what is wrong with it.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 6/6] KVM: PPC: mmio: Reject instructions that access more than mmio.data size
2022-01-07 21:00 [PATCH v3 0/6] KVM: PPC: MMIO fixes Fabiano Rosas
` (4 preceding siblings ...)
2022-01-07 21:00 ` [PATCH v3 5/6] KVM: PPC: mmio: Return to guest after emulation failure Fabiano Rosas
@ 2022-01-07 21:00 ` Fabiano Rosas
2022-01-10 7:38 ` Nicholas Piggin
5 siblings, 1 reply; 17+ messages in thread
From: Fabiano Rosas @ 2022-01-07 21:00 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, npiggin, aik
The MMIO interface between the kernel and userspace uses a structure
that supports a maximum of 8-bytes of data. Instructions that access
more than that need to be emulated in parts.
We currently don't have generic support for splitting the emulation in
parts and each set of instructions needs to be explicitly included.
There's already an error message being printed when a load or store
exceeds the mmio.data buffer but we don't fail the emulation until
later at kvmppc_complete_mmio_load and even then we allow userspace to
make a partial copy of the data, which ends up overwriting some fields
of the mmio structure.
This patch makes the emulation fail earlier at kvmppc_handle_load|store,
which will send a Program interrupt to the guest. This is better than
allowing the guest to proceed with partial data.
Note that this was caught in a somewhat artificial scenario using
quadword instructions (lq/stq), there's no account of an actual guest
in the wild running instructions that are not properly emulated.
(While here, fix the error message to check against 'bytes' and not
'run->mmio.len' which at this point has an old value.)
Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
arch/powerpc/kvm/powerpc.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 56b0faab7a5f..a1643ca988e0 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -1246,7 +1246,8 @@ 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);
+ return EMULATE_FAIL;
}
run->mmio.phys_addr = vcpu->arch.paddr_accessed;
@@ -1335,7 +1336,8 @@ 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);
+ return EMULATE_FAIL;
}
run->mmio.phys_addr = vcpu->arch.paddr_accessed;
--
2.33.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v3 6/6] KVM: PPC: mmio: Reject instructions that access more than mmio.data size
2022-01-07 21:00 ` [PATCH v3 6/6] KVM: PPC: mmio: Reject instructions that access more than mmio.data size Fabiano Rosas
@ 2022-01-10 7:38 ` Nicholas Piggin
2022-01-11 14:32 ` Fabiano Rosas
0 siblings, 1 reply; 17+ messages in thread
From: Nicholas Piggin @ 2022-01-10 7:38 UTC (permalink / raw)
To: Fabiano Rosas, kvm-ppc; +Cc: aik, linuxppc-dev
Excerpts from Fabiano Rosas's message of January 8, 2022 7:00 am:
> The MMIO interface between the kernel and userspace uses a structure
> that supports a maximum of 8-bytes of data. Instructions that access
> more than that need to be emulated in parts.
>
> We currently don't have generic support for splitting the emulation in
> parts and each set of instructions needs to be explicitly included.
>
> There's already an error message being printed when a load or store
> exceeds the mmio.data buffer but we don't fail the emulation until
> later at kvmppc_complete_mmio_load and even then we allow userspace to
> make a partial copy of the data, which ends up overwriting some fields
> of the mmio structure.
>
> This patch makes the emulation fail earlier at kvmppc_handle_load|store,
> which will send a Program interrupt to the guest. This is better than
> allowing the guest to proceed with partial data.
>
> Note that this was caught in a somewhat artificial scenario using
> quadword instructions (lq/stq), there's no account of an actual guest
> in the wild running instructions that are not properly emulated.
>
> (While here, fix the error message to check against 'bytes' and not
> 'run->mmio.len' which at this point has an old value.)
This looks good to me
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> arch/powerpc/kvm/powerpc.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 56b0faab7a5f..a1643ca988e0 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -1246,7 +1246,8 @@ 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);
I wonder though this should probably be ratelimited, informational (or
at least warning because it's a host message), and perhaps a bit more
explanatory that it's a guest problem (or at least lack of host support
for particular guest MMIO sizes).
Thanks,
Nick
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3 6/6] KVM: PPC: mmio: Reject instructions that access more than mmio.data size
2022-01-10 7:38 ` Nicholas Piggin
@ 2022-01-11 14:32 ` Fabiano Rosas
0 siblings, 0 replies; 17+ messages in thread
From: Fabiano Rosas @ 2022-01-11 14:32 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 January 8, 2022 7:00 am:
>> The MMIO interface between the kernel and userspace uses a structure
>> that supports a maximum of 8-bytes of data. Instructions that access
>> more than that need to be emulated in parts.
>>
>> We currently don't have generic support for splitting the emulation in
>> parts and each set of instructions needs to be explicitly included.
>>
>> There's already an error message being printed when a load or store
>> exceeds the mmio.data buffer but we don't fail the emulation until
>> later at kvmppc_complete_mmio_load and even then we allow userspace to
>> make a partial copy of the data, which ends up overwriting some fields
>> of the mmio structure.
>>
>> This patch makes the emulation fail earlier at kvmppc_handle_load|store,
>> which will send a Program interrupt to the guest. This is better than
>> allowing the guest to proceed with partial data.
>>
>> Note that this was caught in a somewhat artificial scenario using
>> quadword instructions (lq/stq), there's no account of an actual guest
>> in the wild running instructions that are not properly emulated.
>>
>> (While here, fix the error message to check against 'bytes' and not
>> 'run->mmio.len' which at this point has an old value.)
>
> This looks good to me
>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
>>
>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
>> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> arch/powerpc/kvm/powerpc.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
>> index 56b0faab7a5f..a1643ca988e0 100644
>> --- a/arch/powerpc/kvm/powerpc.c
>> +++ b/arch/powerpc/kvm/powerpc.c
>> @@ -1246,7 +1246,8 @@ 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);
>
> I wonder though this should probably be ratelimited, informational (or
> at least warning because it's a host message), and perhaps a bit more
> explanatory that it's a guest problem (or at least lack of host support
> for particular guest MMIO sizes).
Yes, I'll ratelimit it an try to make it clear that this is something
that happened inside the guest but it lacks support in KVM. Then
hopefully people will tell to us if they ever need that support.
^ permalink raw reply [flat|nested] 17+ messages in thread