Xen devel - Apr 2013 - [RFC] Re-introduce VCPUOP_register_vcpu_info back for PVHVM guests. (v1)

Hey Jan,

While I''ve been digging around CPU hotplug for PVHVM I noticed an
oddity
when running with different versions of hypervisor. I found out that the
big change you did in Xen 4.2 of splitting the PV and HVM arch structure
introduce the regression wherein VCPUOP_register_vcpu_info will not
work for PVHVM guests.

Interestingly enough if I introduce this back to the hypervisor the
CPU hotplug path gets even more broken for PVHVM :-).. But that is a Linux
problem.

But if it was not intentional, please see the patch and if you are 
happy also please back-port it to Xen 4.2 stable branch.

For details on the hypercall please see commit
c58ae69360ccf2495a19bf4ca107e21cf873c75b (VCPUOP_register_vcpu_info) and
the c/s 23143 (git commit 6b063a4a6f44245a727aa04ef76408b2e00af9c7)
(x86: move pv-only members of struct vcpu to struct pv_vcpu)
that introduced the regression.

The current code allows the PVHVM guest to make this hypercall.
But for PVHVM guest it always returns -EINVAL (-22) for Xen 4.2
and above. Xen 4.1 and earlier worked.

The reason is that the check in map_vcpu_info would fail
at:

  if ( v->arch.vcpu_info_mfn != INVALID_MFN )

The reason is that the vcpu_info_mfn for PVHVM guests ends up by
defualt with the value of zero (introduced by c/s 23143).

The code in vcpu_initialise which initialized vcpu_info_mfn to a
valid value (INVALID_MFN), would never be called for PVHVM:

    if ( is_hvm_domain(d) )
    {
        rc = hvm_vcpu_initialise(v);
        goto done;
    }

    v->arch.pv_vcpu.vcpu_info_mfn = INVALID_MFN;

while previously it would be:

     v->arch.vcpu_info_mfn = INVALID_MFN;

[right at the start of the function in Xen 4.1]

This fixes the problem with Linux advertising this error:
register_vcpu_info failed: err=-22

CC: jbeulich@suse.com
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/arch/x86/domain.c        | 13 +++++++------
 xen/include/asm-x86/domain.h |  6 +++---
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 6f9cbed..14b6d13 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -387,13 +387,14 @@ int vcpu_initialise(struct vcpu *v)
 
     vmce_init_vcpu(v);
 
+    v->arch.vcpu_info_mfn = INVALID_MFN;
+
     if ( is_hvm_domain(d) )
     {
         rc = hvm_vcpu_initialise(v);
         goto done;
     }
 
-    v->arch.pv_vcpu.vcpu_info_mfn = INVALID_MFN;
 
     spin_lock_init(&v->arch.pv_vcpu.shadow_ldt_lock);
 
@@ -963,14 +964,14 @@ unmap_vcpu_info(struct vcpu *v)
 {
     unsigned long mfn;
 
-    if ( v->arch.pv_vcpu.vcpu_info_mfn == INVALID_MFN )
+    if ( v->arch.vcpu_info_mfn == INVALID_MFN )
         return;
 
-    mfn = v->arch.pv_vcpu.vcpu_info_mfn;
+    mfn = v->arch.vcpu_info_mfn;
     unmap_domain_page_global(v->vcpu_info);
 
     v->vcpu_info = &dummy_vcpu_info;
-    v->arch.pv_vcpu.vcpu_info_mfn = INVALID_MFN;
+    v->arch.vcpu_info_mfn = INVALID_MFN;
 
     put_page_and_type(mfn_to_page(mfn));
 }
@@ -993,7 +994,7 @@ map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned
offset)
     if ( offset > (PAGE_SIZE - sizeof(vcpu_info_t)) )
         return -EINVAL;
 
-    if ( v->arch.pv_vcpu.vcpu_info_mfn != INVALID_MFN )
+    if ( v->arch.vcpu_info_mfn != INVALID_MFN )
         return -EINVAL;
 
     /* Run this command on yourself or on other offline VCPUS. */
@@ -1030,7 +1031,7 @@ map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned
offset)
     }
 
     v->vcpu_info = new_info;
-    v->arch.pv_vcpu.vcpu_info_mfn = page_to_mfn(page);
+    v->arch.vcpu_info_mfn = page_to_mfn(page);
 
     /* Set new vcpu_info pointer /before/ setting pending flags. */
     wmb();
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 6f9744a..43b4d3d 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -374,9 +374,6 @@ struct pv_vcpu
     /* Current LDT details. */
     unsigned long shadow_ldt_mapcnt;
     spinlock_t shadow_ldt_lock;
-
-    /* Guest-specified relocation of vcpu_info. */
-    unsigned long vcpu_info_mfn;
 };
 
 struct arch_vcpu
