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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.04.13 at 03:00, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> 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:
>> > @@ -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?

If more than one mode needs it, it should be part of arch_vcpu,
not any of the sub-structures.

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

The you should also state so in the overview mail.

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

Not tagging these places is just calling for problems once you start
that work, especially if they sit in rarely executed code paths.

>> > --- 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 :)....

But the point is that PVH isn't s sub-mode of HVM, and hence the
structure shouldn't be a sub-structure either.

> Perhaps, pvh_hvm_vcpu_ext  should be called hvm_pvh_vcpu_ext

It doesn't matter which order you put these - two prefixes are
just wrong. If it's a PVH construct, pvh_ is the prefix to go with.

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

Okay - see above.

Jan


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