Re: Re: [PATCH v2] KVM: PPC: Book3S HV: Fix L2 guest reboot failure due to empty 'arch_compat'
Hi Vaibhav,
Thanks for looking into the patch.
On 2024/02/05 11:05 PM, Vaibhav Jain wrote:
> Hi Amit,
>
> Thanks for the patch. Minor comment on the patch below:
>
> Amit Machhiwal writes:
>
>
>
> >
> > +static inline unsigned long map_pcr_to_cap(unsigned long pcr)
> > +{
> > + unsigned long cap = 0;
> > +
> > + switch (pcr) {
> > + case PCR_ARCH_300:
> > + cap = H_GUEST_CAP_POWER9;
> > + break;
> > + case PCR_ARCH_31:
> > + cap = H_GUEST_CAP_POWER10;
> Though CONFIG_CC_IMPLICIT_FALLTHROUGH and '-Wimplicit-fallthrough'
> doesnt explicitly flag this usage, please consider using the
> 'fallthrough;' keyword here.
>
> However you probably dont want this switch-case to fallthrough so please
> use a 'break' instead.
Sure, v3 on the way.
>
> > + default:
> > + break;
> > + }
> > +
> > + return cap;
> > +}
> > +
> >
>
>
> With the suggested change above
>
> Reviewed-by: Vaibhav Jain
Thanks!
>
> --
> Cheers
> ~ Vaibhav
~Amit
Re: [PATCH v2] KVM: PPC: Book3S HV: Fix L2 guest reboot failure due to empty 'arch_compat'
Hi Amit,
Thanks for the patch. Minor comment on the patch below:
Amit Machhiwal writes:
>
> +static inline unsigned long map_pcr_to_cap(unsigned long pcr)
> +{
> + unsigned long cap = 0;
> +
> + switch (pcr) {
> + case PCR_ARCH_300:
> + cap = H_GUEST_CAP_POWER9;
> + break;
> + case PCR_ARCH_31:
> + cap = H_GUEST_CAP_POWER10;
Though CONFIG_CC_IMPLICIT_FALLTHROUGH and '-Wimplicit-fallthrough'
doesnt explicitly flag this usage, please consider using the
'fallthrough;' keyword here.
However you probably dont want this switch-case to fallthrough so please
use a 'break' instead.
> + default:
> + break;
> + }
> +
> + return cap;
> +}
> +
>
With the suggested change above
Reviewed-by: Vaibhav Jain
--
Cheers
~ Vaibhav
Re: [PATCH v2] KVM: PPC: Book3S HV: Fix L2 guest reboot failure due to empty 'arch_compat'
Amit Machhiwal writes:
> Currently, rebooting a pseries nested qemu-kvm guest (L2) results in
> below error as L1 qemu sends PVR value 'arch_compat' == 0 via
> ppc_set_compat ioctl. This triggers a condition failure in
> kvmppc_set_arch_compat() resulting in an EINVAL.
>
> qemu-system-ppc64: Unable to set CPU compatibility mode in KVM: Invalid
> argument
>
> Also, a value of 0 for arch_compat generally refers the default
> compatibility of the host. But, arch_compat, being a Guest Wide Element
> in nested API v2, cannot be set to 0 in GSB as PowerVM (L0) expects a
> non-zero value. A value of 0 triggers a kernel trap during a reboot and
> consequently causes it to fail:
>
> [ 22.106360] reboot: Restarting system
> KVM: unknown exit, hardware reason ffea
> NIP 0100 LR fe44 CTR XER
> 20040092 CPU#0
> MSR 1000 HID0 HF 6c00 iidx 3 didx 3
> TB DECR 0
> GPR00 c2a8c300 7fe0
> GPR04 1002 82803033
> GPR08 0a00 0004 2fff
> GPR12 c2e1 000105639200 0004
> GPR16 00010563a090
> GPR20 000105639e20 0001056399c8 7fffe54abab0 000105639288
> GPR24 0001 0001
> GPR28 c2b30840
> CR [ - - - - - - - - ] RES 000@
> SRR0 SRR1 PVR 00800200 VRSAVE
>
> SPRG0 SPRG1 SPRG2 SPRG3
>
> SPRG4 SPRG5 SPRG6 SPRG7
>
> HSRR0 HSRR1
> CFAR
> LPCR 00020400
> PTCR DAR DSISR
>
> kernel:trap=0xffea | pc=0x100 | msr=0x1000
>
> This patch updates kvmppc_set_arch_compat() to use the host PVR value if
> 'compat_pvr' == 0 indicating that qemu doesn't want to enforce any
> specific PVR compat mode.
>
Reviewed-by: Aneesh Kumar K.V (IBM)
>
> Fixes: 19d31c5f1157 ("KVM: PPC: Add support for nestedv2 guests")
> Signed-off-by: Amit Machhiwal
> ---
>
> Changes v1 -> v2:
> - Added descriptive error log in the patch description when
> `arch_compat == 0` passed in GSB
> - Added a helper function for PCR to capabilities mapping
> - Added relevant comments around the changes being made
>
> v1:
> https://lore.kernel.org/lkml/20240118095653.2588129-1-amach...@linux.ibm.com/
>
> arch/powerpc/kvm/book3s_hv.c | 25 +++--
> arch/powerpc/kvm/book3s_hv_nestedv2.c | 23 +--
> 2 files changed, 44 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 52427fc2a33f..270ab9cf9a54 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -391,6 +391,23 @@ static void kvmppc_set_pvr_hv(struct kvm_vcpu *vcpu, u32
> pvr)
> /* Dummy value used in computing PCR value below */
> #define PCR_ARCH_31(PCR_ARCH_300 << 1)
>
> +static inline unsigned long map_pcr_to_cap(unsigned long pcr)
> +{
> + unsigned long cap = 0;
> +
> + switch (pcr) {
> + case PCR_ARCH_300:
> + cap = H_GUEST_CAP_POWER9;
> + break;
> + case PCR_ARCH_31:
> + cap = H_GUEST_CAP_POWER10;
> + default:
> + break;
> + }
> +
> + return cap;
> +}
> +
> static int kvmppc_set_arch_compat(struct kvm_vcpu *vcpu, u32 arch_compat)
> {
> unsigned long host_pcr_bit = 0, guest_pcr_bit = 0, cap = 0;
> @@ -424,11 +441,9 @@ static int kvmppc_set_arch_compat(struct kvm_vcpu *vcpu,
> u32 arch_compat)
> break;
> case PVR_ARCH_300:
> guest_pcr_bit = PCR_ARCH_300;
> - cap = H_GUEST_CAP_POWER9;
> break;
> case PVR_ARCH_31:
> guest_pcr_bit = PCR_ARCH_31;
> - cap = H_GUEST_CAP_POWER10;
> break;
> default:
> return -EINVAL;
> @@ -440,6 +455,12 @@ static int kvmppc_set_arch_compat(struct kvm_vcpu *vcpu,
> u32 arch_compat)
> return -EINVAL;
>
> if (kvmhv_on_pseries() && kvmhv_is_nestedv2()) {
> + /*
> + * 'arch_compat == 0' would mean the guest should default to
> + * L1's compatibility. In this case, the guest would pick
> + * host's PCR and evaluate the corresponding capabilities.
> + */
> + cap = map_pcr_to_cap(guest_pcr_bit);
>
[PATCH v2] KVM: PPC: Book3S HV: Fix L2 guest reboot failure due to empty 'arch_compat'
Currently, rebooting a pseries nested qemu-kvm guest (L2) results in
below error as L1 qemu sends PVR value 'arch_compat' == 0 via
ppc_set_compat ioctl. This triggers a condition failure in
kvmppc_set_arch_compat() resulting in an EINVAL.
qemu-system-ppc64: Unable to set CPU compatibility mode in KVM: Invalid
argument
Also, a value of 0 for arch_compat generally refers the default
compatibility of the host. But, arch_compat, being a Guest Wide Element
in nested API v2, cannot be set to 0 in GSB as PowerVM (L0) expects a
non-zero value. A value of 0 triggers a kernel trap during a reboot and
consequently causes it to fail:
[ 22.106360] reboot: Restarting system
KVM: unknown exit, hardware reason ffea
NIP 0100 LR fe44 CTR XER
20040092 CPU#0
MSR 1000 HID0 HF 6c00 iidx 3 didx 3
TB DECR 0
GPR00 c2a8c300 7fe0
GPR04 1002 82803033
GPR08 0a00 0004 2fff
GPR12 c2e1 000105639200 0004
GPR16 00010563a090
GPR20 000105639e20 0001056399c8 7fffe54abab0 000105639288
GPR24 0001 0001
GPR28 c2b30840
CR [ - - - - - - - - ] RES 000@
SRR0 SRR1 PVR 00800200 VRSAVE
SPRG0 SPRG1 SPRG2 SPRG3
SPRG4 SPRG5 SPRG6 SPRG7
HSRR0 HSRR1
CFAR
LPCR 00020400
PTCR DAR DSISR
kernel:trap=0xffea | pc=0x100 | msr=0x1000
This patch updates kvmppc_set_arch_compat() to use the host PVR value if
'compat_pvr' == 0 indicating that qemu doesn't want to enforce any
specific PVR compat mode.
Fixes: 19d31c5f1157 ("KVM: PPC: Add support for nestedv2 guests")
Signed-off-by: Amit Machhiwal
---
Changes v1 -> v2:
- Added descriptive error log in the patch description when
`arch_compat == 0` passed in GSB
- Added a helper function for PCR to capabilities mapping
- Added relevant comments around the changes being made
v1:
https://lore.kernel.org/lkml/20240118095653.2588129-1-amach...@linux.ibm.com/
arch/powerpc/kvm/book3s_hv.c | 25 +++--
arch/powerpc/kvm/book3s_hv_nestedv2.c | 23 +--
2 files changed, 44 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 52427fc2a33f..270ab9cf9a54 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -391,6 +391,23 @@ static void kvmppc_set_pvr_hv(struct kvm_vcpu *vcpu, u32
pvr)
/* Dummy value used in computing PCR value below */
#define PCR_ARCH_31(PCR_ARCH_300 << 1)
+static inline unsigned long map_pcr_to_cap(unsigned long pcr)
+{
+ unsigned long cap = 0;
+
+ switch (pcr) {
+ case PCR_ARCH_300:
+ cap = H_GUEST_CAP_POWER9;
+ break;
+ case PCR_ARCH_31:
+ cap = H_GUEST_CAP_POWER10;
+ default:
+ break;
+ }
+
+ return cap;
+}
+
static int kvmppc_set_arch_compat(struct kvm_vcpu *vcpu, u32 arch_compat)
{
unsigned long host_pcr_bit = 0, guest_pcr_bit = 0, cap = 0;
@@ -424,11 +441,9 @@ static int kvmppc_set_arch_compat(struct kvm_vcpu *vcpu,
u32 arch_compat)
break;
case PVR_ARCH_300:
guest_pcr_bit = PCR_ARCH_300;
- cap = H_GUEST_CAP_POWER9;
break;
case PVR_ARCH_31:
guest_pcr_bit = PCR_ARCH_31;
- cap = H_GUEST_CAP_POWER10;
break;
default:
return -EINVAL;
@@ -440,6 +455,12 @@ static int kvmppc_set_arch_compat(struct kvm_vcpu *vcpu,
u32 arch_compat)
return -EINVAL;
if (kvmhv_on_pseries() && kvmhv_is_nestedv2()) {
+ /*
+* 'arch_compat == 0' would mean the guest should default to
+* L1's compatibility. In this case, the guest would pick
+* host's PCR and evaluate the corresponding capabilities.
+*/
+ cap = map_pcr_to_cap(guest_pcr_bit);
if (!(cap & nested_capabilities))
return -EINVAL;
}
diff --git a/arch/powerpc/kvm/book3s_hv_nestedv2.c
b/arch/powerpc/kvm/book3s_hv_nestedv2.c
index 5378eb40b162..6042bdc70230 1006
