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 <c...@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.au...@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.au...@redhat.com>
>>> Signed-off-by: Yi Liu <yi.l....@intel.com>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@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
...
Thanks
Zhenzhong
