On Wed, 20 Jan 2021 11:24:45 +1100 David Gibson <da...@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 <danielhb...@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];
>
pgpWxUbNAqDDx.pgp
Description: OpenPGP digital signature
