Re: [Xen-devel] [PATCH 08/27] x86/hvm: Dispatch cpuid_viridian_leaves() from guest_cpuid()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.01.17 at 13:39, <andrew.cooper3@xxxxxxxxxx> wrote:
> One check against EFER_SVME is replaced with the more appropriate cpu_has_svm,
> when determining whether MSR bitmaps are available.

I don't think this is correct - start_svm() may fail, in which case
the CPUID flag doesn't get cleared, yet EFER.SVME also doesn't
get set. How about comparing hvm_funcs (if not NULL) ->name
against "SVM"?

> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>

Cc: Paul Durrant <paul.durrant@xxxxxxxxxx>

> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -319,8 +319,21 @@ int init_domain_cpuid_policy(struct domain *d)
>  void guest_cpuid(const struct vcpu *v, unsigned int leaf,
>                   unsigned int subleaf, struct cpuid_leaf *res)
>  {
> +    const struct domain *d = v->domain;
> +
>      *res = EMPTY_LEAF;
>  
> +    /*
> +     * First pass:
> +     * - Dispatch the virtualised leaves to their respective handlers.
> +     */
> +    switch ( leaf )
> +    {
> +    case 0x40000000 ... 0x400000ff:
> +        if ( is_viridian_domain(d) )
> +            return cpuid_viridian_leaves(v, leaf, subleaf, res);
> +    }

Can we please have a break statement above here?

> +void cpuid_viridian_leaves(const struct vcpu *v, unsigned int leaf,
> +                           unsigned int subleaf, struct cpuid_leaf *res)
>  {
> -    struct domain *d = current->domain;
> +    const struct domain *d = v->domain;
>  
> -    if ( !is_viridian_domain(d) )
> -        return 0;
> +    ASSERT(is_viridian_domain(d));
> +    ASSERT(leaf >= 0x40000000 && leaf < 0x40000100);
>  
>      leaf -= 0x40000000;
> -    if ( leaf > 6 )
> -        return 0;
>  
> -    *eax = *ebx = *ecx = *edx = 0;
>      switch ( leaf )
>      {
>      case 0:
> -        *eax = 0x40000006; /* Maximum leaf */
> -        *ebx = 0x7263694d; /* Magic numbers  */
> -        *ecx = 0x666F736F;
> -        *edx = 0x76482074;
> +        res->a = 0x40000006; /* Maximum leaf */
> +        res->b = 0x7263694d; /* Magic numbers  */
> +        res->c = 0x666F736F;
> +        res->d = 0x76482074;
>          break;
> +
>      case 1:
> -        *eax = 0x31237648; /* Version number */
> +        res->a = 0x31237648; /* Version number */
>          break;
> +
>      case 2:
>          /* Hypervisor information, but only if the guest has set its
>             own version number. */
>          if ( d->arch.hvm_domain.viridian.guest_os_id.raw == 0 )
>              break;
> -        *eax = 1; /* Build number */
> -        *ebx = (xen_major_version() << 16) | xen_minor_version();
> -        *ecx = 0; /* SP */
> -        *edx = 0; /* Service branch and number */
> +        res->a = 1; /* Build number */
> +        res->b = (xen_major_version() << 16) | xen_minor_version();

I think the comments warrant the zeroing of ECX and EDX to be
retained.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel