Hi Cédric,
Thanks for looking into this patch. Please find my response inline:
On 2025/04/04 01:29 PM, Cédric Le Goater wrote:
> On 4/4/25 11:17, Amit Machhiwal wrote:
> > An L2 KVM guest fails to boot inside a pSeries LPAR when booted with a
> > memory more than 128 GB and PCI device passthrough. The L2 guest also
> > crashes when it is booted with a memory greater than 128 GB and a PCI
> > device is hotplugged later.
> >
> > The issue arises from a conditional check for `levels > 1` in
> > `spapr_tce_create_table()` within L1 KVM. This check is meant to prevent
> > multi-level TCEs, which are not supported by the PowerVM hypervisor. As
> > a result, when QEMU makes a `VFIO_IOMMU_SPAPR_TCE_CREATE` ioctl call
> > with `levels > 1`, it triggers the conditional check and returns
> > `EINVAL`, causing the guest to crash with the following errors:
> >
> > 2025-03-04T06:36:36.133117Z qemu-system-ppc64: Failed to create a window,
> > ret = -1 (Invalid argument)
> > 2025-03-04T06:36:36.133176Z qemu-system-ppc64: Failed to create SPAPR
> > window: Invalid argument
> > qemu: hardware error: vfio: DMA mapping failed, unable to continue
> >
> > Fix this by checking the supported DDW "levels" returned by the
> > VFIO_IOMMU_SPAPR_TCE_GET_INFO ioctl before attempting the TCE create
> > ioctl in KVM.
> >
> > The patch has been tested on KVM guests with memory configurations of up
> > to 390GB, and 450GB on PowerVM and bare-metal environments respectively.
> > > Signed-off-by: Amit Machhiwal <amach...@linux.ibm.com>
> > ---
> > hw/vfio/spapr.c | 35 ++++++++++++++++++++++++++---------
> > 1 file changed, 26 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
> > index 1a5d1611f2cd..07498218fea9 100644
> > --- a/hw/vfio/spapr.c
> > +++ b/hw/vfio/spapr.c
> > @@ -26,6 +26,7 @@ typedef struct VFIOSpaprContainer {
> > VFIOContainer container;
> > MemoryListener prereg_listener;
> > QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
> > + unsigned int levels;
> > } VFIOSpaprContainer;
> > OBJECT_DECLARE_SIMPLE_TYPE(VFIOSpaprContainer, VFIO_IOMMU_SPAPR);
> > @@ -236,9 +237,11 @@ static int vfio_spapr_create_window(VFIOContainer
> > *container,
> > {
> > int ret = 0;
> > VFIOContainerBase *bcontainer = &container->bcontainer;
> > + VFIOSpaprContainer *scontainer = container_of(container,
> > VFIOSpaprContainer,
> > + container);
> > IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
> > uint64_t pagesize = memory_region_iommu_get_min_page_size(iommu_mr),
> > pgmask;
> > - unsigned entries, bits_total, bits_per_level, max_levels;
> > + unsigned entries, bits_total, bits_per_level, max_levels, ddw_levels;
> > struct vfio_iommu_spapr_tce_create create = { .argsz = sizeof(create)
> > };
> > long rampagesize = qemu_minrampagesize();
> > @@ -291,16 +294,28 @@ static int vfio_spapr_create_window(VFIOContainer
> > *container,
> > */
> > bits_per_level = ctz64(qemu_real_host_page_size()) + 8;
> > create.levels = bits_total / bits_per_level;
> > - if (bits_total % bits_per_level) {
> > - ++create.levels;
> > - }
> > - max_levels = (64 - create.page_shift) /
> > ctz64(qemu_real_host_page_size());
> > - for ( ; create.levels <= max_levels; ++create.levels) {
> > - ret = ioctl(container->fd, VFIO_IOMMU_SPAPR_TCE_CREATE, &create);
> > - if (!ret) {
> > - break;
> > +
> > + ddw_levels = scontainer->levels;
> > + if (ddw_levels > 1) {
> > + if (bits_total % bits_per_level) {
> > + ++create.levels;
> > }
> > + max_levels = (64 - create.page_shift) /
> > ctz64(qemu_real_host_page_size());
> > + for ( ; create.levels <= max_levels; ++create.levels) {
> > + ret = ioctl(container->fd, VFIO_IOMMU_SPAPR_TCE_CREATE,
> > &create);
> > + if (!ret) {
> > + break;
> > + }
> > + }
> > + } else { /* ddw_levels == 1 */
> > + if (create.levels > ddw_levels) {
> > + error_report("Host doesn't support multi-level TCE tables. "
> > + "Use larger IO page size. Supported mask is
> > 0x%lx",
> > + bcontainer->pgsizes);
>
> While at it, please modify vfio_spapr_create_window(), add an 'Error **'
> parameter to report errors to the caller with error_setg(errp ...)
Sure, I'll include the suggested changes and send a v2 soon.
Thanks,
Amit
>
> Thanks,
>
> C.
>
>
>
>
> > + }
> > + ret = ioctl(container->fd, VFIO_IOMMU_SPAPR_TCE_CREATE, &create);
> > }
> > +
> > if (ret) {
> > error_report("Failed to create a window, ret = %d (%m)", ret);
> > return -errno;
> > @@ -502,6 +517,8 @@ static bool
> > vfio_spapr_container_setup(VFIOContainerBase *bcontainer,
> > goto listener_unregister_exit;
> > }
> > + scontainer->levels = info.ddw.levels;
> > +
> > if (v2) {
> > bcontainer->pgsizes = info.ddw.pgsizes;
> > /*
> >
> > base-commit: 0adf626718bc0ca9c46550249a76047f8e45da15
>
