Re: [Xen-devel] [PATCH 08/17] [V3]PVH xen: domain creation code changes

[Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>]

On Mon, 15 Apr 2013 16:35:35 +0100
"Jan Beulich" <JBeulich@xxxxxxxx> wrote:

> >>> On 13.04.13 at 03:02, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> >>> wrote:
> > @@ -859,19 +868,26 @@ int arch_set_info_guest(
> >  
> >          if ( !cr3_page )
> >          {
> > -            destroy_gdt(v);
> > +            if ( !is_pvh_vcpu(v) )
> > +                destroy_gdt(v);
> 
> Can't the check rather be done in destroy_gdt(), not the least
> because the pattern here repeats further down?
> 
> > @@ -938,6 +955,13 @@ int arch_set_info_guest(
> >  
> >      update_cr3(v);
> >  
> > +    if ( is_pvh_vcpu(v) )
> 
> Can you get here for a PVH vCPU? The update_cr3() right before
> that is suspicious - you shouldn't need that for PVH.

Yes. The call will result in call to vmx_update_pvh_cr which will
vmwrite GUEST_CR3.

> > @@ -968,16 +992,21 @@ void arch_vcpu_reset(struct vcpu *v)
> >  static void
> >  unmap_vcpu_info(struct vcpu *v)
> >  {
> > -    unsigned long mfn;
> > +    unsigned long mfn, *mfnp;
> > +
> > +    if ( is_pvh_vcpu(v) )
> > +        mfnp = &v->arch.hvm_vcpu.hvm_pvh.vcpu_info_mfn;
> > +    else
> > +        mfnp = &v->arch.pv_vcpu.vcpu_info_mfn;
> 
> This suggests you want to pull out the vcpu_info_mfn field, at
> once also making it available for future use in HVM guests.

Hmmm.. thats why I created separate pvh struct, altho in HVM struct
but to make it clear it was PVH only. Where would you wanna see it
put?

> > @@ -639,7 +639,8 @@ static void hap_update_cr3(struct vcpu *v, int
> > do_locking) const struct paging_mode *
> >  hap_paging_get_mode(struct vcpu *v)
> >  {
> > -    return !hvm_paging_enabled(v)   ? &hap_paging_real_mode :
> > +    return is_pvh_vcpu(v) ? &hap_paging_long_mode :
> > +        !hvm_paging_enabled(v)   ? &hap_paging_real_mode :
> >          hvm_long_mode_enabled(v) ? &hap_paging_long_mode :
> >          hvm_pae_enabled(v)       ? &hap_paging_pae_mode  :
> >                                     &hap_paging_protected_mode;
> 
> In the series description you say that only 32-bit kernel support is
> missing, yet this doesn't look right for a 32-bit PVH guest.

Both, the changes in kernel, and changes in xen are missing.
When ready to work on 32bit support, I was gonna start with this patch
series looking for places, but I could tag this funct to find it easily 
too.

> > --- a/xen/include/asm-x86/hvm/vcpu.h
> > +++ b/xen/include/asm-x86/hvm/vcpu.h
> > @@ -104,6 +104,13 @@ struct nestedvcpu {
> >  
> >  #define vcpu_nestedhvm(v) ((v)->arch.hvm_vcpu.nvcpu)
> >  
> > +/* add any PVH specific fields here */
> > +struct pvh_hvm_vcpu_ext
> > +{
> > +    /* Guest-specified relocation of vcpu_info. */
> > +    unsigned long vcpu_info_mfn;
> > +};
> > +
> >  struct hvm_vcpu {
> >      /* Guest control-register and EFER values, just as the guest
> > sees them. */
> >      unsigned long       guest_cr[5];
> > @@ -170,6 +177,8 @@ struct hvm_vcpu {
> >      struct hvm_trap     inject_trap;
> >  
> >      struct viridian_vcpu viridian;
> > +
> > +    struct pvh_hvm_vcpu_ext hvm_pvh;
> 
> Same here - hvm_pvh_ (or equally pvh_hvm_) just make no sense.

It reminds reader that pvh struct is part of hvm struct, and it helps
with greping/cscoping to find the field. Lmk what name you'd like,
I really don't care at this point :)....

Perhaps, pvh_hvm_vcpu_ext  should be called hvm_pvh_vcpu_ext

> Also, as you add this to hvm_vcpu and iirc you only dropped
> the union with the PV side for arch_domain - are you not using
> _any_ field in pv_vcpu?

Nop. the only field from pv_vcpu needed for pvh is vcpu_info_mfn. 

thanks
Mukesh

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