|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 09/13] x86/sysctl: Implement XEN_SYSCTL_get_cpumsr_policy
Oh, here we go - the title doesn't suggest this is about CPUID as well.
>>> On 03.07.18 at 22:55, <andrew.cooper3@xxxxxxxxxx> 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.
> @@ -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?
> --- 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. And with this I now
wonder whether the pointers in struct policy_group shouldn't all
be const qualified.
> @@ -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?
> + /* 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 ...
> + 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) )
> + {
> + if ( (ret = x86_msr_copy_to_buffer(
> + group->dp, group->vp,
> + sysctl->u.cpumsr_policy.msr_policy,
> + &sysctl->u.cpumsr_policy.nr_msrs)) )
> + break;
... be allowed to reach here despite the earlier 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.
> + * 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.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |