Re: [Xen-devel] [PATCH] iommu: specify page_count rather than page_order to iommu_map/unmap()...

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 22 January 2019 10:47
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>; Ian
> Jackson <Ian.Jackson@xxxxxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>;
> Wei Liu <wei.liu2@xxxxxxxxxx>; Sander Eikelenboom <linux@xxxxxxxxxxxxxx>;
> Chao Gao <chao.gao@xxxxxxxxx>; Jun Nakajima <jun.nakajima@xxxxxxxxx>;
> Kevin Tian <kevin.tian@xxxxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>;
> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org)
> <tim@xxxxxxx>
> Subject: RE: [PATCH] iommu: specify page_count rather than page_order to
> iommu_map/unmap()...
> 
> >>> On 21.01.19 at 14:22, <Paul.Durrant@xxxxxxxxxx> wrote:
> >>  -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: 21 January 2019 12:05
> >> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> >> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
> >> <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>;
> Ian
> >> Jackson <Ian.Jackson@xxxxxxxxxx>; Roger Pau Monne
> <roger.pau@xxxxxxxxxx>;
> >> Wei Liu <wei.liu2@xxxxxxxxxx>; Sander Eikelenboom
> <linux@xxxxxxxxxxxxxx>;
> >> Chao Gao <chao.gao@xxxxxxxxx>; Jun Nakajima <jun.nakajima@xxxxxxxxx>;
> >> Kevin Tian <kevin.tian@xxxxxxxxx>; Stefano Stabellini
> >> <sstabellini@xxxxxxxxxx>; xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>;
> >> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org)
> >> <tim@xxxxxxx>
> >> Subject: RE: [PATCH] iommu: specify page_count rather than page_order
> to
> >> iommu_map/unmap()...
> >>
> >> >>> On 21.01.19 at 12:56, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> >>  -----Original Message-----
> >> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> >> Sent: 21 January 2019 11:28
> >> >> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> >> >> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
> >> >> <Andrew.Cooper3@xxxxxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>;
> >> Wei
> >> >> Liu <wei.liu2@xxxxxxxxxx>; Sander Eikelenboom
> <linux@xxxxxxxxxxxxxx>;
> >> >> George Dunlap <George.Dunlap@xxxxxxxxxx>; Ian Jackson
> >> >> <Ian.Jackson@xxxxxxxxxx>; Chao Gao <chao.gao@xxxxxxxxx>; Jun
> Nakajima
> >> >> <jun.nakajima@xxxxxxxxx>; Kevin Tian <kevin.tian@xxxxxxxxx>; Stefano
> >> >> Stabellini <sstabellini@xxxxxxxxxx>; xen-devel <xen-
> >> >> devel@xxxxxxxxxxxxxxxxxxxx>; Konrad Rzeszutek Wilk
> >> >> <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>
> >> >> Subject: Re: [PATCH] iommu: specify page_count rather than
> page_order
> >> to
> >> >> iommu_map/unmap()...
> >> >>
> >> >> >>> On 18.01.19 at 17:03, <paul.durrant@xxxxxxxxxx> wrote:
> >> >> > ...and remove alignment assertions.
> >> >> >
> >> >> > Testing shows that certain callers of iommu_legacy_map/unmap()
> >> specify
> >> >> > order > 0 ranges that are not order aligned thus causing one of
> the
> >> >> > IS_ALIGNED() assertions to fire.
> >> >>
> >> >> As said before - without a much better explanation of why the
> current
> >> >> order-based model is unsuitable (so far I've been provided only
> vague
> >> >> pointers into "somewhere in PVH Dom0 boot code" iirc) to understand
> >> >> why it's undesirable to simply make those call sites obey to the
> >> current
> >> >> requirements, I'm not happy to see us go this route.
> >> >
> >> > I thought...
> >> >
> >> > "Using a count actually makes more sense because the valid
> >> > set of mapping orders is specific to the IOMMU implementation and to
> it
> >> > should be up to the implementation specific code to translate a
> mapping
> >> > count into an optimal set of mapping orders (when the code is finally
> >> > modified to support orders > 0)."
> >> >
> >> > ...was reasonably clear. Is that not a reasonable justification? What
> >> else
> >> > could I say?
> >>
> >> Well, I was hoping to be pointed at the (apparently multiple) call
> sites
> >> where making them match the current function pattern is more involved
> >> and/or less desirable than changing the functions here.
> >
> > AFAICT, one of them is memory.c:populate_physmap() where the extent
> order
> > comes from the memop_args and the memory comes from
> alloc_domheap_pages(),
> > which I don't believe aligns memory on the specified order.
> 
> Of course it does (in MFN space). What I notice is that the gpfn passed
> in is not validated to be suitably aligned for the specified order. With
> guest_physmap_add_entry() processing each 4k page separately this
> doesn't currently have any bad effects, but I think it's a bug
> nevertheless. After all the comment in struct xen_memory_reservation's
> declaration says "size/alignment of each". The issue with fixing flaws
> like this is that there's always the risk of causing regressions with
> existing guests.
> 
> > Regardless of the
> > alignment though, the fact that order comes from a hypercall argument
> and may
> > not match any of the orders supported by the IOMMU implementation makes
> me
> > think that using a page count is better.
> 
> Splitting up guest requests is orthogonal to whether a count or an
> order is more suitable as a parameter.

Ok, I'm not prepared to argue the point any more. I withdraw the patch.

  Paul

> 
> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel