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

[Jean-Philippe Brucker <jean-philippe@linaro.org>]

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 <bbhushan2@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
>