Re: [PATCH] x86/PV: fix unintended dependency of m2p-strict mode on migration-v2

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

On 01/02/16 16:51, Jan Beulich wrote:
>>>> On 01.02.16 at 17:34, <andrew.cooper3@citrix.com> wrote:
>> On 01/02/16 16:28, Jan Beulich wrote:
>>>>>> On 01.02.16 at 15:07, <andrew.cooper3@citrix.com> wrote:
>>>> On 01/02/16 13:20, Jan Beulich wrote:
>>>>> Ping? (I'd really like to get this resolved, so we don't need to
>>>>> indefinitely run with non-upstream behavior in our distros.)
>>>> My remaining issue is whether this loop gets executed by default.
>>>>
>>>> I realise that there is a difference between legacy and v2 migration,
>>>> and that v2 migration by default worked.  If that means we managed to
>>>> skip this loop in its entirety for v2, then I am far less concerned
>>>> about the overhead.
>>> But had been there before: Of course we could skip the loop if
>>> the bit in d->vm_assist doesn't change. But as expressed before,
>>> with you having already indicated that perhaps it would be better
>>> to have v2 migration do the relevant operations in the other (v1)
>>> order, the moment that was actually done the benefit of avoiding
>>> the loop would be gone.
>>>
>>> To be clear - if rendering the code dead (which is what you ask
>>> for) until v2 migration happens to get changed is the only way to
>>> get this code in, I will submit a v2 with that extra conditional.
>> Migration v2 currently loads vcpu context before pinning the pagetables,
>> which means that the vm_assist should get set up properly, before L4
>> tables are processed.
>>
>> It was my understanding that this is the correct way around, and
>> m2p-strict mode only broke when you backported it to migration v1 systems?
> Yes, but when we discussed this you said "in hindsight, it would have
> been more sensible to swap pin and vCPU context, but I guess that
> would break m2p-strict in the same way", and whether one is more
> "correct" than the other is pretty questionable.

Right, but as it currently stands, migration v2 gets things the correct
way around?

Does that mean that, at the moment, this loop ends up getting skipped?

If it does, then the patch is fine.

The suggestion to swap the actions around was only to work around the
apparent error caused by continutions while loading vcpu0's state,
caused by auditing and pinning all the pagetables hanging off %cr3/%cr1.

If however there is a legitimate reason, such as this, to keep the
current order of operations, then it would be counterproductive to make
any changes to migration v2.

~Andrew