diff mbox series

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

Message ID 20231026103104.1686921-7-zhenzhong.duan@intel.com
State New
Headers show
Series vfio: Adopt iommufd | expand

Commit Message

Duan, Zhenzhong Oct. 26, 2023, 10:30 a.m. UTC 
Introduce a dumb VFIOContainer base object and its targetted interface.
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

Comments

Cédric Le Goater Oct. 27, 2023, 2:02 p.m. UTC | #1 
On 10/26/23 12:30, Zhenzhong Duan wrote:
> Introduce a dumb VFIOContainer base object and its targetted interface.

targeted


> 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

> + */
> +
> +#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.


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

> +    int (*attach_device)(char *name, VFIODevice *vbasedev,

cont char *name  ?

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 */
Duan, Zhenzhong Oct. 30, 2023, 2:40 a.m. UTC | #2 
>-----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.

>
>> +    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 */
Cédric Le Goater Oct. 31, 2023, 7:57 a.m. UTC | #3 
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.

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

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 */
>
Duan, Zhenzhong Oct. 31, 2023, 8:31 a.m. UTC | #4 
>-----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 */
>>
diff mbox series

Patch

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/>.
+ */
+
+#ifndef HW_VFIO_VFIO_BASE_CONTAINER_H
+#define HW_VFIO_VFIO_BASE_CONTAINER_H
+
+#include "exec/memory.h"
+#ifndef CONFIG_USER_ONLY
+#include "exec/hwaddr.h"
+#endif
+
+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);
+    int (*attach_device)(char *name, VFIODevice *vbasedev,
+                         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 */