Re: [PATCH v2 1/4] xen/libxc: Allow changes to hypervisor CPUID leaf from config file

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 03/11/2014 06:10 AM, Andrew Cooper wrote:
> On 11/03/14 03:54, Boris Ostrovsky wrote:
>> Currently only "real" cpuid leaves can be overwritten by users via
>> 'cpuid' option in the configuration file. This patch provides ability to
>> do the same for hypervisor leaves (those in the 0x40000000 range).
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> How?  There is nothing stopping leaves in 0x40000000 being set in the
> policy with XEN_DOMCTL_set_cpuid, but I dont see anything which plumbs
> this together at the Xen level.

Right. What this patch mostly provides is ability to query the 
hypervisor (via sysctl) about default values of hypervisor CPUID leaf 
from userspace. We cannot use CPUID instruction here (for dom0). And 
/dev/cpu/<n>/cpuid may not exist.

We then use these values plus whatever the user requested (because the 
user may ask for only one of the 4 registers) to pass to the subsequent 
XEN_DOMCTL_set_cpuid call.

>
>> ---
>>   tools/libxc/xc_cpuid_x86.c   |   23 ++++++++++++++++++++++-
>>   tools/libxc/xc_misc.c        |   18 ++++++++++++++++++
>>   tools/libxc/xenctrl.h        |    2 ++
>>   xen/arch/x86/domain.c        |   19 ++++++++++++++++---
>>   xen/arch/x86/sysctl.c        |   17 +++++++++++++++++
>>   xen/arch/x86/traps.c         |    3 +++
>>   xen/include/asm-x86/domain.h |    7 +++++++
>>   xen/include/public/sysctl.h  |   18 ++++++++++++++++++
>>   8 files changed, 103 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
>> index bbbf9b8..544a0fd 100644
>> --- a/tools/libxc/xc_cpuid_x86.c
>> +++ b/tools/libxc/xc_cpuid_x86.c
>> @@ -33,6 +33,8 @@
>>   #define DEF_MAX_INTELEXT  0x80000008u
>>   #define DEF_MAX_AMDEXT    0x8000001cu
>>   
>> +#define HYPERVISOR_LEAF(idx) (((idx) & 0x40000000) == 0x40000000)
>> +
> This check is wrong.

Because of Viridian leaves? Or something else?

