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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 27/03/14 16:37, Jan Beulich wrote:
>>>> On 27.03.14 at 17:03, <andrew.cooper3@citrix.com> wrote:
>> On 27/03/14 15:28, Ian Campbell 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?
>>>
>>> Do the protocol docs in xg_save_restore need updating due to this
>>> change?
>>>
>>> How does N->N+1 migration work out?
>> Having just rewritten all of this code from scratch, the details are
>> painfully present in my brain.
>>
>> The answer is badly.
>>
>> The blob is exactly 128 bytes long, starting from the beginning of the
>> domctl union.  changeset e1dc98e3c "x86/vMCE: save/restore MCA
>> capabilities" introduced an explicit memset(0) on the content of the
>> struct domctl, meaning that this newly interpreted 'msr_count' shall
>> have the value 0.
> As just written to Ian in another reply - this new field isn't even being
> looked at when the size stored at the beginning of the structure doesn't
> indicate the presence of the msrs[] handle.

I am frankly having a very hard time following the new logic in
buffer_tail_pv(), so cant yet comment on when it is actually doing the
correct thing in terms of calculating whether a block of msrs is present
in the stream.

However, it never nops out evc->msr_count in the case that msrs is not
present in the stream, meaning that the "if (
domctl.u.ext_vcpucontext.msr_count )" will be performing logic based on
uninitialised stack junk for migration streams from 4.1 or before.

>
>> As this is a new feature, so not applicable for backport, can it wait
>> until my migration stream v2 work is submitted?  The migration code
>> churn to support this in v2 would be far less, and would also guarantee
>> no breakage of older migration streams.
> I wouldn't want to postpone this by too much (and certainly not past
> a re-write of the migration code) because - other than upstream Xen -
> we may want/need to backport this for SLE12.
>
> Jan
>

Backporting to SLE12 is certainly your perogative, but the tools side of
this patch is not fit for committing yet.

The hypervisor side looks ok, although I would suggest that the
activate_debugregs() logic get split out for reviewing purposes as it
appears to be an orthogonal fix.

As far as the migration stream v2 goes, it is one of the highest
priority tasks in XenServer at the moment, and I am working full time on
it.  The current state is that 64bit PV non-live migrate is working. 
32bit PV guests are pending a formal fix to the toolstacks incorrect
assumptions about which L2 entries are safe to shoot.  I plan to get the
live bit working then present an RFC series upstream.  HVM migration is
substantially easier than PV migration, and will follow shortly afterwards.

~Andrew