[PATCH] vfio: container: Fix missing allocation of VFIOSpaprContainer

[PATCH] vfio: container: Fix missing allocation of VFIOSpaprContainer

[Shivaprasad G Bhat]

The commit 6ad359ec29 "(vfio/spapr: Move prereg_listener into
spapr container)" began to use the newly introduced VFIOSpaprContainer
structure.

After several refactors, today the container_of(container,
VFIOSpaprContainer, ABC) is used when VFIOSpaprContainer is actually
not allocated. On PPC64 systems, this dereference is leading to corruption
showing up as glibc malloc assertion during guest start when using vfio.

Patch adds the missing allocation while also making the structure movement
to vfio common header file.

Fixes: 6ad359ec29 "(vfio/spapr: Move prereg_listener into spapr container)"
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
---
 hw/vfio/container.c           |    6 ++++--
 hw/vfio/spapr.c               |    6 ------
 include/hw/vfio/vfio-common.h |    6 ++++++
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 77bdec276e..ecaf5786d9 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -539,6 +539,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
 {
     VFIOContainer *container;
     VFIOContainerBase *bcontainer;
+    VFIOSpaprContainer *scontainer;
     int ret, fd;
     VFIOAddressSpace *space;

@@ -611,7 +612,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
         goto close_fd_exit;
     }

-    container = g_malloc0(sizeof(*container));
+    scontainer = g_malloc0(sizeof(*scontainer));
+    container = &scontainer->container;
     container->fd = fd;
     bcontainer = &container->bcontainer;

@@ -675,7 +677,7 @@ unregister_container_exit:
     vfio_cpr_unregister_container(bcontainer);

 free_container_exit:
-    g_free(container);
+    g_free(scontainer);

 close_fd_exit:
     close(fd);
diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
index 0d949bb728..78d218b7e7 100644
--- a/hw/vfio/spapr.c
+++ b/hw/vfio/spapr.c
@@ -24,12 +24,6 @@
 #include "qapi/error.h"
 #include "trace.h"

