Re: [PATCH v10 09/10] virtio-iommu: Set supported page size mask

[Jean-Philippe Brucker]

On Mon, Oct 19, 2020 at 05:35:39PM -0400, Peter Xu wrote:
> > +    /*
> > +     * Disallow shrinking the page size. For example if an endpoint only
> > +     * supports 64kB pages, we can't globally enable 4kB pages. But that
> > +     * shouldn't happen, the host is unlikely to setup differing page 
> > granules.
> > +     * The other bits are only hints describing optimal block sizes.
> > +     */
> > +    if (new_granule < old_granule) {
> > +        error_setg(errp, "memory region shrinks the virtio-iommu page 
> > granule");
> > +        return -1;
> > +    }
> 
> My understanding is that shrink is actually allowed, instead we should forbid
> growing of the mask?  For example, initially the old_granule will always 
> points
> to the guest page size.  Then as long as the host page size (which new_granule
> represents) is smaller than the old_granule, then it seems fine... Or am I 
> wrong?

The case I was checking against is two assigned devices with different
page sizes. First one sets a 64kB page size, then the second one shouldn't
be able to shrink it back to 4kB, because the guest would create mappings
not aligned on 64kB, which can't be applied by the pIOMMU of the first
device.

But let's forget this case for now, in practice all assigned devices use
the host page size.

> 
> Another thing, IIUC this function will be majorly called in vfio code when the
> container page mask will be passed into it.  If there're multiple vfio
> containers that support different host IOMMU page sizes, then IIUC the order 
> of
> the call to virtio_iommu_set_page_size_mask() is undefined.  It's probably
> related to which "-device vfio-pci,..." parameter is earlier.
> 
> To make this simpler, I'm thinking whether we should just forbid the case 
> where
> devices have different iommu page sizes.  So when assigned devices are used, 
> we
> make sure all host iommu page sizes are the same, and the value should be
> smaller than guest page size.  Otherwise we'll simply fall back to guest 
> psize.

Mostly agree, I need to simplify this function.

I don't think we care about guest page size, though. Currently our default
mask is TARGET_PAGE_MASK, which is the smallest size supported by vCPUs
(generally 4kB), but it doesn't really mean guest page size, since the
guest can choose a larger granule at runtime. Besides virtio-iommu can in
theory map at byte granule if there isn't any assigned device, so our
default mask could as well be ~0ULL (but doesn't work at the moment, I've
tried).

So what I'd like to do for next version:

* Set qemu_real_host_page_mask as the default page mask, instead of the
  rather arbitrary TARGET_PAGE_MASK. Otherwise we cannot hotplug assigned
  devices on a 64kB host, since TARGET_PAGE_MASK is pretty much always
  4kB.

* Disallow changing the page size. It's simpler and works in
  practice if we default to qemu_real_host_page_mask.

* For non-hotplug devices, allow changing the rest of the mask. For
  hotplug devices, only warn about it.

Thanks,
Jean