Re: [PATCH v4] spapr: Add SPAPR_CAP_AIL_MODE_3 for AIL mode 3 support for H_SET_MODE hcall

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 5/15/23 13:02, Nicholas Piggin wrote:
> The behaviour of the Address Translation Mode on Interrupt resource is
> not consistently supported by all CPU versions or all KVM versions: KVM
> HV does not support mode 2, and does not support mode 3 on POWER7 or
> early POWER9 processesors. KVM PR only supports mode 0. TCG supports all
> modes (0, 2, 3) on CPUs with support for the corresonding LPCR[AIL] mode.
> This leads to inconsistencies in guest behaviour and could cause problems
> migrating guests.
> 
> This was not noticable for Linux guests for a long time because the
> kernel only uses modes 0 and 3, and it used to consider AIL-3 to be
> advisory in that it would always keep the AIL-0 vectors around, so it
> did not matter whether or not interrupts were delivered according to
> the AIL mode. Recent Linux guests depend on AIL mode 3 working as
> specified in order to support the SCV facility interrupt. If AIL-3 can
> not be provided, then H_SET_MODE must return an error to Linux so it can
> disable the SCV facility (failure to do so can lead to userspace being
> able to crash the guest kernel).
> 
> Add the ail-mode-3 capability to specify that AIL-3 is supported. AIL-0
> is implied as the baseline, and AIL-2 is no longer supported by spapr.
> AIL-2 is not known to be used by any software, but support in TCG could
> be restored with an ail-mode-2 capability quite easily if a regression
> is reported.
> 
> Modify the H_SET_MODE Address Translation Mode on Interrupt resource
> handler to check capabilities and correctly return error if not
> supported.
> 
> KVM has a cap to advertise support for AIL-3.
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---

Queued in gitlab.com/danielhb/qemu/tree/ppc-next. Thanks,


Daniel

