Re: [Xen-devel] [PATCH] x86/domctl: Adjust size calculations for XEN_DOMCTL_get{_ext_vcpucontext, vcpuextstate}

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.04.14 at 11:43, <andrew.cooper3@xxxxxxxxxx> wrote:
> XEN_DOMCTL_get_ext_vcpucontext suffers from the same issue but while trying to
> fix that in similar way, I discovered that it had a genuine bug when returning
> the count of MSRs to the toolstack.  When running the hypercall on an active
> vcpu, the vcpu can arbitrarily alter the count returned to the toolstack by
> clearing and setting relevant MSRs.

Did you perhaps overlook the vcpu_pause() there?

> @@ -865,41 +867,54 @@ long arch_do_domctl(
>              evc->vmce.mci_ctl2_bank0 = v->arch.vmce.bank[0].mci_ctl2;
>              evc->vmce.mci_ctl2_bank1 = v->arch.vmce.bank[1].mci_ctl2;
>  
> -            i = ret = 0;
> +            /* Count maximum number of optional msrs */
>              if ( boot_cpu_has(X86_FEATURE_DBEXT) )
> +                nr_msrs += 4;
> +
> +            if ( guest_handle_is_null(evc->msrs) ||
> +                 (evc->msr_count < nr_msrs) )
> +            {
> +                evc->msr_count = nr_msrs;
> +                ret = guest_handle_is_null(evc->msrs) ? 0 : -ENOBUFS;
> +            }

Won't this, with the migration part still not implemented in libxc,
result in guests using any of these getting migrated with a non-
zero MSR count, rather than the migration failing on the sender
side?

I'm also not really in favor of forcing the tools to allocate memory
for the array if in fact no MSRs are being used by the guest.

> @@ -1177,7 +1192,8 @@ long arch_do_domctl(
>          {
>              unsigned int size = PV_XSAVE_SIZE(v->arch.xcr0_accum);
>  
> -            if ( !evc->size && !evc->xfeature_mask )
> +            if ( (evc->size == 0 && evc->xfeature_mask == 0) ||
> +                 guest_handle_is_null(domctl->u.vcpuextstate.buffer) )

And strong reason not to keep the shorter ! operators?

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -594,11 +594,13 @@ struct xen_domctl_ext_vcpucontext {
>      uint8_t          syscall32_disables_events;
>      uint8_t          sysenter_disables_events;
>      /*
> -     * When, for the "get" version, msr_count is too small to cover all MSRs
> -     * the hypervisor needs to be saved, the call will return -ENOBUFS and
> -     * set msr_count to the required (minimum) value. Furthermore, for both
> -     * "get" and "set", that field as well as the msrs one only get looked at
> -     * if the size field above covers the structure up to the entire msrs 
> one.
> +     * Number of msr entries pointed to by the 'msrs' guest handle.
> +     *
> +     * For the "get" subop, if the guest handle is NULL, Xen shall write back
> +     * the maximum number of msrs it might save.  If msr_count is fewer than

I think there's "If the handle is non-NULL" missing at the beginning of
the second sentence.

Jan

> +     * the maximum, Xen shall return -ENOBUFS.  When Xen actually writes into
> +     * the buffer, this field shall reflect the actual number of msrs written
> +     * which might be fewer than the maximum, if some MSRs are not in use.




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel