Re: [Xen-devel] [PATCH v2 RFC 2/4] x86/PV: support data breakpoint extension registers

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.03.14 at 16:28, <Ian.Campbell@xxxxxxxxxx> wrote:
> On Wed, 2014-03-26 at 14:13 +0000, Jan Beulich wrote:
>> Introducing an extension to XEN_DOMCTL_[gs]et_ext_vcpucontext similar
>> to the generic MSR save/restore logic recently added for HVM.
> 
> "x86: generic MSRs save/restore" I guess?
> 
> Is the intention here that the extvcpu context is a blob of opaque data
> from the pov of the migration tools?

No, these tools will need to know about the structure of the data
(as seen in the patch).

> Do the protocol docs in xg_save_restore need updating due to this
> change?

Not sure about that - this doesn't seem to talk about what's inside
the "bodies".

> How does N->N+1 migration work out?

Since the structure size grows, and that size is embedded in the
structure, input coming from N will be treated as not having any
MSRs.

>> @@ -2130,9 +2162,25 @@ int xc_domain_restore(xc_interface *xch,
>>              goto vcpu_ext_state_restore;
>>          memcpy(&domctl.u.ext_vcpucontext, vcpup, 128);
>>          vcpup += 128;
>> +        if ( domctl.u.ext_vcpucontext.msr_count )
>> +        {
>> +            size_t sz = domctl.u.ext_vcpucontext.msr_count * sizeof(*msrs);
>> +
>> +            msrs = xc_hypercall_buffer_alloc(xch, msrs, sz);
>> +            if ( !msrs )
>> +            {
>> +                PERROR("No memory for vcpu%d MSRs", i);
>> +                goto out;
>> +            }
>> +            memcpy(msrs, vcpup, sz);
> 
> This seems to be opencoding the hypercall bounce buffer stuff to some
> extent.

Just like in the xstate logic. Aiui there's no way currently for
do_domctl() to know that in some cases embedded handles need
following. Or at least I didn't spot it, so if there is a mechanism to
avoid this open coding, please point me at it.

>> +            vcpup += sz;
>> +            set_xen_guest_handle(domctl.u.ext_vcpucontext.msrs, msrs);
>> +        }
>>          domctl.cmd = XEN_DOMCTL_set_ext_vcpucontext;
>>          domctl.domain = dom;
>>          frc = xc_domctl(xch, &domctl);
>> +        if ( msrs )
>> +            xc_hypercall_buffer_free(xch, msrs);
> 
> I think you don't need the if ( msrs ) here.

Ah, right - I saw xc__hypercall_buffer_free() dereference its argument,
not paying close enough attention that what gets passed here is the
address of an on-stack variable. Will drop.

>> @@ -563,6 +563,16 @@ typedef struct xen_domctl_pin_mem_cachea
>>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_pin_mem_cacheattr_t);
>>  
>> 
>> +#if defined(__i386__) || defined(__x86_64__)
>> +struct xen_domctl_ext_vcpu_msr {
>> +    uint32_t         index;
>> +    uint32_t         reserved;
>> +    uint64_aligned_t value;
>> +};
> 
> Maybe this is just me forgetting x86 stuff but are these "data
> breakpoint extension registers" actually MSRs?

Yes, they are (and they're AMD-specific).

> If so why is this not part of the generic MSR support which you
> referenced earlier? At least at the domctl level it seems to be generic
> here too?

Because that was HVM-only. Here a similar extensible mechanism is
being introduced for PV.

Jan



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