On Mon, Nov 02, 2020 at 05:47:25PM -0500, Peter Xu wrote:
> On Fri, Oct 30, 2020 at 07:05:09PM +0100, Jean-Philippe Brucker wrote:
> > From: Bharat Bhushan <bbhush...@marvell.com>
> >
> > The virtio-iommu device can deal with arbitrary page sizes for virtual
> > endpoints, but for endpoints assigned with VFIO it must follow the page
> > granule used by the host IOMMU driver.
> >
> > Implement the interface to set the vIOMMU page size mask, called by VFIO
> > for each endpoint. We assume that all host IOMMU drivers use the same
> > page granule (the host page granule). Override the page_size_mask field
> > in the virtio config space.
>
> (Nit: Seems slightly mismatched with the code below)
>
> [...]
>
> > + /*
> > + * After the machine is finalized, we can't change the mask anymore.
> > If by
> > + * chance the hotplugged device supports the same granule, we can still
> > + * accept it. Having a different masks is possible but the guest will
> > use
> > + * sub-optimal block sizes, so warn about it.
> > + */
> > + if (qdev_hotplug) {
> > + int new_granule = ctz64(new_mask);
> > + int cur_granule = ctz64(cur_mask);
> > +
> > + if (new_granule != cur_granule) {
> > + error_setg(errp, "virtio-iommu page mask 0x%"PRIx64
> > + " is incompatible with mask 0x%"PRIx64, cur_mask,
> > + new_mask);
> > + return -1;
> > + } else if (new_mask != cur_mask) {
> > + warn_report("virtio-iommu page mask 0x%"PRIx64
> > + " does not match 0x%"PRIx64, cur_mask, new_mask);
>
> IMHO, new_mask!=cur_mask is ok, as long as it's a superset of reported
> cur_mask, then it'll still guarantee to work.
>
> Meanwhile, checking against granule seems not safe enough if the guest driver
> started to use huge pages in iommu pgtables...
As the guest doesn't directly touch the page tables I think it is safe,
albeit slow. If the host IOMMU driver cannot support one huge page for a
mapping, then it will use several small pages instead.
>
> In summary:
>
> if (qdev_hotplug) {
> if ((new_mask & cur_mask) == cur_mask) {
> /* Superset of old mask; we're good. Keep the old mask since
> same */
> return 0;
That looks correct, but a bit too restrictive. If we start with
cur_mask = 0xfffffffffffff000, and we hotplug a VFIO device with
new_mask = 0x40201000 (4k page, 2M 1G blocks), then this code rejects it
even though it would work.
Thanks,
Jean
> } else {
> /* Guest driver can use any psize in cur_mask, not safe to
> continue */
> error_setg(...);
> return -1;
> }
> }
>
> Maybe we can also work on top too (if this is the only reason to repost,
> especially if Michael would like to pick this up sooner), so I just raise this
> up.
>
> Thanks,
>
> --
> Peter Xu
>
