Re: Re: [PATCH] KVM: PPC: Book3S HV: Fix L2 guest reboot failure due to empty 'arch_compat'
Hi Aneesh,
Thanks for looking into the patch. My comments are inline below.
On 2024/01/24 01:06 PM, Aneesh Kumar K.V wrote:
> 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
> >
> > 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.
> >
> > Signed-off-by: Amit Machhiwal
> > ---
> > arch/powerpc/kvm/book3s_hv.c | 2 +-
> > arch/powerpc/kvm/book3s_hv_nestedv2.c | 12 ++--
> > 2 files changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> > index 1ed6ec140701..9573d7f4764a 100644
> > --- a/arch/powerpc/kvm/book3s_hv.c
> > +++ b/arch/powerpc/kvm/book3s_hv.c
> > @@ -439,7 +439,7 @@ static int kvmppc_set_arch_compat(struct kvm_vcpu
> > *vcpu, u32 arch_compat)
> > if (guest_pcr_bit > host_pcr_bit)
> > return -EINVAL;
> >
> > - if (kvmhv_on_pseries() && kvmhv_is_nestedv2()) {
> > + if (kvmhv_on_pseries() && kvmhv_is_nestedv2() && arch_compat) {
> > if (!(cap & nested_capabilities))
> > return -EINVAL;
> > }
> >
>
> Instead of that arch_compat check, would it better to do
>
> if (kvmhv_on_pseries() && kvmhv_is_nestedv2()) {
> if (cap && !(cap & nested_capabilities))
> return -EINVAL;
> }
>
> ie, if a capability is requested, then check against nested_capbilites
> to see if the capability exist.
The above condition check will cause problems when we would try to boot
a machine below Power 9.
For example, if we passed the arch_compat == PVR_ARCH_207, cap will
remain 0 resulting the above check into a false condition. Consequently,
we would never return an -EINVAL in that case resulting the arch
compatilbility request succeed when it doesn't support nested papr
guest.
>
>
> > diff --git a/arch/powerpc/kvm/book3s_hv_nestedv2.c
> > b/arch/powerpc/kvm/book3s_hv_nestedv2.c
> > index fd3c4f2d9480..069a1fcfd782 100644
> > --- a/arch/powerpc/kvm/book3s_hv_nestedv2.c
> > +++ b/arch/powerpc/kvm/book3s_hv_nestedv2.c
> > @@ -138,6 +138,7 @@ static int gs_msg_ops_vcpu_fill_info(struct
> > kvmppc_gs_buff *gsb,
> > vector128 v;
> > int rc, i;
> > u16 iden;
> > + u32 arch_compat = 0;
> >
> > vcpu = gsm->data;
> >
> > @@ -347,8 +348,15 @@ static int gs_msg_ops_vcpu_fill_info(struct
> > kvmppc_gs_buff *gsb,
> > break;
> > }
> > case KVMPPC_GSID_LOGICAL_PVR:
> > - rc = kvmppc_gse_put_u32(gsb, iden,
> > - vcpu->arch.vcore->arch_compat);
> > + if (!vcpu->arch.vcore->arch_compat) {
> > + if (cpu_has_feature(CPU_FTR_ARCH_31))
> > + arch_compat = PVR_ARCH_31;
> > + else if (cpu_has_feature(CPU_FTR_ARCH_300))
> > + arch_compat = PVR_ARCH_300;
> > + } else {
> > + arch_compat = vcpu->arch.vcore->arch_compat;
> > + }
> > + rc = kvmppc_gse_put_u32(gsb, iden, arch_compat);
> >
>
> Won't a arch_compat = 0 work here?. ie, where you observing the -EINVAL from
> the first hunk or does this hunk have an impact?
>
No, an arch_compat == 0 won't work in nested API v2. That's because the
guest wide PVR cannot be 0, and if arch_compat == 0, then suppported
host PVR value should be mentioned.
If we were to skip this hunk (keeping the arch_compat == 0), a system
reboot of L2 guest would fail and result into a kernel trap as below:
[ 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
Re: [PATCH] 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
>
> 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.
>
> Signed-off-by: Amit Machhiwal
> ---
> arch/powerpc/kvm/book3s_hv.c | 2 +-
> arch/powerpc/kvm/book3s_hv_nestedv2.c | 12 ++--
> 2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 1ed6ec140701..9573d7f4764a 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -439,7 +439,7 @@ static int kvmppc_set_arch_compat(struct kvm_vcpu *vcpu,
> u32 arch_compat)
> if (guest_pcr_bit > host_pcr_bit)
> return -EINVAL;
>
> - if (kvmhv_on_pseries() && kvmhv_is_nestedv2()) {
> + if (kvmhv_on_pseries() && kvmhv_is_nestedv2() && arch_compat) {
> if (!(cap & nested_capabilities))
> return -EINVAL;
> }
>
Instead of that arch_compat check, would it better to do
if (kvmhv_on_pseries() && kvmhv_is_nestedv2()) {
if (cap && !(cap & nested_capabilities))
return -EINVAL;
}
ie, if a capability is requested, then check against nested_capbilites
to see if the capability exist.
> diff --git a/arch/powerpc/kvm/book3s_hv_nestedv2.c
> b/arch/powerpc/kvm/book3s_hv_nestedv2.c
> index fd3c4f2d9480..069a1fcfd782 100644
> --- a/arch/powerpc/kvm/book3s_hv_nestedv2.c
> +++ b/arch/powerpc/kvm/book3s_hv_nestedv2.c
> @@ -138,6 +138,7 @@ static int gs_msg_ops_vcpu_fill_info(struct
> kvmppc_gs_buff *gsb,
> vector128 v;
> int rc, i;
> u16 iden;
> + u32 arch_compat = 0;
>
> vcpu = gsm->data;
>
> @@ -347,8 +348,15 @@ static int gs_msg_ops_vcpu_fill_info(struct
> kvmppc_gs_buff *gsb,
> break;
> }
> case KVMPPC_GSID_LOGICAL_PVR:
> - rc = kvmppc_gse_put_u32(gsb, iden,
> - vcpu->arch.vcore->arch_compat);
> + if (!vcpu->arch.vcore->arch_compat) {
> + if (cpu_has_feature(CPU_FTR_ARCH_31))
> + arch_compat = PVR_ARCH_31;
> + else if (cpu_has_feature(CPU_FTR_ARCH_300))
> + arch_compat = PVR_ARCH_300;
> + } else {
> + arch_compat = vcpu->arch.vcore->arch_compat;
> + }
> + rc = kvmppc_gse_put_u32(gsb, iden, arch_compat);
>
Won't a arch_compat = 0 work here?. ie, where you observing the -EINVAL from
the first hunk or does this hunk have an impact?
> break;
> }
>
> --
> 2.43.0
-aneesh
Re: [PATCH] KVM: PPC: Book3S HV: Fix L2 guest reboot failure due to empty 'arch_compat'
On Thu, Jan 18, 2024 at 03:26:53PM +0530, Amit Machhiwal wrote:
> 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
>
> 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.
>
> Signed-off-by: Amit Machhiwal
> ---
> arch/powerpc/kvm/book3s_hv.c | 2 +-
> arch/powerpc/kvm/book3s_hv_nestedv2.c | 12 ++--
> 2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 1ed6ec140701..9573d7f4764a 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -439,7 +439,7 @@ static int kvmppc_set_arch_compat(struct kvm_vcpu *vcpu,
> u32 arch_compat)
> if (guest_pcr_bit > host_pcr_bit)
> return -EINVAL;
>
> - if (kvmhv_on_pseries() && kvmhv_is_nestedv2()) {
> + if (kvmhv_on_pseries() && kvmhv_is_nestedv2() && arch_compat) {
> 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 fd3c4f2d9480..069a1fcfd782 100644
> --- a/arch/powerpc/kvm/book3s_hv_nestedv2.c
> +++ b/arch/powerpc/kvm/book3s_hv_nestedv2.c
> @@ -138,6 +138,7 @@ static int gs_msg_ops_vcpu_fill_info(struct
> kvmppc_gs_buff *gsb,
> vector128 v;
> int rc, i;
> u16 iden;
> + u32 arch_compat = 0;
>
> vcpu = gsm->data;
>
> @@ -347,8 +348,15 @@ static int gs_msg_ops_vcpu_fill_info(struct
> kvmppc_gs_buff *gsb,
> break;
> }
> case KVMPPC_GSID_LOGICAL_PVR:
> - rc = kvmppc_gse_put_u32(gsb, iden,
> - vcpu->arch.vcore->arch_compat);
> + if (!vcpu->arch.vcore->arch_compat) {
> + if (cpu_has_feature(CPU_FTR_ARCH_31))
> + arch_compat = PVR_ARCH_31;
> + else if (cpu_has_feature(CPU_FTR_ARCH_300))
> + arch_compat = PVR_ARCH_300;
> + } else {
> + arch_compat = vcpu->arch.vcore->arch_compat;
> + }
> + rc = kvmppc_gse_put_u32(gsb, iden, arch_compat);
> break;
> }
>
> --
> 2.43.0
>
I tested this patch on pseries Power 10 machine with KVM support :
Without this patch, with the latest mainline as host,the kvm guest on
pseries/powervm fails to reboot and with this patch, reboot works fine.
Tested-by: Gautam Menghani
[PATCH] 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
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.
Signed-off-by: Amit Machhiwal
---
arch/powerpc/kvm/book3s_hv.c | 2 +-
arch/powerpc/kvm/book3s_hv_nestedv2.c | 12 ++--
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 1ed6ec140701..9573d7f4764a 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -439,7 +439,7 @@ static int kvmppc_set_arch_compat(struct kvm_vcpu *vcpu,
u32 arch_compat)
if (guest_pcr_bit > host_pcr_bit)
return -EINVAL;
- if (kvmhv_on_pseries() && kvmhv_is_nestedv2()) {
+ if (kvmhv_on_pseries() && kvmhv_is_nestedv2() && arch_compat) {
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 fd3c4f2d9480..069a1fcfd782 100644
--- a/arch/powerpc/kvm/book3s_hv_nestedv2.c
+++ b/arch/powerpc/kvm/book3s_hv_nestedv2.c
@@ -138,6 +138,7 @@ static int gs_msg_ops_vcpu_fill_info(struct kvmppc_gs_buff
*gsb,
vector128 v;
int rc, i;
u16 iden;
+ u32 arch_compat = 0;
vcpu = gsm->data;
@@ -347,8 +348,15 @@ static int gs_msg_ops_vcpu_fill_info(struct kvmppc_gs_buff
*gsb,
break;
}
case KVMPPC_GSID_LOGICAL_PVR:
- rc = kvmppc_gse_put_u32(gsb, iden,
- vcpu->arch.vcore->arch_compat);
+ if (!vcpu->arch.vcore->arch_compat) {
+ if (cpu_has_feature(CPU_FTR_ARCH_31))
+ arch_compat = PVR_ARCH_31;
+ else if (cpu_has_feature(CPU_FTR_ARCH_300))
+ arch_compat = PVR_ARCH_300;
+ } else {
+ arch_compat = vcpu->arch.vcore->arch_compat;
+ }
+ rc = kvmppc_gse_put_u32(gsb, iden, arch_compat);
break;
}
--
2.43.0
