Re: [Xen-devel] [V10 PATCH 10/23] PVH xen: domain create, context switch related code changes

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.08.13 at 17:32, George Dunlap <George.Dunlap@xxxxxxxxxxxxx> wrote:
> On Wed, Jul 24, 2013 at 2:59 AM, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> 
> wrote:
>> This patch mostly contains changes to arch/x86/domain.c to allow for a PVH
>> domain creation. The new function pvh_set_vcpu_info(), introduced in the
>> previous patch, is called here to set some guest context in the VMCS.
>> This patch also changes the context_switch code in the same file to follow
>> HVM behaviour for PVH.
>>
>> Changes in V2:
>>   - changes to read_segment_register() moved to this patch.
>>   - The other comment was to create NULL functions for pvh_set_vcpu_info
>>     and pvh_read_descriptor which are implemented in later patch, but since
>>     I disable PVH creation until all patches are checked in, it is not 
> needed.
>>     But it helps breaking down of patches.
>>
>> Changes in V3:
>>   - Fix read_segment_register() macro to make sure args are evaluated once,
>>     and use # instead of STR for name in the macro.
>>
>> Changes in V4:
>>   - Remove pvh substruct in the hvm substruct, as the vcpu_info_mfn has been
>>     moved out of pv_vcpu struct.
>>   - rename hvm_pvh_* functions to hvm_*.
>>
>> Changes in V5:
>>   - remove pvh_read_descriptor().
>>
>> Changes in V7:
>>   - remove hap_update_cr3() and read_segment_register changes from here.
>>
>> Signed-off-by: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>>  xen/arch/x86/domain.c |   56 
>> ++++++++++++++++++++++++++++++++----------------
>>  xen/arch/x86/mm.c     |    3 ++
>>  2 files changed, 40 insertions(+), 19 deletions(-)
>>
>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
>> index c361abf..fccb4ee 100644
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -385,7 +385,7 @@ int vcpu_initialise(struct vcpu *v)
>>
>>      vmce_init_vcpu(v);
>>
>> -    if ( is_hvm_domain(d) )
>> +    if ( !is_pv_domain(d) )
>>      {
>>          rc = hvm_vcpu_initialise(v);
>>          goto done;
>> @@ -452,7 +452,7 @@ void vcpu_destroy(struct vcpu *v)
>>
>>      vcpu_destroy_fpu(v);
>>
>> -    if ( is_hvm_vcpu(v) )
>> +    if ( !is_pv_vcpu(v) )
>>          hvm_vcpu_destroy(v);
>>      else
>>          xfree(v->arch.pv_vcpu.trap_ctxt);
>> @@ -464,7 +464,7 @@ int arch_domain_create(struct domain *d, unsigned int 
> domcr_flags)
>>      int rc = -ENOMEM;
>>
>>      d->arch.hvm_domain.hap_enabled =
>> -        is_hvm_domain(d) &&
>> +        !is_pv_domain(d) &&
>>          hvm_funcs.hap_supported &&
>>          (domcr_flags & DOMCRF_hap);
>>      d->arch.hvm_domain.mem_sharing_enabled = 0;
>> @@ -512,7 +512,7 @@ int arch_domain_create(struct domain *d, unsigned int 
> domcr_flags)
>>      mapcache_domain_init(d);
>>
>>      HYPERVISOR_COMPAT_VIRT_START(d) =
>> -        is_hvm_domain(d) ? ~0u : __HYPERVISOR_COMPAT_VIRT_START;
>> +        is_pv_domain(d) ? __HYPERVISOR_COMPAT_VIRT_START : ~0u;
>>
>>      if ( (rc = paging_domain_init(d, domcr_flags)) != 0 )
>>          goto fail;
>> @@ -555,7 +555,7 @@ int arch_domain_create(struct domain *d, unsigned int 
> domcr_flags)
>>      }
>>      spin_lock_init(&d->arch.e820_lock);
>>
>> -    if ( is_hvm_domain(d) )
>> +    if ( !is_pv_domain(d) )
>>      {
>>          if ( (rc = hvm_domain_initialise(d)) != 0 )
>>          {
>> @@ -651,7 +651,7 @@ int arch_set_info_guest(
>>  #define c(fld) (compat ? (c.cmp->fld) : (c.nat->fld))
>>      flags = c(flags);
>>
>> -    if ( !is_hvm_vcpu(v) )
>> +    if ( is_pv_vcpu(v) )
>>      {
>>          if ( !compat )
>>          {
>> @@ -704,7 +704,7 @@ int arch_set_info_guest(
>>      v->fpu_initialised = !!(flags & VGCF_I387_VALID);
>>
>>      v->arch.flags &= ~TF_kernel_mode;
>> -    if ( (flags & VGCF_in_kernel) || is_hvm_vcpu(v)/*???*/ )
>> +    if ( (flags & VGCF_in_kernel) || !is_pv_vcpu(v)/*???*/ )
>>          v->arch.flags |= TF_kernel_mode;
>>
>>      v->arch.vgc_flags = flags;
>> @@ -719,7 +719,7 @@ int arch_set_info_guest(
>>      if ( !compat )
>>      {
>>          memcpy(&v->arch.user_regs, &c.nat->user_regs, 
>> sizeof(c.nat->user_regs));
>> -        if ( !is_hvm_vcpu(v) )
>> +        if ( is_pv_vcpu(v) )
>>              memcpy(v->arch.pv_vcpu.trap_ctxt, c.nat->trap_ctxt,
>>                     sizeof(c.nat->trap_ctxt));
>>      }
>> @@ -735,10 +735,13 @@ int arch_set_info_guest(
>>
>>      v->arch.user_regs.eflags |= 2;
>>
>> -    if ( is_hvm_vcpu(v) )
>> +    if ( !is_pv_vcpu(v) )
>>      {
>>          hvm_set_info_guest(v);
>> -        goto out;
>> +        if ( is_hvm_vcpu(v) || v->is_initialised )
>> +            goto out;
>> +        else
>> +            goto pvh_skip_pv_stuff;
>>      }
>>
>>      init_int80_direct_trap(v);
>> @@ -853,6 +856,7 @@ int arch_set_info_guest(
>>
>>      set_bit(_VPF_in_reset, &v->pause_flags);
>>
>> + pvh_skip_pv_stuff:
> 
> Any idea what this set_bit(_VPF_in_reset) stuff is?  It looks like
> it's set above, and cleared down near the bottom of the function if
> nothing gets screwed up.

This is related to the preemptible vCPU reset (which
arch_set_info_guest() just re-uses), making sure that while there
is an incomplete state update for a vCPU 8because it may have got
preempted) the vCPU can't be unpaused.

> It seems like if that set/clear pair is important, then PVH should do
> them both as well, shouldn't it?

I thought I had checked this once - does it now bypass one of the
two?

But then again, this is all about PV memory management, so perhaps
it was that way when I checked, and I decided it was fine.

Jan

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