Re: [PATCH v3 02/13] libs/guest: allow fetching a specific CPUID leaf from a cpu policy

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

On 04/05/2021 14:47, Roger Pau Monné wrote:
> On Tue, May 04, 2021 at 12:59:43PM +0100, Andrew Cooper wrote:
>> On 30/04/2021 16:52, Roger Pau Monne wrote:
>>> @@ -822,3 +825,28 @@ int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t p,
>>>      errno = 0;
>>>      return 0;
>>>  }
>>> +
>>> +int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy,
>>> +                            uint32_t leaf, uint32_t subleaf,
>>> +                            xen_cpuid_leaf_t *out)
>>> +{
>>> +    unsigned int nr_leaves = ARRAY_SIZE(policy->leaves);
>>> +    xen_cpuid_leaf_t *tmp;
>>> +    int rc;
>>> +
>>> +    rc = xc_cpu_policy_serialise(xch, policy, policy->leaves, &nr_leaves,
>>> +                                 NULL, 0);
>>> +    if ( rc )
>>> +        return rc;
>> Sorry for not spotting this last time.
>>
>> You don't need to serialise.  You can look up leaf/subleaf in O(1) time
>> from cpuid_policy, which was a design goal of the structure originally.
>>
>> It is probably best to adapt most of the first switch statement in
>> guest_cpuid() to be a libx86 function.  The asserts aren't massively
>> interesting to keep, and instead of messing around with nospec, just
>> have the function return a pointer into the cpuid_policy (or NULL), and
>> have a single block_speculation() in Xen.
> libx86 already has array_access_nospec, so I think it's fine to just
> leave the code as-is instead of adding a block_speculation in Xen and
> dropping the array_access_nospec accessors?

The same libx86 function should be used to simplify
x86_cpuid_copy_from_buffer(), which has a similar opencoded construct
for looking up the leaf/subleaf.

You might need some macro trickery to make a const and non-const
versions, or have the main version non-const, and a const-qualified
inline helper which casts away constness on the input but replaces it on
the output.

The new code can't use array_access_nospec() because it is no longer
accessing the array - merely returning a pointer.  array_index_nospec()
might be an acceptable alternative.

>> We'll also want a unit test
>> to go with this new function to check that out-of-range leaves don't
>> result in out-of-bounds reads.
> Sure.
>
> Also, whats your opinion regarding xc_cpu_policy_get_msr, should I
> also split part of guest_rdmsr and place it in libx86 in order to
> fetch the MSRs present in msr_policy?

That's harder to say.  I'd like to avoid the serialise call, but the
current msr_policy structure currently uses uint32_t for space reasons,
so you can't just create a uint64_t pointer to it.

Perhaps we should bite the bullet and use uint64_t unilaterally, so we
can create a lookup_msr_by_index() or equiv.  The next big block of MSRs
going into the policy are the VT-x ones, and they'll need to be 64 bits
wide.

~Andrew