[RFC] Re-introduce VCPUOP_register_vcpu_info back for PVHVM guests. (v1)

[RFC] Re-introduce VCPUOP_register_vcpu_info back for PVHVM guests. (v1)

[Konrad Rzeszutek Wilk]

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.

[PATCH] Allow VCPUOP_register_vcpu_info to work again on PVHVM guests.

[Konrad Rzeszutek Wilk]

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

Re: [RFC] Re-introduce VCPUOP_register_vcpu_info back for PVHVM guests. (v1)

[Jan Beulich]

>>> 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

Re: [RFC] Re-introduce VCPUOP_register_vcpu_info back for PVHVM guests. (v1)

[George Dunlap]

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

Re: [RFC] Re-introduce VCPUOP_register_vcpu_info back for PVHVM guests. (v1)

[Jan Beulich]

>>> 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

Re: [RFC] Re-introduce VCPUOP_register_vcpu_info back for PVHVM guests. (v1)

[George Dunlap]

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

Re: [RFC] Re-introduce VCPUOP_register_vcpu_info back for PVHVM guests. (v1)

[Jan Beulich]

>>> 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

Re: [RFC] Re-introduce VCPUOP_register_vcpu_info back for PVHVM guests. (v1)

[George Dunlap]

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

Re: [RFC] Re-introduce VCPUOP_register_vcpu_info back for PVHVM guests. (v1)

[George Dunlap]

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