-typedef struct VFIOSpaprContainer {
-    VFIOContainer container;
-    MemoryListener prereg_listener;
-    QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
-} VFIOSpaprContainer;
-
 static bool vfio_prereg_listener_skipped_section(MemoryRegionSection *section)
 {
     if (memory_region_is_iommu(section->mr)) {
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index b9da6c08ef..010fa68ac6 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -82,6 +82,12 @@ typedef struct VFIOContainer {
     QLIST_HEAD(, VFIOGroup) group_list;
 } VFIOContainer;

+typedef struct VFIOSpaprContainer {
+    VFIOContainer container;
+    MemoryListener prereg_listener;
+    QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
+} VFIOSpaprContainer;
+
 typedef struct VFIOHostDMAWindow {
     hwaddr min_iova;
     hwaddr max_iova;

RE: [PATCH] vfio: container: Fix missing allocation of VFIOSpaprContainer

[Duan, Zhenzhong]

>-----Original Message-----
>From: Shivaprasad G Bhat <sbhat@linux.ibm.com>
>Subject: [PATCH] vfio: container: Fix missing allocation of
>VFIOSpaprContainer
>
>The commit 6ad359ec29 "(vfio/spapr: Move prereg_listener into
>spapr container)" began to use the newly introduced VFIOSpaprContainer
>structure.
>
>After several refactors, today the container_of(container,
>VFIOSpaprContainer, ABC) is used when VFIOSpaprContainer is actually
>not allocated. On PPC64 systems, this dereference is leading to corruption
>showing up as glibc malloc assertion during guest start when using vfio.
>
>Patch adds the missing allocation while also making the structure movement
>to vfio common header file.
>
>Fixes: 6ad359ec29 "(vfio/spapr: Move prereg_listener into spapr container)"
>Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>

Reviewed-by: Zhenzhong Duan <Zhenzhong.duan@intel.com>

An alternative way is to introduce a VFIOIOMMUClass::create or
VFIOIOMMUClass::get_container_size.
But that needs some refactor to vfio_connect_container().

Thanks
Zhenzhong

>---
> hw/vfio/container.c           |    6 ++++--
> hw/vfio/spapr.c               |    6 ------
> include/hw/vfio/vfio-common.h |    6 ++++++
> 3 files changed, 10 insertions(+), 8 deletions(-)
>
>diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>index 77bdec276e..ecaf5786d9 100644
>--- a/hw/vfio/container.c
>+++ b/hw/vfio/container.c
>@@ -539,6 +539,7 @@ static int vfio_connect_container(VFIOGroup *group,
>AddressSpace *as,
> {
>     VFIOContainer *container;
>     VFIOContainerBase *bcontainer;
>+    VFIOSpaprContainer *scontainer;
>     int ret, fd;
>     VFIOAddressSpace *space;
>
>@@ -611,7 +612,8 @@ static int vfio_connect_container(VFIOGroup *group,
>AddressSpace *as,
>         goto close_fd_exit;
>     }
>
>-    container = g_malloc0(sizeof(*container));
>+    scontainer = g_malloc0(sizeof(*scontainer));
>+    container = &scontainer->container;
>     container->fd = fd;
>     bcontainer = &container->bcontainer;
>
>@@ -675,7 +677,7 @@ unregister_container_exit:
>     vfio_cpr_unregister_container(bcontainer);
>
> free_container_exit:
>-    g_free(container);
>+    g_free(scontainer);
>
> close_fd_exit:
>     close(fd);
>diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
>index 0d949bb728..78d218b7e7 100644
>--- a/hw/vfio/spapr.c
>+++ b/hw/vfio/spapr.c
>@@ -24,12 +24,6 @@
> #include "qapi/error.h"
> #include "trace.h"
>
>-typedef struct VFIOSpaprContainer {
>-    VFIOContainer container;
>-    MemoryListener prereg_listener;
>-    QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
>-} VFIOSpaprContainer;
>-
> static bool vfio_prereg_listener_skipped_section(MemoryRegionSection
>*section)
> {
>     if (memory_region_is_iommu(section->mr)) {
>diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>common.h
>index b9da6c08ef..010fa68ac6 100644
>--- a/include/hw/vfio/vfio-common.h
>+++ b/include/hw/vfio/vfio-common.h
>@@ -82,6 +82,12 @@ typedef struct VFIOContainer {
>     QLIST_HEAD(, VFIOGroup) group_list;
> } VFIOContainer;
>
>+typedef struct VFIOSpaprContainer {
>+    VFIOContainer container;
>+    MemoryListener prereg_listener;
>+    QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
>+} VFIOSpaprContainer;
>+
> typedef struct VFIOHostDMAWindow {
>     hwaddr min_iova;
>     hwaddr max_iova;
>

Re: [PATCH] vfio: container: Fix missing allocation of VFIOSpaprContainer

[Cédric Le Goater]

Hello Shivaprasad,

On 5/9/24 21:14, Shivaprasad G Bhat wrote:
> The commit 6ad359ec29 "(vfio/spapr: Move prereg_listener into
> spapr container)" began to use the newly introduced VFIOSpaprContainer
> structure.
> 
> After several refactors, today the container_of(container,
> VFIOSpaprContainer, ABC) is used when VFIOSpaprContainer is actually
> not allocated. On PPC64 systems, this dereference is leading to corruption
> showing up as glibc malloc assertion during guest start when using vfio.
> 
> Patch adds the missing allocation while also making the structure movement
> to vfio common header file.
> 
> Fixes: 6ad359ec29 "(vfio/spapr: Move prereg_listener into spapr container)"
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> ---
>   hw/vfio/container.c           |    6 ++++--
>   hw/vfio/spapr.c               |    6 ------
>   include/hw/vfio/vfio-common.h |    6 ++++++
>   3 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 77bdec276e..ecaf5786d9 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -539,6 +539,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>   {
>       VFIOContainer *container;
>       VFIOContainerBase *bcontainer;
> +    VFIOSpaprContainer *scontainer;

We should do our best to avoid any direct use of ppc related attributes
in the common VFIO code. This comment also applies to VFIO_SPAPR_TCE*
which are still there because the clean up is not finished. So, this
proposal will have to be reworked.

The first step is to finish the QOMification of VFIOContainer, so
that the VFIOContainer instance is created in vfio_connect_container()
with :

     container = qdev_new(iommu_type_name);

This means reworking this part (and vfio_set_iommu()) :

     ...
     container = g_malloc0(sizeof(*container));
     container->fd = fd;
     bcontainer = &container->bcontainer;

     if (!vfio_set_iommu(container, group->fd, space, errp)) {
         goto free_container_exit;
     }
     ...

VFIOSpaprContainer can then implement its own .init_instance() handler
to allocate/initialize attributes required by the pseries machines.

While doing this, please try to reduce the use of ->iommu_type which
is a design shortcut. I would like to completely remove it at some
point.

Thanks,

C.







>       int ret, fd;
>       VFIOAddressSpace *space;
> 
> @@ -611,7 +612,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>           goto close_fd_exit;
>       }
> 
> -    container = g_malloc0(sizeof(*container));
> +    scontainer = g_malloc0(sizeof(*scontainer));
> +    container = &scontainer->container;
>       container->fd = fd;
>       bcontainer = &container->bcontainer;
> 
> @@ -675,7 +677,7 @@ unregister_container_exit:
>       vfio_cpr_unregister_container(bcontainer);
> 
>   free_container_exit:
> -    g_free(container);
> +    g_free(scontainer);
> 
>   close_fd_exit:
>       close(fd);
> diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
> index 0d949bb728..78d218b7e7 100644
> --- a/hw/vfio/spapr.c
> +++ b/hw/vfio/spapr.c
> @@ -24,12 +24,6 @@
>   #include "qapi/error.h"
>   #include "trace.h"
> 
> -typedef struct VFIOSpaprContainer {
> -    VFIOContainer container;
> -    MemoryListener prereg_listener;
> -    QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
> -} VFIOSpaprContainer;
> -
>   static bool vfio_prereg_listener_skipped_section(MemoryRegionSection *section)
>   {
>       if (memory_region_is_iommu(section->mr)) {
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index b9da6c08ef..010fa68ac6 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -82,6 +82,12 @@ typedef struct VFIOContainer {
>       QLIST_HEAD(, VFIOGroup) group_list;
>   } VFIOContainer;
> 
> +typedef struct VFIOSpaprContainer {
> +    VFIOContainer container;
> +    MemoryListener prereg_listener;
> +    QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
> +} VFIOSpaprContainer;
> +
>   typedef struct VFIOHostDMAWindow {
>       hwaddr min_iova;
>       hwaddr max_iova;
> 
> 

Re: [PATCH] vfio: container: Fix missing allocation of VFIOSpaprContainer

[Shivaprasad G Bhat]

On 5/13/24 17:53, Cédric Le Goater wrote:
> Hello Shivaprasad,
>
> On 5/9/24 21:14, Shivaprasad G Bhat wrote:
>> The commit 6ad359ec29 "(vfio/spapr: Move prereg_listener into
>> spapr container)" began to use the newly introduced VFIOSpaprContainer
>> structure.
>>
>> After several refactors, today the container_of(container,
>> VFIOSpaprContainer, ABC) is used when VFIOSpaprContainer is actually
>> not allocated. On PPC64 systems, this dereference is leading to 
>> corruption
>> showing up as glibc malloc assertion during guest start when using vfio.
>>
>> Patch adds the missing allocation while also making the structure 
>> movement
>> to vfio common header file.
>>
>> Fixes: 6ad359ec29 "(vfio/spapr: Move prereg_listener into spapr 
>> container)"
>> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
>> ---
>>   hw/vfio/container.c           |    6 ++++--
>>   hw/vfio/spapr.c               |    6 ------
>>   include/hw/vfio/vfio-common.h |    6 ++++++
>>   3 files changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>> index 77bdec276e..ecaf5786d9 100644
>> --- a/hw/vfio/container.c
>> +++ b/hw/vfio/container.c
>> @@ -539,6 +539,7 @@ static int vfio_connect_container(VFIOGroup 
>> *group, AddressSpace *as,
>>   {
>>       VFIOContainer *container;
>>       VFIOContainerBase *bcontainer;
>> +    VFIOSpaprContainer *scontainer;
>
> We should do our best to avoid any direct use of ppc related attributes
> in the common VFIO code. This comment also applies to VFIO_SPAPR_TCE*
> which are still there because the clean up is not finished. So, this
> proposal will have to be reworked.
>
Sure.
> The first step is to finish the QOMification of VFIOContainer, so
> that the VFIOContainer instance is created in vfio_connect_container()
> with :
>
>     container = qdev_new(iommu_type_name);

This requires the VFIOContainer to be a DeviceState object.

The existing base class TYPE_VFIO_IOMMU is an InterfaceClass.

I attempted VFIOContainer object declaration with TYPE_VFIO_IOMMU,

like

OBJECT_DECLARE_SIMPLE_TYPE(VFIOContainer, VFIO_IOMMU_LEGACY)


>
> This means reworking this part (and vfio_set_iommu()) :
>
>     ...
>     container = g_malloc0(sizeof(*container));
>     container->fd = fd;
>     bcontainer = &container->bcontainer;
>
>     if (!vfio_set_iommu(container, group->fd, space, errp)) {
>         goto free_container_exit;
>     }
>     ...
>
> VFIOSpaprContainer can then implement its own .init_instance() handler
> to allocate/initialize attributes required by the pseries machines.


With my above changes,

I see the instance_init() is not supported for the InterfaceClass with the

checks from below

commit 422ca1432f7b44f2a9f3ad94a65d36927da021fa
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Wed Sep 12 16:53:03 2018 +0400

     qom/object: add some interface asserts

Did you suggest me something else?


Thank you,

Shivaprasad

>
> While doing this, please try to reduce the use of ->iommu_type which
> is a design shortcut. I would like to completely remove it at some
> point.
>
> Thanks,
>
> C.
>
>
>
>
>
>
>
>>       int ret, fd;
>>       VFIOAddressSpace *space;
>>
>> @@ -611,7 +612,8 @@ static int vfio_connect_container(VFIOGroup 
>> *group, AddressSpace *as,
>>           goto close_fd_exit;
>>       }
>>
>> -    container = g_malloc0(sizeof(*container));
>> +    scontainer = g_malloc0(sizeof(*scontainer));
>> +    container = &scontainer->container;
>>       container->fd = fd;
>>       bcontainer = &container->bcontainer;
>>
>> @@ -675,7 +677,7 @@ unregister_container_exit:
>>       vfio_cpr_unregister_container(bcontainer);
>>
>>   free_container_exit:
>> -    g_free(container);
>> +    g_free(scontainer);
>>
>>   close_fd_exit:
>>       close(fd);
>> diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
>> index 0d949bb728..78d218b7e7 100644
>> --- a/hw/vfio/spapr.c
>> +++ b/hw/vfio/spapr.c
>> @@ -24,12 +24,6 @@
>>   #include "qapi/error.h"
>>   #include "trace.h"
>>
>> -typedef struct VFIOSpaprContainer {
>> -    VFIOContainer container;
>> -    MemoryListener prereg_listener;
>> -    QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
>> -} VFIOSpaprContainer;
>> -
>>   static bool 
>> vfio_prereg_listener_skipped_section(MemoryRegionSection *section)
>>   {
>>       if (memory_region_is_iommu(section->mr)) {
>> diff --git a/include/hw/vfio/vfio-common.h 
>> b/include/hw/vfio/vfio-common.h
>> index b9da6c08ef..010fa68ac6 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -82,6 +82,12 @@ typedef struct VFIOContainer {
>>       QLIST_HEAD(, VFIOGroup) group_list;
>>   } VFIOContainer;
>>
>> +typedef struct VFIOSpaprContainer {
>> +    VFIOContainer container;
>> +    MemoryListener prereg_listener;
>> +    QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
>> +} VFIOSpaprContainer;
>> +
>>   typedef struct VFIOHostDMAWindow {
>>       hwaddr min_iova;
>>       hwaddr max_iova;
>>
>>
>

Re: [PATCH] vfio: container: Fix missing allocation of VFIOSpaprContainer

[Cédric Le Goater]

On 5/22/24 18:15, Shivaprasad G Bhat wrote:
> On 5/13/24 17:53, Cédric Le Goater wrote:
>> Hello Shivaprasad,
>>
>> On 5/9/24 21:14, Shivaprasad G Bhat wrote:
>>> The commit 6ad359ec29 "(vfio/spapr: Move prereg_listener into
>>> spapr container)" began to use the newly introduced VFIOSpaprContainer
>>> structure.
>>>
>>> After several refactors, today the container_of(container,
>>> VFIOSpaprContainer, ABC) is used when VFIOSpaprContainer is actually
>>> not allocated. On PPC64 systems, this dereference is leading to corruption
>>> showing up as glibc malloc assertion during guest start when using vfio.
>>>
>>> Patch adds the missing allocation while also making the structure movement
>>> to vfio common header file.
>>>
>>> Fixes: 6ad359ec29 "(vfio/spapr: Move prereg_listener into spapr container)"
>>> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
>>> ---
>>>   hw/vfio/container.c           |    6 ++++--
>>>   hw/vfio/spapr.c               |    6 ------
>>>   include/hw/vfio/vfio-common.h |    6 ++++++
>>>   3 files changed, 10 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>>> index 77bdec276e..ecaf5786d9 100644
>>> --- a/hw/vfio/container.c
>>> +++ b/hw/vfio/container.c
>>> @@ -539,6 +539,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>>>   {
>>>       VFIOContainer *container;
>>>       VFIOContainerBase *bcontainer;
>>> +    VFIOSpaprContainer *scontainer;
>>
>> We should do our best to avoid any direct use of ppc related attributes
>> in the common VFIO code. This comment also applies to VFIO_SPAPR_TCE*
>> which are still there because the clean up is not finished. So, this
>> proposal will have to be reworked.
>>
> Sure.
>> The first step is to finish the QOMification of VFIOContainer, so
>> that the VFIOContainer instance is created in vfio_connect_container()
>> with :
>>
>>     container = qdev_new(iommu_type_name);
> 
> This requires the VFIOContainer to be a DeviceState object.
> 
> The existing base class TYPE_VFIO_IOMMU is an InterfaceClass.
> 
> I attempted VFIOContainer object declaration with TYPE_VFIO_IOMMU,
> 
> like
> 
> OBJECT_DECLARE_SIMPLE_TYPE(VFIOContainer, VFIO_IOMMU_LEGACY)
> 
> 
>>
>> This means reworking this part (and vfio_set_iommu()) :
>>
>>     ...
>>     container = g_malloc0(sizeof(*container));
>>     container->fd = fd;
>>     bcontainer = &container->bcontainer;
>>
>>     if (!vfio_set_iommu(container, group->fd, space, errp)) {
>>         goto free_container_exit;
>>     }
>>     ...
>>
>> VFIOSpaprContainer can then implement its own .init_instance() handler
>> to allocate/initialize attributes required by the pseries machines.
> 
> 
> With my above changes,
> 
> I see the instance_init() is not supported for the InterfaceClass with the


Yes. We need an Object, hence my remark on "QOMification of VFIOContainer".
VFIOContainerBase needs to be reworked.

Thanks,

C.

> 
> checks from below
> 
> commit 422ca1432f7b44f2a9f3ad94a65d36927da021fa
> Author: Marc-André Lureau <marcandre.lureau@redhat.com>
> Date:   Wed Sep 12 16:53:03 2018 +0400
> 
>      qom/object: add some interface asserts
> 
> Did you suggest me something else?



> 
> 
> Thank you,
> 
> Shivaprasad
> 
>>
>> While doing this, please try to reduce the use of ->iommu_type which
>> is a design shortcut. I would like to completely remove it at some
>> point.
>>
>> Thanks,
>>
>> C.
>>
>>
>>
>>
>>
>>
>>
>>>       int ret, fd;
>>>       VFIOAddressSpace *space;
>>>
>>> @@ -611,7 +612,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>>>           goto close_fd_exit;
>>>       }
>>>
>>> -    container = g_malloc0(sizeof(*container));
>>> +    scontainer = g_malloc0(sizeof(*scontainer));
>>> +    container = &scontainer->container;
>>>       container->fd = fd;
>>>       bcontainer = &container->bcontainer;
>>>
>>> @@ -675,7 +677,7 @@ unregister_container_exit:
>>>       vfio_cpr_unregister_container(bcontainer);
>>>
>>>   free_container_exit:
>>> -    g_free(container);
>>> +    g_free(scontainer);
>>>
>>>   close_fd_exit:
>>>       close(fd);
>>> diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
>>> index 0d949bb728..78d218b7e7 100644
>>> --- a/hw/vfio/spapr.c
>>> +++ b/hw/vfio/spapr.c
>>> @@ -24,12 +24,6 @@
>>>   #include "qapi/error.h"
>>>   #include "trace.h"
>>>
>>> -typedef struct VFIOSpaprContainer {
>>> -    VFIOContainer container;
>>> -    MemoryListener prereg_listener;
>>> -    QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
>>> -} VFIOSpaprContainer;
>>> -
>>>   static bool vfio_prereg_listener_skipped_section(MemoryRegionSection *section)
>>>   {
>>>       if (memory_region_is_iommu(section->mr)) {
>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>>> index b9da6c08ef..010fa68ac6 100644
>>> --- a/include/hw/vfio/vfio-common.h
>>> +++ b/include/hw/vfio/vfio-common.h
>>> @@ -82,6 +82,12 @@ typedef struct VFIOContainer {
>>>       QLIST_HEAD(, VFIOGroup) group_list;
>>>   } VFIOContainer;
>>>
>>> +typedef struct VFIOSpaprContainer {
>>> +    VFIOContainer container;
>>> +    MemoryListener prereg_listener;
>>> +    QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
>>> +} VFIOSpaprContainer;
>>> +
>>>   typedef struct VFIOHostDMAWindow {
>>>       hwaddr min_iova;
>>>       hwaddr max_iova;
>>>
>>>
>>

Re: [PATCH] vfio: container: Fix missing allocation of VFIOSpaprContainer

[Cédric Le Goater]

Shivaprasad,

On 5/9/24 9:14 PM, Shivaprasad G Bhat wrote:
> The commit 6ad359ec29 "(vfio/spapr: Move prereg_listener into
> spapr container)" began to use the newly introduced VFIOSpaprContainer
> structure.
> 
> After several refactors, today the container_of(container,
> VFIOSpaprContainer, ABC) is used when VFIOSpaprContainer is actually
> not allocated. On PPC64 systems, this dereference is leading to corruption
> showing up as glibc malloc assertion during guest start when using vfio.
> 
> Patch adds the missing allocation while also making the structure movement
> to vfio common header file.
> 
> Fixes: 6ad359ec29 "(vfio/spapr: Move prereg_listener into spapr container)"
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>

Could you please give vfio-9.1 a try ? Thanks,

C.

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

> ---
>   hw/vfio/container.c           |    6 ++++--
>   hw/vfio/spapr.c               |    6 ------
>   include/hw/vfio/vfio-common.h |    6 ++++++
>   3 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 77bdec276e..ecaf5786d9 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -539,6 +539,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>   {
>       VFIOContainer *container;
>       VFIOContainerBase *bcontainer;
> +    VFIOSpaprContainer *scontainer;
>       int ret, fd;
>       VFIOAddressSpace *space;
> 
> @@ -611,7 +612,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>           goto close_fd_exit;
>       }
> 
> -    container = g_malloc0(sizeof(*container));
> +    scontainer = g_malloc0(sizeof(*scontainer));
> +    container = &scontainer->container;
>       container->fd = fd;
>       bcontainer = &container->bcontainer;
> 
> @@ -675,7 +677,7 @@ unregister_container_exit:
>       vfio_cpr_unregister_container(bcontainer);
> 
>   free_container_exit:
> -    g_free(container);
> +    g_free(scontainer);
> 
>   close_fd_exit:
>       close(fd);
> diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
> index 0d949bb728..78d218b7e7 100644
> --- a/hw/vfio/spapr.c
> +++ b/hw/vfio/spapr.c
> @@ -24,12 +24,6 @@
>   #include "qapi/error.h"
>   #include "trace.h"
> 
> -typedef struct VFIOSpaprContainer {
> -    VFIOContainer container;
> -    MemoryListener prereg_listener;
> -    QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
> -} VFIOSpaprContainer;
> -
>   static bool vfio_prereg_listener_skipped_section(MemoryRegionSection *section)
>   {
>       if (memory_region_is_iommu(section->mr)) {
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index b9da6c08ef..010fa68ac6 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -82,6 +82,12 @@ typedef struct VFIOContainer {
>       QLIST_HEAD(, VFIOGroup) group_list;
>   } VFIOContainer;
> 
> +typedef struct VFIOSpaprContainer {
> +    VFIOContainer container;
> +    MemoryListener prereg_listener;
> +    QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
> +} VFIOSpaprContainer;
> +
>   typedef struct VFIOHostDMAWindow {
>       hwaddr min_iova;
>       hwaddr max_iova;
> 
> 
> 

Re: [PATCH] vfio: container: Fix missing allocation of VFIOSpaprContainer

[Shivaprasad G Bhat]

Hi Cédric,

On 6/20/24 6:37 PM, Cédric Le Goater wrote:
> Shivaprasad,
>
> On 5/9/24 9:14 PM, Shivaprasad G Bhat wrote:
>> The commit 6ad359ec29 "(vfio/spapr: Move prereg_listener into
>> spapr container)" began to use the newly introduced VFIOSpaprContainer
>> structure.
>>
>> After several refactors, today the container_of(container,
>> VFIOSpaprContainer, ABC) is used when VFIOSpaprContainer is actually
>> not allocated. On PPC64 systems, this dereference is leading to 
>> corruption
>> showing up as glibc malloc assertion during guest start when using vfio.
>>
>> Patch adds the missing allocation while also making the structure 
>> movement
>> to vfio common header file.
>>
>> Fixes: 6ad359ec29 "(vfio/spapr: Move prereg_listener into spapr 
>> container)"
>> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
>
> Could you please give vfio-9.1 a try ? Thanks,
>
Yes. This is working fine for ppc64.


Thank you!


Regards,

Shivaprasad


> C.
>
> https://github.com/legoater/qemu/commits/vfio-9.1
>
<snip>

Re: [PATCH] vfio: container: Fix missing allocation of VFIOSpaprContainer

[Cédric Le Goater]

On 6/21/24 10:17 AM, Shivaprasad G Bhat wrote:
> Hi Cédric,
> 
> On 6/20/24 6:37 PM, Cédric Le Goater wrote:
>> Shivaprasad,
>>
>> On 5/9/24 9:14 PM, Shivaprasad G Bhat wrote:
>>> The commit 6ad359ec29 "(vfio/spapr: Move prereg_listener into
>>> spapr container)" began to use the newly introduced VFIOSpaprContainer
>>> structure.
>>>
>>> After several refactors, today the container_of(container,
>>> VFIOSpaprContainer, ABC) is used when VFIOSpaprContainer is actually
>>> not allocated. On PPC64 systems, this dereference is leading to corruption
>>> showing up as glibc malloc assertion during guest start when using vfio.
>>>
>>> Patch adds the missing allocation while also making the structure movement
>>> to vfio common header file.
>>>
>>> Fixes: 6ad359ec29 "(vfio/spapr: Move prereg_listener into spapr container)"
>>> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
>>
>> Could you please give vfio-9.1 a try ? Thanks,
>>
> Yes. This is working fine for ppc64.

Could you please describe the host/guest OS, hypervisor, processor
and adapter ?

Thanks,

C.

Re: [PATCH] vfio: container: Fix missing allocation of VFIOSpaprContainer

[Shivaprasad G Bhat]

On 6/21/24 2:19 PM, Cédric Le Goater wrote:
>
> Could you please describe the host/guest OS, hypervisor, processor
> and adapter ?
>
Here is the environment info,


pSeries:

Host : Power10 PowerVM  Lpar

Kernel: Upstream 6.10.0-rc4 + VFIO fixes posted at 
171810893836.1721.2640631616827396553.stgit@linux.ibm.com

Hypervisor : KVM on PowerVM & also tried without KVM using TCG

Guest : 6.8.5-301.fc40.ppc64le Fedora 40 distro kernel

Adapter: Non-Volatile memory controller: Samsung Electronics Co Ltd NVMe 
SSD Controller PM173X


PowerNV:

Host: Power9 Baremetal

Kernel: kernel-core-6.9.4-200 - Fedora 40 distro kernel

Hypervisor: KVM

Guest : 6.8.5-301.fc40.ppc64le - Fedora 40 distro kernel

Adapter: Non-Volatile memory controller: Samsung Electronics Co Ltd NVMe 
SSD Controller PM173X


Thanks,

Shivaprasad

> Thanks,
>
> C. 

Re: [PATCH] vfio: container: Fix missing allocation of VFIOSpaprContainer

[Cédric Le Goater]

On 6/21/24 4:47 PM, Shivaprasad G Bhat wrote:
> 
> On 6/21/24 2:19 PM, Cédric Le Goater wrote:
>>
>> Could you please describe the host/guest OS, hypervisor, processor
>> and adapter ?
>>
> Here is the environment info,
> 
> 
> pSeries:
> 
> Host : Power10 PowerVM  Lpar
> 
> Kernel: Upstream 6.10.0-rc4 + VFIO fixes posted at 171810893836.1721.2640631616827396553.stgit@linux.ibm.com

Great. You should report there too and probably send a PR to Alex to
contribute your changes to the vfio tests.

> Hypervisor : KVM on PowerVM & 

OK. So, this is using the newer nested v2 implementation. With the
legacy XICS IRQ controller or XIVE ? in-kernel device or emulated ?

> also tried without KVM using TCG

Ah nice. Good to know that real HW passthrough works in TCG also.

> Guest : 6.8.5-301.fc40.ppc64le Fedora 40 distro kernel
> 
> Adapter: Non-Volatile memory controller: Samsung Electronics Co Ltd NVMe SSD Controller PM173X
>
> PowerNV:
> 
> Host: Power9 Baremetal
> 
> Kernel: kernel-core-6.9.4-200 - Fedora 40 distro kernel

Is there a requirement on the kernel version ? Would an older debian
6.1 work for instance ?

> Hypervisor: KVM
> 
> Guest : 6.8.5-301.fc40.ppc64le - Fedora 40 distro kernel
> 
> Adapter: Non-Volatile memory controller: Samsung Electronics Co Ltd NVMe SSD Controller PM173X

Nice. XIVE I suppose. What about TCG ?

Thanks a lot,

C.

Re: [PATCH] vfio: container: Fix missing allocation of VFIOSpaprContainer

[Shivaprasad G Bhat]

On 6/21/24 8:40 PM, Cédric Le Goater wrote:
> On 6/21/24 4:47 PM, Shivaprasad G Bhat wrote:
>>
>> On 6/21/24 2:19 PM, Cédric Le Goater wrote:
>>>
>>> Could you please describe the host/guest OS, hypervisor, processor
>>> and adapter ?
>>>
>> Here is the environment info,
>>
>>
>> pSeries:
>>
>> Host : Power10 PowerVM  Lpar
>>
>> Kernel: Upstream 6.10.0-rc4 + VFIO fixes posted at 
>> 171810893836.1721.2640631616827396553.stgit@linux.ibm.com
>
> Great. You should report there too and probably send a PR to Alex to
> contribute your changes to the vfio tests.

Could you clarify which tree you are referring to ? I see his tree

https://github.com/awilliam/tests is bit old and updated recently, however

I have been using those tests for my unit testing.


>
>> Hypervisor : KVM on PowerVM & 
>
> OK. So, this is using the newer nested v2 implementation.

Yes. However, this was working for userspace before too with limitations

like DMA windows were being borrowed, and no customization

of window size etc.


> With the
> legacy XICS IRQ controller or XIVE ? in-kernel device or emulated ?

Emulated XIVE.

>
>> also tried without KVM using TCG
>
> Ah nice. Good to know that real HW passthrough works in TCG also.
>
>> Guest : 6.8.5-301.fc40.ppc64le Fedora 40 distro kernel
>>
>> Adapter: Non-Volatile memory controller: Samsung Electronics Co Ltd 
>> NVMe SSD Controller PM173X
>>
>> PowerNV:
>>
>> Host: Power9 Baremetal
>>
>> Kernel: kernel-core-6.9.4-200 - Fedora 40 distro kernel
>
> Is there a requirement on the kernel version ? Would an older debian
> 6.1 work for instance ?

This went through cycles of breakage and fixes. It worked on 5.18(not sure

about older ones before that), and broke afterwards. Recently fixed

and working from 6.4, broken on 6.7. Fixed and working in 6.8

onwards now.


>
>> Hypervisor: KVM
>>
>> Guest : 6.8.5-301.fc40.ppc64le - Fedora 40 distro kernel
>>
>> Adapter: Non-Volatile memory controller: Samsung Electronics Co Ltd 
>> NVMe SSD Controller PM173X
>
> Nice. XIVE I suppose. 

Yes.


> What about TCG ?

Yes, TCG too works, missed to mention.


Thanks,

Shivaprasad

> Thanks a lot,
>
> C.
>

Re: [PATCH] vfio: container: Fix missing allocation of VFIOSpaprContainer

[Cédric Le Goater]

...

> Could you clarify which tree you are referring to ? I see his tree
> 
> https://github.com/awilliam/tests is bit old and updated recently, however
> 
> I have been using those tests for my unit testing.

Yes, this tree.

...

> This went through cycles of breakage and fixes. It worked on 5.18(not sure
> 
> about older ones before that), and broke afterwards. Recently fixed
> 
> and working from 6.4, broken on 6.7. Fixed and working in 6.8
> 
> onwards now.

Good. It should be fixed in the next debian.


> Yes, TCG too works, missed to mention.

and a TCG guest under an intel host ? This used to work.

Thanks,

C.

Re: [PATCH] vfio: container: Fix missing allocation of VFIOSpaprContainer

[Shivaprasad G Bhat]

On 6/28/24 4:07 PM, Cédric Le Goater wrote:
> ...
>
>> Could you clarify which tree you are referring to ? I see his tree
>>
>> https://github.com/awilliam/tests is bit old and updated recently, 
>> however
>>
>> I have been using those tests for my unit testing.
>
> Yes, this tree.
>
Thanks!
> ...
>
>> This went through cycles of breakage and fixes. It worked on 5.18(not 
>> sure
>>
>> about older ones before that), and broke afterwards. Recently fixed
>>
>> and working from 6.4, broken on 6.7. Fixed and working in 6.8
>>
>> onwards now.
>
> Good. It should be fixed in the next debian.
>
>
>> Yes, TCG too works, missed to mention.
>
> and a TCG guest under an intel host ? This used to work.
>

Yes. pSeries TCG guest on intel host works too.


Regards,

Shivaprasad