[PATCH 1/2] spapr: Add SPAPR_CAP_AIL_MODE_3 for AIL mode 3 support for H_SET_MODE hcall

[PATCH 1/2] spapr: Add SPAPR_CAP_AIL_MODE_3 for AIL mode 3 support for H_SET_MODE hcall

[Nicholas Piggin]

The behaviour of the Address Translation Mode on Interrupt resource is
not consistently supported by all CPU versions or all KVM versions. In
particular KVM HV only supports mode 0 on POWER7 processors and some
early POWER9 processors, and does not support mode 2 anywhere. KVM PR
only supports mode 0. TCG supports all modes (0, 2, 3).

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. KVM for a long time would not always honor the AIL mode,
contrary to architecture.

Recent Linux guests depend on the AIL mode working as specified in order
to support the SCV facility interrupt. If AIL-3 can not be provided,
then Linux must be given an error so it can disable the SCV facility,
rather than silently failing.

Add the ail-mode-3 capability to specify that AIL-3 is supported. AIL-0
is implied as the baseline. AIL-2 is no longer supported by spapr. It
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.

A heuristic is added for KVM to determine AIL-3 support before the
introduction of a new KVM CAP.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
Since the RFC, I made this a single mode cap for ail-3, suggested
by David. Also split out the KVM CAP into patch 2 because that is
not ready for merge yet (this patch can go in ahead of it).

Thanks,
Nick

 hw/ppc/spapr.c         |  6 ++++++
 hw/ppc/spapr_caps.c    | 23 +++++++++++++++++++++++
 hw/ppc/spapr_hcall.c   | 23 +++++++++++++----------
 include/hw/ppc/spapr.h |  4 +++-
 target/ppc/kvm.c       | 23 +++++++++++++++++++++++
 target/ppc/kvm_ppc.h   |  6 ++++++
 6 files changed, 74 insertions(+), 11 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 3d6ec309dd..15a02d3e78 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4606,6 +4606,12 @@ 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. Default to true, (PR KVM, POWER7 and earlier, and KVM on
+     * some early POWER9s only support 0).
+     */
+    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 ed7c077a0d..5fd4a53c33 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -613,6 +613,20 @@ static void cap_rpt_invalidate_apply(SpaprMachineState *spapr,
     }
 }
 
+static void cap_ail_mode_3_apply(SpaprMachineState *spapr,
+                                     uint8_t val, Error **errp)
+{
+    ERRP_GUARD();
+
+    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",
@@ -730,6 +744,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,
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 222c1b6bbd..5dec056796 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -811,32 +811,35 @@ 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. */
+    if (mflags == 1 || mflags == 2) {
         return H_UNSUPPORTED_FLAG;
     }
 
-    if (mflags == 2 && (pcc->insns_flags2 & PPC2_ISA310)) {
-        /* AIL=2 is reserved in POWER10 (ISA v3.1) */
+    /* Only 0 and 3 are possible */
+    if (mflags != 0 && mflags != 3) {
         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);
 
     return H_SUCCESS;
@@ -853,7 +856,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 ee7504b976..edbf3eeed0 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -77,8 +77,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 dc93b99189..128bc530d4 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2563,6 +2563,29 @@ int kvmppc_has_cap_rpt_invalidate(void)
     return cap_rpt_invalidate;
 }
 
+int kvmppc_supports_ail_3(void)
+{
+    PowerPCCPUClass *pcc = kvm_ppc_get_host_cpu_class();
+
+    /*
+     * KVM PR only supports AIL-0
+     */
+    if (kvmppc_is_pr(kvm_state)) {
+        return 0;
+    }
+
+    /*
+     * KVM HV hosts support AIL-3 on POWER8 and above, except for radix
+     * mode on some early POWER9s, but it's not clear how to cover those
+     * without disabling the feature for many.
+     */
+    if (!(pcc->insns_flags2 & PPC2_ISA207S)) {
+        return 0;
+    }
+
+    return 1;
+}
+
 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 ee9325bf9a..d9d1c54955 100644
--- a/target/ppc/kvm_ppc.h
+++ b/target/ppc/kvm_ppc.h
@@ -73,6 +73,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);
+int 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);
@@ -393,6 +394,11 @@ static inline int kvmppc_has_cap_rpt_invalidate(void)
     return false;
 }
 
