| Message ID | 20210120105406.163074-2-danielhb413@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spapr_caps.c: check user input before warning about TCG only caps | expand |
On Wed, 20 Jan 2021 07:54:06 -0300 Daniel Henrique Barboza <danielhb413@gmail.com> 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 these caps were set in the command line before sending an user > warning. > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- Heh I've just posted a mail suggesting you to do something like that :) Reviewed-by: Greg Kurz <groug@kaod.org> > hw/ppc/spapr_caps.c | 47 ++++++++++++++++++++++++++++++++++----------- > 1 file changed, 36 insertions(+), 11 deletions(-) > > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c > index 9341e9782a..629c24a96d 100644 > --- a/hw/ppc/spapr_caps.c > +++ b/hw/ppc/spapr_caps.c > @@ -244,9 +244,15 @@ static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, > uint8_t kvm_val = kvmppc_get_cap_safe_cache(); > > if (tcg_enabled() && val) { > - /* 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]); > + /* > + * TCG only supports broken, allow other values and print a warning > + * in case the user attempted to set a different value in the command > + * line. > + */ > + if (spapr->cmd_line_caps[SPAPR_CAP_CFPC] != SPAPR_CAP_BROKEN) { > + warn_report("TCG doesn't support requested feature, cap-cfpc=%s", > + cap_cfpc_possible.vals[val]); > + } > } else if (kvm_enabled() && (val > kvm_val)) { > error_setg(errp, > "Requested safe cache capability level not supported by KVM"); > @@ -269,9 +275,15 @@ static void cap_safe_bounds_check_apply(SpaprMachineState *spapr, uint8_t val, > uint8_t kvm_val = kvmppc_get_cap_safe_bounds_check(); > > if (tcg_enabled() && val) { > - /* TCG only supports broken, allow other values and print a warning */ > - warn_report("TCG doesn't support requested feature, cap-sbbc=%s", > - cap_sbbc_possible.vals[val]); > + /* > + * TCG only supports broken, allow other values and print a warning > + * in case the user attempted to set a different value in the command > + * line. > + */ > + if (spapr->cmd_line_caps[SPAPR_CAP_SBBC] != SPAPR_CAP_BROKEN) { > + warn_report("TCG doesn't support requested feature, cap-sbbc=%s", > + cap_sbbc_possible.vals[val]); > + } > } else if (kvm_enabled() && (val > kvm_val)) { > error_setg(errp, > "Requested safe bounds check capability level not supported by KVM"); > @@ -297,9 +309,15 @@ static void cap_safe_indirect_branch_apply(SpaprMachineState *spapr, > uint8_t kvm_val = kvmppc_get_cap_safe_indirect_branch(); > > if (tcg_enabled() && val) { > - /* TCG only supports broken, allow other values and print a warning */ > - warn_report("TCG doesn't support requested feature, cap-ibs=%s", > - cap_ibs_possible.vals[val]); > + /* > + * TCG only supports broken, allow other values and print a warning > + * in case the user attempted to set a different value in the command > + * line. > + */ > + if (spapr->cmd_line_caps[SPAPR_CAP_IBS] != SPAPR_CAP_BROKEN) { > + warn_report("TCG doesn't support requested feature, cap-ibs=%s", > + cap_ibs_possible.vals[val]); > + } > } else if (kvm_enabled() && (val > kvm_val)) { > error_setg(errp, > "Requested safe indirect branch capability level not supported by KVM"); > @@ -483,8 +501,15 @@ static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val, > uint8_t kvm_val = kvmppc_get_cap_count_cache_flush_assist(); > > if (tcg_enabled() && val) { > - /* TCG doesn't implement anything here, but allow with a warning */ > - warn_report("TCG doesn't support requested feature, cap-ccf-assist=on"); > + /* > + * TCG doesn't implement anything here, but allow with a warning > + * in case the user attempted to set a different value in the command > + * line. > + */ > + if (spapr->cmd_line_caps[SPAPR_CAP_CCF_ASSIST] != SPAPR_CAP_OFF) { > + warn_report("TCG doesn't support requested feature, " > + "cap-ccf-assist=on"); > + } > } else if (kvm_enabled() && (val > kvm_val)) { > uint8_t kvm_ibs = kvmppc_get_cap_safe_indirect_branch(); >
On Wed, Jan 20, 2021 at 07:54:06AM -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 these caps were set in the command line before sending an user > warning. > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> Oof. I have real mixed feelings about this. So, yes, the warnings are annoying, but they're not meaningless. They are really indicating that the guest environment is different from the one you requested (implicitly, via the machine version). The fact that they are only warnings, not hard errors, is already a compromise because otherwise there would be no real way to use TCG at all with current machines. In short, the warnings are scary because they're *meant* to be scary. TCG will not, and cannot, supply the Spectre mitigations that are expected on a current machine type. I agree that the current behaviour is pretty irritating, but I don't know that silently pretending TCG can do what's normally expected of that command line is a great option either. > --- > hw/ppc/spapr_caps.c | 47 ++++++++++++++++++++++++++++++++++----------- > 1 file changed, 36 insertions(+), 11 deletions(-) > > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c > index 9341e9782a..629c24a96d 100644 > --- a/hw/ppc/spapr_caps.c > +++ b/hw/ppc/spapr_caps.c > @@ -244,9 +244,15 @@ static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, > uint8_t kvm_val = kvmppc_get_cap_safe_cache(); > > if (tcg_enabled() && val) { > - /* 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]); > + /* > + * TCG only supports broken, allow other values and print a warning > + * in case the user attempted to set a different value in the command > + * line. > + */ > + if (spapr->cmd_line_caps[SPAPR_CAP_CFPC] != SPAPR_CAP_BROKEN) { > + warn_report("TCG doesn't support requested feature, cap-cfpc=%s", > + cap_cfpc_possible.vals[val]); > + } > } else if (kvm_enabled() && (val > kvm_val)) { > error_setg(errp, > "Requested safe cache capability level not supported by KVM"); > @@ -269,9 +275,15 @@ static void cap_safe_bounds_check_apply(SpaprMachineState *spapr, uint8_t val, > uint8_t kvm_val = kvmppc_get_cap_safe_bounds_check(); > > if (tcg_enabled() && val) { > - /* TCG only supports broken, allow other values and print a warning */ > - warn_report("TCG doesn't support requested feature, cap-sbbc=%s", > - cap_sbbc_possible.vals[val]); > + /* > + * TCG only supports broken, allow other values and print a warning > + * in case the user attempted to set a different value in the command > + * line. > + */ > + if (spapr->cmd_line_caps[SPAPR_CAP_SBBC] != SPAPR_CAP_BROKEN) { > + warn_report("TCG doesn't support requested feature, cap-sbbc=%s", > + cap_sbbc_possible.vals[val]); > + } > } else if (kvm_enabled() && (val > kvm_val)) { > error_setg(errp, > "Requested safe bounds check capability level not supported by KVM"); > @@ -297,9 +309,15 @@ static void cap_safe_indirect_branch_apply(SpaprMachineState *spapr, > uint8_t kvm_val = kvmppc_get_cap_safe_indirect_branch(); > > if (tcg_enabled() && val) { > - /* TCG only supports broken, allow other values and print a warning */ > - warn_report("TCG doesn't support requested feature, cap-ibs=%s", > - cap_ibs_possible.vals[val]); > + /* > + * TCG only supports broken, allow other values and print a warning > + * in case the user attempted to set a different value in the command > + * line. > + */ > + if (spapr->cmd_line_caps[SPAPR_CAP_IBS] != SPAPR_CAP_BROKEN) { > + warn_report("TCG doesn't support requested feature, cap-ibs=%s", > + cap_ibs_possible.vals[val]); > + } > } else if (kvm_enabled() && (val > kvm_val)) { > error_setg(errp, > "Requested safe indirect branch capability level not supported by KVM"); > @@ -483,8 +501,15 @@ static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val, > uint8_t kvm_val = kvmppc_get_cap_count_cache_flush_assist(); > > if (tcg_enabled() && val) { > - /* TCG doesn't implement anything here, but allow with a warning */ > - warn_report("TCG doesn't support requested feature, cap-ccf-assist=on"); > + /* > + * TCG doesn't implement anything here, but allow with a warning > + * in case the user attempted to set a different value in the command > + * line. > + */ > + if (spapr->cmd_line_caps[SPAPR_CAP_CCF_ASSIST] != SPAPR_CAP_OFF) { > + warn_report("TCG doesn't support requested feature, " > + "cap-ccf-assist=on"); > + } > } else if (kvm_enabled() && (val > kvm_val)) { > uint8_t kvm_ibs = kvmppc_get_cap_safe_indirect_branch(); >
On 1/22/21 10:46 PM, David Gibson wrote: > On Wed, Jan 20, 2021 at 07:54:06AM -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 these caps were set in the command line before sending an user >> warning. >> >> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > > Oof. I have real mixed feelings about this. > > So, yes, the warnings are annoying, but they're not meaningless. They > are really indicating that the guest environment is different from the > one you requested (implicitly, via the machine version). The fact that > they are only warnings, not hard errors, is already a compromise > because otherwise there would be no real way to use TCG at all with > current machines. > > In short, the warnings are scary because they're *meant* to be scary. > TCG will not, and cannot, supply the Spectre mitigations that are > expected on a current machine type. Quick story: I'm involved in helping folks in a local Brazilian college working with QEMU in Power ([1] for more info). One user is trying to run a pseries TCG guest in Windows 10, and he is having problems with the current pseries machine, while we is still able to do it with the pseries-2.8 one (I'm actually surprised that it works at all in Windows 10 TBH). So this user ask me for help with this scenario because he didn't know how to fix these TCG warnings, because he was thinking that they had something to do with the problem he is having with his use case. I said that these warnings could be safely ignored for TCG. These warnings are indeed scary, as you said. But not in a helpful way. Consider that most QEMU warnings are a call for action for the user to fix something, e.g. an option that's about to be deprecated/no longer supported. In this case we're warning the user of something that the user has no fault on, and more important, can do nothing about it but to ignore. And this user interaction I had made me realize that it's not trivial to ignore warnings when your use case is not working as intended. You will attempt to fix the warnings before trying to open a developer bug and so on. What we're doing here I can call a 'developer warning', something to remind us, developers, that TCG does not implement Spectre caps that are default in the pseries machine. Well, I'd rather document somewhere (in tcg/README, or perhaps create a hw/ppc/README since this is a pseries exclusive behavior) that TCG is ignoring default Spectre caps of the pseries machine, than to issue warnings about it. > > I agree that the current behaviour is pretty irritating, but I don't > know that silently pretending TCG can do what's normally expected of > that command line is a great option either. I can send a patch to change the messages to say something like "this can be safely ignored. Use -machine cap-X=broken to hid it". At least we will inform TCG users that these warning are not their fault and they shouldn't spend their time trying to figure them out. But then, why issue a warning and tell the user "this is warning, please ignore me"? Thanks, DHB [1] https://openpower.ic.unicamp.br/minicloud/ > > >> --- >> hw/ppc/spapr_caps.c | 47 ++++++++++++++++++++++++++++++++++----------- >> 1 file changed, 36 insertions(+), 11 deletions(-) >> >> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c >> index 9341e9782a..629c24a96d 100644 >> --- a/hw/ppc/spapr_caps.c >> +++ b/hw/ppc/spapr_caps.c >> @@ -244,9 +244,15 @@ static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, >> uint8_t kvm_val = kvmppc_get_cap_safe_cache(); >> >> if (tcg_enabled() && val) { >> - /* 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]); >> + /* >> + * TCG only supports broken, allow other values and print a warning >> + * in case the user attempted to set a different value in the command >> + * line. >> + */ >> + if (spapr->cmd_line_caps[SPAPR_CAP_CFPC] != SPAPR_CAP_BROKEN) { >> + warn_report("TCG doesn't support requested feature, cap-cfpc=%s", >> + cap_cfpc_possible.vals[val]); >> + } >> } else if (kvm_enabled() && (val > kvm_val)) { >> error_setg(errp, >> "Requested safe cache capability level not supported by KVM"); >> @@ -269,9 +275,15 @@ static void cap_safe_bounds_check_apply(SpaprMachineState *spapr, uint8_t val, >> uint8_t kvm_val = kvmppc_get_cap_safe_bounds_check(); >> >> if (tcg_enabled() && val) { >> - /* TCG only supports broken, allow other values and print a warning */ >> - warn_report("TCG doesn't support requested feature, cap-sbbc=%s", >> - cap_sbbc_possible.vals[val]); >> + /* >> + * TCG only supports broken, allow other values and print a warning >> + * in case the user attempted to set a different value in the command >> + * line. >> + */ >> + if (spapr->cmd_line_caps[SPAPR_CAP_SBBC] != SPAPR_CAP_BROKEN) { >> + warn_report("TCG doesn't support requested feature, cap-sbbc=%s", >> + cap_sbbc_possible.vals[val]); >> + } >> } else if (kvm_enabled() && (val > kvm_val)) { >> error_setg(errp, >> "Requested safe bounds check capability level not supported by KVM"); >> @@ -297,9 +309,15 @@ static void cap_safe_indirect_branch_apply(SpaprMachineState *spapr, >> uint8_t kvm_val = kvmppc_get_cap_safe_indirect_branch(); >> >> if (tcg_enabled() && val) { >> - /* TCG only supports broken, allow other values and print a warning */ >> - warn_report("TCG doesn't support requested feature, cap-ibs=%s", >> - cap_ibs_possible.vals[val]); >> + /* >> + * TCG only supports broken, allow other values and print a warning >> + * in case the user attempted to set a different value in the command >> + * line. >> + */ >> + if (spapr->cmd_line_caps[SPAPR_CAP_IBS] != SPAPR_CAP_BROKEN) { >> + warn_report("TCG doesn't support requested feature, cap-ibs=%s", >> + cap_ibs_possible.vals[val]); >> + } >> } else if (kvm_enabled() && (val > kvm_val)) { >> error_setg(errp, >> "Requested safe indirect branch capability level not supported by KVM"); >> @@ -483,8 +501,15 @@ static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val, >> uint8_t kvm_val = kvmppc_get_cap_count_cache_flush_assist(); >> >> if (tcg_enabled() && val) { >> - /* TCG doesn't implement anything here, but allow with a warning */ >> - warn_report("TCG doesn't support requested feature, cap-ccf-assist=on"); >> + /* >> + * TCG doesn't implement anything here, but allow with a warning >> + * in case the user attempted to set a different value in the command >> + * line. >> + */ >> + if (spapr->cmd_line_caps[SPAPR_CAP_CCF_ASSIST] != SPAPR_CAP_OFF) { >> + warn_report("TCG doesn't support requested feature, " >> + "cap-ccf-assist=on"); >> + } >> } else if (kvm_enabled() && (val > kvm_val)) { >> uint8_t kvm_ibs = kvmppc_get_cap_safe_indirect_branch(); >> >
diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c index 9341e9782a..629c24a96d 100644 --- a/hw/ppc/spapr_caps.c +++ b/hw/ppc/spapr_caps.c @@ -244,9 +244,15 @@ static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, uint8_t kvm_val = kvmppc_get_cap_safe_cache(); if (tcg_enabled() && val) { - /* 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]); + /* + * TCG only supports broken, allow other values and print a warning + * in case the user attempted to set a different value in the command + * line. + */ + if (spapr->cmd_line_caps[SPAPR_CAP_CFPC] != SPAPR_CAP_BROKEN) { + warn_report("TCG doesn't support requested feature, cap-cfpc=%s", + cap_cfpc_possible.vals[val]); + } } else if (kvm_enabled() && (val > kvm_val)) { error_setg(errp, "Requested safe cache capability level not supported by KVM"); @@ -269,9 +275,15 @@ static void cap_safe_bounds_check_apply(SpaprMachineState *spapr, uint8_t val, uint8_t kvm_val = kvmppc_get_cap_safe_bounds_check(); if (tcg_enabled() && val) { - /* TCG only supports broken, allow other values and print a warning */ - warn_report("TCG doesn't support requested feature, cap-sbbc=%s", - cap_sbbc_possible.vals[val]); + /* + * TCG only supports broken, allow other values and print a warning + * in case the user attempted to set a different value in the command + * line. + */ + if (spapr->cmd_line_caps[SPAPR_CAP_SBBC] != SPAPR_CAP_BROKEN) { + warn_report("TCG doesn't support requested feature, cap-sbbc=%s", + cap_sbbc_possible.vals[val]); + } } else if (kvm_enabled() && (val > kvm_val)) { error_setg(errp, "Requested safe bounds check capability level not supported by KVM"); @@ -297,9 +309,15 @@ static void cap_safe_indirect_branch_apply(SpaprMachineState *spapr, uint8_t kvm_val = kvmppc_get_cap_safe_indirect_branch(); if (tcg_enabled() && val) { - /* TCG only supports broken, allow other values and print a warning */ - warn_report("TCG doesn't support requested feature, cap-ibs=%s", - cap_ibs_possible.vals[val]); + /* + * TCG only supports broken, allow other values and print a warning + * in case the user attempted to set a different value in the command + * line. + */ + if (spapr->cmd_line_caps[SPAPR_CAP_IBS] != SPAPR_CAP_BROKEN) { + warn_report("TCG doesn't support requested feature, cap-ibs=%s", + cap_ibs_possible.vals[val]); + } } else if (kvm_enabled() && (val > kvm_val)) { error_setg(errp, "Requested safe indirect branch capability level not supported by KVM"); @@ -483,8 +501,15 @@ static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val, uint8_t kvm_val = kvmppc_get_cap_count_cache_flush_assist(); if (tcg_enabled() && val) { - /* TCG doesn't implement anything here, but allow with a warning */ - warn_report("TCG doesn't support requested feature, cap-ccf-assist=on"); + /* + * TCG doesn't implement anything here, but allow with a warning + * in case the user attempted to set a different value in the command + * line. + */ + if (spapr->cmd_line_caps[SPAPR_CAP_CCF_ASSIST] != SPAPR_CAP_OFF) { + warn_report("TCG doesn't support requested feature, " + "cap-ccf-assist=on"); + } } else if (kvm_enabled() && (val > kvm_val)) { uint8_t kvm_ibs = kvmppc_get_cap_safe_indirect_branch();
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 these caps were set in the command line before sending an user warning. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- hw/ppc/spapr_caps.c | 47 ++++++++++++++++++++++++++++++++++----------- 1 file changed, 36 insertions(+), 11 deletions(-)