Re: [PATCH] tools/cpuid: Plumb nested_virt down into xc_cpuid_apply_policy()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.09.2020 15:48, Andrew Cooper wrote:
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -427,7 +427,7 @@ static int xc_cpuid_xend_policy(
>  
>  int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>                            const uint32_t *featureset, unsigned int 
> nr_features,
> -                          bool pae, bool itsc,
> +                          bool pae, bool itsc, bool nested_virt,

This increasing number of bools next to each other bears an
increasing risk of callers getting the order wrong. Luckily
there's just one within the tree, so perhaps not an immediate
problem.

> @@ -559,7 +559,11 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t 
> domid, bool restore,
>          p->extd.itsc = itsc;
>  
>          if ( di.hvm )
> +        {
>              p->basic.pae = pae;
> +            p->basic.vmx = nested_virt;
> +            p->extd.svm = nested_virt;
> +        }
>      }
>  
>      if ( !di.hvm )
> @@ -625,14 +629,6 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t 
> domid, bool restore,
>              }
>              break;
>          }
> -
> -        /*
> -         * These settings are necessary to cause earlier HVM_PARAM_NESTEDHVM
> -         * to be reflected correctly in CPUID.  Xen will discard these bits 
> if
> -         * configuration hasn't been set for the domain.
> -         */
> -        p->basic.vmx = true;
> -        p->extd.svm = true;
>      }

While I can see how the first sentence in the comment has become
irrelevant now, what about the second? It's still odd to see both
svm and vmx getting set to identical values. Therefore perhaps
worth to retain a shorter comment there?

Jan