From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [V7 PATCH 3/7] pvh dom0: implement XENMEM_add_to_physmap_range for x86 Date: Wed, 18 Dec 2013 10:34:31 +0000 Message-ID: <52B18847020000780010E9EC@nat28.tlf.novell.com> References: <1387247911-28846-1-git-send-email-mukesh.rathor@oracle.com> <1387247911-28846-4-git-send-email-mukesh.rathor@oracle.com> <52B05A9A020000780010E22C@nat28.tlf.novell.com> <1387288771.27441.59.camel@kazak.uk.xensource.com> <52B06F8B020000780010E2B8@nat28.tlf.novell.com> <1387291240.27441.66.camel@kazak.uk.xensource.com> <52B0779B020000780010E366@nat28.tlf.novell.com> <52B16303020000780010E882@nat28.tlf.novell.com> <1387361225.27441.91.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VtESX-0006Uv-88 for xen-devel@lists.xenproject.org; Wed, 18 Dec 2013 10:34:33 +0000 In-Reply-To: <1387361225.27441.91.camel@kazak.uk.xensource.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: george.dunlap@eu.citrix.com, xen-devel , Keir Fraser , tim@xen.org List-Id: xen-devel@lists.xenproject.org >>> On 18.12.13 at 11:07, Ian Campbell wrote: > On Wed, 2013-12-18 at 07:55 +0000, Jan Beulich wrote: >> >>> On 17.12.13 at 16:11, "Jan Beulich" wrote: >> >>>> On 17.12.13 at 15:40, Ian Campbell wrote: >> >> On Tue, 2013-12-17 at 14:36 +0000, Jan Beulich wrote: >> >>> >>> On 17.12.13 at 14:59, Ian Campbell wrote: >> >>> > We could change the code but we could also tighten the interface >> >>> > requirements, either by explicit specifying that the range is handled in >> >>> > reverse order or by mandating that index/gpfn must not be repeated >> >>> > (whether or not we actively try and detect such cases). >> >>> >> >>> Specifying that this gets processed backwards would be, well, >> >>> backwards. Requiring no duplicates (or else getting undefined >> >>> behavior) would be possible. But processing the operation in the >> >>> conventional order doesn't seem all that hard. >> >> >> >> The reason I thought it would be tricky was finding somewhere to stash >> >> the progress over the continuation. Do you have a cunning plan? >> > >> > Just like we do in other cases - in the struct that was passed to >> > us by the caller (incrementing the handles and decrementing the >> > count as needed). >> >> And I was wrong with this - there's no proper precedent of us >> modifying hypercall interface structures except where certain >> fields are specified to be outputs. >> >> For XENMEM_add_to_physmap_range none of the fields is, so >> even copying back the size field (like currently done on ARM, >> and like also done for XENMEM_add_to_physmap's >> XENMAPSPACE_gmfn_range sub-case) isn't really correct. >> Instead the general method for encoding the continuation >> point in mem-ops is to put the resume index in the high bits of >> the first hypercall argument. >> >> Whether we want to change the specification here (clarifying >> that all of the structure may be modified by the hypervisor in >> the course of executing the hypercall) instead of fixing the >> implementation is open for discussion. > > Isn't x86's xenmem_add_to_physmap precedent here? It modifies idx, gpfn > and size which are not specified as outputs (more by omission than being > explicitly inputs, but the implication of the comments is that they are > inputs). Right, as I said above. Yet that went in without anyone noticing the inconsistent behavior, and hence I'd call it a bug. > I'm happy to change the spec here. I think in general very few guests > are going to be relying on the guest handle after the hypercall (they > have the original array in their hand) and in this specific case I know > that neither the x86 nor ARM implementation on Linux do so, and those > are the only existing dom0s which use this h/call right now I think. If a kernel chose to use static (say per-CPU) arrays and a static interface structure here, it might easily set the handles just once... > The alternative is to use MEMOP_EXTENT_SHIFT/MEMOP_CMD_MASK? I'd rather > avoid that if we don't have to... Admittedly it's not the nicest model, but that's how other memops work. Question really is whether we want to allow the inconsistency here, or generally allow the modification of documented to be input only interface structure fields. Jan