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

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

On 08/08/13 02:42, Mukesh Rathor wrote:
> On Wed, 7 Aug 2013 11:48:26 +0100
> George Dunlap <George.Dunlap@xxxxxxxxxxxxx> wrote:
>
> > On Wed, Jul 24, 2013 at 2:59 AM, Mukesh Rathor
> > <mukesh.rathor@xxxxxxxxxx> wrote:
> ..........
> > > +
> > >           v->arch.guest_table = pagetable_from_page(cr3_page);
> > > -        if ( c.nat->ctrlreg[1] )
> > > +        if ( c.nat->ctrlreg[1] && !is_pvh_vcpu(v) )
> > >           {
> > >               cr3_gfn = xen_cr3_to_pfn(c.nat->ctrlreg[1]);
> > >               cr3_page = get_page_from_gfn(d, cr3_gfn, NULL,
> > > P2M_ALLOC); @@ -954,6 +965,13 @@ int arch_set_info_guest(
> > >
> > >       update_cr3(v);
> > >
> > > +    if ( is_pvh_vcpu(v) )
> > > +    {
> > > +        /* Set VMCS fields. */
> > > +        if ( (rc = pvh_set_vcpu_info(v, c.nat)) != 0 )
> > > +            return rc;
> > > +    }
> > BTW, would it make sense to pull the code above, which sets FS and GS
> > for PV guests into a separate function, and then do something like the
> > following?
> >
> >   is_pv_vcpu(v) ?
> >     pv_set_vcpu_info() :
> >     pvh_set_vcpu_info();
> Perhaps! But, these patches have been out for about 7 months now, and
> are tested by us and Andrew for PV and HVM regressions. I prefer not
> changing common code at this point, unless buggy. May be we can do an
> incremental change later if you really want this function to be
> re-org'd.
I'm keen to get the patches in, but I'm also keen on making the changes 
as clean as possible. It's a lot harder to go through and clean things 
up later.  I don't think this level of re-arranging should be too much 
additional delay, or have very much risk of regression.
Obviously the other maintainers may feel differently.  I'm sorry I'm a 
bit late to the party and only now able to find time to give my input.  
Once I have a view of the whole series, I can come back and give maybe a 
"Not-perfectly-happy-but-wont-NACK" or something like that, and others 
can decide whether to hold out for the change or just get it done with. :-)
BTW the testing thing I don't think is a valid argument; we're early in 
the dev cycle, so a regression isn't a big deal; and in any case the 
risk of regression is the same whether the change happens before it's 
committed or afterwards.
 -George

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