[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH for-4.12 1/8] dom0/pvh: align allocation and mapping order to start address

On Tue, Feb 05, 2019 at 12:42:02AM -0700, Jan Beulich wrote:
> >>> On 04.02.19 at 18:11, <roger.pau@xxxxxxxxxx> wrote:
> > On Mon, Feb 04, 2019 at 09:41:34AM -0700, Jan Beulich wrote:
> >> >>> On 30.01.19 at 11:36, <roger.pau@xxxxxxxxxx> wrote:
> >> > Due to the recent changes in the iommu mapping logic, the start
> >> > addresses provided need to be aligned to the order intended to be
> >> > mapped.
> >> 
> >> Irrespective of your reply to Wei's similar request (where you've
> >> provided links to mails showing crashes) I'd like you to explain
> >> this better. This is in particular because I don't really see what
> >> "recent changes in the iommu mapping logic" you talk about.
> > 
> > Commit 725bf00a87f ("iommu / p2m: add a page_order parameter to
> > iommu_map/unmap_page()...") added the following two asserts to
> > iommu_map:
> > 
> > ASSERT(IS_ALIGNED(dfn_x(dfn), (1ul << page_order)));
> > ASSERT(IS_ALIGNED(mfn_x(mfn), (1ul << page_order)));
> > 
> > Previously iommu_map would add unaligned entries without complaining,
> > but now in debug builds the assert will trigger.
> Right, but the assertions were added to ensure expected behavior,
> not to change anything. It was a bug to call the functions without
> suitably aligned frame numbers.

Hm, OK. As a note p2m related map/unmap functions will still work
correctly when called with non-aligned addresses and orders, and will
then call the iommu helpers with those addresses and orders. In fact
the code that triggers the asserts in the bug reports provided are
from calls into the iommu helpers made by the p2m mapping functions.

Although I understand your point about the alignment none of the p2m
or iommu related functions ever had this requirement in the code, so
users of those interface were not aware of such requirement until the
assert was added.

> > Maybe adding a 'Fixes' tag would help identifying what this commit is
> > trying to address?
> Yes, but as per above it wouldn't be the commit adding the assertions
> that the one here fixes (and hence I think the wording of the commit
> message ought to change as well).

Would you be fine with the following message:

The p2m and iommu mapping code always had the requirement that
addresses and orders must be aligned when populating the p2m or the
iommu page tables.

PVH dom0 builder didn't take this requirement into account, and can
call into the p2m/iommu mapping helpers with addresses and orders that
are not aligned.

Fix this by making sure the orders passed to the physmap population
helpers are always aligned to the guest address to be populated.

Thanks, Roger.

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.