Re: [Xen-devel] [PATCH 02/18] PVH xen: add XENMEM_add_to_physmap_range

[Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>]

On Fri, 31 May 2013 11:14:03 +0100
"Jan Beulich" <JBeulich@xxxxxxxx> wrote:

> >>> On 31.05.13 at 11:38, Ian Campbell <Ian.Campbell@xxxxxxxxxx>
> >>> wrote:
> > On Fri, 2013-05-31 at 10:28 +0100, Jan Beulich wrote:
> >> >>> On 25.05.13 at 03:25, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> >> >>> wrote:
> >> >  static int xenmem_add_to_physmap_once(
> >> >      struct domain *d,
> >> > -    const struct xen_add_to_physmap *xatp)
> >> > +    const struct xen_add_to_physmap *xatp,
> >> > +    domid_t foreign_domid)
> >> 
> >> The patch could be a bit smaller afaict if you used the otherwise
> >> unused here domain ID field in xatp for passing the domain ID you
> >> care about here (I hinted at that in the last round already, where
> >> I also asked Stefano why we have three domains here in the first
> >> place).
> > 
> > This interface is already used on ARM, I don't think we want PVH to
> > use a different variation of the same thing so I don't think it is
> > right to put this on Mukesh.
> 
> I expressed this wrong if you understood it this way: I'm not
> asking Mukesh to alter the interface - I had asked Stefano why
> it was the way it is, and while I'm not happy with the situation,
> I appreciate that changing it again is not a very good idea.
> 
> All I'm asking is, instead of introducing a new function parameter
> here, to use an otherwise unused field of the xatp input
> structure. This is based on the fact that the field is being
> evaluated once directly in arch_memory_op(), and then no longer
> needed.

Not a good programming practice IMO to alias fields unnecessarily. The 
documented purpose of the domid field in struct is not same as the 
parameter being passed:

struct xen_add_to_physmap {
    /* Which domain to change the mapping for. */
    domid_t domid;

As the function grows, and struct gets passed further down, it would be 
bewildering to someone reading and understanding the code. 

Since we've already discussed the existence of two domid fields, it seems
appropriate to leave it as it is.

> That would also allow doing the setting just once before loops get
> started, rather than having to pass the same value again on each
> loop iteration.
> 
> The one caveat is that for the continuation creation of
> XENMEM_add_to_physmap the field would need to be prevented
> from getting written back to guest memory. But I consider it bad
> practice anyway to copy back whole structures when only
> selected fields (often just one) need updating, and hence
> switching that operation to do a couple of individual field writes
> would be nice regardless of that change, even if this adds two or
> three lines of code.

Right, since this patch only deals with XENMEM_add_to_physmap_range, I'll
not worry about that.

thanks,
Mukesh

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