> Since v3:
> - Integrated the KVM cap test into this patch since it is in Linux now.
> - Removed the KVM fallback heuristic that detects whether AIL-3 might be
>    supported if the KVM cap could be missing. This was possibly a bit
>    complicated and dangerous because it prevents KVM from revoking the
>    cap in future in some cases if it became necessary. Eat the
>    performance cost on older KVM for now, and we can consider re-adding
>    the heuristic if that becomes a problem.
> 
>   hw/ppc/spapr.c         |  5 +++++
>   hw/ppc/spapr_caps.c    | 37 +++++++++++++++++++++++++++++++++++++
>   hw/ppc/spapr_hcall.c   | 24 +++++++++++++-----------
>   include/hw/ppc/spapr.h |  4 +++-
>   target/ppc/kvm.c       |  7 +++++++
>   target/ppc/kvm_ppc.h   |  6 ++++++
>   6 files changed, 71 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b58e69afd7..2736233f92 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4673,6 +4673,11 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>       smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
>       smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON;
>       smc->default_caps.caps[SPAPR_CAP_RPT_INVALIDATE] = SPAPR_CAP_OFF;
> +
> +    /* This cap specifies whether the AIL 3 mode for H_SET_RESOURCE is
> +     * supported. The default is modified by default_caps_with_cpu().
> +     */
> +    smc->default_caps.caps[SPAPR_CAP_AIL_MODE_3] = SPAPR_CAP_ON;
>       spapr_caps_add_properties(smc);
>       smc->irq = &spapr_irq_dual;
>       smc->dr_phb_enabled = true;
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index b4283055c1..3fd45a6dec 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -614,6 +614,33 @@ static void cap_rpt_invalidate_apply(SpaprMachineState *spapr,
>       }
>   }
>   
> +static void cap_ail_mode_3_apply(SpaprMachineState *spapr,
> +                                     uint8_t val, Error **errp)
> +{
> +    ERRP_GUARD();
> +    PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> +
> +    if (!val) {
> +        return;
> +    }
> +
> +    if (tcg_enabled()) {
> +        /* AIL-3 is only supported on POWER8 and above CPUs. */
> +        if (!(pcc->insns_flags2 & PPC2_ISA207S)) {
> +            error_setg(errp, "TCG only supports cap-ail-mode-3 on POWER8 and later CPUs");
> +            error_append_hint(errp, "Try appending -machine cap-ail-mode-3=off\n");
> +            return;
> +        }
> +    } else if (kvm_enabled()) {
> +        if (!kvmppc_supports_ail_3()) {
> +            error_setg(errp, "KVM implementation does not support cap-ail-mode-3");
> +            error_append_hint(errp, "Try appending -machine cap-ail-mode-3=off\n");
> +            return;
> +        }
> +    }
> +}
> +
>   SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>       [SPAPR_CAP_HTM] = {
>           .name = "htm",
> @@ -731,6 +758,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>           .type = "bool",
>           .apply = cap_rpt_invalidate_apply,
>       },
> +    [SPAPR_CAP_AIL_MODE_3] = {
> +        .name = "ail-mode-3",
> +        .description = "Alternate Interrupt Location (AIL) mode 3 support",
> +        .index = SPAPR_CAP_AIL_MODE_3,
> +        .get = spapr_cap_get_bool,
> +        .set = spapr_cap_set_bool,
> +        .type = "bool",
> +        .apply = cap_ail_mode_3_apply,
> +    },
>   };
>   
>   static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
> @@ -750,6 +786,7 @@ static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
>                                  0, spapr->max_compat_pvr)) {
>           caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
>           caps.caps[SPAPR_CAP_CFPC] = SPAPR_CAP_BROKEN;
> +        caps.caps[SPAPR_CAP_AIL_MODE_3] = SPAPR_CAP_OFF;
>       }
>   
>       if (!ppc_type_check_compat(cputype, CPU_POWERPC_LOGICAL_2_06_PLUS,
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 1c102c8c0d..b904755575 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -817,30 +817,32 @@ static target_ulong h_set_mode_resource_le(PowerPCCPU *cpu,
>   }
>   
>   static target_ulong h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu,
> +                                                        SpaprMachineState *spapr,
>                                                           target_ulong mflags,
>                                                           target_ulong value1,
>                                                           target_ulong value2)
>   {
> -    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> -
> -    if (!(pcc->insns_flags2 & PPC2_ISA207S)) {
> -        return H_P2;
> -    }
>       if (value1) {
>           return H_P3;
>       }
> +
>       if (value2) {
>           return H_P4;
>       }
>   
> -    if (mflags == 1) {
> -        /* AIL=1 is reserved in POWER8/POWER9/POWER10 */
> +    /*
> +     * AIL-1 is not architected, and AIL-2 is not supported by QEMU spapr.
> +     * It is supported for faithful emulation of bare metal systems, but for
> +     * compatibility concerns we leave it out of the pseries machine.
> +     */
> +    if (mflags != 0 && mflags != 3) {
>           return H_UNSUPPORTED_FLAG;
>       }
>   
> -    if (mflags == 2 && (pcc->insns_flags2 & PPC2_ISA310)) {
> -        /* AIL=2 is reserved in POWER10 (ISA v3.1) */
> -        return H_UNSUPPORTED_FLAG;
> +    if (mflags == 3) {
> +        if (!spapr_get_cap(spapr, SPAPR_CAP_AIL_MODE_3)) {
> +            return H_UNSUPPORTED_FLAG;
> +        }
>       }
>   
>       spapr_set_all_lpcrs(mflags << LPCR_AIL_SHIFT, LPCR_AIL);
> @@ -859,7 +861,7 @@ static target_ulong h_set_mode(PowerPCCPU *cpu, SpaprMachineState *spapr,
>           ret = h_set_mode_resource_le(cpu, spapr, args[0], args[2], args[3]);
>           break;
>       case H_SET_MODE_RESOURCE_ADDR_TRANS_MODE:
> -        ret = h_set_mode_resource_addr_trans_mode(cpu, args[0],
> +        ret = h_set_mode_resource_addr_trans_mode(cpu, spapr, args[0],
>                                                     args[2], args[3]);
>           break;
>       }
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 5c8aabd444..bd5a6c4780 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -78,8 +78,10 @@ typedef enum {
>   #define SPAPR_CAP_FWNMI                 0x0A
>   /* Support H_RPT_INVALIDATE */
>   #define SPAPR_CAP_RPT_INVALIDATE        0x0B
> +/* Support for AIL modes */
> +#define SPAPR_CAP_AIL_MODE_3            0x0C
>   /* Num Caps */
> -#define SPAPR_CAP_NUM                   (SPAPR_CAP_RPT_INVALIDATE + 1)
> +#define SPAPR_CAP_NUM                   (SPAPR_CAP_AIL_MODE_3 + 1)
>   
>   /*
>    * Capability Values
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 336e663bc3..a7f2de9d10 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -88,6 +88,7 @@ static int cap_ppc_nested_kvm_hv;
>   static int cap_large_decr;
>   static int cap_fwnmi;
>   static int cap_rpt_invalidate;
> +static int cap_ail_mode_3;
>   
>   static uint32_t debug_inst_opcode;
>   
> @@ -152,6 +153,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>       }
>   
>       cap_rpt_invalidate = kvm_vm_check_extension(s, KVM_CAP_PPC_RPT_INVALIDATE);
> +    cap_ail_mode_3 = kvm_vm_check_extension(s, KVM_CAP_PPC_AIL_MODE_3);
>       kvm_ppc_register_host_cpu_type();
>   
>       return 0;
> @@ -2560,6 +2562,11 @@ int kvmppc_has_cap_rpt_invalidate(void)
>       return cap_rpt_invalidate;
>   }
>   
> +bool kvmppc_supports_ail_3(void)
> +{
> +    return cap_ail_mode_3;
> +}
> +
>   PowerPCCPUClass *kvm_ppc_get_host_cpu_class(void)
>   {
>       uint32_t host_pvr = mfpvr();
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index 5fd9753953..611debc3ce 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -76,6 +76,7 @@ int kvmppc_set_cap_nested_kvm_hv(int enable);
>   int kvmppc_get_cap_large_decr(void);
>   int kvmppc_enable_cap_large_decr(PowerPCCPU *cpu, int enable);
>   int kvmppc_has_cap_rpt_invalidate(void);
> +bool kvmppc_supports_ail_3(void);
>   int kvmppc_enable_hwrng(void);
>   int kvmppc_put_books_sregs(PowerPCCPU *cpu);
>   PowerPCCPUClass *kvm_ppc_get_host_cpu_class(void);
> @@ -396,6 +397,11 @@ static inline int kvmppc_has_cap_rpt_invalidate(void)
>       return false;
>   }
>   
> +static inline bool kvmppc_supports_ail_3(void)
> +{
> +    return false;
> +}
> +
>   static inline int kvmppc_enable_hwrng(void)
>   {
>       return -1;