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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 11/03/14 14:07, Boris Ostrovsky wrote:
> 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.

The XEN_FORCED_EMULATION prefix would be fine, and not require a new
custom hypercall, but only an HVM guest is going to care whether it can
find this magic leaf.

>
> 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.

I currently have a project to fix this braindead thinking of having Xen
and libxc guess at what eachother supports and will report.

>
>>
>>> ---
>>>   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?

It should be (((idx) & 0xf0000000) == 0x40000000)

According to the AMD and Intel manuals, it is strictly leaf 0x40000000
reserved for hypervisor use, not 0xC0000000 or others.

>
>>
>>>   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

All of this smells like it would be substantially more simple under the
proposition in my design to fix heterogeneous feature levelling, where
Xen presents a full and completely CPUID policy for PV and HVM domains
for consumption my the toolstack.

This removes the need for these "bypass the current domains policy so I
can see something about real hardware to build another domain against",
and avoids having libxc create a wrong idea of what it thinks Xen will
provide to the guest, just to have Xen fix up later anyway.

~Andrew