Re: [Qemu-devel] [PATCH] vfio: Align iova also to IOMMU page size

[Alex Williamson]

On Thu, 2015-12-03 at 12:02 +0300, Pavel Fedin wrote:
>  Hello!
> 
> > >  My device defines this BAR to be of 2M size. In this case qemu splits it 
> > > up into three
> > > regions:
> > >  1) Region below the MSI-X table (it's called "mmap", for me it's empty 
> > > because table offset
> > > is 0)
> > >  2) MSI-X table itself (20 vectors = 0x00000140 bytes for me).
> > >  3) Region above the MSi-X table (it's called "mmap msix-hi"). Size for 
> > > my case is 2M -
> > > REAL_HOST_PAGE_ALIGN(0x140) = 0x1FF000.
> > > Regions (1) and (3) are passed through and directly mapped by VFIO, 
> > > region (2) is emulated.
> > >  Now, we have PBA. The device says that PBA is placed in memory specified 
> > > by the same BAR as
> > > table, and its offset is 0x000f0000. PBA is also emulated by qemu, and as 
> > > a result it overlaps
> > > "mmap msix-hi" region, which breaks up into two fragments as a 
> > > consequence:
> > >  3a) offset 0x00000 size 0x0EF000
> > >  3b) offset 0xEF008 size 0x10FFF8
> > >  PBA sits between these fragments, having only 8 bytes in size.
> > >  Attempt to map (3b) causes the problem. vfio_mmap_region() is expected 
> > > to align this up,
> > > and it does, but it does it to a minimum possible granularity for ARM 
> > > platform, which, as qemu
> > > thinks, is always 1K.
> > 
> > Yep, on x86 we'd have target page size == host page size == minimum
> > iommu page size and we'd align that up to something that can be mapped,
> > which means we wouldn't have an iommu mapping for peer-to-peer in this
> > space, but nobody is likely to notice.
> 
>  So, OK, to keep the quoting short... I've also read your previous reply 
> about that "rounding up just works". Let's sum things up.
> 
> 1. Rounding up "just works" in my case too. So i don't see a direct reason to 
> change it.
> 2. The only problem is rounding up to 1K, which TARGET_PAGE_ALIGN() does on 
> ARM. What we need is 4K,
>    which is appropriate for host too. And, by the way, for modern ARM guests 
> too. So, we can do either of:
>    a) Keep my original approach - TARGET_PAGE_ALIGN(), then align to 
> vfio_container_granularity().
>    b) Just align to vfio_container_granularity() instead of using 
> TARGET_PAGE_ALIGN().
>    c) Use HOST_PAGE_ALIGN() instead of TARGET_PAGE_ALIGN() (what Peter 
> suggested).
> 
>    On x86 there would be no difference, because all these alignments are 
> identical. On ARM, actually, all of these approaches would also give correct 
> result, because it's only TARGET_PAGE_ALIGN() wrong in this case. So - what 
> do you think is the most appropriate? Let's make a choice and i'll send a 
> proper patch.

I feel a lot more comfortable if we limit the scope to MMIO regions of
PCI devices.  The problems I brought up before about the device not
being able to DMA to a target aligned RAM address are still a
possibility that I think we want to catch.  To do that, I think we just
need:

Object *obj = memory_region_owner(section->mr);

if (object_dynamic_cast(obj, "pci-device")) {
    /* HOST_PAGE_ALIGN... */
} else {
    /* TARGET_PAGE_ALIGN... */
}

Thanks,
Alex