[PATCH 1/1] spapr_caps.c: disable KVM specific caps when running with TCG

[PATCH 1/1] spapr_caps.c: disable KVM specific caps when running with TCG

[Daniel Henrique Barboza]

Commit 006e9d361869 added warning messages for cap-cfpc, cap-ibs and
cap-sbbc when enabled under TCG. Commit 8ff43ee404d3 did the same thing
when introducing cap-ccf-assist.

These warning messages, although benign to the machine launch, can make
users a bit confused. E.g:

$ sudo ./ppc64-softmmu/qemu-system-ppc64
qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-cfpc=workaround
qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-sbbc=workaround
qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-ibs=workaround
qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-ccf-assist=on

We're complaining about "TCG doesn't support requested feature" when the
user didn't request any of those caps in the command line.

Check if we're running with TCG and change the defaults in spapr_caps_init().
Note that this change doesn't impact backward compatibility or migration
to older QEMU versions because we never activated these caps with TCG
in the first place.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_caps.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index 9341e9782a..53eea2b11e 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -781,6 +781,21 @@ void spapr_caps_init(SpaprMachineState *spapr)
     /* Compute the actual set of caps we should run with */
     default_caps = default_caps_with_cpu(spapr, MACHINE(spapr)->cpu_type);
 
+   /*
+    * These are KVM specific caps that TCG doesn't support, but will
+    * throw an warning if enabled by default (see 006e9d361869 and
+    * 8ff43ee404d3). This behavior can make the user wonder why a warning
+    * is being shown for caps that the user didn't enable in the
+    * command line.
+    *
+    * Disable them for TCG. */
+    if (tcg_enabled()) {
+        default_caps.caps[SPAPR_CAP_CFPC] = SPAPR_CAP_BROKEN;
+        default_caps.caps[SPAPR_CAP_SBBC] = SPAPR_CAP_BROKEN;
+        default_caps.caps[SPAPR_CAP_IBS] = SPAPR_CAP_BROKEN;
+        default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_BROKEN;
+    }
+
     for (i = 0; i < SPAPR_CAP_NUM; i++) {
         /* Store the defaults */
         spapr->def.caps[i] = default_caps.caps[i];
-- 
2.26.2

Re: [PATCH 1/1] spapr_caps.c: disable KVM specific caps when running with TCG

[David Gibson]

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

On Tue, Jan 19, 2021 at 05:58:24PM -0300, Daniel Henrique Barboza wrote:
> Commit 006e9d361869 added warning messages for cap-cfpc, cap-ibs and
> cap-sbbc when enabled under TCG. Commit 8ff43ee404d3 did the same thing
> when introducing cap-ccf-assist.
> 
> These warning messages, although benign to the machine launch, can make
> users a bit confused. E.g:
> 
> $ sudo ./ppc64-softmmu/qemu-system-ppc64
> qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-cfpc=workaround
> qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-sbbc=workaround
> qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-ibs=workaround
> qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-ccf-assist=on
> 
> We're complaining about "TCG doesn't support requested feature" when the
> user didn't request any of those caps in the command line.
> 
> Check if we're running with TCG and change the defaults in spapr_caps_init().
> Note that this change doesn't impact backward compatibility or migration
> to older QEMU versions because we never activated these caps with TCG
> in the first place.

Nack.  Changing those capabilities changes guest visible properties of
the guest environment.  Silently altering guest visible
characteristics based on whether or not we're running with KVM is not
acceptable (we did it in the past and it caused a lot of grief).

> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  hw/ppc/spapr_caps.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 9341e9782a..53eea2b11e 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -781,6 +781,21 @@ void spapr_caps_init(SpaprMachineState *spapr)
>      /* Compute the actual set of caps we should run with */
>      default_caps = default_caps_with_cpu(spapr, MACHINE(spapr)->cpu_type);
>  
> +   /*
> +    * These are KVM specific caps that TCG doesn't support, but will
> +    * throw an warning if enabled by default (see 006e9d361869 and
> +    * 8ff43ee404d3). This behavior can make the user wonder why a warning
> +    * is being shown for caps that the user didn't enable in the
> +    * command line.
> +    *
> +    * Disable them for TCG. */
> +    if (tcg_enabled()) {
> +        default_caps.caps[SPAPR_CAP_CFPC] = SPAPR_CAP_BROKEN;
> +        default_caps.caps[SPAPR_CAP_SBBC] = SPAPR_CAP_BROKEN;
> +        default_caps.caps[SPAPR_CAP_IBS] = SPAPR_CAP_BROKEN;
> +        default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_BROKEN;
> +    }
> +
>      for (i = 0; i < SPAPR_CAP_NUM; i++) {
>          /* Store the defaults */
>          spapr->def.caps[i] = default_caps.caps[i];

-- 
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/1] spapr_caps.c: disable KVM specific caps when running with TCG

[Greg Kurz]

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

On Wed, 20 Jan 2021 11:24:45 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Jan 19, 2021 at 05:58:24PM -0300, Daniel Henrique Barboza wrote:
> > Commit 006e9d361869 added warning messages for cap-cfpc, cap-ibs and
> > cap-sbbc when enabled under TCG. Commit 8ff43ee404d3 did the same thing
> > when introducing cap-ccf-assist.
> > 
> > These warning messages, although benign to the machine launch, can make
> > users a bit confused. E.g:
> > 
> > $ sudo ./ppc64-softmmu/qemu-system-ppc64
> > qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-cfpc=workaround
> > qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-sbbc=workaround
> > qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-ibs=workaround
> > qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-ccf-assist=on
> > 
> > We're complaining about "TCG doesn't support requested feature" when the
> > user didn't request any of those caps in the command line.
> > 
> > Check if we're running with TCG and change the defaults in spapr_caps_init().
> > Note that this change doesn't impact backward compatibility or migration
> > to older QEMU versions because we never activated these caps with TCG
> > in the first place.
> 
> Nack.  Changing those capabilities changes guest visible properties of
> the guest environment.  Silently altering guest visible
> characteristics based on whether or not we're running with KVM is not
> acceptable (we did it in the past and it caused a lot of grief).
> 

I definitely agree with the nack, but I also agree with the
intention behind this patch. Since we know if a capability
was requested from the command line, the warning can be
restricted to this case with something like:

--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -243,7 +243,7 @@ static void cap_safe_cache_apply(SpaprMachineState *spapr, >
     ERRP_GUARD();
     uint8_t kvm_val =  kvmppc_get_cap_safe_cache();
 
-    if (tcg_enabled() && val) {
+    if (tcg_enabled() && val && spapr->cmd_line_caps[SPAPR_CAP_CFPC]) {
         /* TCG only supports broken, allow other values and print a warning */
         warn_report("TCG doesn't support requested feature, cap-cfpc=%s",
                     cap_cfpc_possible.vals[val]);

A further improvement would be to only issue these warnings at
machine init instead of printing them again and again at each
reboot. This should be possible in spapr_caps_init() because
the accelerator has been set and the capabilities have been
parsed already.

> > 
> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > ---
> >  hw/ppc/spapr_caps.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> > index 9341e9782a..53eea2b11e 100644
> > --- a/hw/ppc/spapr_caps.c
> > +++ b/hw/ppc/spapr_caps.c
> > @@ -781,6 +781,21 @@ void spapr_caps_init(SpaprMachineState *spapr)
> >      /* Compute the actual set of caps we should run with */
> >      default_caps = default_caps_with_cpu(spapr, MACHINE(spapr)->cpu_type);
> >  
> > +   /*
> > +    * These are KVM specific caps that TCG doesn't support, but will
> > +    * throw an warning if enabled by default (see 006e9d361869 and
> > +    * 8ff43ee404d3). This behavior can make the user wonder why a warning
> > +    * is being shown for caps that the user didn't enable in the
> > +    * command line.
> > +    *
> > +    * Disable them for TCG. */
> > +    if (tcg_enabled()) {
> > +        default_caps.caps[SPAPR_CAP_CFPC] = SPAPR_CAP_BROKEN;
> > +        default_caps.caps[SPAPR_CAP_SBBC] = SPAPR_CAP_BROKEN;
> > +        default_caps.caps[SPAPR_CAP_IBS] = SPAPR_CAP_BROKEN;
> > +        default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_BROKEN;
> > +    }
> > +
> >      for (i = 0; i < SPAPR_CAP_NUM; i++) {
> >          /* Store the defaults */
> >          spapr->def.caps[i] = default_caps.caps[i];
> 


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