Re: [PATCH v2 1/1] spapr_caps.c: check user input before warning about TCG only caps
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
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))
Re: [PATCH v2 1/1] spapr_caps.c: check user input before warning about TCG only caps
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
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() &&
Re: [PATCH v2 1/1] spapr_caps.c: check user input before warning about TCG only caps
On Wed, 20 Jan 2021 07:54:06 -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
> ---
Heh I've just posted a mail suggesting you to do something like that :)
Reviewed-by: Greg Kurz
> 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
> +
[PATCH v2 1/1] spapr_caps.c: check user input before warning about TCG only caps
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
---
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();
--
2.26.2
