Re: [PATCH 06/13] libx86: Introduce a helper to serialise a cpuid_policy object

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Sergey Dyasli <sergey.dyasli@citrix.com>,
	Xen-devel <xen-devel@lists.xen.org>,
	Wei Liu <wei.liu2@citrix.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH 06/13] libx86: Introduce a helper to serialise a cpuid_policy object
Date: Thu, 5 Jul 2018 14:39:49 +0100	[thread overview]
Message-ID: <8dc55214-a18e-3cc7-8086-4b20b849b058@citrix.com> (raw)
In-Reply-To: <5B3DD96802000078001D1592@prv1-mh.provo.novell.com>

On 05/07/18 09:40, Jan Beulich wrote:
>>>> On 04.07.18 at 18:23, <andrew.cooper3@citrix.com> wrote:
>> On 04/07/18 09:51, Jan Beulich wrote:
>>>>>> On 04.07.18 at 10:42, <wei.liu2@citrix.com> wrote:
>>>> On Tue, Jul 03, 2018 at 09:55:19PM +0100, Andrew Cooper wrote:
>>>>> --- a/xen/include/public/arch-x86/xen.h
>>>>> +++ b/xen/include/public/arch-x86/xen.h
>>>>> @@ -314,6 +314,17 @@ struct xen_arch_domainconfig {
>>>>>  #define XEN_ACPI_GPE0_CPUHP_BIT      2
>>>>>  #endif
>>>>>  
>>>>> +/*
>>>>> + * Representations of architectural CPUID information.  Used as the
>>>>> + * serialised version of Xen's internal representation.
>>>>> + */
>>>>> +typedef struct xen_cpuid_leaf {
>>>>> +#define XEN_CPUID_NO_SUBLEAF 0xffffffffu
>>>>> +    uint32_t leaf, subleaf;
>>>>> +    uint32_t a, b, c, d;
>>>>> +} xen_cpuid_leaf_t;
>>>>> +DEFINE_XEN_GUEST_HANDLE(xen_cpuid_leaf_t);
>>>>> +
>>>> Is putting this in public header while leaving
>>>> CPUID_MAX_SERIALISED_LEAVES in a private header useful?
>>>>
>>>> How does an external user know how big a buffer it needs to provide to
>>>> accommodate the return structure?
>>> Yeah, this is indeed hard to tell from this patch alone. And going
>>> over the titles of the remainder of the series it's also not clear
>>> where a use of this structure in a domctl/sysctl would appear.
>> An array of (some number of) xen_cpuid_leaf_t objects is the externally
>> visible representation of the policy, which is why this definition is
>> externally visible.
>>
>> Content of this nature will (eventually) exist in the migration stream,
>> and it is also the interface by which toolstacks (libxl and Xapi) can
>> make under-specified modifications to the policies.
> But then how can a merge be as simple as the one done here?
> Shouldn't you e.g. match CPUID entries by leaf/subleaf, rather than
> relying on a given ordering within the arrays?

I'm confused as to where you think there is any implied ordering within
the array.

When serialising the object (this patch), we write the values out in a
fixed order because that is the natural way to do it.

When deserialising the object (patch 13), we walk the array linearly
filling in the cpuid_policy object based on the leaf/subleaf.  The array
can have the leaves in any order and this won't affect the resulting
deserialisation.  (The array can also contain duplicate leaves, which we
can't reasonably be detect, and why there is a sanity upper bounds check).

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel