1. Patchew
  2. QEMU
  3. [PATCH v2 00/27] vfio: Adopt iommufd
  4. View patch

[PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and targetted interface

Zhenzhong Duan posted 27 patches 1 year, 1 month ago
There is a newer version of this series
[PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and targetted interface
Posted by Zhenzhong Duan 1 year, 1 month ago 
From: Eric Auger <eric.auger@redhat.com>

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 | 82 +++++++++++++++++++++++++++
 2 files changed, 84 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 34648e518e..9651cf921c 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 VFIOLegacyContainer {
+    VFIOContainer bcontainer;
     VFIOAddressSpace *space;
     int fd; /* /dev/vfio/vfio, empowered by the attached groups */
     MemoryListener listener;
@@ -200,12 +202,6 @@ typedef struct VFIODisplay {
     } dmabuf;
 } VFIODisplay;
 
-typedef struct {
-    unsigned long *bitmap;
-    hwaddr size;
-    hwaddr pages;
-} VFIOBitmap;
-
 void vfio_host_win_add(VFIOLegacyContainer *container,
                        hwaddr min_iova, hwaddr max_iova,
                        uint64_t iova_pgsizes);
diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
new file mode 100644
index 0000000000..afc8543d22
--- /dev/null
+++ b/include/hw/vfio/vfio-container-base.h
@@ -0,0 +1,82 @@
+/*
+ * 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 VFIOContainer VFIOContainer;
+typedef struct VFIODevice VFIODevice;
+typedef struct VFIOIOMMUBackendOpsClass VFIOIOMMUBackendOpsClass;
+
+typedef struct {
+    unsigned long *bitmap;
+    hwaddr size;
+    hwaddr pages;
+} VFIOBitmap;
+
+/*
+ * This is the base object for vfio container backends
+ */
+struct VFIOContainer {
+    VFIOIOMMUBackendOpsClass *ops;
+};
+
+#define TYPE_VFIO_IOMMU_BACKEND_OPS "vfio-iommu-backend-ops"
+
+DECLARE_CLASS_CHECKERS(VFIOIOMMUBackendOpsClass,
+                       VFIO_IOMMU_BACKEND_OPS, TYPE_VFIO_IOMMU_BACKEND_OPS)
+
+struct VFIOIOMMUBackendOpsClass {
+    /*< private >*/
+    ObjectClass parent_class;
+
+    /*< public >*/
+    /* required */
+    int (*dma_map)(VFIOContainer *bcontainer,
+                   hwaddr iova, ram_addr_t size,
+                   void *vaddr, bool readonly);
+    int (*dma_unmap)(VFIOContainer *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)(VFIOContainer *bcontainer, bool start);
+    int (*query_dirty_bitmap)(VFIOContainer *bcontainer, VFIOBitmap *vbmap,
+                              hwaddr iova, hwaddr size);
+
+    /* SPAPR specific */
+    int (*add_window)(VFIOContainer *bcontainer,
+                      MemoryRegionSection *section,
+                      Error **errp);
+    void (*del_window)(VFIOContainer *bcontainer,
+                       MemoryRegionSection *section);
+};
+
+#endif /* HW_VFIO_VFIO_BASE_CONTAINER_H */
-- 
2.34.1
Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and targetted interface
Posted by Cédric Le Goater 1 year, 1 month ago 
On 10/16/23 10:31, Zhenzhong Duan wrote:
> From: Eric Auger <eric.auger@redhat.com>
> 
> 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 | 82 +++++++++++++++++++++++++++
>   2 files changed, 84 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 34648e518e..9651cf921c 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 VFIOLegacyContainer {
> +    VFIOContainer bcontainer;

That's the parent class, right ?

>       VFIOAddressSpace *space;
>       int fd; /* /dev/vfio/vfio, empowered by the attached groups */
>       MemoryListener listener;
> @@ -200,12 +202,6 @@ typedef struct VFIODisplay {
>       } dmabuf;
>   } VFIODisplay;
>   
> -typedef struct {
> -    unsigned long *bitmap;
> -    hwaddr size;
> -    hwaddr pages;
> -} VFIOBitmap;
> -
>   void vfio_host_win_add(VFIOLegacyContainer *container,
>                          hwaddr min_iova, hwaddr max_iova,
>                          uint64_t iova_pgsizes);
> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
> new file mode 100644
> index 0000000000..afc8543d22
> --- /dev/null
> +++ b/include/hw/vfio/vfio-container-base.h
> @@ -0,0 +1,82 @@
> +/*
> + * 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 VFIOContainer VFIOContainer;
> +typedef struct VFIODevice VFIODevice;
> +typedef struct VFIOIOMMUBackendOpsClass VFIOIOMMUBackendOpsClass;
> +
> +typedef struct {
> +    unsigned long *bitmap;
> +    hwaddr size;
> +    hwaddr pages;
> +} VFIOBitmap;
> +
> +/*
> + * This is the base object for vfio container backends
> + */
> +struct VFIOContainer {
> +    VFIOIOMMUBackendOpsClass *ops;

This is unexpected.

I thought that an abstract QOM model for VFIOContainer was going
to be introduced with a VFIOContainerClass with the ops below
(VFIOIOMMUBackendOpsClass).

Then, we would call :

    VFIOContainerClass *vcc = VFIO_CONTAINER_GET_CLASS(container);

to get the specific implementation for the current container.

I don't understand the VFIOIOMMUBackendOpsClass pointer and
TYPE_VFIO_IOMMU_BACKEND_OPS. It seems redundant.

Thanks,

C.

> +};
> +
> +#define TYPE_VFIO_IOMMU_BACKEND_OPS "vfio-iommu-backend-ops"
> +
> +DECLARE_CLASS_CHECKERS(VFIOIOMMUBackendOpsClass,
> +                       VFIO_IOMMU_BACKEND_OPS, TYPE_VFIO_IOMMU_BACKEND_OPS)
> +
> +struct VFIOIOMMUBackendOpsClass {
> +    /*< private >*/
> +    ObjectClass parent_class;
> +
> +    /*< public >*/
> +    /* required */
> +    int (*dma_map)(VFIOContainer *bcontainer,
> +                   hwaddr iova, ram_addr_t size,
> +                   void *vaddr, bool readonly);
> +    int (*dma_unmap)(VFIOContainer *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)(VFIOContainer *bcontainer, bool start);
> +    int (*query_dirty_bitmap)(VFIOContainer *bcontainer, VFIOBitmap *vbmap,
> +                              hwaddr iova, hwaddr size);
> +
> +    /* SPAPR specific */
> +    int (*add_window)(VFIOContainer *bcontainer,
> +                      MemoryRegionSection *section,
> +                      Error **errp);
> +    void (*del_window)(VFIOContainer *bcontainer,
> +                       MemoryRegionSection *section);
> +};
> +
> +#endif /* HW_VFIO_VFIO_BASE_CONTAINER_H */
RE: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and targetted interface
Posted by Duan, Zhenzhong 1 year, 1 month ago 
Hi Cédric,

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Tuesday, October 17, 2023 11:51 PM
>Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and
>targetted interface
>
>On 10/16/23 10:31, Zhenzhong Duan wrote:
>> From: Eric Auger <eric.auger@redhat.com>
>>
>> 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 | 82 +++++++++++++++++++++++++++
>>   2 files changed, 84 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 34648e518e..9651cf921c 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 VFIOLegacyContainer {
>> +    VFIOContainer bcontainer;
>
>That's the parent class, right ?

Right.

>
>>       VFIOAddressSpace *space;
>>       int fd; /* /dev/vfio/vfio, empowered by the attached groups */
>>       MemoryListener listener;
>> @@ -200,12 +202,6 @@ typedef struct VFIODisplay {
>>       } dmabuf;
>>   } VFIODisplay;
>>
>> -typedef struct {
>> -    unsigned long *bitmap;
>> -    hwaddr size;
>> -    hwaddr pages;
>> -} VFIOBitmap;
>> -
>>   void vfio_host_win_add(VFIOLegacyContainer *container,
>>                          hwaddr min_iova, hwaddr max_iova,
>>                          uint64_t iova_pgsizes);
>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>container-base.h
>> new file mode 100644
>> index 0000000000..afc8543d22
>> --- /dev/null
>> +++ b/include/hw/vfio/vfio-container-base.h
>> @@ -0,0 +1,82 @@
>> +/*
>> + * 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 VFIOContainer VFIOContainer;
>> +typedef struct VFIODevice VFIODevice;
>> +typedef struct VFIOIOMMUBackendOpsClass VFIOIOMMUBackendOpsClass;
>> +
>> +typedef struct {
>> +    unsigned long *bitmap;
>> +    hwaddr size;
>> +    hwaddr pages;
>> +} VFIOBitmap;
>> +
>> +/*
>> + * This is the base object for vfio container backends
>> + */
>> +struct VFIOContainer {
>> +    VFIOIOMMUBackendOpsClass *ops;
>
>This is unexpected.
>
>I thought that an abstract QOM model for VFIOContainer was going
>to be introduced with a VFIOContainerClass with the ops below
>(VFIOIOMMUBackendOpsClass).
>
>Then, we would call :
>
>    VFIOContainerClass *vcc = VFIO_CONTAINER_GET_CLASS(container);
>
>to get the specific implementation for the current container.
>
>I don't understand the VFIOIOMMUBackendOpsClass pointer and
>TYPE_VFIO_IOMMU_BACKEND_OPS. It seems redundant.

The original implementation was abstract QOM model. But it wasn't accepted,
see https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/ for details.

Thanks
Zhenzhong
Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and targetted interface
Posted by Cédric Le Goater 1 year, 1 month ago 
On 10/18/23 04:41, Duan, Zhenzhong wrote:
> Hi Cédric,
> 
>> -----Original Message-----
>> From: Cédric Le Goater <clg@redhat.com>
>> Sent: Tuesday, October 17, 2023 11:51 PM
>> Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and
>> targetted interface
>>
>> On 10/16/23 10:31, Zhenzhong Duan wrote:
>>> From: Eric Auger <eric.auger@redhat.com>
>>>
>>> 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 | 82 +++++++++++++++++++++++++++
>>>    2 files changed, 84 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 34648e518e..9651cf921c 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 VFIOLegacyContainer {
>>> +    VFIOContainer bcontainer;
>>
>> That's the parent class, right ?
> 
> Right.
> 
>>
>>>        VFIOAddressSpace *space;
>>>        int fd; /* /dev/vfio/vfio, empowered by the attached groups */
>>>        MemoryListener listener;
>>> @@ -200,12 +202,6 @@ typedef struct VFIODisplay {
>>>        } dmabuf;
>>>    } VFIODisplay;
>>>
>>> -typedef struct {
>>> -    unsigned long *bitmap;
>>> -    hwaddr size;
>>> -    hwaddr pages;
>>> -} VFIOBitmap;
>>> -
>>>    void vfio_host_win_add(VFIOLegacyContainer *container,
>>>                           hwaddr min_iova, hwaddr max_iova,
>>>                           uint64_t iova_pgsizes);
>>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>> container-base.h
>>> new file mode 100644
>>> index 0000000000..afc8543d22
>>> --- /dev/null
>>> +++ b/include/hw/vfio/vfio-container-base.h
>>> @@ -0,0 +1,82 @@
>>> +/*
>>> + * 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 VFIOContainer VFIOContainer;
>>> +typedef struct VFIODevice VFIODevice;
>>> +typedef struct VFIOIOMMUBackendOpsClass VFIOIOMMUBackendOpsClass;
>>> +
>>> +typedef struct {
>>> +    unsigned long *bitmap;
>>> +    hwaddr size;
>>> +    hwaddr pages;
>>> +} VFIOBitmap;
>>> +
>>> +/*
>>> + * This is the base object for vfio container backends
>>> + */
>>> +struct VFIOContainer {
>>> +    VFIOIOMMUBackendOpsClass *ops;
>>
>> This is unexpected.
>>
>> I thought that an abstract QOM model for VFIOContainer was going
>> to be introduced with a VFIOContainerClass with the ops below
>> (VFIOIOMMUBackendOpsClass).
>>
>> Then, we would call :
>>
>>     VFIOContainerClass *vcc = VFIO_CONTAINER_GET_CLASS(container);
>>
>> to get the specific implementation for the current container.
>>
>> I don't understand the VFIOIOMMUBackendOpsClass pointer and
>> TYPE_VFIO_IOMMU_BACKEND_OPS. It seems redundant.
> 
> The original implementation was abstract QOM model. But it wasn't accepted,
> see https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/ for details.

I see the idea was challenged, not rejected. I need to dig in further and this
will take time.

Thanks,

C.
RE: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and targetted interface
Posted by Duan, Zhenzhong 1 year, 1 month ago 
>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Wednesday, October 18, 2023 4:04 PM
>Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and
>targetted interface
>
>On 10/18/23 04:41, Duan, Zhenzhong wrote:
>> Hi Cédric,
>>
>>> -----Original Message-----
>>> From: Cédric Le Goater <clg@redhat.com>
>>> Sent: Tuesday, October 17, 2023 11:51 PM
>>> Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer
>and
>>> targetted interface
>>>
>>> On 10/16/23 10:31, Zhenzhong Duan wrote:
>>>> From: Eric Auger <eric.auger@redhat.com>
>>>>
>>>> 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 | 82
>+++++++++++++++++++++++++++
>>>>    2 files changed, 84 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 34648e518e..9651cf921c 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 VFIOLegacyContainer {
>>>> +    VFIOContainer bcontainer;
>>>
>>> That's the parent class, right ?
>>
>> Right.
>>
>>>
>>>>        VFIOAddressSpace *space;
>>>>        int fd; /* /dev/vfio/vfio, empowered by the attached groups */
>>>>        MemoryListener listener;
>>>> @@ -200,12 +202,6 @@ typedef struct VFIODisplay {
>>>>        } dmabuf;
>>>>    } VFIODisplay;
>>>>
>>>> -typedef struct {
>>>> -    unsigned long *bitmap;
>>>> -    hwaddr size;
>>>> -    hwaddr pages;
>>>> -} VFIOBitmap;
>>>> -
>>>>    void vfio_host_win_add(VFIOLegacyContainer *container,
>>>>                           hwaddr min_iova, hwaddr max_iova,
>>>>                           uint64_t iova_pgsizes);
>>>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>>> container-base.h
>>>> new file mode 100644
>>>> index 0000000000..afc8543d22
>>>> --- /dev/null
>>>> +++ b/include/hw/vfio/vfio-container-base.h
>>>> @@ -0,0 +1,82 @@
>>>> +/*
>>>> + * 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 VFIOContainer VFIOContainer;
>>>> +typedef struct VFIODevice VFIODevice;
>>>> +typedef struct VFIOIOMMUBackendOpsClass VFIOIOMMUBackendOpsClass;
>>>> +
>>>> +typedef struct {
>>>> +    unsigned long *bitmap;
>>>> +    hwaddr size;
>>>> +    hwaddr pages;
>>>> +} VFIOBitmap;
>>>> +
>>>> +/*
>>>> + * This is the base object for vfio container backends
>>>> + */
>>>> +struct VFIOContainer {
>>>> +    VFIOIOMMUBackendOpsClass *ops;
>>>
>>> This is unexpected.
>>>
>>> I thought that an abstract QOM model for VFIOContainer was going
>>> to be introduced with a VFIOContainerClass with the ops below
>>> (VFIOIOMMUBackendOpsClass).
>>>
>>> Then, we would call :
>>>
>>>     VFIOContainerClass *vcc = VFIO_CONTAINER_GET_CLASS(container);
>>>
>>> to get the specific implementation for the current container.
>>>
>>> I don't understand the VFIOIOMMUBackendOpsClass pointer and
>>> TYPE_VFIO_IOMMU_BACKEND_OPS. It seems redundant.
>>
>> The original implementation was abstract QOM model. But it wasn't accepted,
>> see https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/ for details.
>
>I see the idea was challenged, not rejected. I need to dig in further and this
>will take time.
Thanks for help looking into it.

+David, Hi David, I'm digging into your concern of using QOM to abstract base
container and legacy VFIOContainer:
"The QOM class of things is visible to the user/config layer via QMP (and sometimes command line).
It doesn't necessarily correspond to guest visible differences, but it often does."

AIUI, while it's true when the QOM type includes TYPE_USER_CREATABLE interface,
otherwise, user can't create an object of this type. Only difference is user will see
"object type '%s' isn't supported by object-add" instead of "invalid object type: %s".

Is your expectation to not permit user to create this object or only want user
to see "invalid object type: %s".
If you mean the first, then I think QOM could be suitable if we don't include
TYPE_USER_CREATABLE interface?

Thanks
Zhenzhong
Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and targetted interface
Posted by Cédric Le Goater 1 year, 1 month ago 
On 10/19/23 04:29, Duan, Zhenzhong wrote:
>> -----Original Message-----
>> From: Cédric Le Goater <clg@redhat.com>
>> Sent: Wednesday, October 18, 2023 4:04 PM
>> Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and
>> targetted interface
>>
>> On 10/18/23 04:41, Duan, Zhenzhong wrote:
>>> Hi Cédric,
>>>
>>>> -----Original Message-----
>>>> From: Cédric Le Goater <clg@redhat.com>
>>>> Sent: Tuesday, October 17, 2023 11:51 PM
>>>> Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer
>> and
>>>> targetted interface
>>>>
>>>> On 10/16/23 10:31, Zhenzhong Duan wrote:
>>>>> From: Eric Auger <eric.auger@redhat.com>
>>>>>
>>>>> 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 | 82
>> +++++++++++++++++++++++++++
>>>>>     2 files changed, 84 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 34648e518e..9651cf921c 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 VFIOLegacyContainer {
>>>>> +    VFIOContainer bcontainer;
>>>>
>>>> That's the parent class, right ?
>>>
>>> Right.
>>>
>>>>
>>>>>         VFIOAddressSpace *space;
>>>>>         int fd; /* /dev/vfio/vfio, empowered by the attached groups */
>>>>>         MemoryListener listener;
>>>>> @@ -200,12 +202,6 @@ typedef struct VFIODisplay {
>>>>>         } dmabuf;
>>>>>     } VFIODisplay;
>>>>>
>>>>> -typedef struct {
>>>>> -    unsigned long *bitmap;
>>>>> -    hwaddr size;
>>>>> -    hwaddr pages;
>>>>> -} VFIOBitmap;
>>>>> -
>>>>>     void vfio_host_win_add(VFIOLegacyContainer *container,
>>>>>                            hwaddr min_iova, hwaddr max_iova,
>>>>>                            uint64_t iova_pgsizes);
>>>>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>>>> container-base.h
>>>>> new file mode 100644
>>>>> index 0000000000..afc8543d22
>>>>> --- /dev/null
>>>>> +++ b/include/hw/vfio/vfio-container-base.h
>>>>> @@ -0,0 +1,82 @@
>>>>> +/*
>>>>> + * 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 VFIOContainer VFIOContainer;
>>>>> +typedef struct VFIODevice VFIODevice;
>>>>> +typedef struct VFIOIOMMUBackendOpsClass VFIOIOMMUBackendOpsClass;
>>>>> +
>>>>> +typedef struct {
>>>>> +    unsigned long *bitmap;
>>>>> +    hwaddr size;
>>>>> +    hwaddr pages;
>>>>> +} VFIOBitmap;
>>>>> +
>>>>> +/*
>>>>> + * This is the base object for vfio container backends
>>>>> + */
>>>>> +struct VFIOContainer {
>>>>> +    VFIOIOMMUBackendOpsClass *ops;
>>>>
>>>> This is unexpected.
>>>>
>>>> I thought that an abstract QOM model for VFIOContainer was going
>>>> to be introduced with a VFIOContainerClass with the ops below
>>>> (VFIOIOMMUBackendOpsClass).
>>>>
>>>> Then, we would call :
>>>>
>>>>      VFIOContainerClass *vcc = VFIO_CONTAINER_GET_CLASS(container);
>>>>
>>>> to get the specific implementation for the current container.
>>>>
>>>> I don't understand the VFIOIOMMUBackendOpsClass pointer and
>>>> TYPE_VFIO_IOMMU_BACKEND_OPS. It seems redundant.
>>>
>>> The original implementation was abstract QOM model. But it wasn't accepted,
>>> see https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/ for details.
>>
>> I see the idea was challenged, not rejected. I need to dig in further and this
>> will take time.
> Thanks for help looking into it.
> 
> +David, Hi David, I'm digging into your concern of using QOM to abstract base
> container and legacy VFIOContainer:
> "The QOM class of things is visible to the user/config layer via QMP (and sometimes command line).
> It doesn't necessarily correspond to guest visible differences, but it often does."
> 
> AIUI, while it's true when the QOM type includes TYPE_USER_CREATABLE interface,
> otherwise, user can't create an object of this type. Only difference is user will see
> "object type '%s' isn't supported by object-add" instead of "invalid object type: %s".
> 
> Is your expectation to not permit user to create this object or only want user
> to see "invalid object type: %s".
> If you mean the first, then I think QOM could be suitable if we don't include
> TYPE_USER_CREATABLE interface?

I was imagining some kind of QOM hierarchy under the vfio device
with various QOM interfaces (similar to the ops) to define the
possible IOMMU backends. The fact that we use the IOMMUFD object
from the command line made it more plausible. I might be mistaking.

Anyhow, the series looks pretty good. There are other aspect to
check, like are all this iommu ops well suited for the need ?
Let's stress the models in parallel of the reviews. If we could get
some of it for 8.2 that'd be nice. It's on top of my list now.

Thanks,

C.


> Thanks
> Zhenzhong
RE: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and targetted interface
Posted by Duan, Zhenzhong 1 year, 1 month ago 
Hi Cédric,

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Thursday, October 19, 2023 8:18 PM
>Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and
>targetted interface
>
>On 10/19/23 04:29, Duan, Zhenzhong wrote:
>>> -----Original Message-----
>>> From: Cédric Le Goater <clg@redhat.com>
>>> Sent: Wednesday, October 18, 2023 4:04 PM
>>> Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer
>and
>>> targetted interface
>>>
>>> On 10/18/23 04:41, Duan, Zhenzhong wrote:
>>>> Hi Cédric,
>>>>
>>>>> -----Original Message-----
>>>>> From: Cédric Le Goater <clg@redhat.com>
>>>>> Sent: Tuesday, October 17, 2023 11:51 PM
>>>>> Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer
>>> and
>>>>> targetted interface
>>>>>
>>>>> On 10/16/23 10:31, Zhenzhong Duan wrote:
>>>>>> From: Eric Auger <eric.auger@redhat.com>
>>>>>>
>>>>>> 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 | 82
>>> +++++++++++++++++++++++++++
>>>>>>     2 files changed, 84 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 34648e518e..9651cf921c 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 VFIOLegacyContainer {
>>>>>> +    VFIOContainer bcontainer;
>>>>>
>>>>> That's the parent class, right ?
>>>>
>>>> Right.
>>>>
>>>>>
>>>>>>         VFIOAddressSpace *space;
>>>>>>         int fd; /* /dev/vfio/vfio, empowered by the attached groups */
>>>>>>         MemoryListener listener;
>>>>>> @@ -200,12 +202,6 @@ typedef struct VFIODisplay {
>>>>>>         } dmabuf;
>>>>>>     } VFIODisplay;
>>>>>>
>>>>>> -typedef struct {
>>>>>> -    unsigned long *bitmap;
>>>>>> -    hwaddr size;
>>>>>> -    hwaddr pages;
>>>>>> -} VFIOBitmap;
>>>>>> -
>>>>>>     void vfio_host_win_add(VFIOLegacyContainer *container,
>>>>>>                            hwaddr min_iova, hwaddr max_iova,
>>>>>>                            uint64_t iova_pgsizes);
>>>>>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>>>>> container-base.h
>>>>>> new file mode 100644
>>>>>> index 0000000000..afc8543d22
>>>>>> --- /dev/null
>>>>>> +++ b/include/hw/vfio/vfio-container-base.h
>>>>>> @@ -0,0 +1,82 @@
>>>>>> +/*
>>>>>> + * 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 VFIOContainer VFIOContainer;
>>>>>> +typedef struct VFIODevice VFIODevice;
>>>>>> +typedef struct VFIOIOMMUBackendOpsClass
>VFIOIOMMUBackendOpsClass;
>>>>>> +
>>>>>> +typedef struct {
>>>>>> +    unsigned long *bitmap;
>>>>>> +    hwaddr size;
>>>>>> +    hwaddr pages;
>>>>>> +} VFIOBitmap;
>>>>>> +
>>>>>> +/*
>>>>>> + * This is the base object for vfio container backends
>>>>>> + */
>>>>>> +struct VFIOContainer {
>>>>>> +    VFIOIOMMUBackendOpsClass *ops;
>>>>>
>>>>> This is unexpected.
>>>>>
>>>>> I thought that an abstract QOM model for VFIOContainer was going
>>>>> to be introduced with a VFIOContainerClass with the ops below
>>>>> (VFIOIOMMUBackendOpsClass).
>>>>>
>>>>> Then, we would call :
>>>>>
>>>>>      VFIOContainerClass *vcc = VFIO_CONTAINER_GET_CLASS(container);
>>>>>
>>>>> to get the specific implementation for the current container.
>>>>>
>>>>> I don't understand the VFIOIOMMUBackendOpsClass pointer and
>>>>> TYPE_VFIO_IOMMU_BACKEND_OPS. It seems redundant.
>>>>
>>>> The original implementation was abstract QOM model. But it wasn't
>accepted,
>>>> see https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/ for details.
>>>
>>> I see the idea was challenged, not rejected. I need to dig in further and this
>>> will take time.
>> Thanks for help looking into it.
>>
>> +David, Hi David, I'm digging into your concern of using QOM to abstract base
>> container and legacy VFIOContainer:
>> "The QOM class of things is visible to the user/config layer via QMP (and
>sometimes command line).
>> It doesn't necessarily correspond to guest visible differences, but it often does."
>>
>> AIUI, while it's true when the QOM type includes TYPE_USER_CREATABLE
>interface,
>> otherwise, user can't create an object of this type. Only difference is user will
>see
>> "object type '%s' isn't supported by object-add" instead of "invalid object
>type: %s".
>>
>> Is your expectation to not permit user to create this object or only want user
>> to see "invalid object type: %s".
>> If you mean the first, then I think QOM could be suitable if we don't include
>> TYPE_USER_CREATABLE interface?
>
>I was imagining some kind of QOM hierarchy under the vfio device
>with various QOM interfaces (similar to the ops) to define the
>possible IOMMU backends. The fact that we use the IOMMUFD object
>from the command line made it more plausible. I might be mistaking.

Got your point.
This way we introduce a new QOM type "vfio-pci-iommufd" for iommufd support,
and vfio-pci keep same for legacy backend, e.g:

#qemu  -object iommufd,id=iommufd0 \
              -device vfio-pci-iommufd,iommufd=iommufd0,id=vfio0... \
             -device vfio-pci,id=vfio1...

Below is a draft for vfio-pci based on your imagination. not pasted here the change
for platform/ap/ccw vfio device which should be similar.
Not clear if this fully match your imagination.

diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
index 5345986993..54a6ce4d73 100644
--- a/include/hw/vfio/vfio-container-base.h
+++ b/include/hw/vfio/vfio-container-base.h
@@ -124,7 +124,7 @@ DECLARE_CLASS_CHECKERS(VFIOIOMMUBackendOpsClass,

 struct VFIOIOMMUBackendOpsClass {
     /*< private >*/
-    ObjectClass parent_class;
+    InterfaceClass parent_class;

     /*< public >*/
     /* required */
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index edb787d3d1..829deddc7d 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3725,6 +3725,11 @@ static void vfio_pci_dev_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *pdc = PCI_DEVICE_CLASS(klass);
+    IOMMU_Backend_Ops_Class *be_ops = IOMMU_BACKEND_OPS_CLASS(klass);
+
+    be_ops->dma_map = vfio_legacy_dma_map;
+    be_ops->dma_unmap = vfio_legacy_dma_unmap;
+    ...

     dc->reset = vfio_pci_reset;
     device_class_set_props(dc, vfio_pci_dev_properties);
@@ -3749,10 +3754,40 @@ static const TypeInfo vfio_pci_dev_info = {
     .interfaces = (InterfaceInfo[]) {
         { INTERFACE_PCIE_DEVICE },
         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+        { INTERFACE_IOMMU_BACKEND_OPS },
         { }
     },
 };

+static Property vfio_pci_dev_iommufd_properties[] = {
+#ifdef CONFIG_IOMMUFD
+    DEFINE_PROP_LINK("iommufd", VFIOPCIDevice, vbasedev.iommufd,
+                     TYPE_IOMMUFD_BACKEND, IOMMUFDBackend *),
+#endif
+};
+
+static void vfio_pci_iommufd_dev_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    IOMMU_Backend_Ops_Class *be_ops = IOMMU_BACKEND_OPS_CLASS(klass);
+
+    device_class_set_props(dc, vfio_pci_dev_iommufd_properties);
+
+    be_ops->dma_map = iommufd_map;
+    be_ops->dma_unmap = iommufd_unmap;
+    ...
+    /* Unimplemented ops */
+    be_ops->set_dirty_page_tracking = NULL;
+    ...
+}
+
+static const TypeInfo vfio_pci_iommufd_dev_info = {
+    .name = TYPE_VFIO_PCI_IOMMUFD,
+    .parent = TYPE_VFIO_PCI,
+    .instance_size = sizeof(VFIOPCIDevice),
+    .class_init = vfio_pci_iommufd_dev_class_init,
+};
+
 static Property vfio_pci_dev_nohotplug_properties[] = {
     DEFINE_PROP_BOOL("ramfb", VFIOPCIDevice, enable_ramfb, false),
     DEFINE_PROP_ON_OFF_AUTO("x-ramfb-migrate", VFIOPCIDevice, ramfb_migrate,
@@ -3770,13 +3805,20 @@ static void vfio_pci_nohotplug_dev_class_init(ObjectClass *klass, void *data)

 static const TypeInfo vfio_pci_nohotplug_dev_info = {
     .name = TYPE_VFIO_PCI_NOHOTPLUG,
-    .parent = TYPE_VFIO_PCI,
+    .parent = TYPE_VFIO_PCI_IOMMUFD,
     .instance_size = sizeof(VFIOPCIDevice),
     .class_init = vfio_pci_nohotplug_dev_class_init,
 };

 static void register_vfio_pci_dev_type(void)
 {
+    static const TypeInfo iommu_be_ops_interface_info = {
+        .name          = TYPE_IOMMU_BACKEND_OPS,
+        .parent        = TYPE_INTERFACE,
+        .class_size = sizeof(VFIOIOMMUBackendOpsClass),
+    };
+
+    type_register_static(&iommu_be_ops_interface_info);
     type_register_static(&vfio_pci_dev_info);
     type_register_static(&vfio_pci_nohotplug_dev_info);
 }

Thanks
Zhenzhong

>
>Anyhow, the series looks pretty good. There are other aspect to
>check, like are all this iommu ops well suited for the need ?
>Let's stress the models in parallel of the reviews. If we could get
>some of it for 8.2 that'd be nice. It's on top of my list now.
>
>Thanks,
>
>C.
>
>
>> Thanks
>> Zhenzhong
Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and targetted interface
Posted by Eric Auger 1 year, 1 month ago 
Hi,
On 10/20/23 07:48, Duan, Zhenzhong wrote:
> Hi Cédric,
>
>> -----Original Message-----
>> From: Cédric Le Goater <clg@redhat.com>
>> Sent: Thursday, October 19, 2023 8:18 PM
>> Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and
>> targetted interface
>>
>> On 10/19/23 04:29, Duan, Zhenzhong wrote:
>>>> -----Original Message-----
>>>> From: Cédric Le Goater <clg@redhat.com>
>>>> Sent: Wednesday, October 18, 2023 4:04 PM
>>>> Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer
>> and
>>>> targetted interface
>>>>
>>>> On 10/18/23 04:41, Duan, Zhenzhong wrote:
>>>>> Hi Cédric,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Cédric Le Goater <clg@redhat.com>
>>>>>> Sent: Tuesday, October 17, 2023 11:51 PM
>>>>>> Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer
>>>> and
>>>>>> targetted interface
>>>>>>
>>>>>> On 10/16/23 10:31, Zhenzhong Duan wrote:
>>>>>>> From: Eric Auger <eric.auger@redhat.com>
>>>>>>>
>>>>>>> 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 | 82
>>>> +++++++++++++++++++++++++++
>>>>>>>     2 files changed, 84 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 34648e518e..9651cf921c 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 VFIOLegacyContainer {
>>>>>>> +    VFIOContainer bcontainer;
>>>>>> That's the parent class, right ?
>>>>> Right.
>>>>>
>>>>>>>         VFIOAddressSpace *space;
>>>>>>>         int fd; /* /dev/vfio/vfio, empowered by the attached groups */
>>>>>>>         MemoryListener listener;
>>>>>>> @@ -200,12 +202,6 @@ typedef struct VFIODisplay {
>>>>>>>         } dmabuf;
>>>>>>>     } VFIODisplay;
>>>>>>>
>>>>>>> -typedef struct {
>>>>>>> -    unsigned long *bitmap;
>>>>>>> -    hwaddr size;
>>>>>>> -    hwaddr pages;
>>>>>>> -} VFIOBitmap;
>>>>>>> -
>>>>>>>     void vfio_host_win_add(VFIOLegacyContainer *container,
>>>>>>>                            hwaddr min_iova, hwaddr max_iova,
>>>>>>>                            uint64_t iova_pgsizes);
>>>>>>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>>>>>> container-base.h
>>>>>>> new file mode 100644
>>>>>>> index 0000000000..afc8543d22
>>>>>>> --- /dev/null
>>>>>>> +++ b/include/hw/vfio/vfio-container-base.h
>>>>>>> @@ -0,0 +1,82 @@
>>>>>>> +/*
>>>>>>> + * 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 VFIOContainer VFIOContainer;
>>>>>>> +typedef struct VFIODevice VFIODevice;
>>>>>>> +typedef struct VFIOIOMMUBackendOpsClass
>> VFIOIOMMUBackendOpsClass;
>>>>>>> +
>>>>>>> +typedef struct {
>>>>>>> +    unsigned long *bitmap;
>>>>>>> +    hwaddr size;
>>>>>>> +    hwaddr pages;
>>>>>>> +} VFIOBitmap;
>>>>>>> +
>>>>>>> +/*
>>>>>>> + * This is the base object for vfio container backends
>>>>>>> + */
>>>>>>> +struct VFIOContainer {
>>>>>>> +    VFIOIOMMUBackendOpsClass *ops;
>>>>>> This is unexpected.
>>>>>>
>>>>>> I thought that an abstract QOM model for VFIOContainer was going
>>>>>> to be introduced with a VFIOContainerClass with the ops below
>>>>>> (VFIOIOMMUBackendOpsClass).
>>>>>>
>>>>>> Then, we would call :
>>>>>>
>>>>>>      VFIOContainerClass *vcc = VFIO_CONTAINER_GET_CLASS(container);
>>>>>>
>>>>>> to get the specific implementation for the current container.
>>>>>>
>>>>>> I don't understand the VFIOIOMMUBackendOpsClass pointer and
>>>>>> TYPE_VFIO_IOMMU_BACKEND_OPS. It seems redundant.
>>>>> The original implementation was abstract QOM model. But it wasn't
>> accepted,
>>>>> see https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/ for details.
>>>> I see the idea was challenged, not rejected. I need to dig in further and this
>>>> will take time.
>>> Thanks for help looking into it.
>>>
>>> +David, Hi David, I'm digging into your concern of using QOM to abstract base
>>> container and legacy VFIOContainer:
>>> "The QOM class of things is visible to the user/config layer via QMP (and
>> sometimes command line).
>>> It doesn't necessarily correspond to guest visible differences, but it often does."
>>>
>>> AIUI, while it's true when the QOM type includes TYPE_USER_CREATABLE
>> interface,
>>> otherwise, user can't create an object of this type. Only difference is user will
>> see
>>> "object type '%s' isn't supported by object-add" instead of "invalid object
>> type: %s".
>>> Is your expectation to not permit user to create this object or only want user
>>> to see "invalid object type: %s".
>>> If you mean the first, then I think QOM could be suitable if we don't include
>>> TYPE_USER_CREATABLE interface?
>> I was imagining some kind of QOM hierarchy under the vfio device
>> with various QOM interfaces (similar to the ops) to define the
>> possible IOMMU backends. The fact that we use the IOMMUFD object
> >from the command line made it more plausible. I might be mistaking.
>
> Got your point.
> This way we introduce a new QOM type "vfio-pci-iommufd" for iommufd support,
> and vfio-pci keep same for legacy backend, e.g:
>
> #qemu  -object iommufd,id=iommufd0 \
>               -device vfio-pci-iommufd,iommufd=iommufd0,id=vfio0... \
>              -device vfio-pci,id=vfio1...
you would need to do that for all types for vfio devices, ap, ccw,
platform. Looks heavy to me. Why would we need to use a different
vfio-pci-* device while we could switch the iommu backend according to
the "iommufd" prop presence. The initial discussion was about QOMyfying
the container instead.

Thanks

Eric
 
>
> Below is a draft for vfio-pci based on your imagination. not pasted here the change
> for platform/ap/ccw vfio device which should be similar.
> Not clear if this fully match your imagination.
>
> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
> index 5345986993..54a6ce4d73 100644
> --- a/include/hw/vfio/vfio-container-base.h
> +++ b/include/hw/vfio/vfio-container-base.h
> @@ -124,7 +124,7 @@ DECLARE_CLASS_CHECKERS(VFIOIOMMUBackendOpsClass,
>
>  struct VFIOIOMMUBackendOpsClass {
>      /*< private >*/
> -    ObjectClass parent_class;
> +    InterfaceClass parent_class;
>
>      /*< public >*/
>      /* required */
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index edb787d3d1..829deddc7d 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3725,6 +3725,11 @@ static void vfio_pci_dev_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *pdc = PCI_DEVICE_CLASS(klass);
> +    IOMMU_Backend_Ops_Class *be_ops = IOMMU_BACKEND_OPS_CLASS(klass);
> +
> +    be_ops->dma_map = vfio_legacy_dma_map;
> +    be_ops->dma_unmap = vfio_legacy_dma_unmap;
> +    ...
>
>      dc->reset = vfio_pci_reset;
>      device_class_set_props(dc, vfio_pci_dev_properties);
> @@ -3749,10 +3754,40 @@ static const TypeInfo vfio_pci_dev_info = {
>      .interfaces = (InterfaceInfo[]) {
>          { INTERFACE_PCIE_DEVICE },
>          { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> +        { INTERFACE_IOMMU_BACKEND_OPS },
>          { }
>      },
>  };
>
> +static Property vfio_pci_dev_iommufd_properties[] = {
> +#ifdef CONFIG_IOMMUFD
> +    DEFINE_PROP_LINK("iommufd", VFIOPCIDevice, vbasedev.iommufd,
> +                     TYPE_IOMMUFD_BACKEND, IOMMUFDBackend *),
> +#endif
> +};
> +
> +static void vfio_pci_iommufd_dev_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    IOMMU_Backend_Ops_Class *be_ops = IOMMU_BACKEND_OPS_CLASS(klass);
> +
> +    device_class_set_props(dc, vfio_pci_dev_iommufd_properties);
> +
> +    be_ops->dma_map = iommufd_map;
> +    be_ops->dma_unmap = iommufd_unmap;
> +    ...
> +    /* Unimplemented ops */
> +    be_ops->set_dirty_page_tracking = NULL;
> +    ...
> +}
> +
> +static const TypeInfo vfio_pci_iommufd_dev_info = {
> +    .name = TYPE_VFIO_PCI_IOMMUFD,
> +    .parent = TYPE_VFIO_PCI,
> +    .instance_size = sizeof(VFIOPCIDevice),
> +    .class_init = vfio_pci_iommufd_dev_class_init,
> +};
> +
>  static Property vfio_pci_dev_nohotplug_properties[] = {
>      DEFINE_PROP_BOOL("ramfb", VFIOPCIDevice, enable_ramfb, false),
>      DEFINE_PROP_ON_OFF_AUTO("x-ramfb-migrate", VFIOPCIDevice, ramfb_migrate,
> @@ -3770,13 +3805,20 @@ static void vfio_pci_nohotplug_dev_class_init(ObjectClass *klass, void *data)
>
>  static const TypeInfo vfio_pci_nohotplug_dev_info = {
>      .name = TYPE_VFIO_PCI_NOHOTPLUG,
> -    .parent = TYPE_VFIO_PCI,
> +    .parent = TYPE_VFIO_PCI_IOMMUFD,
>      .instance_size = sizeof(VFIOPCIDevice),
>      .class_init = vfio_pci_nohotplug_dev_class_init,
>  };
>
>  static void register_vfio_pci_dev_type(void)
>  {
> +    static const TypeInfo iommu_be_ops_interface_info = {
> +        .name          = TYPE_IOMMU_BACKEND_OPS,
> +        .parent        = TYPE_INTERFACE,
> +        .class_size = sizeof(VFIOIOMMUBackendOpsClass),
> +    };
> +
> +    type_register_static(&iommu_be_ops_interface_info);
>      type_register_static(&vfio_pci_dev_info);
>      type_register_static(&vfio_pci_nohotplug_dev_info);
>  }
>
> Thanks
> Zhenzhong
>
>> Anyhow, the series looks pretty good. There are other aspect to
>> check, like are all this iommu ops well suited for the need ?
>> Let's stress the models in parallel of the reviews. If we could get
>> some of it for 8.2 that'd be nice. It's on top of my list now.
>>
>> Thanks,
>>
>> C.
>>
>>
>>> Thanks
>>> Zhenzhong
Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and targetted interface
Posted by Cédric Le Goater 1 year, 1 month ago 
On 10/20/23 10:19, Eric Auger wrote:
> Hi,
> On 10/20/23 07:48, Duan, Zhenzhong wrote:
>> Hi Cédric,
>>
>>> -----Original Message-----
>>> From: Cédric Le Goater <clg@redhat.com>
>>> Sent: Thursday, October 19, 2023 8:18 PM
>>> Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and
>>> targetted interface
>>>
>>> On 10/19/23 04:29, Duan, Zhenzhong wrote:
>>>>> -----Original Message-----
>>>>> From: Cédric Le Goater <clg@redhat.com>
>>>>> Sent: Wednesday, October 18, 2023 4:04 PM
>>>>> Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer
>>> and
>>>>> targetted interface
>>>>>
>>>>> On 10/18/23 04:41, Duan, Zhenzhong wrote:
>>>>>> Hi Cédric,
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Cédric Le Goater <clg@redhat.com>
>>>>>>> Sent: Tuesday, October 17, 2023 11:51 PM
>>>>>>> Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer
>>>>> and
>>>>>>> targetted interface
>>>>>>>
>>>>>>> On 10/16/23 10:31, Zhenzhong Duan wrote:
>>>>>>>> From: Eric Auger <eric.auger@redhat.com>
>>>>>>>>
>>>>>>>> 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 | 82
>>>>> +++++++++++++++++++++++++++
>>>>>>>>      2 files changed, 84 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 34648e518e..9651cf921c 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 VFIOLegacyContainer {
>>>>>>>> +    VFIOContainer bcontainer;
>>>>>>> That's the parent class, right ?
>>>>>> Right.
>>>>>>
>>>>>>>>          VFIOAddressSpace *space;
>>>>>>>>          int fd; /* /dev/vfio/vfio, empowered by the attached groups */
>>>>>>>>          MemoryListener listener;
>>>>>>>> @@ -200,12 +202,6 @@ typedef struct VFIODisplay {
>>>>>>>>          } dmabuf;
>>>>>>>>      } VFIODisplay;
>>>>>>>>
>>>>>>>> -typedef struct {
>>>>>>>> -    unsigned long *bitmap;
>>>>>>>> -    hwaddr size;
>>>>>>>> -    hwaddr pages;
>>>>>>>> -} VFIOBitmap;
>>>>>>>> -
>>>>>>>>      void vfio_host_win_add(VFIOLegacyContainer *container,
>>>>>>>>                             hwaddr min_iova, hwaddr max_iova,
>>>>>>>>                             uint64_t iova_pgsizes);
>>>>>>>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>>>>>>> container-base.h
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000000..afc8543d22
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/include/hw/vfio/vfio-container-base.h
>>>>>>>> @@ -0,0 +1,82 @@
>>>>>>>> +/*
>>>>>>>> + * 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 VFIOContainer VFIOContainer;
>>>>>>>> +typedef struct VFIODevice VFIODevice;
>>>>>>>> +typedef struct VFIOIOMMUBackendOpsClass
>>> VFIOIOMMUBackendOpsClass;
>>>>>>>> +
>>>>>>>> +typedef struct {
>>>>>>>> +    unsigned long *bitmap;
>>>>>>>> +    hwaddr size;
>>>>>>>> +    hwaddr pages;
>>>>>>>> +} VFIOBitmap;
>>>>>>>> +
>>>>>>>> +/*
>>>>>>>> + * This is the base object for vfio container backends
>>>>>>>> + */
>>>>>>>> +struct VFIOContainer {
>>>>>>>> +    VFIOIOMMUBackendOpsClass *ops;
>>>>>>> This is unexpected.
>>>>>>>
>>>>>>> I thought that an abstract QOM model for VFIOContainer was going
>>>>>>> to be introduced with a VFIOContainerClass with the ops below
>>>>>>> (VFIOIOMMUBackendOpsClass).
>>>>>>>
>>>>>>> Then, we would call :
>>>>>>>
>>>>>>>       VFIOContainerClass *vcc = VFIO_CONTAINER_GET_CLASS(container);
>>>>>>>
>>>>>>> to get the specific implementation for the current container.
>>>>>>>
>>>>>>> I don't understand the VFIOIOMMUBackendOpsClass pointer and
>>>>>>> TYPE_VFIO_IOMMU_BACKEND_OPS. It seems redundant.
>>>>>> The original implementation was abstract QOM model. But it wasn't
>>> accepted,
>>>>>> see https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/ for details.
>>>>> I see the idea was challenged, not rejected. I need to dig in further and this
>>>>> will take time.
>>>> Thanks for help looking into it.
>>>>
>>>> +David, Hi David, I'm digging into your concern of using QOM to abstract base
>>>> container and legacy VFIOContainer:
>>>> "The QOM class of things is visible to the user/config layer via QMP (and
>>> sometimes command line).
>>>> It doesn't necessarily correspond to guest visible differences, but it often does."
>>>>
>>>> AIUI, while it's true when the QOM type includes TYPE_USER_CREATABLE
>>> interface,
>>>> otherwise, user can't create an object of this type. Only difference is user will
>>> see
>>>> "object type '%s' isn't supported by object-add" instead of "invalid object
>>> type: %s".
>>>> Is your expectation to not permit user to create this object or only want user
>>>> to see "invalid object type: %s".
>>>> If you mean the first, then I think QOM could be suitable if we don't include
>>>> TYPE_USER_CREATABLE interface?
>>> I was imagining some kind of QOM hierarchy under the vfio device
>>> with various QOM interfaces (similar to the ops) to define the
>>> possible IOMMU backends. The fact that we use the IOMMUFD object
>> >from the command line made it more plausible. I might be mistaking.
>>
>> Got your point.
>> This way we introduce a new QOM type "vfio-pci-iommufd" for iommufd support,
>> and vfio-pci keep same for legacy backend, e.g:
>>
>> #qemu  -object iommufd,id=iommufd0 \
>>                -device vfio-pci-iommufd,iommufd=iommufd0,id=vfio0... \
>>               -device vfio-pci,id=vfio1...
> you would need to do that for all types for vfio devices, ap, ccw,
> platform. Looks heavy to me. Why would we need to use a different
> vfio-pci-* device while we could switch the iommu backend according to
> the "iommufd" prop presence. The initial discussion was about QOMyfying
> the container instead.

yes.

I took a closer look at the first part which adds the backend ops,
including patch 19 adding the iommufd backend, not saying that I have
identified all the dark corners.

A QOM-like design would have introduced a VFIOLegacyContainer,
inheriting from VFIOContainer (same for VFIOIOMMUFDContainer) with a
VFIOContainerClass to implement the specific backend ops.
VFIOspaprContainer would have made sense also.

But QOM doesn't seem well adapted for the current needs. So let's try
a simpler approach. It seems that VFIOIOMMUBackendOpsClass is
useless. IMO, it could be a callbacks structure like we have for
memory regions initialized with vfio_container_init(). This would
remove some noise around the QOM typeinfo definitions.

'struct vfio_iommu_ops' reads/sounds like a good name.

Can we try that in a v3 ? It should not be such an earthquake.

spapr has some singularities which would be good to isolate in a
vfio_iommu_spapr_ops to remove all the VFIO_SPAPR_TCE_*_IOMMU code in
container.c. vfio_legacy_{add,del}_section_window are SPAPR specific.

FYI, I did some adjustements bc of the recent introduction of iova_ranges
in my branch :

  https://github.com/legoater/qemu/commits/vfio-8.2

Thanks,

C.
RE: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and targetted interface
Posted by Duan, Zhenzhong 1 year, 1 month ago 

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Monday, October 23, 2023 11:29 PM
><david@gibson.dropbear.id.au>
>Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and
>targetted interface
>
>On 10/20/23 10:19, Eric Auger wrote:
>> Hi,
>> On 10/20/23 07:48, Duan, Zhenzhong wrote:
>>> Hi Cédric,
>>>
>>>> -----Original Message-----
>>>> From: Cédric Le Goater <clg@redhat.com>
>>>> Sent: Thursday, October 19, 2023 8:18 PM
>>>> Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer
>and
>>>> targetted interface
>>>>
>>>> On 10/19/23 04:29, Duan, Zhenzhong wrote:
>>>>>> -----Original Message-----
>>>>>> From: Cédric Le Goater <clg@redhat.com>
>>>>>> Sent: Wednesday, October 18, 2023 4:04 PM
>>>>>> Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for
>VFIOContainer
>>>> and
>>>>>> targetted interface
>>>>>>
>>>>>> On 10/18/23 04:41, Duan, Zhenzhong wrote:
>>>>>>> Hi Cédric,
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Cédric Le Goater <clg@redhat.com>
>>>>>>>> Sent: Tuesday, October 17, 2023 11:51 PM
>>>>>>>> Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for
>VFIOContainer
>>>>>> and
>>>>>>>> targetted interface
>>>>>>>>
>>>>>>>> On 10/16/23 10:31, Zhenzhong Duan wrote:
>>>>>>>>> From: Eric Auger <eric.auger@redhat.com>
>>>>>>>>>
>>>>>>>>> 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 | 82
>>>>>> +++++++++++++++++++++++++++
>>>>>>>>>      2 files changed, 84 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 34648e518e..9651cf921c 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 VFIOLegacyContainer {
>>>>>>>>> +    VFIOContainer bcontainer;
>>>>>>>> That's the parent class, right ?
>>>>>>> Right.
>>>>>>>
>>>>>>>>>          VFIOAddressSpace *space;
>>>>>>>>>          int fd; /* /dev/vfio/vfio, empowered by the attached groups */
>>>>>>>>>          MemoryListener listener;
>>>>>>>>> @@ -200,12 +202,6 @@ typedef struct VFIODisplay {
>>>>>>>>>          } dmabuf;
>>>>>>>>>      } VFIODisplay;
>>>>>>>>>
>>>>>>>>> -typedef struct {
>>>>>>>>> -    unsigned long *bitmap;
>>>>>>>>> -    hwaddr size;
>>>>>>>>> -    hwaddr pages;
>>>>>>>>> -} VFIOBitmap;
>>>>>>>>> -
>>>>>>>>>      void vfio_host_win_add(VFIOLegacyContainer *container,
>>>>>>>>>                             hwaddr min_iova, hwaddr max_iova,
>>>>>>>>>                             uint64_t iova_pgsizes);
>>>>>>>>> diff --git a/include/hw/vfio/vfio-container-base.h
>b/include/hw/vfio/vfio-
>>>>>>>> container-base.h
>>>>>>>>> new file mode 100644
>>>>>>>>> index 0000000000..afc8543d22
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/include/hw/vfio/vfio-container-base.h
>>>>>>>>> @@ -0,0 +1,82 @@
>>>>>>>>> +/*
>>>>>>>>> + * 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 VFIOContainer VFIOContainer;
>>>>>>>>> +typedef struct VFIODevice VFIODevice;
>>>>>>>>> +typedef struct VFIOIOMMUBackendOpsClass
>>>> VFIOIOMMUBackendOpsClass;
>>>>>>>>> +
>>>>>>>>> +typedef struct {
>>>>>>>>> +    unsigned long *bitmap;
>>>>>>>>> +    hwaddr size;
>>>>>>>>> +    hwaddr pages;
>>>>>>>>> +} VFIOBitmap;
>>>>>>>>> +
>>>>>>>>> +/*
>>>>>>>>> + * This is the base object for vfio container backends
>>>>>>>>> + */
>>>>>>>>> +struct VFIOContainer {
>>>>>>>>> +    VFIOIOMMUBackendOpsClass *ops;
>>>>>>>> This is unexpected.
>>>>>>>>
>>>>>>>> I thought that an abstract QOM model for VFIOContainer was going
>>>>>>>> to be introduced with a VFIOContainerClass with the ops below
>>>>>>>> (VFIOIOMMUBackendOpsClass).
>>>>>>>>
>>>>>>>> Then, we would call :
>>>>>>>>
>>>>>>>>       VFIOContainerClass *vcc = VFIO_CONTAINER_GET_CLASS(container);
>>>>>>>>
>>>>>>>> to get the specific implementation for the current container.
>>>>>>>>
>>>>>>>> I don't understand the VFIOIOMMUBackendOpsClass pointer and
>>>>>>>> TYPE_VFIO_IOMMU_BACKEND_OPS. It seems redundant.
>>>>>>> The original implementation was abstract QOM model. But it wasn't
>>>> accepted,
>>>>>>> see https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/ for
>details.
>>>>>> I see the idea was challenged, not rejected. I need to dig in further and this
>>>>>> will take time.
>>>>> Thanks for help looking into it.
>>>>>
>>>>> +David, Hi David, I'm digging into your concern of using QOM to abstract
>base
>>>>> container and legacy VFIOContainer:
>>>>> "The QOM class of things is visible to the user/config layer via QMP (and
>>>> sometimes command line).
>>>>> It doesn't necessarily correspond to guest visible differences, but it often
>does."
>>>>>
>>>>> AIUI, while it's true when the QOM type includes TYPE_USER_CREATABLE
>>>> interface,
>>>>> otherwise, user can't create an object of this type. Only difference is user
>will
>>>> see
>>>>> "object type '%s' isn't supported by object-add" instead of "invalid object
>>>> type: %s".
>>>>> Is your expectation to not permit user to create this object or only want
>user
>>>>> to see "invalid object type: %s".
>>>>> If you mean the first, then I think QOM could be suitable if we don't include
>>>>> TYPE_USER_CREATABLE interface?
>>>> I was imagining some kind of QOM hierarchy under the vfio device
>>>> with various QOM interfaces (similar to the ops) to define the
>>>> possible IOMMU backends. The fact that we use the IOMMUFD object
>>> >from the command line made it more plausible. I might be mistaking.
>>>
>>> Got your point.
>>> This way we introduce a new QOM type "vfio-pci-iommufd" for iommufd
>support,
>>> and vfio-pci keep same for legacy backend, e.g:
>>>
>>> #qemu  -object iommufd,id=iommufd0 \
>>>                -device vfio-pci-iommufd,iommufd=iommufd0,id=vfio0... \
>>>               -device vfio-pci,id=vfio1...
>> you would need to do that for all types for vfio devices, ap, ccw,
>> platform. Looks heavy to me. Why would we need to use a different
>> vfio-pci-* device while we could switch the iommu backend according to
>> the "iommufd" prop presence. The initial discussion was about QOMyfying
>> the container instead.
>
>yes.
>
>I took a closer look at the first part which adds the backend ops,
>including patch 19 adding the iommufd backend, not saying that I have
>identified all the dark corners.
>
>A QOM-like design would have introduced a VFIOLegacyContainer,
>inheriting from VFIOContainer (same for VFIOIOMMUFDContainer) with a
>VFIOContainerClass to implement the specific backend ops.
>VFIOspaprContainer would have made sense also.
>
>But QOM doesn't seem well adapted for the current needs. So let's try
>a simpler approach. It seems that VFIOIOMMUBackendOpsClass is
>useless. IMO, it could be a callbacks structure like we have for
>memory regions initialized with vfio_container_init(). This would
>remove some noise around the QOM typeinfo definitions.

Yes, good suggestion, will do.

>
>'struct vfio_iommu_ops' reads/sounds like a good name.
>
>Can we try that in a v3 ? It should not be such an earthquake.

Sure.

>
>spapr has some singularities which would be good to isolate in a
>vfio_iommu_spapr_ops to remove all the VFIO_SPAPR_TCE_*_IOMMU code in
>container.c. vfio_legacy_{add,del}_section_window are SPAPR specific.

Yes, let me try it.

Thanks
Zhenzhong
>
>FYI, I did some adjustements bc of the recent introduction of iova_ranges
>in my branch :
>
>  https://github.com/legoater/qemu/commits/vfio-8.2
>
>Thanks,
>
>C.
>
Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and targetted interface
Posted by Cédric Le Goater 1 year, 1 month ago 
Hello Zhenzhong,

>>> you would need to do that for all types for vfio devices, ap, ccw,
>>> platform. Looks heavy to me. Why would we need to use a different
>>> vfio-pci-* device while we could switch the iommu backend according to
>>> the "iommufd" prop presence. The initial discussion was about QOMyfying
>>> the container instead.
>>
>> yes.
>>
>> I took a closer look at the first part which adds the backend ops,
>> including patch 19 adding the iommufd backend, not saying that I have
>> identified all the dark corners.
>>
>> A QOM-like design would have introduced a VFIOLegacyContainer,
>> inheriting from VFIOContainer (same for VFIOIOMMUFDContainer) with a
>> VFIOContainerClass to implement the specific backend ops.
>> VFIOspaprContainer would have made sense also.
>>
>> But QOM doesn't seem well adapted for the current needs. So let's try
>> a simpler approach. It seems that VFIOIOMMUBackendOpsClass is
>> useless. IMO, it could be a callbacks structure like we have for
>> memory regions initialized with vfio_container_init(). This would
>> remove some noise around the QOM typeinfo definitions.
> 
> Yes, good suggestion, will do.
>
Thanks,

>>
>> 'struct vfio_iommu_ops' reads/sounds like a good name.

This should be read as VFIOIOMMUOps :)

>> Can we try that in a v3 ? It should not be such an earthquake.
> 
> Sure.
> 
>>
>> spapr has some singularities which would be good to isolate in a
>> vfio_iommu_spapr_ops to remove all the VFIO_SPAPR_TCE_*_IOMMU code in
>> container.c. vfio_legacy_{add,del}_section_window are SPAPR specific.
> 
> Yes, let me try it.

There is a large amount of spapr code in container.c. If it could
be moved under its own backend (and in hw/ppc/spapr_pci_vfio.c),
we would have two backends : spapr and legacy. We could possibly
merge this first part in 8.2.

I have access to a PPC P9 system. I can make sure we are not breaking
support.

C.



> Thanks
> Zhenzhong
>>
>> FYI, I did some adjustements bc of the recent introduction of iova_ranges
>> in my branch :
>>
>>   https://github.com/legoater/qemu/commits/vfio-8.2
>>
>> Thanks,
>>
>> C.
>>
>
RE: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and targetted interface
Posted by Duan, Zhenzhong 1 year, 1 month ago 

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Sent: Friday, October 20, 2023 4:19 PM
>Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and
>targetted interface
>
>Hi,
>On 10/20/23 07:48, Duan, Zhenzhong wrote:
>> Hi Cédric,
>>
>>> -----Original Message-----
>>> From: Cédric Le Goater <clg@redhat.com>
>>> Sent: Thursday, October 19, 2023 8:18 PM
>>> Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer
>and
>>> targetted interface
>>>
>>> On 10/19/23 04:29, Duan, Zhenzhong wrote:
>>>>> -----Original Message-----
>>>>> From: Cédric Le Goater <clg@redhat.com>
>>>>> Sent: Wednesday, October 18, 2023 4:04 PM
>>>>> Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer
>>> and
>>>>> targetted interface
>>>>>
>>>>> On 10/18/23 04:41, Duan, Zhenzhong wrote:
>>>>>> Hi Cédric,
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Cédric Le Goater <clg@redhat.com>
>>>>>>> Sent: Tuesday, October 17, 2023 11:51 PM
>>>>>>> Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for
>VFIOContainer
>>>>> and
>>>>>>> targetted interface
>>>>>>>
>>>>>>> On 10/16/23 10:31, Zhenzhong Duan wrote:
>>>>>>>> From: Eric Auger <eric.auger@redhat.com>
>>>>>>>>
>>>>>>>> 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 | 82
>>>>> +++++++++++++++++++++++++++
>>>>>>>>     2 files changed, 84 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 34648e518e..9651cf921c 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 VFIOLegacyContainer {
>>>>>>>> +    VFIOContainer bcontainer;
>>>>>>> That's the parent class, right ?
>>>>>> Right.
>>>>>>
>>>>>>>>         VFIOAddressSpace *space;
>>>>>>>>         int fd; /* /dev/vfio/vfio, empowered by the attached groups */
>>>>>>>>         MemoryListener listener;
>>>>>>>> @@ -200,12 +202,6 @@ typedef struct VFIODisplay {
>>>>>>>>         } dmabuf;
>>>>>>>>     } VFIODisplay;
>>>>>>>>
>>>>>>>> -typedef struct {
>>>>>>>> -    unsigned long *bitmap;
>>>>>>>> -    hwaddr size;
>>>>>>>> -    hwaddr pages;
>>>>>>>> -} VFIOBitmap;
>>>>>>>> -
>>>>>>>>     void vfio_host_win_add(VFIOLegacyContainer *container,
>>>>>>>>                            hwaddr min_iova, hwaddr max_iova,
>>>>>>>>                            uint64_t iova_pgsizes);
>>>>>>>> diff --git a/include/hw/vfio/vfio-container-base.h
>b/include/hw/vfio/vfio-
>>>>>>> container-base.h
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000000..afc8543d22
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/include/hw/vfio/vfio-container-base.h
>>>>>>>> @@ -0,0 +1,82 @@
>>>>>>>> +/*
>>>>>>>> + * 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 VFIOContainer VFIOContainer;
>>>>>>>> +typedef struct VFIODevice VFIODevice;
>>>>>>>> +typedef struct VFIOIOMMUBackendOpsClass
>>> VFIOIOMMUBackendOpsClass;
>>>>>>>> +
>>>>>>>> +typedef struct {
>>>>>>>> +    unsigned long *bitmap;
>>>>>>>> +    hwaddr size;
>>>>>>>> +    hwaddr pages;
>>>>>>>> +} VFIOBitmap;
>>>>>>>> +
>>>>>>>> +/*
>>>>>>>> + * This is the base object for vfio container backends
>>>>>>>> + */
>>>>>>>> +struct VFIOContainer {
>>>>>>>> +    VFIOIOMMUBackendOpsClass *ops;
>>>>>>> This is unexpected.
>>>>>>>
>>>>>>> I thought that an abstract QOM model for VFIOContainer was going
>>>>>>> to be introduced with a VFIOContainerClass with the ops below
>>>>>>> (VFIOIOMMUBackendOpsClass).
>>>>>>>
>>>>>>> Then, we would call :
>>>>>>>
>>>>>>>      VFIOContainerClass *vcc = VFIO_CONTAINER_GET_CLASS(container);
>>>>>>>
>>>>>>> to get the specific implementation for the current container.
>>>>>>>
>>>>>>> I don't understand the VFIOIOMMUBackendOpsClass pointer and
>>>>>>> TYPE_VFIO_IOMMU_BACKEND_OPS. It seems redundant.
>>>>>> The original implementation was abstract QOM model. But it wasn't
>>> accepted,
>>>>>> see https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/ for details.
>>>>> I see the idea was challenged, not rejected. I need to dig in further and this
>>>>> will take time.
>>>> Thanks for help looking into it.
>>>>
>>>> +David, Hi David, I'm digging into your concern of using QOM to abstract
>base
>>>> container and legacy VFIOContainer:
>>>> "The QOM class of things is visible to the user/config layer via QMP (and
>>> sometimes command line).
>>>> It doesn't necessarily correspond to guest visible differences, but it often
>does."
>>>>
>>>> AIUI, while it's true when the QOM type includes TYPE_USER_CREATABLE
>>> interface,
>>>> otherwise, user can't create an object of this type. Only difference is user will
>>> see
>>>> "object type '%s' isn't supported by object-add" instead of "invalid object
>>> type: %s".
>>>> Is your expectation to not permit user to create this object or only want user
>>>> to see "invalid object type: %s".
>>>> If you mean the first, then I think QOM could be suitable if we don't include
>>>> TYPE_USER_CREATABLE interface?
>>> I was imagining some kind of QOM hierarchy under the vfio device
>>> with various QOM interfaces (similar to the ops) to define the
>>> possible IOMMU backends. The fact that we use the IOMMUFD object
>> >from the command line made it more plausible. I might be mistaking.
>>
>> Got your point.
>> This way we introduce a new QOM type "vfio-pci-iommufd" for iommufd
>support,
>> and vfio-pci keep same for legacy backend, e.g:
>>
>> #qemu  -object iommufd,id=iommufd0 \
>>               -device vfio-pci-iommufd,iommufd=iommufd0,id=vfio0... \
>>              -device vfio-pci,id=vfio1...
>you would need to do that for all types for vfio devices, ap, ccw,
>platform. Looks heavy to me.

Yes, this is a try out following Cédric's imagination, pasted here just for discuss.

>Why would we need to use a different
>vfio-pci-* device while we could switch the iommu backend according to
>the "iommufd" prop presence. The initial discussion was about QOMyfying
>the container instead.

We indeed are switching backend according to "iommufd" prop though not
QOMtfying the container in this series.

Thanks
Zhenzhong

>
>Thanks
>
>Eric
>
>>
>> Below is a draft for vfio-pci based on your imagination. not pasted here the
>change
>> for platform/ap/ccw vfio device which should be similar.
>> Not clear if this fully match your imagination.
>>
>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>container-base.h
>> index 5345986993..54a6ce4d73 100644
>> --- a/include/hw/vfio/vfio-container-base.h
>> +++ b/include/hw/vfio/vfio-container-base.h
>> @@ -124,7 +124,7 @@
>DECLARE_CLASS_CHECKERS(VFIOIOMMUBackendOpsClass,
>>
>>  struct VFIOIOMMUBackendOpsClass {
>>      /*< private >*/
>> -    ObjectClass parent_class;
>> +    InterfaceClass parent_class;
>>
>>      /*< public >*/
>>      /* required */
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index edb787d3d1..829deddc7d 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -3725,6 +3725,11 @@ static void vfio_pci_dev_class_init(ObjectClass
>*klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>      PCIDeviceClass *pdc = PCI_DEVICE_CLASS(klass);
>> +    IOMMU_Backend_Ops_Class *be_ops =
>IOMMU_BACKEND_OPS_CLASS(klass);
>> +
>> +    be_ops->dma_map = vfio_legacy_dma_map;
>> +    be_ops->dma_unmap = vfio_legacy_dma_unmap;
>> +    ...
>>
>>      dc->reset = vfio_pci_reset;
>>      device_class_set_props(dc, vfio_pci_dev_properties);
>> @@ -3749,10 +3754,40 @@ static const TypeInfo vfio_pci_dev_info = {
>>      .interfaces = (InterfaceInfo[]) {
>>          { INTERFACE_PCIE_DEVICE },
>>          { INTERFACE_CONVENTIONAL_PCI_DEVICE },
>> +        { INTERFACE_IOMMU_BACKEND_OPS },
>>          { }
>>      },
>>  };
>>
>> +static Property vfio_pci_dev_iommufd_properties[] = {
>> +#ifdef CONFIG_IOMMUFD
>> +    DEFINE_PROP_LINK("iommufd", VFIOPCIDevice, vbasedev.iommufd,
>> +                     TYPE_IOMMUFD_BACKEND, IOMMUFDBackend *),
>> +#endif
>> +};
>> +
>> +static void vfio_pci_iommufd_dev_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    IOMMU_Backend_Ops_Class *be_ops =
>IOMMU_BACKEND_OPS_CLASS(klass);
>> +
>> +    device_class_set_props(dc, vfio_pci_dev_iommufd_properties);
>> +
>> +    be_ops->dma_map = iommufd_map;
>> +    be_ops->dma_unmap = iommufd_unmap;
>> +    ...
>> +    /* Unimplemented ops */
>> +    be_ops->set_dirty_page_tracking = NULL;
>> +    ...
>> +}
>> +
>> +static const TypeInfo vfio_pci_iommufd_dev_info = {
>> +    .name = TYPE_VFIO_PCI_IOMMUFD,
>> +    .parent = TYPE_VFIO_PCI,
>> +    .instance_size = sizeof(VFIOPCIDevice),
>> +    .class_init = vfio_pci_iommufd_dev_class_init,
>> +};
>> +
>>  static Property vfio_pci_dev_nohotplug_properties[] = {
>>      DEFINE_PROP_BOOL("ramfb", VFIOPCIDevice, enable_ramfb, false),
>>      DEFINE_PROP_ON_OFF_AUTO("x-ramfb-migrate", VFIOPCIDevice,
>ramfb_migrate,
>> @@ -3770,13 +3805,20 @@ static void
>vfio_pci_nohotplug_dev_class_init(ObjectClass *klass, void *data)
>>
>>  static const TypeInfo vfio_pci_nohotplug_dev_info = {
>>      .name = TYPE_VFIO_PCI_NOHOTPLUG,
>> -    .parent = TYPE_VFIO_PCI,
>> +    .parent = TYPE_VFIO_PCI_IOMMUFD,
>>      .instance_size = sizeof(VFIOPCIDevice),
>>      .class_init = vfio_pci_nohotplug_dev_class_init,
>>  };
>>
>>  static void register_vfio_pci_dev_type(void)
>>  {
>> +    static const TypeInfo iommu_be_ops_interface_info = {
>> +        .name          = TYPE_IOMMU_BACKEND_OPS,
>> +        .parent        = TYPE_INTERFACE,
>> +        .class_size = sizeof(VFIOIOMMUBackendOpsClass),
>> +    };
>> +
>> +    type_register_static(&iommu_be_ops_interface_info);
>>      type_register_static(&vfio_pci_dev_info);
>>      type_register_static(&vfio_pci_nohotplug_dev_info);
>>  }
>>
>> Thanks
>> Zhenzhong
>>
>>> Anyhow, the series looks pretty good. There are other aspect to
>>> check, like are all this iommu ops well suited for the need ?
>>> Let's stress the models in parallel of the reviews. If we could get
>>> some of it for 8.2 that'd be nice. It's on top of my list now.
>>>
>>> Thanks,
>>>
>>> C.
>>>
>>>
>>>> Thanks
>>>> Zhenzhong

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.