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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

Jan


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