+static inline int kvmppc_supports_ail_3(void)
+{
+    return false;
+}
+
 static inline int kvmppc_enable_hwrng(void)
 {
     return -1;
-- 
2.23.0

[PATCH 2/2] target/ppc/kvm: Use KVM_CAP_PPC_AIL_MODE_3 to determine cap-ail-mode-3 support

[Nicholas Piggin]

Use KVM_CAP_PPC_AIL_MODE_3 to determine cap-ail-mode-3 support for KVM
guests. Keep the fallback heuristic for KVM hosts that pre-date this
CAP.

This is only proposed the KVM CAP has not yet been allocated. I will
ask to merge the new KVM cap when there are no objections on the QEMU
side.

not-yet-Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/spapr_caps.c       |  2 +-
 linux-headers/linux/kvm.h |  1 +
 target/ppc/kvm.c          | 18 +++++++++++++++++-
 target/ppc/kvm_ppc.h      |  4 ++--
 4 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index 5fd4a53c33..5cc80776d0 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -619,7 +619,7 @@ static void cap_ail_mode_3_apply(SpaprMachineState *spapr,
     ERRP_GUARD();
 
     if (kvm_enabled()) {
-        if (!kvmppc_supports_ail_3()) {
+        if (!kvmppc_has_cap_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;
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 02c5e7b7bb..d91f578200 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -1130,6 +1130,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_BINARY_STATS_FD 203
 #define KVM_CAP_EXIT_ON_EMULATION_FAILURE 204
 #define KVM_CAP_ARM_MTE 205
+#define KVM_CAP_PPC_AIL_MODE_3 210
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 128bc530d4..d0d0bdaac4 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -90,6 +90,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;
 
@@ -154,6 +155,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;
@@ -2563,10 +2565,24 @@ int kvmppc_has_cap_rpt_invalidate(void)
     return cap_rpt_invalidate;
 }
 
-int kvmppc_supports_ail_3(void)
+int kvmppc_has_cap_ail_3(void)
 {
     PowerPCCPUClass *pcc = kvm_ppc_get_host_cpu_class();
 
+    if (cap_ail_mode_3) {
+        return 1;
+    }
+
+    if (kvm_ioctl(kvm_state, KVM_CHECK_EXTENSION, KVM_CAP_PPC_AIL_MODE_3) == 0) {
+        return 0;
+    }
+
+    /*
+     * For KVM hosts that pre-date this cap, special-case the test because
+     * the performance cost for disabling the feature unconditionally is
+     * prohibitive.
+     */
+
     /*
      * KVM PR only supports AIL-0
      */
diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
index d9d1c54955..efafa67b83 100644
--- a/target/ppc/kvm_ppc.h
+++ b/target/ppc/kvm_ppc.h
@@ -73,7 +73,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);
-int kvmppc_supports_ail_3(void);
+int kvmppc_has_cap_ail_3(void);
 int kvmppc_enable_hwrng(void);
 int kvmppc_put_books_sregs(PowerPCCPU *cpu);
 PowerPCCPUClass *kvm_ppc_get_host_cpu_class(void);
@@ -394,7 +394,7 @@ static inline int kvmppc_has_cap_rpt_invalidate(void)
     return false;
 }
 
-static inline int kvmppc_supports_ail_3(void)
+static inline int kvmppc_has_cap_ail_3(void)
 {
     return false;
 }
-- 
2.23.0

Re: [PATCH 2/2] target/ppc/kvm: Use KVM_CAP_PPC_AIL_MODE_3 to determine cap-ail-mode-3 support

[Fabiano Rosas]

Nicholas Piggin <npiggin@gmail.com> writes:

> Use KVM_CAP_PPC_AIL_MODE_3 to determine cap-ail-mode-3 support for KVM
> guests. Keep the fallback heuristic for KVM hosts that pre-date this
> CAP.
>
> This is only proposed the KVM CAP has not yet been allocated. I will
> ask to merge the new KVM cap when there are no objections on the QEMU
> side.
>
> not-yet-Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  hw/ppc/spapr_caps.c       |  2 +-
>  linux-headers/linux/kvm.h |  1 +
>  target/ppc/kvm.c          | 18 +++++++++++++++++-
>  target/ppc/kvm_ppc.h      |  4 ++--
>  4 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 5fd4a53c33..5cc80776d0 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -619,7 +619,7 @@ static void cap_ail_mode_3_apply(SpaprMachineState *spapr,
>      ERRP_GUARD();
>  
>      if (kvm_enabled()) {
> -        if (!kvmppc_supports_ail_3()) {
> +        if (!kvmppc_has_cap_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;
> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
> index 02c5e7b7bb..d91f578200 100644
> --- a/linux-headers/linux/kvm.h
> +++ b/linux-headers/linux/kvm.h
> @@ -1130,6 +1130,7 @@ struct kvm_ppc_resize_hpt {
>  #define KVM_CAP_BINARY_STATS_FD 203
>  #define KVM_CAP_EXIT_ON_EMULATION_FAILURE 204
>  #define KVM_CAP_ARM_MTE 205
> +#define KVM_CAP_PPC_AIL_MODE_3 210
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 128bc530d4..d0d0bdaac4 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -90,6 +90,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;
>  
> @@ -154,6 +155,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;
> @@ -2563,10 +2565,24 @@ int kvmppc_has_cap_rpt_invalidate(void)
>      return cap_rpt_invalidate;
>  }
>  
> -int kvmppc_supports_ail_3(void)
> +int kvmppc_has_cap_ail_3(void)
>  {
>      PowerPCCPUClass *pcc = kvm_ppc_get_host_cpu_class();
>  
> +    if (cap_ail_mode_3) {
> +        return 1;
> +    }
> +
> +    if (kvm_ioctl(kvm_state, KVM_CHECK_EXTENSION, KVM_CAP_PPC_AIL_MODE_3) == 0) {
> +        return 0;
> +    }

This is not needed here it seems.

> +
> +    /*
> +     * For KVM hosts that pre-date this cap, special-case the test because
> +     * the performance cost for disabling the feature unconditionally is
> +     * prohibitive.
> +     */
> +
>      /*
>       * KVM PR only supports AIL-0
>       */
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index d9d1c54955..efafa67b83 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -73,7 +73,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);
> -int kvmppc_supports_ail_3(void);
> +int kvmppc_has_cap_ail_3(void);
>  int kvmppc_enable_hwrng(void);
>  int kvmppc_put_books_sregs(PowerPCCPU *cpu);
>  PowerPCCPUClass *kvm_ppc_get_host_cpu_class(void);
> @@ -394,7 +394,7 @@ static inline int kvmppc_has_cap_rpt_invalidate(void)
>      return false;
>  }
>  
> -static inline int kvmppc_supports_ail_3(void)
> +static inline int kvmppc_has_cap_ail_3(void)
>  {
>      return false;
>  }

Re: [PATCH 2/2] target/ppc/kvm: Use KVM_CAP_PPC_AIL_MODE_3 to determine cap-ail-mode-3 support

[Nicholas Piggin]

Excerpts from Fabiano Rosas's message of February 14, 2022 11:13 pm:
> Nicholas Piggin <npiggin@gmail.com> writes:
> 
>> Use KVM_CAP_PPC_AIL_MODE_3 to determine cap-ail-mode-3 support for KVM
>> guests. Keep the fallback heuristic for KVM hosts that pre-date this
>> CAP.
>>
>> This is only proposed the KVM CAP has not yet been allocated. I will
>> ask to merge the new KVM cap when there are no objections on the QEMU
>> side.
>>
>> not-yet-Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>  hw/ppc/spapr_caps.c       |  2 +-
>>  linux-headers/linux/kvm.h |  1 +
>>  target/ppc/kvm.c          | 18 +++++++++++++++++-
>>  target/ppc/kvm_ppc.h      |  4 ++--
>>  4 files changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
>> index 5fd4a53c33..5cc80776d0 100644
>> --- a/hw/ppc/spapr_caps.c
>> +++ b/hw/ppc/spapr_caps.c
>> @@ -619,7 +619,7 @@ static void cap_ail_mode_3_apply(SpaprMachineState *spapr,
>>      ERRP_GUARD();
>>  
>>      if (kvm_enabled()) {
>> -        if (!kvmppc_supports_ail_3()) {
>> +        if (!kvmppc_has_cap_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;
>> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
>> index 02c5e7b7bb..d91f578200 100644
>> --- a/linux-headers/linux/kvm.h
>> +++ b/linux-headers/linux/kvm.h
>> @@ -1130,6 +1130,7 @@ struct kvm_ppc_resize_hpt {
>>  #define KVM_CAP_BINARY_STATS_FD 203
>>  #define KVM_CAP_EXIT_ON_EMULATION_FAILURE 204
>>  #define KVM_CAP_ARM_MTE 205
>> +#define KVM_CAP_PPC_AIL_MODE_3 210
>>  
>>  #ifdef KVM_CAP_IRQ_ROUTING
>>  
>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>> index 128bc530d4..d0d0bdaac4 100644
>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -90,6 +90,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;
>>  
>> @@ -154,6 +155,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;
>> @@ -2563,10 +2565,24 @@ int kvmppc_has_cap_rpt_invalidate(void)
>>      return cap_rpt_invalidate;
>>  }
>>  
>> -int kvmppc_supports_ail_3(void)
>> +int kvmppc_has_cap_ail_3(void)
>>  {
>>      PowerPCCPUClass *pcc = kvm_ppc_get_host_cpu_class();
>>  
>> +    if (cap_ail_mode_3) {
>> +        return 1;
>> +    }
>> +
>> +    if (kvm_ioctl(kvm_state, KVM_CHECK_EXTENSION, KVM_CAP_PPC_AIL_MODE_3) == 0) {
>> +        return 0;
>> +    }
> 
> This is not needed here it seems.

This is to test whether the capability is recognised by the HV. 
kvm_vm_check_extension() treats ioctl error as 0 capability but we want 
to do this extra heuristic.

I'm not sure if there's a better standard way to do this.

Thanks,
Nick

Re: [PATCH 1/2] spapr: Add SPAPR_CAP_AIL_MODE_3 for AIL mode 3 support for H_SET_MODE hcall

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 9756 bytes --]

On Mon, Feb 14, 2022 at 09:17:48PM +1000, 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. In
> particular KVM HV only supports mode 0 on POWER7 processors and some
> early POWER9 processors, and does not support mode 2 anywhere. KVM PR
> only supports mode 0. TCG supports all modes (0, 2, 3).
> 
> 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. KVM for a long time would not always honor the AIL mode,
> contrary to architecture.
> 
> Recent Linux guests depend on the AIL mode working as specified in order
> to support the SCV facility interrupt. If AIL-3 can not be provided,
> then Linux must be given an error so it can disable the SCV facility,
> rather than silently failing.
> 
> Add the ail-mode-3 capability to specify that AIL-3 is supported. AIL-0
> is implied as the baseline. AIL-2 is no longer supported by spapr. It
> 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.
> 
> A heuristic is added for KVM to determine AIL-3 support before the
> introduction of a new KVM CAP.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> Since the RFC, I made this a single mode cap for ail-3, suggested
> by David. Also split out the KVM CAP into patch 2 because that is
> not ready for merge yet (this patch can go in ahead of it).
> 
> Thanks,
> Nick
> 
>  hw/ppc/spapr.c         |  6 ++++++
>  hw/ppc/spapr_caps.c    | 23 +++++++++++++++++++++++
>  hw/ppc/spapr_hcall.c   | 23 +++++++++++++----------
>  include/hw/ppc/spapr.h |  4 +++-
>  target/ppc/kvm.c       | 23 +++++++++++++++++++++++
>  target/ppc/kvm_ppc.h   |  6 ++++++
>  6 files changed, 74 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 3d6ec309dd..15a02d3e78 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4606,6 +4606,12 @@ 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. Default to true, (PR KVM, POWER7 and earlier, and KVM on
> +     * some early POWER9s only support 0).
> +     */
> +    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 ed7c077a0d..5fd4a53c33 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -613,6 +613,20 @@ static void cap_rpt_invalidate_apply(SpaprMachineState *spapr,
>      }
>  }
>  
> +static void cap_ail_mode_3_apply(SpaprMachineState *spapr,
> +                                     uint8_t val, Error **errp)
> +{
> +    ERRP_GUARD();
> +
> +    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",
> @@ -730,6 +744,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,
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 222c1b6bbd..5dec056796 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -811,32 +811,35 @@ 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. */
> +    if (mflags == 1 || mflags == 2) {

This test is redundant with the one below, isn't it?

>          return H_UNSUPPORTED_FLAG;
>      }
>  
> -    if (mflags == 2 && (pcc->insns_flags2 & PPC2_ISA310)) {
> -        /* AIL=2 is reserved in POWER10 (ISA v3.1) */
> +    /* Only 0 and 3 are possible */
> +    if (mflags != 0 && mflags != 3) {
>          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);
>  
>      return H_SUCCESS;
> @@ -853,7 +856,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 ee7504b976..edbf3eeed0 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -77,8 +77,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 dc93b99189..128bc530d4 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2563,6 +2563,29 @@ int kvmppc_has_cap_rpt_invalidate(void)
>      return cap_rpt_invalidate;
>  }
>  
> +int kvmppc_supports_ail_3(void)

Returning bool would make more sense, no?

> +{
> +    PowerPCCPUClass *pcc = kvm_ppc_get_host_cpu_class();
> +
> +    /*
> +     * KVM PR only supports AIL-0
> +     */
> +    if (kvmppc_is_pr(kvm_state)) {
> +        return 0;
> +    }
> +
> +    /*
> +     * KVM HV hosts support AIL-3 on POWER8 and above, except for radix
> +     * mode on some early POWER9s, but it's not clear how to cover those
> +     * without disabling the feature for many.
> +     */
> +    if (!(pcc->insns_flags2 & PPC2_ISA207S)) {

This effectively means that the pseries machine type simply won't
start with a !PPC2_ISA207S cpu model.  I'm not sure if we support any
such CPUs in any case.  If we do, we should probably change the
default value for this cap based on cpu model in
default_caps_with_cpu().

> +        return 0;
> +    }
> +
> +    return 1;
> +}
> +
>  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 ee9325bf9a..d9d1c54955 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -73,6 +73,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);
> +int 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);
> @@ -393,6 +394,11 @@ static inline int kvmppc_has_cap_rpt_invalidate(void)
>      return false;
>  }
>  
> +static inline int kvmppc_supports_ail_3(void)
> +{
> +    return false;
> +}
> +
>  static inline int kvmppc_enable_hwrng(void)
>  {
>      return -1;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] target/ppc/kvm: Use KVM_CAP_PPC_AIL_MODE_3 to determine cap-ail-mode-3 support

[Fabiano Rosas]

Nicholas Piggin <npiggin@gmail.com> writes:

> Excerpts from Fabiano Rosas's message of February 14, 2022 11:13 pm:
>> Nicholas Piggin <npiggin@gmail.com> writes:
>> 
>>> Use KVM_CAP_PPC_AIL_MODE_3 to determine cap-ail-mode-3 support for KVM
>>> guests. Keep the fallback heuristic for KVM hosts that pre-date this
>>> CAP.
>>>
>>> This is only proposed the KVM CAP has not yet been allocated. I will
>>> ask to merge the new KVM cap when there are no objections on the QEMU
>>> side.
>>>
>>> not-yet-Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>>  hw/ppc/spapr_caps.c       |  2 +-
>>>  linux-headers/linux/kvm.h |  1 +
>>>  target/ppc/kvm.c          | 18 +++++++++++++++++-
>>>  target/ppc/kvm_ppc.h      |  4 ++--
>>>  4 files changed, 21 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
>>> index 5fd4a53c33..5cc80776d0 100644
>>> --- a/hw/ppc/spapr_caps.c
>>> +++ b/hw/ppc/spapr_caps.c
>>> @@ -619,7 +619,7 @@ static void cap_ail_mode_3_apply(SpaprMachineState *spapr,
>>>      ERRP_GUARD();
>>>  
>>>      if (kvm_enabled()) {
>>> -        if (!kvmppc_supports_ail_3()) {
>>> +        if (!kvmppc_has_cap_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;
>>> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
>>> index 02c5e7b7bb..d91f578200 100644
>>> --- a/linux-headers/linux/kvm.h
>>> +++ b/linux-headers/linux/kvm.h
>>> @@ -1130,6 +1130,7 @@ struct kvm_ppc_resize_hpt {
>>>  #define KVM_CAP_BINARY_STATS_FD 203
>>>  #define KVM_CAP_EXIT_ON_EMULATION_FAILURE 204
>>>  #define KVM_CAP_ARM_MTE 205
>>> +#define KVM_CAP_PPC_AIL_MODE_3 210
>>>  
>>>  #ifdef KVM_CAP_IRQ_ROUTING
>>>  
>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>> index 128bc530d4..d0d0bdaac4 100644
>>> --- a/target/ppc/kvm.c
>>> +++ b/target/ppc/kvm.c
>>> @@ -90,6 +90,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;
>>>  
>>> @@ -154,6 +155,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;
>>> @@ -2563,10 +2565,24 @@ int kvmppc_has_cap_rpt_invalidate(void)
>>>      return cap_rpt_invalidate;
>>>  }
>>>  
>>> -int kvmppc_supports_ail_3(void)
>>> +int kvmppc_has_cap_ail_3(void)
>>>  {
>>>      PowerPCCPUClass *pcc = kvm_ppc_get_host_cpu_class();
>>>  
>>> +    if (cap_ail_mode_3) {
>>> +        return 1;
>>> +    }
>>> +
>>> +    if (kvm_ioctl(kvm_state, KVM_CHECK_EXTENSION, KVM_CAP_PPC_AIL_MODE_3) == 0) {
>>> +        return 0;
>>> +    }
>> 
>> This is not needed here it seems.
>
> This is to test whether the capability is recognised by the HV. 
> kvm_vm_check_extension() treats ioctl error as 0 capability but we want 
> to do this extra heuristic.

Do you intend to make the KVM capability return < 0 in case AIL_3 is not
supported? AFAICS the unknown capability won't result in an ioctl error
as kvm_vm_ioctl_check_extension always returns >= 0.

>
> I'm not sure if there's a better standard way to do this.
>
> Thanks,
> Nick

Re: [PATCH 2/2] target/ppc/kvm: Use KVM_CAP_PPC_AIL_MODE_3 to determine cap-ail-mode-3 support

[Nicholas Piggin]

Excerpts from Fabiano Rosas's message of February 15, 2022 10:21 pm:
> Nicholas Piggin <npiggin@gmail.com> writes:
> 
>> Excerpts from Fabiano Rosas's message of February 14, 2022 11:13 pm:
>>> Nicholas Piggin <npiggin@gmail.com> writes:
>>> 
>>>> Use KVM_CAP_PPC_AIL_MODE_3 to determine cap-ail-mode-3 support for KVM
>>>> guests. Keep the fallback heuristic for KVM hosts that pre-date this
>>>> CAP.
>>>>
>>>> This is only proposed the KVM CAP has not yet been allocated. I will
>>>> ask to merge the new KVM cap when there are no objections on the QEMU
>>>> side.
>>>>
>>>> not-yet-Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>> ---
>>>>  hw/ppc/spapr_caps.c       |  2 +-
>>>>  linux-headers/linux/kvm.h |  1 +
>>>>  target/ppc/kvm.c          | 18 +++++++++++++++++-
>>>>  target/ppc/kvm_ppc.h      |  4 ++--
>>>>  4 files changed, 21 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
>>>> index 5fd4a53c33..5cc80776d0 100644
>>>> --- a/hw/ppc/spapr_caps.c
>>>> +++ b/hw/ppc/spapr_caps.c
>>>> @@ -619,7 +619,7 @@ static void cap_ail_mode_3_apply(SpaprMachineState *spapr,
>>>>      ERRP_GUARD();
>>>>  
>>>>      if (kvm_enabled()) {
>>>> -        if (!kvmppc_supports_ail_3()) {
>>>> +        if (!kvmppc_has_cap_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;
>>>> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
>>>> index 02c5e7b7bb..d91f578200 100644
>>>> --- a/linux-headers/linux/kvm.h
>>>> +++ b/linux-headers/linux/kvm.h
>>>> @@ -1130,6 +1130,7 @@ struct kvm_ppc_resize_hpt {
>>>>  #define KVM_CAP_BINARY_STATS_FD 203
>>>>  #define KVM_CAP_EXIT_ON_EMULATION_FAILURE 204
>>>>  #define KVM_CAP_ARM_MTE 205
>>>> +#define KVM_CAP_PPC_AIL_MODE_3 210
>>>>  
>>>>  #ifdef KVM_CAP_IRQ_ROUTING
>>>>  
>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>>> index 128bc530d4..d0d0bdaac4 100644
>>>> --- a/target/ppc/kvm.c
>>>> +++ b/target/ppc/kvm.c
>>>> @@ -90,6 +90,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;
>>>>  
>>>> @@ -154,6 +155,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;
>>>> @@ -2563,10 +2565,24 @@ int kvmppc_has_cap_rpt_invalidate(void)
>>>>      return cap_rpt_invalidate;
>>>>  }
>>>>  
>>>> -int kvmppc_supports_ail_3(void)
>>>> +int kvmppc_has_cap_ail_3(void)
>>>>  {
>>>>      PowerPCCPUClass *pcc = kvm_ppc_get_host_cpu_class();
>>>>  
>>>> +    if (cap_ail_mode_3) {
>>>> +        return 1;
>>>> +    }
>>>> +
>>>> +    if (kvm_ioctl(kvm_state, KVM_CHECK_EXTENSION, KVM_CAP_PPC_AIL_MODE_3) == 0) {
>>>> +        return 0;
>>>> +    }
>>> 
>>> This is not needed here it seems.
>>
>> This is to test whether the capability is recognised by the HV. 
>> kvm_vm_check_extension() treats ioctl error as 0 capability but we want 
>> to do this extra heuristic.
> 
> Do you intend to make the KVM capability return < 0 in case AIL_3 is not
> supported?

No it should return 0 in that case.

> AFAICS the unknown capability won't result in an ioctl error
> as kvm_vm_ioctl_check_extension always returns >= 0.

Oh. There is no way to tell that a host does not recognise a cap? :(

Great for the purity test, unknown cap == unsupported. For a practical 
point of view to catch such bugs and oversights there should have been
some way to handle it.

I'm not sure what to do then. Try some even worse hack and leave that
in for a couple of years to give upstream KVM a chance to trickle down.

Thanks,
Nick

Re: [PATCH 1/2] spapr: Add SPAPR_CAP_AIL_MODE_3 for AIL mode 3 support for H_SET_MODE hcall

[Nicholas Piggin]

Excerpts from David Gibson's message of February 15, 2022 11:10 am:
> On Mon, Feb 14, 2022 at 09:17:48PM +1000, Nicholas Piggin wrote:
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 222c1b6bbd..5dec056796 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -811,32 +811,35 @@ 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. */
>> +    if (mflags == 1 || mflags == 2) {
> 
> This test is redundant with the one below, isn't it?

Ah, yes.

> 
>>          return H_UNSUPPORTED_FLAG;
>>      }
>>  
>> -    if (mflags == 2 && (pcc->insns_flags2 & PPC2_ISA310)) {
>> -        /* AIL=2 is reserved in POWER10 (ISA v3.1) */
>> +    /* Only 0 and 3 are possible */
>> +    if (mflags != 0 && mflags != 3) {
>>          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);
>>  
>>      return H_SUCCESS;
>> @@ -853,7 +856,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 ee7504b976..edbf3eeed0 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -77,8 +77,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 dc93b99189..128bc530d4 100644
>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -2563,6 +2563,29 @@ int kvmppc_has_cap_rpt_invalidate(void)
>>      return cap_rpt_invalidate;
>>  }
>>  
>> +int kvmppc_supports_ail_3(void)
> 
> Returning bool would make more sense, no?

It would.

> 
>> +{
>> +    PowerPCCPUClass *pcc = kvm_ppc_get_host_cpu_class();
>> +
>> +    /*
>> +     * KVM PR only supports AIL-0
>> +     */
>> +    if (kvmppc_is_pr(kvm_state)) {
>> +        return 0;
>> +    }
>> +
>> +    /*
>> +     * KVM HV hosts support AIL-3 on POWER8 and above, except for radix
>> +     * mode on some early POWER9s, but it's not clear how to cover those
>> +     * without disabling the feature for many.
>> +     */
>> +    if (!(pcc->insns_flags2 & PPC2_ISA207S)) {
> 
> This effectively means that the pseries machine type simply won't
> start with a !PPC2_ISA207S cpu model.  I'm not sure if we support any
> such CPUs in any case.

Oh, would that at least give an error to disable cap-ail-mode-3?

> If we do, we should probably change the
> default value for this cap based on cpu model in
> default_caps_with_cpu().

We allegedly still support POWER7 KVM in Linux. I've never tested it
and I don't know how much it's used at all. Probably should keep it
working here if possible. I'll look into default_caps_with_cpu().

Thanks,
Nick

Re: [PATCH 2/2] target/ppc/kvm: Use KVM_CAP_PPC_AIL_MODE_3 to determine cap-ail-mode-3 support

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4682 bytes --]

>On Wed, Feb 16, 2022 at 11:46:58AM +1000, Nicholas Piggin wrote:
> Excerpts from Fabiano Rosas's message of February 15, 2022 10:21 pm:
> > Nicholas Piggin <npiggin@gmail.com> writes:
> > 
> >> Excerpts from Fabiano Rosas's message of February 14, 2022 11:13 pm:
> >>> Nicholas Piggin <npiggin@gmail.com> writes:
> >>> 
> >>>> Use KVM_CAP_PPC_AIL_MODE_3 to determine cap-ail-mode-3 support for KVM
> >>>> guests. Keep the fallback heuristic for KVM hosts that pre-date this
> >>>> CAP.
> >>>>
> >>>> This is only proposed the KVM CAP has not yet been allocated. I will
> >>>> ask to merge the new KVM cap when there are no objections on the QEMU
> >>>> side.
> >>>>
> >>>> not-yet-Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> >>>> ---
> >>>>  hw/ppc/spapr_caps.c       |  2 +-
> >>>>  linux-headers/linux/kvm.h |  1 +
> >>>>  target/ppc/kvm.c          | 18 +++++++++++++++++-
> >>>>  target/ppc/kvm_ppc.h      |  4 ++--
> >>>>  4 files changed, 21 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> >>>> index 5fd4a53c33..5cc80776d0 100644
> >>>> --- a/hw/ppc/spapr_caps.c
> >>>> +++ b/hw/ppc/spapr_caps.c
> >>>> @@ -619,7 +619,7 @@ static void cap_ail_mode_3_apply(SpaprMachineState *spapr,
> >>>>      ERRP_GUARD();
> >>>>  
> >>>>      if (kvm_enabled()) {
> >>>> -        if (!kvmppc_supports_ail_3()) {
> >>>> +        if (!kvmppc_has_cap_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;
> >>>> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
> >>>> index 02c5e7b7bb..d91f578200 100644
> >>>> --- a/linux-headers/linux/kvm.h
> >>>> +++ b/linux-headers/linux/kvm.h
> >>>> @@ -1130,6 +1130,7 @@ struct kvm_ppc_resize_hpt {
> >>>>  #define KVM_CAP_BINARY_STATS_FD 203
> >>>>  #define KVM_CAP_EXIT_ON_EMULATION_FAILURE 204
> >>>>  #define KVM_CAP_ARM_MTE 205
> >>>> +#define KVM_CAP_PPC_AIL_MODE_3 210
> >>>>  
> >>>>  #ifdef KVM_CAP_IRQ_ROUTING
> >>>>  
> >>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> >>>> index 128bc530d4..d0d0bdaac4 100644
> >>>> --- a/target/ppc/kvm.c
> >>>> +++ b/target/ppc/kvm.c
> >>>> @@ -90,6 +90,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;
> >>>>  
> >>>> @@ -154,6 +155,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;
> >>>> @@ -2563,10 +2565,24 @@ int kvmppc_has_cap_rpt_invalidate(void)
> >>>>      return cap_rpt_invalidate;
> >>>>  }
> >>>>  
> >>>> -int kvmppc_supports_ail_3(void)
> >>>> +int kvmppc_has_cap_ail_3(void)
> >>>>  {
> >>>>      PowerPCCPUClass *pcc = kvm_ppc_get_host_cpu_class();
> >>>>  
> >>>> +    if (cap_ail_mode_3) {
> >>>> +        return 1;
> >>>> +    }
> >>>> +
> >>>> +    if (kvm_ioctl(kvm_state, KVM_CHECK_EXTENSION, KVM_CAP_PPC_AIL_MODE_3) == 0) {
> >>>> +        return 0;
> >>>> +    }
> >>> 
> >>> This is not needed here it seems.
> >>
> >> This is to test whether the capability is recognised by the HV. 
> >> kvm_vm_check_extension() treats ioctl error as 0 capability but we want 
> >> to do this extra heuristic.
> > 
> > Do you intend to make the KVM capability return < 0 in case AIL_3 is not
> > supported?
> 
> No it should return 0 in that case.
> 
> > AFAICS the unknown capability won't result in an ioctl error
> > as kvm_vm_ioctl_check_extension always returns >= 0.
> 
> Oh. There is no way to tell that a host does not recognise a cap? :(

Not short of giving it all non-zero values when you create the new cap.

> Great for the purity test, unknown cap == unsupported. For a practical 
> point of view to catch such bugs and oversights there should have been
> some way to handle it.

Yeah :(.

> I'm not sure what to do then. Try some even worse hack and leave that
> in for a couple of years to give upstream KVM a chance to trickle down.
> 
> Thanks,
> Nick
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] spapr: Add SPAPR_CAP_AIL_MODE_3 for AIL mode 3 support for H_SET_MODE hcall

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5183 bytes --]

On Wed, Feb 16, 2022 at 11:50:34AM +1000, Nicholas Piggin wrote:
> Excerpts from David Gibson's message of February 15, 2022 11:10 am:
> > On Mon, Feb 14, 2022 at 09:17:48PM +1000, Nicholas Piggin wrote:
> >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> >> index 222c1b6bbd..5dec056796 100644
> >> --- a/hw/ppc/spapr_hcall.c
> >> +++ b/hw/ppc/spapr_hcall.c
> >> @@ -811,32 +811,35 @@ 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. */
> >> +    if (mflags == 1 || mflags == 2) {
> > 
> > This test is redundant with the one below, isn't it?
> 
> Ah, yes.
> 
> > 
> >>          return H_UNSUPPORTED_FLAG;
> >>      }
> >>  
> >> -    if (mflags == 2 && (pcc->insns_flags2 & PPC2_ISA310)) {
> >> -        /* AIL=2 is reserved in POWER10 (ISA v3.1) */
> >> +    /* Only 0 and 3 are possible */
> >> +    if (mflags != 0 && mflags != 3) {
> >>          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);
> >>  
> >>      return H_SUCCESS;
> >> @@ -853,7 +856,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 ee7504b976..edbf3eeed0 100644
> >> --- a/include/hw/ppc/spapr.h
> >> +++ b/include/hw/ppc/spapr.h
> >> @@ -77,8 +77,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 dc93b99189..128bc530d4 100644
> >> --- a/target/ppc/kvm.c
> >> +++ b/target/ppc/kvm.c
> >> @@ -2563,6 +2563,29 @@ int kvmppc_has_cap_rpt_invalidate(void)
> >>      return cap_rpt_invalidate;
> >>  }
> >>  
> >> +int kvmppc_supports_ail_3(void)
> > 
> > Returning bool would make more sense, no?
> 
> It would.
> 
> > 
> >> +{
> >> +    PowerPCCPUClass *pcc = kvm_ppc_get_host_cpu_class();
> >> +
> >> +    /*
> >> +     * KVM PR only supports AIL-0
> >> +     */
> >> +    if (kvmppc_is_pr(kvm_state)) {
> >> +        return 0;
> >> +    }
> >> +
> >> +    /*
> >> +     * KVM HV hosts support AIL-3 on POWER8 and above, except for radix
> >> +     * mode on some early POWER9s, but it's not clear how to cover those
> >> +     * without disabling the feature for many.
> >> +     */
> >> +    if (!(pcc->insns_flags2 & PPC2_ISA207S)) {
> > 
> > This effectively means that the pseries machine type simply won't
> > start with a !PPC2_ISA207S cpu model.  I'm not sure if we support any
> > such CPUs in any case.
> 
> Oh, would that at least give an error to disable cap-ail-mode-3?

Yes, it should.  Which for something as obscure as POWER7 may be acceptable.

> > If we do, we should probably change the
> > default value for this cap based on cpu model in
> > default_caps_with_cpu().
> 
> We allegedly still support POWER7 KVM in Linux. I've never tested it
> and I don't know how much it's used at all. Probably should keep it
> working here if possible. I'll look into default_caps_with_cpu().
> 
> Thanks,
> Nick
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] spapr: Add SPAPR_CAP_AIL_MODE_3 for AIL mode 3 support for H_SET_MODE hcall

[Cédric Le Goater]

>> If we do, we should probably change the
>> default value for this cap based on cpu model in
>> default_caps_with_cpu().
> 
> We allegedly still support POWER7 KVM in Linux. I've never tested it
> and I don't know how much it's used at all. Probably should keep it
> working here if possible. I'll look into default_caps_with_cpu().

AFAICT, KVM has been very much broken on these sytems for a while.

We still have an OPAL POWER7 system running a debian11 in the labs
if you want to check. But, honestly, I wouldn't worry too much about
it. POWER8 and above should be the focus. Anything below: POWER7,
POWER5+, 970 work well enough under TCG.


Thanks,

C.