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
