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

[Alex Williamson]

On Thu, 2015-12-03 at 17:36 +0000, Peter Maydell wrote:
> On 3 December 2015 at 17:19, Alex Williamson <address@hidden> wrote:
> > On Thu, 2015-12-03 at 16:33 +0000, Peter Maydell wrote:
> >> On 3 December 2015 at 16:26, Alex Williamson <address@hidden> wrote:
> >> > 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... */
> >> > }
> >>
> >> This looks very odd to me, in two ways: (a) behaving differently
> >> for PCI passthrough vs other kinds of passthrough,
> >
> > It's a matter of risk.  If we align an MMIO range out of existence all
> > we've prevented is peer-to-peer DMA between assigned devices.  Chances
> > of anyone caring about that are slim to none.  If we align RAM out of
> > existence, that's a much, much more significant risk that we've just
> > introduced a data integrity issue for the VM.
> 
> I don't see why this is different for PCI devices versus
> memory-mapped passthrough devices, though. If what you mean
> is "is this MemoryRegion not RAM" maybe you want
> if (!memory_region_is_ram(mr))  ?

We would already skip it entirely if that worked, see
hw/vfio/common.c:vfio_listener_skipped_section().  MemoryRegions backed
by a ram ptr still sneak through that.  We're dealing with sub-host-page
MemoryRegionSections that are backed by an mmap to a vfio region.

> >>  and (b) caring
> >> about TARGET_PAGE_ALIGN at all. TARGET_PAGE_ALIGN really isn't
> >> something vfio should need to care about I think.
> >
> > But I think we do.  If a RAM address is target page aligned, it could be
> > a valid DMA target for the device.
> 
> TARGET_PAGE_ALIGN doesn't tell you whether an address is actually
> page aligned for the guest, though. In fact, you can't tell what
> page size the guest happens to be using (or what the alignment
> restrictions on doing DMA might be, or the page size being used by
> the IOMMU, which isn't necessarily the guest page size either).

TAGET_PAGE_ALIGN tells us that it *could* be a valid DMA target though.
The VM model is capable of using that as a page size, which means we
assume it is and want to generate a fault.

> > If we align it out of existence and
> > the device is programmed to perform a DMA to that address, the IOMMU
> > will block it, the VM will not be informed and will continue executing
> > with invalid data.
> 
> Shouldn't this cause the device to say "hey, my DMA transaction
> failed, I will flag that up as an error" ? (That's not much better
> as a failure situation, of course.)

DMA reads can get a target abort, DMA writes fail silently, other than
the possible IOMMU fault from the IOMMU driver, for which we have
nothing resembling an infrastructure for collecting that and reporting
it out to the user.  Thanks,

Alex