On 04/07/18 10:43, Jan Beulich wrote:
> Oh, here we go - the title doesn't suggest this is about CPUID as well.
>
>>>> On 03.07.18 at 22:55, <andrew.coop...@citrix.com> wrote:
>> Extend the xen-cpuid utility to be able to dump the system policies. An
>> example output is:
>>
>> Xen reports there are maximum 113 leaves and 3 MSRs
>> Raw policy: 93 leaves, 3 MSRs
>> CPUID:
>> leaf subleaf -> eax ebx ecx edx
>> 00000000:ffffffff -> 0000000d:756e6547:6c65746e:49656e69
> I'd like to suggest to suppress the :fffffff when there are no sub-leaves.
This is a developer tool, rather than a user tool, and it is dumping raw
data.
If there were an easy formatter way of expressing "uint32_t or blank"
then yes, but I'm not aware of one.
>
>> @@ -377,7 +458,7 @@ int main(int argc, char **argv)
>> if ( i == nr_features )
>> break;
>>
>> - if ( *ptr == ':' )
>> + if ( *ptr == ':' || *ptr == '-' )
> None of the other changes to this file give any hint why a dash needs
> recognizing here all of the sudden. Is this a stray / leftover change?
Hmm - at a guess that is a XenServer compatibility improvement, but
probably can be pulled out into a separate change.
Xapi represents feature bitmaps with dash delimiters rather than colon
delimiters, and this alters the parsing to accept both forms.
>
>> --- a/xen/arch/x86/sysctl.c
>> +++ b/xen/arch/x86/sysctl.c
>> @@ -32,22 +32,32 @@
>> #include <asm/cpuid.h>
>>
>> const struct policy_group system_policies[] = {
>> - {
>> + [ XEN_SYSCTL_cpumsr_policy_raw ] = {
> Aha - this clarifies a question I had on the earlier patch. But it would
> be nice if that other patch was self contained also in the way of
> allowing readers to understand the intentions.
One thing I could do is introduce the XEN_SYSCTL_cpumsr_policy_* defines
in the previous patch? I don't want to merge the two patches as that is
too many moving parts to review in a single patch.
> And with this I now
> wonder whether the pointers in struct policy_group shouldn't all
> be const qualified.
Unfortunately that doesn't work with the logic to create a policy_group
for an individual domain during audit.
>
>> @@ -318,6 +328,74 @@ long arch_do_sysctl(
>> break;
>> }
>>
>> + case XEN_SYSCTL_get_cpumsr_policy:
>> + {
>> + const struct policy_group *group;
>> +
>> + /* Bad policy index? */
>> + if ( sysctl->u.cpumsr_policy.index >= ARRAY_SIZE(system_policies) )
>> + {
>> + ret = -EINVAL;
>> + break;
>> + }
>> + group = &system_policies[sysctl->u.cpumsr_policy.index];
> Isn't this introducing at least half of a Spectre v1 gadget?
Nope :(
It's both halves of the Spectre gadget, when you account for the
dereference when calling x86_*_copy_to_buffer() slightly lower.
I suppose we want to port the Linux array nospec lookup logic so we can
protect the clearly-visible gadgets.
>
>> + /* Request for maximum number of leaves/MSRs? */
>> + if ( guest_handle_is_null(sysctl->u.cpumsr_policy.cpuid_policy) )
>> + {
>> + sysctl->u.cpumsr_policy.nr_leaves = CPUID_MAX_SERIALISED_LEAVES;
>> + if ( __copy_field_to_guest(u_sysctl, sysctl,
>> + u.cpumsr_policy.nr_leaves) )
>> + {
>> + ret = -EFAULT;
>> + break;
>> + }
>> + }
>> + if ( guest_handle_is_null(sysctl->u.cpumsr_policy.msr_policy) )
>> + {
>> + sysctl->u.cpumsr_policy.nr_msrs = MSR_MAX_SERIALISED_ENTRIES;
>> + if ( __copy_field_to_guest(u_sysctl, sysctl,
>> + u.cpumsr_policy.nr_msrs) )
>> + {
>> + ret = -EFAULT;
>> + break;
>> + }
>> + }
>> +
>> + /* Serialise the information the caller wants. */
>> + if ( !guest_handle_is_null(sysctl->u.cpumsr_policy.cpuid_policy) )
>> + {
>> + if ( (ret = x86_cpuid_copy_to_buffer(
>> + group->cp,
>> + sysctl->u.cpumsr_policy.cpuid_policy,
>> + &sysctl->u.cpumsr_policy.nr_leaves)) )
>> + break;
> Coming back to an earlier question, I realize the null handle logic
> above is supposed to allow sizing the buffers, but I think it would
> be better to allow single invocations to generally work, making a
> second invocation necessary just as a fallback. IOW I think the
> code here wants to return to the caller the required number of
> slots in case of -ENOBUFS. And it should the also ...
I don't agree. Whatever happens, the toolstack has to (once) make a
hypercall requesting the size of the buffers, and there is no plausible
manipulation where the toolstack would start with one sized buffer, and
dynamically size it based on -ENOBUFS.
The independent handling (e.g. only getting CPUID or MSR) is of more use
to the toolstack than having Xen try to stumble on in the face of a hard
error.
>
>> --- a/xen/include/public/sysctl.h
>> +++ b/xen/include/public/sysctl.h
>> @@ -1063,6 +1063,43 @@ struct xen_sysctl_set_parameter {
>> uint16_t pad[3]; /* IN: MUST be zero. */
>> };
>>
>> +#if defined(__i386__) || defined(__x86_64__)
>> +/*
>> + * XEN_SYSCTL_get_cpumsr_policy (x86 specific)
> Perhaps express the "x86 specific" also in the opcode name? And make
> more obvious that this is about CPUID and MSRs at the same time? E.g.
> XEN_SYSCTL_x86_get_cpuid_msr_policy?
>
> I'm sure you have reasons to munge it all into a single operation.
(Answering in reverse order)
The get operations don't strictly need to be a single operation. The
set operation specifically must be a single operation, and the getters
have an interface to match.
As for naming, cpumsr_policy wasn't chosen by me, but I can't think of
anything better. The code is currently consistent and, while I'm open
to a rename, it will impact large quantities of the series.
One concern I have if we end up with a new block of information. I was
hoping for a generic name, but simply "policy" on its own is too
generic. cpumsr is, I believe, a contraction of cpuid_msr to avoid
excessive code volume.
Suggestions welcome.
>
>> + * Return information about CPUID and MSR policies available on this host.
>> + * - Raw: The real H/W values.
>> + * - Host: The values Xen is using, (after command line overrides,
>> etc).
>> + * - Max_*: Maximum set of features a PV or HVM guest can use.
>> Includes
>> + * experimental features outside of security support.
>> + * - Default_*: Default set of features a PV or HVM guest can use. This is
>> + * the security supported set.
>> + */
>> +struct xen_sysctl_cpumsr_policy {
>> +#define XEN_SYSCTL_cpumsr_policy_raw 0
>> +#define XEN_SYSCTL_cpumsr_policy_host 1
>> +#define XEN_SYSCTL_cpumsr_policy_pv_max 2
>> +#define XEN_SYSCTL_cpumsr_policy_hvm_max 3
>> +#define XEN_SYSCTL_cpumsr_policy_pv_default 4
>> +#define XEN_SYSCTL_cpumsr_policy_hvm_default 5
>> + uint32_t index; /* IN: Which policy to query? */
>> + uint32_t nr_leaves; /* IN/OUT: Number of leaves in/written to
>> + * 'cpuid_policy', or the maximum number of
>> leaves if
>> + * any of the guest handles is NULL.
>> + * NB. All policies come from the same space,
>> + * so have the same maximum length. */
>> + uint32_t nr_msrs; /* IN/OUT: Number of MSRs in/written to
>> + * 'msr_domain_policy', or the maximum number of
>> MSRs
>> + * if any of the guest handles is NULL.
>> + * NB. All policies come from the same space,
>> + * so have the same maximum length. */
>> + XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_policy; /* OUT: */
> Explicit padding (checked to be zero in the handler) above here
> please.
Why? SYSCTLs are unstable and we don't perform similar checks for other
subops.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
