On Wed, Apr 23, 2025 at 07:01:23PM +0200, Cédric Le Goater wrote:
> On 4/9/25 15:48, John Levon wrote:
> > Use the new flags parameter to indicate when we want to unmap
> > everything; no functional change is intended.
>
> I find these changes confusing. Most likely there are not well presented
> or I am missing something. Some more below.
I don't see any way to further break up the change unfortunately.
> > +/*
> > + * DMA - Mapping and unmapping for the "type1" IOMMU interface used on x86
> > + */
> > +static int vfio_legacy_dma_unmap(const VFIOContainerBase *bcontainer,
> > + hwaddr iova, ram_addr_t size,
> > + IOMMUTLBEntry *iotlb, int flags)
> > +{
> > + int ret;
> > +
> > + if ((flags & ~(VFIO_DMA_UNMAP_FLAG_ALL)) != 0) {
>
> VFIO_DMA_UNMAP_FLAG_ALL is a kernel uapi flag. It should be used only with
> the corresponding ioctl(VFIO_IOMMU_UNMAP_DMA) and not internally between
> QEMU routines.
Happy to use a different define for the flags if you like, but surely it's
better to have a flags field so it's extendable and it's always clear what the
meaning is? Problem with a boolean is you just see "true" or "false" in the
caller and have no real idea what it means until you look it up.
> I think adding a 'bool unmap_all' paremeter to vfio_legacy_dma_unmap() would
> make more sense.
Having said that I'm OK with going back to just a simple boolean if you'd really
prefer.
> > }
> > - ret = vfio_container_dma_unmap(bcontainer, iova,
> > - int128_get64(llsize), NULL, 0);
> > + ret = vfio_container_dma_unmap(bcontainer, iova,
> > int128_get64(llsize),
> > + NULL, flags);
>
> Why not unmap the halves here instead of in the backends ?
The whole point of the change is that right now the generic listener.c code has
a workaround that is specific to one particular backend. vfio-user doesn't have
any need to unmap in halves and in fact *has* to pass an "unmap all" flag.
In theory, neither does vfio if the flag is supported, but I dropped that patch
as I couldn't figure out a clean way to use it WRT the dirty tracking code.
regards
john
