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

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

On 08/08/13 02:42, Mukesh Rathor wrote:
> On Wed, 7 Aug 2013 11:48:26 +0100
> George Dunlap <George.Dunlap@eu.citrix.com> wrote:
>
>> On Wed, Jul 24, 2013 at 2:59 AM, Mukesh Rathor
>> <mukesh.rathor@oracle.com> 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