| Message ID | 171528203026.8420.10620440513237875837.stgit@ltcd48-lp2.aus.stglabs.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | vfio: container: Fix missing allocation of VFIOSpaprContainer | expand |
>-----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; >
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; > >
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; >> >> >
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; >>> >>> >>
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; > > >
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>
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.
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.
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.
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. >
... > 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.
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
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;
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(-)