[Qemu-devel] [QEMU-PPC] [PATCH V4 1/2] ppc/spapr-caps: Disallow setting workaround for spapr-cap-ibs

[Qemu-devel] [QEMU-PPC] [PATCH V4 1/2] ppc/spapr-caps: Disallow setting workaround for spapr-cap-ibs

[Suraj Jitindar Singh]

The spapr-cap cap-ibs can only have values broken or fixed as there is
no explicit workaround required. Currently setting the value workaround
for this cap will hit an assert if the guest makes the hcall
h_get_cpu_characteristics.

Report an error when attempting to apply the setting with a more helpful
error message.

Reported-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>

---

V3 -> V4:
 - Add this patch back from V1 to replace
   "ppc/spapr-caps: Convert spapr-cap-ibs to be a boolean"
   as this was deemed to be a better solution
---
 hw/ppc/spapr_caps.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index e69d308560..99a4b71d19 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -205,7 +205,9 @@ static void cap_safe_bounds_check_apply(sPAPRMachineState *spapr, uint8_t val,
 static void cap_safe_indirect_branch_apply(sPAPRMachineState *spapr,
                                            uint8_t val, Error **errp)
 {
-    if (tcg_enabled() && val) {
+    if (val == SPAPR_CAP_WORKAROUND) { /* Can only be Broken or Fixed */
+        error_setg(errp, "Requested safe indirect branch capability level \"workaround\" not valid, try cap-ibs=fixed");
+    } else if (tcg_enabled() && val) {
         /* TODO - for now only allow broken for TCG */
         error_setg(errp, "Requested safe indirect branch capability level not supported by tcg, try a different value for cap-ibs");
     } else if (kvm_enabled() && (val > kvmppc_get_cap_safe_indirect_branch())) {
@@ -263,7 +265,7 @@ sPAPRCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
     },
     [SPAPR_CAP_IBS] = {
         .name = "ibs",
-        .description = "Indirect Branch Serialisation" VALUE_DESC_TRISTATE,
+        .description = "Indirect Branch Serialisation (broken, fixed)",
         .index = SPAPR_CAP_IBS,
         .get = spapr_cap_get_tristate,
         .set = spapr_cap_set_tristate,
-- 
2.13.6

[Qemu-devel] [QEMU-PPC] [PATCH V4 2/2] ppc/spapr-caps: For pseries-2.12 change spapr-cap defaults

[Suraj Jitindar Singh]

For the pseries-2.12 machine type, make the spapr-caps SPAPR_CAP_CFPC
and SPAPR_CAP_SBBC default to workaround. Thus if the host is capable
the guest will be able to take advantage of these workarounds by default.
Otherwise if the host doesn't have these capabilities qemu will fail to
start and they will have to be explicitly disabled on the command line
with:
-machine pseries,cap-cfpc=broken,cap-sbbc=broken

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>

---

V2 -> V3:
 - Set caps to workaround in the class default rather than the
   pseries-2.12 initialiser.
---
 hw/ppc/spapr.c      |  6 ++++--
 hw/ppc/spapr_caps.c | 10 ++++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 83c9d66dd5..69f59aabf1 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3915,8 +3915,8 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
     smc->default_caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_ON;
     smc->default_caps.caps[SPAPR_CAP_DFP] = SPAPR_CAP_ON;
-    smc->default_caps.caps[SPAPR_CAP_CFPC] = SPAPR_CAP_BROKEN;
-    smc->default_caps.caps[SPAPR_CAP_SBBC] = SPAPR_CAP_BROKEN;
+    smc->default_caps.caps[SPAPR_CAP_CFPC] = SPAPR_CAP_WORKAROUND;
+    smc->default_caps.caps[SPAPR_CAP_SBBC] = SPAPR_CAP_WORKAROUND;
     smc->default_caps.caps[SPAPR_CAP_IBS] = SPAPR_CAP_BROKEN;
     spapr_caps_add_properties(smc, &error_abort);
 }
@@ -4000,6 +4000,8 @@ static void spapr_machine_2_11_class_options(MachineClass *mc)
 
     spapr_machine_2_12_class_options(mc);
     smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_ON;
+    smc->default_caps.caps[SPAPR_CAP_CFPC] = SPAPR_CAP_BROKEN;
+    smc->default_caps.caps[SPAPR_CAP_SBBC] = SPAPR_CAP_BROKEN;
     SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_11);
 }
 
diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index 99a4b71d19..7b0ecb3eca 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -283,11 +283,21 @@ static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
 
     caps = smc->default_caps;
 
+    if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_00,
+                          0, spapr->max_compat_pvr)) {
+        caps.caps[SPAPR_CAP_CFPC] = SPAPR_CAP_BROKEN;
+    }
+
     if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_2_07,
                           0, spapr->max_compat_pvr)) {
         caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
     }
 
