Re: [PATCH v2 1/1] spapr_caps.c: check user input before warning about TCG only caps

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6337 bytes --]

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();
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]