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

[Greg Kurz <groug@kaod.org>]

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