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

[Peter Xu <peterx@redhat.com>]

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...

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;
        } 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