Re: [PATCH 07/14] vfio: specify VFIO_DMA_UNMAP_FLAG_ALL to callback

["Cédric Le Goater" <clg@redhat.com>]

On 4/23/25 19:17, John Levon wrote:
> 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.

yes. VFIO_DMA_UNMAP_FLAG_ALL is a kernel interface and we don't
need more than one flag today.

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

It's due to the ARM IO space size AFAICT.

> vfio-user doesn't have
> any need to unmap in halves and in fact *has* to pass an "unmap all" flag.

OK. So this flag is a vfio-user requirement. Why can't we call
vfio_container_dma_unmap() twice from vfio_listener_region_del() ?


Thanks,

C.



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