Re: [Xen-devel] [PATCH v3] x86/p2m: use large pages for MMIO mappings

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Fri, 2016-01-15 at 07:39 -0700, Jan Beulich wrote:
> > > > On 15.01.16 at 14:57, <ian.campbell@xxxxxxxxxx> wrote:
> > On Fri, 2016-01-15 at 03:47 -0700, Jan Beulich wrote:
> > > > > > On 15.01.16 at 11:09, <ian.campbell@xxxxxxxxxx> wrote:
> > > > On Thu, 2016-01-14 at 03:04 -0700, Jan Beulich wrote:
> > > > > - ARM side unimplemented (and hence libxc for now made cope with
> > > > > both
> > > > > Â models),
> > > > 
> > > > So, one model is the one described in the commit message:
> > > > 
> > > > > - zero (success, everything done)
> > > > > - positive (success, this many done, more to do: re-invoke)
> > > > > - negative (error)
> > > > 
> > > > What is the other one? I'd expect ARM to already implement a subset
> > > > of
> > > > this
> > > > (i.e. 0 or negative, perhaps with a subset of the possible errno
> > > > values), 
> > > > which I'd then expect libxc to just cope with without it
> > > > constituting a
> > > > second model.
> > > 
> > > Well, first of all ARM doesn't get switched away from the current
> > > model (yet), i.e. returning -E2BIG out of do_domctl().
> > 
> > Which AFAICT is a valid behaviour under the new model described in the
> > commit message specifically the "negative (error)" case.
> > 
> > I think the core of my objection/confusion here is describing this as
> > two
> > different models when what is being introduced is a single API which
> > can
> > fail either partially or entirely, with that being at the discretion of
> > the
> > internals. In any case libxc needs to cope with the complete gamut of
> > behaviours of the interface.
> > 
> > IOW rather than describing a new API and referring obliquely to ARM
> > only
> > supporting an old model I think this needs a complete description of
> > the
> > interface covering the full possibilities of the API.
> > 
> > > ÂAnd then
> > > the difference between what the patch implements and what the
> > > non-commit message comment describes is how errors get handled:
> > > The patch makes a negative error value returned upon error, with
> > > the caller having no way to tell at what point the error occurred
> > > (and with a best effort undo in the case of "map"). The described
> > > alternative would return the number of succeeded entries unless
> > > an error occurred on the very first MFN, without any attempt to
> > > undo the part that was done successfully. I.e. it would leave it
> > > to the caller to decide what to do, and whether/when to roll back.
> > 
> > That's (probably, I don't quite follow all the details as written)
> > fine,
> > but the interface should be described as a single API with the possible
> > failure cases each spelled out, i.e. not described as a split/contrast
> > between old vs. new or x86 vs. arm behaviour. The fact that x86 and arm
> > might currently exhibit different subsets of the possibilities offered
> > by
> > the API is of at best secondary interest.
> 
> I don't think I agree - there are two models. The meaning of
> -E2BIG for the caller to retry with a smaller amount doesn't exist in
> the new model anymore, and hence libxc wouldn't need to deal
> with that case anymore if the ARM side got updated too.

If ARM still has this behaviour then it is still part of the interface
IMHO, whether or not x86 chooses to use this particular possibility or not.

>  Whereas
> positive return values don't exist in the present (prior to the patch)
> model.

If there were two models in the way you suggest then there would surely be
an ifdef somewhere in libxc. The fact that the two behaviours can coexist
means to me that they are two halves of the same model (irrespective of
arch code opting in to different halves, and irrespective if having updated
ARM there are then fewer possible error cases and a follow up
simplification to libxc).

Anyway, the current three-bullet point description of the new ABI in the
commit message is clearly insufficient for the complexity whether we want
to split hairs about how many models there are here or not.

At the very least the interface (_all_ aspects of it) should be thoroughly
described in domctl.h next toÂXEN_DOMCTL_memory_mapping (which I just
noticed describes E2BIG and isn't changed here at all).

Ian.

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