@@ -442,6 +439,9 @@ struct arch_vcpu
 
     /* A secondary copy of the vcpu time info. */
     XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
+
+    /* Guest-specified relocation of vcpu_info. */
+    unsigned long vcpu_info_mfn;
 } __cacheline_aligned;
 
 /* Shorthands to improve code legibility. */
-- 
1.8.1.4

>>> On 16.04.13 at 23:09, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> While I''ve been digging around CPU hotplug for PVHVM I noticed an
oddity
> when running with different versions of hypervisor. I found out that the
> big change you did in Xen 4.2 of splitting the PV and HVM arch structure
> introduce the regression wherein VCPUOP_register_vcpu_info will not
> work for PVHVM guests.
> 
> Interestingly enough if I introduce this back to the hypervisor the
> CPU hotplug path gets even more broken for PVHVM :-).. But that is a Linux
> problem.
> 
> But if it was not intentional, please see the patch and if you are 
> happy also please back-port it to Xen 4.2 stable branch.
Yes, I apparently overlooked that HVM is permitted to use
VCPUOP_register_vcpu_info.

Jan

On Tue, Apr 16, 2013 at 10:09 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> Hey Jan,
>
> While I''ve been digging around CPU hotplug for PVHVM I noticed an
oddity
> when running with different versions of hypervisor. I found out that the
> big change you did in Xen 4.2 of splitting the PV and HVM arch structure
> introduce the regression wherein VCPUOP_register_vcpu_info will not
> work for PVHVM guests.
>
> Interestingly enough if I introduce this back to the hypervisor the
> CPU hotplug path gets even more broken for PVHVM :-).. But that is a Linux
> problem.
Except that we really don''t want to be trashing people''s
existing,
working versions of Linux, especially if they''re distro-provided
kernels.  I haven''t tested a distro kernel yet, but for my own
locally-built 2.6.37 kernel, this c/s breaks it running in PVHVM mode.

 -George

>>> On 17.04.13 at 17:31, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
> On Tue, Apr 16, 2013 at 10:09 PM, Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
>> Hey Jan,
>>
>> While I''ve been digging around CPU hotplug for PVHVM I noticed
an oddity
>> when running with different versions of hypervisor. I found out that
the
>> big change you did in Xen 4.2 of splitting the PV and HVM arch
structure
>> introduce the regression wherein VCPUOP_register_vcpu_info will not
>> work for PVHVM guests.
>>
>> Interestingly enough if I introduce this back to the hypervisor the
>> CPU hotplug path gets even more broken for PVHVM :-).. But that is a
Linux
>> problem.
> 
> Except that we really don''t want to be trashing people''s
existing,
> working versions of Linux, especially if they''re distro-provided
> kernels.  I haven''t tested a distro kernel yet, but for my own
> locally-built 2.6.37 kernel, this c/s breaks it running in PVHVM mode.
Hmm - that''s minimally a reason to not backport it to 4.2, but
perhaps even a reason to revert it from staging.

Jan

On 17/04/13 17:23, Jan Beulich wrote:
>>>> On 17.04.13 at 17:31, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
>> On Tue, Apr 16, 2013 at 10:09 PM, Konrad Rzeszutek Wilk
>> <konrad.wilk@oracle.com> wrote:
>>> Hey Jan,
>>>
>>> While I''ve been digging around CPU hotplug for PVHVM I
noticed an oddity
>>> when running with different versions of hypervisor. I found out
that the
>>> big change you did in Xen 4.2 of splitting the PV and HVM arch
structure
>>> introduce the regression wherein VCPUOP_register_vcpu_info will not
>>> work for PVHVM guests.
>>>
>>> Interestingly enough if I introduce this back to the hypervisor the
>>> CPU hotplug path gets even more broken for PVHVM :-).. But that is
a Linux
>>> problem.
>> Except that we really don''t want to be trashing
people''s existing,
>> working versions of Linux, especially if they''re
distro-provided
>> kernels.  I haven''t tested a distro kernel yet, but for my own
>> locally-built 2.6.37 kernel, this c/s breaks it running in PVHVM mode.
> Hmm - that''s minimally a reason to not backport it to 4.2, but
> perhaps even a reason to revert it from staging.
Hold off on reverting it -- I thought I had clearly shown it to be the 
c/s that breaks things, but now it''s not so clear... really frustrating
dealing w/ the build system right now...

  -George