+    if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_2_06_PLUS,
+                          0, spapr->max_compat_pvr)) {
+        caps.caps[SPAPR_CAP_SBBC] = SPAPR_CAP_BROKEN;
+    }
+
     if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_2_06,
                           0, spapr->max_compat_pvr)) {
         caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_OFF;
-- 
2.13.6

Re: [Qemu-devel] [QEMU-PPC] [PATCH V4 2/2] ppc/spapr-caps: For pseries-2.12 change spapr-cap defaults

[David Gibson]

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

On Fri, Feb 16, 2018 at 01:33:28PM +1100, Suraj Jitindar Singh wrote:
> For the pseries-2.12 machine type, make the spapr-caps SPAPR_CAP_CFPC
> and SPAPR_CAP_SBBC default to workaround. Thus if the host is capable
> the guest will be able to take advantage of these workarounds by default.
> Otherwise if the host doesn't have these capabilities qemu will fail to
> start and they will have to be explicitly disabled on the command line
> with:
> -machine pseries,cap-cfpc=broken,cap-sbbc=broken
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>

I've decided to hold off on this for a little bit longer for two
reasons: 1) I realised how badly it will break things for machines
which don't yet updated firmware (both POWER8 and POWER9) which
includes a very large proportion of test machines.  2) I've looked
more closely at what x86 has done.  For themthe mitigations are
controlled by CPU options, not machine level, but the "plain" types
are still the unmitigated variants, with variants for the versions
with microcode updated with mitigations.  See
https://www.qemu.org/2018/02/14/qemu-2-11-1-and-spectre-update/

> 
> ---
> 
> V2 -> V3:
>  - Set caps to workaround in the class default rather than the
>    pseries-2.12 initialiser.
> ---
>  hw/ppc/spapr.c      |  6 ++++--
>  hw/ppc/spapr_caps.c | 10 ++++++++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 83c9d66dd5..69f59aabf1 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3915,8 +3915,8 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
>      smc->default_caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_ON;
>      smc->default_caps.caps[SPAPR_CAP_DFP] = SPAPR_CAP_ON;
> -    smc->default_caps.caps[SPAPR_CAP_CFPC] = SPAPR_CAP_BROKEN;
> -    smc->default_caps.caps[SPAPR_CAP_SBBC] = SPAPR_CAP_BROKEN;
> +    smc->default_caps.caps[SPAPR_CAP_CFPC] = SPAPR_CAP_WORKAROUND;
> +    smc->default_caps.caps[SPAPR_CAP_SBBC] = SPAPR_CAP_WORKAROUND;
>      smc->default_caps.caps[SPAPR_CAP_IBS] = SPAPR_CAP_BROKEN;
>      spapr_caps_add_properties(smc, &error_abort);
>  }
> @@ -4000,6 +4000,8 @@ static void spapr_machine_2_11_class_options(MachineClass *mc)
>  
>      spapr_machine_2_12_class_options(mc);
>      smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_ON;
> +    smc->default_caps.caps[SPAPR_CAP_CFPC] = SPAPR_CAP_BROKEN;
> +    smc->default_caps.caps[SPAPR_CAP_SBBC] = SPAPR_CAP_BROKEN;
>      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_11);
>  }
>  
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 99a4b71d19..7b0ecb3eca 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -283,11 +283,21 @@ static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
>  
>      caps = smc->default_caps;
>  
> +    if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_00,
> +                          0, spapr->max_compat_pvr)) {
> +        caps.caps[SPAPR_CAP_CFPC] = SPAPR_CAP_BROKEN;
> +    }
> +
>      if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_2_07,
>                            0, spapr->max_compat_pvr)) {
>          caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
>      }
>  
> +    if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_2_06_PLUS,
> +                          0, spapr->max_compat_pvr)) {
> +        caps.caps[SPAPR_CAP_SBBC] = SPAPR_CAP_BROKEN;
> +    }
> +
>      if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_2_06,
>                            0, spapr->max_compat_pvr)) {
>          caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_OFF;

-- 
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: [Qemu-devel] [QEMU-PPC] [PATCH V4 1/2] ppc/spapr-caps: Disallow setting workaround for spapr-cap-ibs

[David Gibson]

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

On Fri, Feb 16, 2018 at 01:33:27PM +1100, Suraj Jitindar Singh wrote:
> The spapr-cap cap-ibs can only have values broken or fixed as there is
> no explicit workaround required. Currently setting the value workaround
> for this cap will hit an assert if the guest makes the hcall
> h_get_cpu_characteristics.
> 
> Report an error when attempting to apply the setting with a more helpful
> error message.
> 
> Reported-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>

Applied, thanks.

> 
> ---
> 
> V3 -> V4:
>  - Add this patch back from V1 to replace
>    "ppc/spapr-caps: Convert spapr-cap-ibs to be a boolean"
>    as this was deemed to be a better solution
> ---
>  hw/ppc/spapr_caps.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index e69d308560..99a4b71d19 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -205,7 +205,9 @@ static void cap_safe_bounds_check_apply(sPAPRMachineState *spapr, uint8_t val,
>  static void cap_safe_indirect_branch_apply(sPAPRMachineState *spapr,
>                                             uint8_t val, Error **errp)
>  {
> -    if (tcg_enabled() && val) {
> +    if (val == SPAPR_CAP_WORKAROUND) { /* Can only be Broken or Fixed */
> +        error_setg(errp, "Requested safe indirect branch capability level \"workaround\" not valid, try cap-ibs=fixed");
> +    } else if (tcg_enabled() && val) {
>          /* TODO - for now only allow broken for TCG */
>          error_setg(errp, "Requested safe indirect branch capability level not supported by tcg, try a different value for cap-ibs");
>      } else if (kvm_enabled() && (val > kvmppc_get_cap_safe_indirect_branch())) {
> @@ -263,7 +265,7 @@ sPAPRCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>      },
>      [SPAPR_CAP_IBS] = {
>          .name = "ibs",
> -        .description = "Indirect Branch Serialisation" VALUE_DESC_TRISTATE,
> +        .description = "Indirect Branch Serialisation (broken, fixed)",
>          .index = SPAPR_CAP_IBS,
>          .get = spapr_cap_get_tristate,
>          .set = spapr_cap_set_tristate,

-- 
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 --]