RE: [PATCH v3 06/37] vfio: Introduce base object for VFIOContainer and targetted interface

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Tuesday, October 31, 2023 3:58 PM
>Subject: Re: [PATCH v3 06/37] vfio: Introduce base object for VFIOContainer and
>targetted interface
>
>On 10/30/23 03:40, Duan, Zhenzhong wrote:
>>
>>
>>> -----Original Message-----
>>> From: Cédric Le Goater <clg@redhat.com>
>>> Sent: Friday, October 27, 2023 10:03 PM
>>> Subject: Re: [PATCH v3 06/37] vfio: Introduce base object for VFIOContainer
>and
>>> targetted interface
>>>
>>> On 10/26/23 12:30, Zhenzhong Duan wrote:
>>>> Introduce a dumb VFIOContainer base object and its targetted interface.
>>>
>>> targeted
>>
>> Will fix.
>>
>>>
>>>
>>>> This is willingly not a QOM object because we don't want it to be
>>>> visible from the user interface.  The VFIOContainer will be smoothly
>>>> populated in subsequent patches as well as interfaces.
>>>>
>>>> No fucntional change intended.
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>>> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>> ---
>>>>    include/hw/vfio/vfio-common.h         |  8 +---
>>>>    include/hw/vfio/vfio-container-base.h | 64
>+++++++++++++++++++++++++++
>>>>    2 files changed, 66 insertions(+), 6 deletions(-)
>>>>    create mode 100644 include/hw/vfio/vfio-container-base.h
>>>>
>>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>common.h
>>>> index b9c7a7e588..d8f293cb57 100644
>>>> --- a/include/hw/vfio/vfio-common.h
>>>> +++ b/include/hw/vfio/vfio-common.h
>>>> @@ -30,6 +30,7 @@
>>>>    #include <linux/vfio.h>
>>>>    #endif
>>>>    #include "sysemu/sysemu.h"
>>>> +#include "hw/vfio/vfio-container-base.h"
>>>>
>>>>    #define VFIO_MSG_PREFIX "vfio %s: "
>>>>
>>>> @@ -81,6 +82,7 @@ typedef struct VFIOAddressSpace {
>>>>    struct VFIOGroup;
>>>>
>>>>    typedef struct VFIOContainer {
>>>> +    VFIOContainerBase bcontainer;
>>>>        VFIOAddressSpace *space;
>>>>        int fd; /* /dev/vfio/vfio, empowered by the attached groups */
>>>>        MemoryListener listener;
>>>> @@ -201,12 +203,6 @@ typedef struct VFIODisplay {
>>>>        } dmabuf;
>>>>    } VFIODisplay;
>>>>
>>>> -typedef struct {
>>>> -    unsigned long *bitmap;
>>>> -    hwaddr size;
>>>> -    hwaddr pages;
>>>> -} VFIOBitmap;
>>>> -
>>>>    VFIOAddressSpace *vfio_get_address_space(AddressSpace *as);
>>>>    void vfio_put_address_space(VFIOAddressSpace *space);
>>>>    bool vfio_devices_all_running_and_saving(VFIOContainer *container);
>>>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>>> container-base.h
>>>> new file mode 100644
>>>> index 0000000000..5becbd51a7
>>>> --- /dev/null
>>>> +++ b/include/hw/vfio/vfio-container-base.h
>>>> @@ -0,0 +1,64 @@
>>>> +/*
>>>> + * VFIO BASE CONTAINER
>>>> + *
>>>> + * Copyright (C) 2023 Intel Corporation.
>>>> + * Copyright Red Hat, Inc. 2023
>>>> + *
>>>> + * Authors: Yi Liu <yi.l.liu@intel.com>
>>>> + *          Eric Auger <eric.auger@redhat.com>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License as published by
>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>> + * (at your option) any later version.
>>>> +
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> + * GNU General Public License for more details.
>>>> +
>>>> + * You should have received a copy of the GNU General Public License along
>>>> + * with this program; if not, see <http://www.gnu.org/licenses/>.
>>>
>>> This should be enough :
>>>
>>>    SPDX-License-Identifier: GPL-2.0-or-later
>>
>> Will do.
>>
>>>
>>>> + */
>>>> +
>>>> +#ifndef HW_VFIO_VFIO_BASE_CONTAINER_H
>>>> +#define HW_VFIO_VFIO_BASE_CONTAINER_H
>>>
>>> HW_VFIO_VFIO_CONTAINER_BASE_H
>>>
>>>> +
>>>> +#include "exec/memory.h"
>>>> +#ifndef CONFIG_USER_ONLY
>>>> +#include "exec/hwaddr.h"
>>>> +#endif
>>>
>>> including "exec/memory.h" should be enough.
>>
>> Will do.
>>
>>>
>>>
>>>> +
>>>> +typedef struct VFIODevice VFIODevice;
>>>> +typedef struct VFIOIOMMUOps VFIOIOMMUOps;
>>>> +
>>>> +typedef struct {
>>>> +    unsigned long *bitmap;
>>>> +    hwaddr size;
>>>> +    hwaddr pages;
>>>> +} VFIOBitmap;
>>>> +
>>>> +/*
>>>> + * This is the base object for vfio container backends
>>>> + */
>>>> +typedef struct VFIOContainerBase {
>>>> +    const VFIOIOMMUOps *ops;
>>>> +} VFIOContainerBase;
>>>> +
>>>> +struct VFIOIOMMUOps {
>>>> +    /* basic feature */
>>>> +    int (*dma_map)(VFIOContainerBase *bcontainer,
>>>> +                   hwaddr iova, ram_addr_t size,
>>>> +                   void *vaddr, bool readonly);
>>>> +    int (*dma_unmap)(VFIOContainerBase *bcontainer,
>>>> +                     hwaddr iova, ram_addr_t size,
>>>> +                     IOMMUTLBEntry *iotlb);
>>>
>>> Could the VFIOContainerBase *parameter be const ?
>>
>> Yes, VFIOContainerBase is not changed by dma_unmap and other
>> functions dma_unmap calls. I tried and found making it const here
>> would impact all functions it called with same parameter be const
>> in following patches which looks unrelated to the patch itself
>> to avoid compile error.
>>
>> E.g. below functions are impacted,
>> vfio_devices_all_running_and_mig_active
>> vfio_devices_all_device_dirty_tracking
>> vfio_devices_query_dirty_bitmap
>> vfio_container_query_dirty_bitmap
>> vfio_legacy_query_dirty_bitmap>
>> To make following patches cleaner to review, I would like to keep
>> current code except you or others have a strong opinion.
>> Another choice I can think of is to add const to all impact functions
>> in a separate patch.
>
>It would be good to have, for later.

OK, I'll add a patch for const change in this series.

>
>On a related topic, I find the code difficult to read because it is
>complex, of course, and dealing with a thick layer of software constructs
>but also because it lacks a consistent naming scheme in the different
>submodules. For instance, iommufd.c has various routine prefixes for
>local routines. This is quite confusing. Same for :
>
>   hw/vfio/common.c
>   hw/vfio/helpers.c
>   hw/vfio/container.c
>   hw/vfio/container-base.c

I see, let me change all routines in iommufd.c to use iommufd_* prefix.
For container-base.c, vfio_container_* is the prefix.
For other files, I can make a prefix cleanup series in future as this series
focus on iommufd.

Thanks
Zhenzhong

>
>This is not necessarily introduced by the changes of this series.
>Something to improve for sure though.
>
>Thanks,
>
>C.
>
>
>>
>>>
>>>> +    int (*attach_device)(char *name, VFIODevice *vbasedev,
>>>
>>> cont char *name  ?
>>
>> Yes, will fix.
>>
>> Thanks
>> Zhenzhong
>>
>>>
>>> The rest looks good to me.
>>>
>>> Thanks
>>>
>>> C.
>>>
>>>
>>>> +                         AddressSpace *as, Error **errp);
>>>> +    void (*detach_device)(VFIODevice *vbasedev);
>>>> +    /* migration feature */
>>>> +    int (*set_dirty_page_tracking)(VFIOContainerBase *bcontainer, bool
>start);
>>>> +    int (*query_dirty_bitmap)(VFIOContainerBase *bcontainer, VFIOBitmap
>>> *vbmap,
>>>> +                              hwaddr iova, hwaddr size);
>>>> +};
>>>> +#endif /* HW_VFIO_VFIO_BASE_CONTAINER_H */
>>