Re: [Xen-devel] [PATCH 1/8] libx86: Introduce x86_cpu_policies_are_compatible()
On 12/09/2019 09:22, Jan Beulich wrote:
> On 12.09.2019 09:59, Andrew Cooper wrote:
>> On 12/09/2019 08:43, Jan Beulich wrote:
>>> On 11.09.2019 22:04, Andrew Cooper wrote:
This helper will eventually be the core "can a guest confiured like this
run
on the CPU?" logic. For now, it is just enough of a stub to allow us to
replace the hypercall interface while retaining the previous behaviour.
It will be expanded as various other bits of CPUID handling get cleaned up.
Signed-off-by: Andrew Cooper
>>> Fundamentally
>>> Reviewed-by: Jan Beulich
>>> but a couple of remarks:
>>>
>>> For one, despite being just testing code, I think the two test[]
>>> arrays could do with constification.
>> Sadly they can't. It is a consequence of struct cpu_policy using
>> non-const pointers.
>>
>> I tried introducing struct const_cpu_policy but that is even worse
>> because it prohibits operating on the system policy objects in Xen.
>>
>> Overall, dropping a const in the unit tests turned out to be the least
>> bad option, unless you have a radically different suggestion to try.
>>
--- a/xen/include/xen/lib/x86/cpu-policy.h
+++ b/xen/include/xen/lib/x86/cpu-policy.h
@@ -11,6 +11,25 @@ struct cpu_policy
struct msr_policy *msr;
};
+struct cpu_policy_errors
+{
+uint32_t leaf, subleaf;
+uint32_t msr;
+};
+
+#define INIT_CPU_POLICY_ERRORS { ~0u, ~0u, ~0u }
>>> Instead of this (and using it in every caller), couldn't the function
>>> fill this first thing? (The initializer isn't strictly needed anyway,
>>> as consumers are supposed to look at the structure only when having
>>> got back an error from the function, but since error paths fill just
>>> a subset of the fields I can see how pre-filling the whole structure
>>> is easier.)
>> At the moment, error pointers are conditionally written on error, which
>> means that all callers passing non-NULL need to initialise.
>>
>> This could be altered to have xc_set_domain_cpu_policy() and
>> x86_cpu_policies_are_compatible() pro-actively set "no error" to begin
>> with, but that doesn't remove the need for INIT_CPU_POLICY_ERRORS in the
>> first place.
> Right, I did notice this in a later patch. But yes, I do think
> having the functions proactively fill the structures would be
> better overall (and remove the need to use the initializer in at
> least some cases, i.e. where there are no other early error paths).
Ok. I'll switch to doing this, and have another prep patch which
adjusts the behaviour of the already existing functions.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/8] libx86: Introduce x86_cpu_policies_are_compatible()
On 12.09.2019 09:59, Andrew Cooper wrote:
> On 12/09/2019 08:43, Jan Beulich wrote:
>> On 11.09.2019 22:04, Andrew Cooper wrote:
>>> This helper will eventually be the core "can a guest confiured like this run
>>> on the CPU?" logic. For now, it is just enough of a stub to allow us to
>>> replace the hypercall interface while retaining the previous behaviour.
>>>
>>> It will be expanded as various other bits of CPUID handling get cleaned up.
>>>
>>> Signed-off-by: Andrew Cooper
>> Fundamentally
>> Reviewed-by: Jan Beulich
>> but a couple of remarks:
>>
>> For one, despite being just testing code, I think the two test[]
>> arrays could do with constification.
>
> Sadly they can't. It is a consequence of struct cpu_policy using
> non-const pointers.
>
> I tried introducing struct const_cpu_policy but that is even worse
> because it prohibits operating on the system policy objects in Xen.
>
> Overall, dropping a const in the unit tests turned out to be the least
> bad option, unless you have a radically different suggestion to try.
>
>>
>>> --- a/xen/include/xen/lib/x86/cpu-policy.h
>>> +++ b/xen/include/xen/lib/x86/cpu-policy.h
>>> @@ -11,6 +11,25 @@ struct cpu_policy
>>> struct msr_policy *msr;
>>> };
>>>
>>> +struct cpu_policy_errors
>>> +{
>>> +uint32_t leaf, subleaf;
>>> +uint32_t msr;
>>> +};
>>> +
>>> +#define INIT_CPU_POLICY_ERRORS { ~0u, ~0u, ~0u }
>> Instead of this (and using it in every caller), couldn't the function
>> fill this first thing? (The initializer isn't strictly needed anyway,
>> as consumers are supposed to look at the structure only when having
>> got back an error from the function, but since error paths fill just
>> a subset of the fields I can see how pre-filling the whole structure
>> is easier.)
>
> At the moment, error pointers are conditionally written on error, which
> means that all callers passing non-NULL need to initialise.
>
> This could be altered to have xc_set_domain_cpu_policy() and
> x86_cpu_policies_are_compatible() pro-actively set "no error" to begin
> with, but that doesn't remove the need for INIT_CPU_POLICY_ERRORS in the
> first place.
Right, I did notice this in a later patch. But yes, I do think
having the functions proactively fill the structures would be
better overall (and remove the need to use the initializer in at
least some cases, i.e. where there are no other early error paths).
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/8] libx86: Introduce x86_cpu_policies_are_compatible()
On 12/09/2019 08:43, Jan Beulich wrote:
> On 11.09.2019 22:04, Andrew Cooper wrote:
>> This helper will eventually be the core "can a guest confiured like this run
>> on the CPU?" logic. For now, it is just enough of a stub to allow us to
>> replace the hypercall interface while retaining the previous behaviour.
>>
>> It will be expanded as various other bits of CPUID handling get cleaned up.
>>
>> Signed-off-by: Andrew Cooper
> Fundamentally
> Reviewed-by: Jan Beulich
> but a couple of remarks:
>
> For one, despite being just testing code, I think the two test[]
> arrays could do with constification.
Sadly they can't. It is a consequence of struct cpu_policy using
non-const pointers.
I tried introducing struct const_cpu_policy but that is even worse
because it prohibits operating on the system policy objects in Xen.
Overall, dropping a const in the unit tests turned out to be the least
bad option, unless you have a radically different suggestion to try.
>
>> --- a/xen/include/xen/lib/x86/cpu-policy.h
>> +++ b/xen/include/xen/lib/x86/cpu-policy.h
>> @@ -11,6 +11,25 @@ struct cpu_policy
>> struct msr_policy *msr;
>> };
>>
>> +struct cpu_policy_errors
>> +{
>> +uint32_t leaf, subleaf;
>> +uint32_t msr;
>> +};
>> +
>> +#define INIT_CPU_POLICY_ERRORS { ~0u, ~0u, ~0u }
> Instead of this (and using it in every caller), couldn't the function
> fill this first thing? (The initializer isn't strictly needed anyway,
> as consumers are supposed to look at the structure only when having
> got back an error from the function, but since error paths fill just
> a subset of the fields I can see how pre-filling the whole structure
> is easier.)
At the moment, error pointers are conditionally written on error, which
means that all callers passing non-NULL need to initialise.
This could be altered to have xc_set_domain_cpu_policy() and
x86_cpu_policies_are_compatible() pro-actively set "no error" to begin
with, but that doesn't remove the need for INIT_CPU_POLICY_ERRORS in the
first place.
>
>> --- /dev/null
>> +++ b/xen/lib/x86/policy.c
>> @@ -0,0 +1,53 @@
>> +#include "private.h"
>> +
>> +#include
>> +
>> +int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
>> +const struct cpu_policy *guest,
>> +struct cpu_policy_errors *e)
>> +{
>> +uint32_t leaf = -1, subleaf = -1, msr = -1;
>> +int ret = -EINVAL;
>> +
>> +#define NA XEN_CPUID_NO_SUBLEAF
>> +#define FAIL_CPUID(l, s) do { leaf = (l); subleaf = (s); goto out; } while
>> ( 0 )
>> +#define FAIL_MSR(m) do { msr = (m); goto out; } while ( 0 )
>> +
>> +if ( guest->cpuid->basic.max_leaf > host->cpuid->basic.max_leaf )
>> +FAIL_CPUID(0, NA);
>> +
>> +if ( guest->cpuid->extd.max_leaf > host->cpuid->extd.max_leaf )
>> +FAIL_CPUID(0x8008, NA);
>> +
>> +/* TODO: Audit more CPUID data. */
>> +
>> +if ( ~host->msr->plaform_info.raw & guest->msr->plaform_info.raw )
> I've noticed this only here, but there are numerous instances elsewhere:
> Could I talk you into fixing the spelling mistake (missing 't' in
> "platform_info") here or in a prereq patch (feel free to add my ack there
> without even posting)?
I'd not even noticed the mistake. I'll get a fixup sorted as you suggest.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/8] libx86: Introduce x86_cpu_policies_are_compatible()
On 11.09.2019 22:04, Andrew Cooper wrote:
> This helper will eventually be the core "can a guest confiured like this run
> on the CPU?" logic. For now, it is just enough of a stub to allow us to
> replace the hypercall interface while retaining the previous behaviour.
>
> It will be expanded as various other bits of CPUID handling get cleaned up.
>
> Signed-off-by: Andrew Cooper
Fundamentally
Reviewed-by: Jan Beulich
but a couple of remarks:
For one, despite being just testing code, I think the two test[]
arrays could do with constification.
> --- a/xen/include/xen/lib/x86/cpu-policy.h
> +++ b/xen/include/xen/lib/x86/cpu-policy.h
> @@ -11,6 +11,25 @@ struct cpu_policy
> struct msr_policy *msr;
> };
>
> +struct cpu_policy_errors
> +{
> +uint32_t leaf, subleaf;
> +uint32_t msr;
> +};
> +
> +#define INIT_CPU_POLICY_ERRORS { ~0u, ~0u, ~0u }
Instead of this (and using it in every caller), couldn't the function
fill this first thing? (The initializer isn't strictly needed anyway,
as consumers are supposed to look at the structure only when having
got back an error from the function, but since error paths fill just
a subset of the fields I can see how pre-filling the whole structure
is easier.)
> --- /dev/null
> +++ b/xen/lib/x86/policy.c
> @@ -0,0 +1,53 @@
> +#include "private.h"
> +
> +#include
> +
> +int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
> +const struct cpu_policy *guest,
> +struct cpu_policy_errors *e)
> +{
> +uint32_t leaf = -1, subleaf = -1, msr = -1;
> +int ret = -EINVAL;
> +
> +#define NA XEN_CPUID_NO_SUBLEAF
> +#define FAIL_CPUID(l, s) do { leaf = (l); subleaf = (s); goto out; } while (
> 0 )
> +#define FAIL_MSR(m) do { msr = (m); goto out; } while ( 0 )
> +
> +if ( guest->cpuid->basic.max_leaf > host->cpuid->basic.max_leaf )
> +FAIL_CPUID(0, NA);
> +
> +if ( guest->cpuid->extd.max_leaf > host->cpuid->extd.max_leaf )
> +FAIL_CPUID(0x8008, NA);
> +
> +/* TODO: Audit more CPUID data. */
> +
> +if ( ~host->msr->plaform_info.raw & guest->msr->plaform_info.raw )
I've noticed this only here, but there are numerous instances elsewhere:
Could I talk you into fixing the spelling mistake (missing 't' in
"platform_info") here or in a prereq patch (feel free to add my ack there
without even posting)?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
