| Message ID | 20210119205824.2222801-1-danielhb413@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] spapr_caps.c: disable KVM specific caps when running with TCG | expand |
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];
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]; >
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];
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(+)