Re: [PATCH RFC v2] pvh: clearly specify used parameters in vcpu_guest_context

[George Dunlap <george.dunlap@eu.citrix.com>]

On 11/19/2013 10:42 AM, Roger Pau Monné wrote:
> On 19/11/13 11:34, George Dunlap wrote:
>> On 11/18/2013 06:18 PM, Roger Pau Monne wrote:
>>> The aim of this patch is to define a stable way in which PVH is
>>> going to do AP bringup.
>>>
>>> Since we are running inside of a HVM container, PVH should only need
>>> to set flags, cr3 and user_regs in order to bring up a vCPU, the rest
>>> can be set once the vCPU is started using the bare metal methods.
>>> Additionally, the guest can also set cr0 and cr4, and those values
>>> will be appended to the default values set by Xen.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> Cc: George Dunlap <george.dunlap@eu.citrix.com>
>>> Cc: Mukesh Rathor <mukesh.rathor@oracle.com>
>>> Cc: Jan Beulich <JBeulich@suse.com>
>>> Cc: Tim Deegan <tim@xen.org>
>>> Cc: Keir Fraser <keir@xen.org>
>>> ---
>>>    xen/arch/x86/domain.c             |   24 ++++++++++++++++--------
>>>    xen/arch/x86/hvm/vmx/vmx.c        |    6 +-----
>>>    xen/include/asm-x86/hvm/hvm.h     |    6 +++---
>>>    xen/include/public/arch-x86/xen.h |    8 ++++----
>>>    4 files changed, 24 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
>>> index a3868f9..51311fc 100644
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -51,6 +51,7 @@
>>>    #include <asm/fixmap.h>
>>>    #include <asm/hvm/hvm.h>
>>>    #include <asm/hvm/support.h>
>>> +#include <asm/hvm/nestedhvm.h>
>>>    #include <asm/debugreg.h>
>>>    #include <asm/msr.h>
>>>    #include <asm/traps.h>
>>> @@ -704,9 +705,13 @@ int arch_set_info_guest(
>>>            /* PVH 32bitfixme */
>>>            ASSERT(!compat);
>>>
>>> -        if ( c(ctrlreg[1]) || c(ldt_base) || c(ldt_ents) ||
>>> +        if ( (c(ctrlreg[0]) & HVM_CR0_GUEST_RESERVED_BITS) ||
>>> +             (c(ctrlreg[4]) & HVM_CR4_GUEST_RESERVED_BITS(v)) ||
>>
>> Good idea. :-)
>>
>>> +             c(ctrlreg[1]) || c(ctrlreg[2]) || c(ctrlreg[5]) ||
>>> +             c(ctrlreg[6]) || c(ctrlreg[7]) || c(ldt_base) ||
>>> c(ldt_ents) ||
>>>                 c(user_regs.cs) || c(user_regs.ss) || c(user_regs.es) ||
>>>                 c(user_regs.ds) || c(user_regs.fs) || c(user_regs.gs) ||
>>> +             c(kernel_ss) || c(kernel_sp) || c.nat->gs_base_kernel ||
>>>                 c.nat->gdt_ents || c.nat->fs_base || c.nat->gs_base_user )
>>>                return -EINVAL;
>>>        }
>>> @@ -745,17 +750,20 @@ int arch_set_info_guest(
>>>
>>>        if ( has_hvm_container_vcpu(v) )
>>>        {
>>> -        /*
>>> -         * NB: TF_kernel_mode is set unconditionally for HVM guests,
>>> -         * so we always use the gs_base_kernel here. If we change this
>>> -         * function to imitate the PV functionality, we'll need to
>>> -         * make it pay attention to the kernel bit.
>>> -         */
>>> -        hvm_set_info_guest(v, compat ? 0 : c.nat->gs_base_kernel);
>>
>> I think we still need to call hvm_set_info_guest() here -- there is
>> other stuff being set up, and it was called (without the extra argument
>> for gs_base_kernel) before the PVH patch series went in.
>>
>> (See c/s 35b1e07.)
>
> I see, I will restore the hvm_set_info_guest call, good catch.
>
>>
>>>
>>>            if ( is_hvm_vcpu(v) || v->is_initialised )
>>>                goto out;
>>>
>>> +        if ( c.nat->ctrlreg[0] ) {
>>> +            v->arch.hvm_vcpu.guest_cr[0] |= c.nat->ctrlreg[0];
>>> +            hvm_update_guest_cr(v, 0);
>>> +        }
>>> +
>>> +        if ( c.nat->ctrlreg[4] ) {
>>> +            v->arch.hvm_vcpu.guest_cr[4] |= c.nat->ctrlreg[4];
>>> +            hvm_update_guest_cr(v, 4);
>>> +        }
>>
>> I thought the idea was that he would set cr0, cr3, and cr4 for all HVM
>> guests, rather than special-casing PVH guests?
>
> Well, I was just trying to make the PVH AP bringup interface stable,
> without messing much with HVM (not that we shouldn't aim to do it for
> HVM guests also), but I think the main concern now is releasing 4.4 with
> a stable interface for PVH guests.

Right -- I guess it's better safe than sorry wrt changing the HVM paths; 
we can make it PVH for now, and make it more generic for 4.5.

  -George

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