From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [V10 PATCH 10/23] PVH xen: domain create, context switch related code changes Date: Thu, 8 Aug 2013 10:14:56 +0100 Message-ID: <52036190.1080604@eu.citrix.com> References: <1374631171-15224-1-git-send-email-mukesh.rathor@oracle.com> <1374631171-15224-11-git-send-email-mukesh.rathor@oracle.com> <20130807184241.6f536448@mantra.us.oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20130807184241.6f536448@mantra.us.oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Mukesh Rathor Cc: "xen-devel@lists.xensource.com" , "keir.xen@gmail.com" List-Id: xen-devel@lists.xenproject.org On 08/08/13 02:42, Mukesh Rathor wrote: > On Wed, 7 Aug 2013 11:48:26 +0100 > George Dunlap wrote: > >> On Wed, Jul 24, 2013 at 2:59 AM, Mukesh Rathor >> 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