Re: [PATCH v1 04/22] vfio/common: Introduce vfio_container_add|del_section_window()

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

On 9/27/23 04:08, Duan, Zhenzhong wrote:
> Hi Cédric,
> 
>> -----Original Message-----
>> From: Duan, Zhenzhong
>> Sent: Thursday, September 21, 2023 6:14 PM
>> Subject: RE: [PATCH v1 04/22] vfio/common: Introduce
>> vfio_container_add|del_section_window()
>>
>> Hi Cédric,
>>
>>> -----Original Message-----
>>> From: Cédric Le Goater <clg@redhat.com>
>>> Sent: Thursday, September 21, 2023 4:29 PM
>>> Subject: Re: [PATCH v1 04/22] vfio/common: Introduce
>>> vfio_container_add|del_section_window()
>>>
>>> Hello Zhenzhong,
>>>
>>> On 8/30/23 12:37, Zhenzhong Duan wrote:
>>>> From: Eric Auger <eric.auger@redhat.com>
>>>>
>>>> Introduce helper functions that isolate the code used for
>>>> VFIO_SPAPR_TCE_v2_IOMMU. This code reliance is IOMMU backend
>>>> specific whereas the rest of the code in the callers, ie.
>>>> vfio_listener_region_add|del is not.
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>> ---
>>>>    hw/vfio/common.c | 156 +++++++++++++++++++++++++++--------------------
>>>>    1 file changed, 89 insertions(+), 67 deletions(-)
>>>>
>>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>>> index 9ca695837f..67150e4575 100644
>>>> --- a/hw/vfio/common.c
>>>> +++ b/hw/vfio/common.c
>>>> @@ -796,6 +796,92 @@ static bool
>>> vfio_get_section_iova_range(VFIOContainer *container,
>>>>        return true;
>>>>    }
>>>>
>>>> +static int vfio_container_add_section_window(VFIOContainer *container,
>>>> +                                             MemoryRegionSection *section,
>>>> +                                             Error **errp)
>>>> +{
>>>> +    VFIOHostDMAWindow *hostwin;
>>>> +    hwaddr pgsize = 0;
>>>> +    int ret;
>>>> +
>>>> +    if (container->iommu_type != VFIO_SPAPR_TCE_v2_IOMMU) {
>>>> +        return 0;
>>>> +    }
>>>
>>> This test makes me think that we should register a specific backend
>>> for the pseries machines, implementing the add/del_window handler,
>>> since others do not need it. Correct ?
>>
>> Yes, introducing a specific backend could help removing above check.
>> But each backend has a VFIOIOMMUBackendOps, we need same check
>> as above to select Ops.
>>
>>>
>>> It would avoid this ugly test. Let's keep that in mind when the
>>> backends are introduced.
>>>
>>>> +
>>>> +    /* For now intersections are not allowed, we may relax this later */
>>>> +    QLIST_FOREACH(hostwin, &container->hostwin_list, hostwin_next) {
>>>> +        if (ranges_overlap(hostwin->min_iova,
>>>> +                           hostwin->max_iova - hostwin->min_iova + 1,
>>>> +                           section->offset_within_address_space,
>>>> +                           int128_get64(section->size))) {
>>>> +            error_setg(errp,
>>>> +                "region [0x%"PRIx64",0x%"PRIx64"] overlaps with existing"
>>>> +                "host DMA window [0x%"PRIx64",0x%"PRIx64"]",
>>>> +                section->offset_within_address_space,
>>>> +                section->offset_within_address_space +
>>>> +                    int128_get64(section->size) - 1,
>>>> +                hostwin->min_iova, hostwin->max_iova);
>>>> +            return -EINVAL;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    ret = vfio_spapr_create_window(container, section, &pgsize);
>>>> +    if (ret) {
>>>> +        error_setg_errno(errp, -ret, "Failed to create SPAPR window");
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    vfio_host_win_add(container, section->offset_within_address_space,
>>>> +                      section->offset_within_address_space +
>>>> +                      int128_get64(section->size) - 1, pgsize);
>>>> +#ifdef CONFIG_KVM
>>>
>>> the ifdef test doesn't seem useful because the compiler should compile
>>> out the section below since, in that case, kvm_enabled() is defined as :
>>>
>>>    #define kvm_enabled()           (0)
>>
>> Looks so, I'll remove it in v2.
> 
> Forgot to let you know, finally I failed to remove the ifdef test in v2 due to
> many "undeclared" compile errors. I guess the reason is grammatical check
> Is triggered before optimization in compiler.
> 
> For example:
> error: ‘KVM_DEV_VFIO_GROUP’ undeclared
> error: ‘vfio_kvm_device_fd’ undeclared


Yes. It would need helpers to hide the kernel structs and defined.
Let's address it later, after the backends are introduced.

Thanks for looking into it.

C.