>
>>   static int hypervisor_is_64bit(xc_interface *xch)
>>   {
>>       xen_capabilities_info_t xen_caps = "";
>> @@ -555,6 +557,9 @@ static int xc_cpuid_policy(
>>   {
>>       xc_dominfo_t        info;
>>   
>> +    if ( HYPERVISOR_LEAF(input[0]) )
>> +        return 0;
>> +
>>       if ( xc_domain_getinfo(xch, domid, 1, &info) == 0 )
>>           return -EINVAL;
>>   
>> @@ -754,7 +759,23 @@ int xc_cpuid_set(
>>   
>>       memset(config_transformed, 0, 4 * sizeof(*config_transformed));
>>   
>> -    cpuid(input, regs);
>> +    if ( HYPERVISOR_LEAF(input[0]) )
>> +    {
>> +        xc_cpuid_t cpuid;
>> +
>> +        cpuid.input[0] = input[0];
>> +        cpuid.input[1] = input[1];
>> +
>> +        if (xc_cpuid(xch, &cpuid))
>> +            regs[0] = regs[1] = regs[2] = regs[3] = 0;
>> +        else {
>> +            regs[0] = cpuid.eax;
>> +            regs[1] = cpuid.ebx;
>> +            regs[2] = cpuid.ecx;
>> +            regs[3] = cpuid.edx;
>> +        }
>> +     } else
>> +        cpuid(input, regs);
>>   
>>       memcpy(polregs, regs, sizeof(regs));
>>       xc_cpuid_policy(xch, domid, input, polregs);
>> diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
>> index 3303454..4e8669b 100644
>> --- a/tools/libxc/xc_misc.c
>> +++ b/tools/libxc/xc_misc.c
>> @@ -159,6 +159,24 @@ int xc_send_debug_keys(xc_interface *xch, char *keys)
>>       return ret;
>>   }
>>   
>> +int xc_cpuid(xc_interface *xch,
>> +             xc_cpuid_t *cpuid)
>> +{
>> +    int ret;
>> +    DECLARE_SYSCTL;
>> +
>> +    sysctl.cmd = XEN_SYSCTL_cpuid_op;
>> +
>> +    memcpy(&sysctl.u.cpuid, cpuid, sizeof(*cpuid));
>> +
>> +    if ( (ret = do_sysctl(xch, &sysctl)) != 0 )
>> +        return ret;
>> +
>> +    memcpy(cpuid, &sysctl.u.cpuid, sizeof(*cpuid));
>> +
>> +    return 0;
>> +}
>> +
>>   int xc_physinfo(xc_interface *xch,
>>                   xc_physinfo_t *put_info)
>>   {
>> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
>> index 13f816b..6c709f5 100644
>> --- a/tools/libxc/xenctrl.h
>> +++ b/tools/libxc/xenctrl.h
>> @@ -1135,6 +1135,7 @@ int xc_send_debug_keys(xc_interface *xch, char *keys);
>>   typedef xen_sysctl_physinfo_t xc_physinfo_t;
>>   typedef xen_sysctl_topologyinfo_t xc_topologyinfo_t;
>>   typedef xen_sysctl_numainfo_t xc_numainfo_t;
>> +typedef xen_sysctl_cpuid_t xc_cpuid_t;
>>   
>>   typedef uint32_t xc_cpu_to_node_t;
>>   typedef uint32_t xc_cpu_to_socket_t;
>> @@ -1146,6 +1147,7 @@ typedef uint32_t xc_node_to_node_dist_t;
>>   int xc_physinfo(xc_interface *xch, xc_physinfo_t *info);
>>   int xc_topologyinfo(xc_interface *xch, xc_topologyinfo_t *info);
>>   int xc_numainfo(xc_interface *xch, xc_numainfo_t *info);
>> +int xc_cpuid(xc_interface *xch, xc_cpuid_t *cpuid);
>>   
>>   int xc_sched_id(xc_interface *xch,
>>                   int *sched_id);
>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
>> index c42a079..98e2b5f 100644
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1997,7 +1997,7 @@ void arch_dump_vcpu_info(struct vcpu *v)
>>           vpmu_dump(v);
>>   }
>>   
>> -void domain_cpuid(
>> +bool_t domain_cpuid_exists(
>>       struct domain *d,
>>       unsigned int  input,
>>       unsigned int  sub_input,
>> @@ -2030,11 +2030,24 @@ void domain_cpuid(
>>                    !d->disable_migrate && !d->arch.vtsc )
>>                   *edx &= ~(1u<<8); /* TSC Invariant */
>>   
>> -            return;
>> +            return 1;
>>           }
>>       }
>>   
>> -    *eax = *ebx = *ecx = *edx = 0;
>> +    return 0;
>> +}
>> +
>> +void domain_cpuid(
>> +    struct domain *d,
>> +    unsigned int  input,
>> +    unsigned int  sub_input,
>> +    unsigned int  *eax,
>> +    unsigned int  *ebx,
>> +    unsigned int  *ecx,
>> +    unsigned int  *edx)
>> +{
>> +    if ( !domain_cpuid_exists(d, input, sub_input, eax, ebx, ecx, edx) )
>> +        *eax = *ebx = *ecx = *edx = 0;
>>   }
>>   
>>   void vcpu_kick(struct vcpu *v)
>> diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
>> index 15d4b91..08f8038 100644
>> --- a/xen/arch/x86/sysctl.c
>> +++ b/xen/arch/x86/sysctl.c
>> @@ -101,6 +101,23 @@ long arch_do_sysctl(
>>       }
>>       break;
>>   
>> +    case XEN_SYSCTL_cpuid_op:
> This would appear to be a cpuid instruction in the context of a domain,
> which should be a domctl, not a sysctl.  I have a different
> implementation of sysctl cpuid posted, which takes a pcpu parameter.

No, this is not intended to handle CPUID instruction. This is invoked 
only as part of a sysctl.

As for domctl vs sysctl --- I in fact would have preferred to do this 
via domctl since we already have a data structure to handle this 
(xen_domctl_cpuid). I decided to use sysctl since the intent here is to 
query what the system provides, not what a domain sees.


>
>> +    {
>> +        struct xen_sysctl_cpuid *cpuid = &sysctl->u.cpuid;
>> +
>> +        if ( !cpuid_hypervisor_leaves(cpuid->input[0], cpuid->input[1],
>> +                                      &cpuid->eax,&cpuid->ebx,
>> +                                      &cpuid->ecx, &cpuid->edx) )
>> +            asm ( "cpuid"
>> +                 :"=a" (cpuid->eax), "=b" (cpuid->ebx),
>> +                  "=c" (cpuid->ecx), "=d" (cpuid->edx)
>> +                 : "0" (cpuid->input[0]), "1" (cpuid->input[1]) );
> This will likely bypass masking/levelling for a domain.  As suggested,
> the hypervisor leaves should be plumbed properly through to be usable
> from domain_cpuid().

Yes, that was the intent. The thinking here is that we provide to the 
sysctl caller what the actual CPUID is. Now, this (the asm part) is not 
used anywhere so if we limit this sysctl to hypervisor leaves (or even 
to leaf 0x40000001 only as Jan suggested) we won't need this.

(Moreover, for 0x40000001-only approach we may not even need this whole 
sysctl business)

Thanks.
-boris


>
> ~Andrew
>
>> +
>> +        if ( __copy_to_guest(u_sysctl, sysctl, 1) )
>> +            ret = -EFAULT;
>> +    }
>> +    break;
>> +
>>       default:
>>           ret = -ENOSYS;
>>           break;
>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>> index c462317..e1f39d9 100644
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -690,6 +690,9 @@ int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
>>       if ( idx > limit )
>>           return 0;
>>   
>> +    if ( domain_cpuid_exists(d, base + idx, sub_idx, eax, ebx, ecx, edx) )
>> +        return 1;
>> +
>>       switch ( idx )
>>       {
>>       case 0:
>> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
>> index 49f7c0c..5fd47de 100644
>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> @@ -471,6 +471,13 @@ unsigned long pv_guest_cr4_fixup(const struct vcpu *, unsigned long guest_cr4);
>>       ((c) & ~(X86_CR4_PGE | X86_CR4_PSE | X86_CR4_TSD |      \
>>                X86_CR4_OSXSAVE | X86_CR4_SMEP | X86_CR4_FSGSBASE))
>>   
>> +bool_t domain_cpuid_exists(struct domain *d,
>> +                           unsigned int  input,
>> +                           unsigned int  sub_input,
>> +                           unsigned int  *eax,
>> +                           unsigned int  *ebx,
>> +                           unsigned int  *ecx,
>> +                           unsigned int  *edx);
>>   void domain_cpuid(struct domain *d,
>>                     unsigned int  input,
>>                     unsigned int  sub_input,
>> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
>> index 8437d31..7c1c662 100644
>> --- a/xen/include/public/sysctl.h
>> +++ b/xen/include/public/sysctl.h
>> @@ -632,6 +632,20 @@ struct xen_sysctl_coverage_op {
>>   typedef struct xen_sysctl_coverage_op xen_sysctl_coverage_op_t;
>>   DEFINE_XEN_GUEST_HANDLE(xen_sysctl_coverage_op_t);
>>   
>> +#if defined(__i386__) || defined(__x86_64__)
>> +/* XEN_SYSCTL_cpuid_op */
>> +/* Get CPUID of a particular leaf */
>> +
>> +struct xen_sysctl_cpuid {
>> +     uint32_t input[2];
>> +     uint32_t eax;
>> +     uint32_t ebx;
>> +     uint32_t ecx;
>> +     uint32_t edx;
>> +};
>> +typedef struct xen_sysctl_cpuid xen_sysctl_cpuid_t;
>> +DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpuid_t);
>> +#endif
>>   
>>   struct xen_sysctl {
>>       uint32_t cmd;
>> @@ -654,6 +668,7 @@ struct xen_sysctl {
>>   #define XEN_SYSCTL_cpupool_op                    18
>>   #define XEN_SYSCTL_scheduler_op                  19
>>   #define XEN_SYSCTL_coverage_op                   20
>> +#define XEN_SYSCTL_cpuid_op                      21
>>       uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
>>       union {
>>           struct xen_sysctl_readconsole       readconsole;
>> @@ -675,6 +690,9 @@ struct xen_sysctl {
>>           struct xen_sysctl_cpupool_op        cpupool_op;
>>           struct xen_sysctl_scheduler_op      scheduler_op;
>>           struct xen_sysctl_coverage_op       coverage_op;
>> +#if defined(__i386__) || defined(__x86_64__)
>> +        struct xen_sysctl_cpuid             cpuid;
>> +#endif
>>           uint8_t                             pad[128];
>>       } u;
>>   };