On 17/04/13 17:23, Jan Beulich wrote:
>>>> On 17.04.13 at 17:31, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
>> On Tue, Apr 16, 2013 at 10:09 PM, Konrad Rzeszutek Wilk
>> <konrad.wilk@oracle.com> wrote:
>>> Hey Jan,
>>>
>>> While I''ve been digging around CPU hotplug for PVHVM I
noticed an oddity
>>> when running with different versions of hypervisor. I found out
that the
>>> big change you did in Xen 4.2 of splitting the PV and HVM arch
structure
>>> introduce the regression wherein VCPUOP_register_vcpu_info will not
>>> work for PVHVM guests.
>>>
>>> Interestingly enough if I introduce this back to the hypervisor the
>>> CPU hotplug path gets even more broken for PVHVM :-).. But that is
a Linux
>>> problem.
>> Except that we really don''t want to be trashing
people''s existing,
>> working versions of Linux, especially if they''re
distro-provided
>> kernels.  I haven''t tested a distro kernel yet, but for my own
>> locally-built 2.6.37 kernel, this c/s breaks it running in PVHVM mode.
> Hmm - that''s minimally a reason to not backport it to 4.2, but
> perhaps even a reason to revert it from staging.
OK sorry about that -- my results were spurious, due to a 
non-deterministic bug that seems to have been in for a while.  (It looks 
like missing interrupts of some kind, so it may even be the RTC c/s... 
I''ll check that tomorrow.)

  -George

>>> On 17.04.13 at 18:24, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> On 17/04/13 17:23, Jan Beulich wrote:
>>>>> On 17.04.13 at 17:31, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
>>> On Tue, Apr 16, 2013 at 10:09 PM, Konrad Rzeszutek Wilk
>>> <konrad.wilk@oracle.com> wrote:
>>>> Hey Jan,
>>>>
>>>> While I''ve been digging around CPU hotplug for PVHVM I
noticed an oddity
>>>> when running with different versions of hypervisor. I found out
that the
>>>> big change you did in Xen 4.2 of splitting the PV and HVM arch
structure
>>>> introduce the regression wherein VCPUOP_register_vcpu_info will
not
>>>> work for PVHVM guests.
>>>>
>>>> Interestingly enough if I introduce this back to the hypervisor
the
>>>> CPU hotplug path gets even more broken for PVHVM :-).. But that
is a Linux
>>>> problem.
>>> Except that we really don''t want to be trashing
people''s existing,
>>> working versions of Linux, especially if they''re
distro-provided
>>> kernels.  I haven''t tested a distro kernel yet, but for my
own
>>> locally-built 2.6.37 kernel, this c/s breaks it running in PVHVM
mode.
>> Hmm - that''s minimally a reason to not backport it to 4.2, but
>> perhaps even a reason to revert it from staging.
> 
> Hold off on reverting it -- I thought I had clearly shown it to be the 
> c/s that breaks things, but now it''s not so clear... really
frustrating
> dealing w/ the build system right now...
What''s the status on this? Sander over the weekend reported
another issue with the patch, so I''m clearly going to keep this off
the stable trees for the time being, but of course we also should
have a clear picture for unstable.

Jan

On Mon, Apr 22, 2013 at 8:24 AM, Jan Beulich <JBeulich@suse.com>
wrote:
>>>> On 17.04.13 at 18:24, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
>> On 17/04/13 17:23, Jan Beulich wrote:
>>>>>> On 17.04.13 at 17:31, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
>>>> On Tue, Apr 16, 2013 at 10:09 PM, Konrad Rzeszutek Wilk
>>>> <konrad.wilk@oracle.com> wrote:
>>>>> Hey Jan,
>>>>>
>>>>> While I''ve been digging around CPU hotplug for
PVHVM I noticed an oddity
>>>>> when running with different versions of hypervisor. I found
out that the
>>>>> big change you did in Xen 4.2 of splitting the PV and HVM
arch structure
>>>>> introduce the regression wherein VCPUOP_register_vcpu_info
will not
>>>>> work for PVHVM guests.
>>>>>
>>>>> Interestingly enough if I introduce this back to the
hypervisor the
>>>>> CPU hotplug path gets even more broken for PVHVM :-).. But
that is a Linux
>>>>> problem.
>>>> Except that we really don''t want to be trashing
people''s existing,
>>>> working versions of Linux, especially if they''re
distro-provided
>>>> kernels.  I haven''t tested a distro kernel yet, but
for my own
>>>> locally-built 2.6.37 kernel, this c/s breaks it running in
PVHVM mode.
>>> Hmm - that''s minimally a reason to not backport it to 4.2,
but
>>> perhaps even a reason to revert it from staging.
>>
>> Hold off on reverting it -- I thought I had clearly shown it to be the
>> c/s that breaks things, but now it''s not so clear... really
frustrating
>> dealing w/ the build system right now...
>
> What''s the status on this? Sander over the weekend reported
> another issue with the patch, so I''m clearly going to keep this
off
> the stable trees for the time being, but of course we also should
> have a clear picture for unstable.
[Re-sending with the full cc list]

I sent an e-mail Friday afternoon saying that my results were spurious
-- so please act as you would if I had never sent the e-mail. :-)

 -George