Re: [PATCH] vfio/common: Do not g_free in vfio_get_iommu_info

[Nicolin Chen]

On Wed, Sep 14, 2022 at 02:03:25PM -0600, Alex Williamson wrote:

> > > > +        container->pgsizes = 4096;
> > >
> > > This might be a separate question/issue: I wonder if we should use
> > > "sysconf(_SC_PAGE_SIZE)" here instead of 4096.
> > >
> > > With a kernel using a larger page size, e.g. CONFIG_ARM64_64K_PAGES,
> > > the IO page size is likely to be 64K too. If the ioctl fails, this
> > > default 4K setup won't work.
> >
> > Perhaps, but IIRC this solution came about because we originally forgot
> > to expose the IOMMU_INFO flag to indicate the pgsize field was valid.
> > At the time we only supported 4K systems, so it made sense to provide
> > this default, though it is indeed dated.
> >
> > TBH, I don't really see why we should try to continue if the ioctl
> > itself fails, so maybe this should be:

OK. Makes sense to me.

> > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > index ace9562a9ba1..ad188b7649e6 100644
> > --- a/hw/vfio/common.c
> > +++ b/hw/vfio/common.c
> > @@ -2111,29 +2111,31 @@ static int vfio_connect_container(VFIOGroup *group, 
> > AddressSpace *as,
> >      {
> >          struct vfio_iommu_type1_info *info;
> >
> > -        /*
> > -         * FIXME: This assumes that a Type1 IOMMU can map any 64-bit
> > -         * IOVA whatsoever.  That's not actually true, but the current
> > -         * kernel interface doesn't tell us what it can map, and the
> > -         * existing Type1 IOMMUs generally support any IOVA we're
> > -         * going to actually try in practice.
> > -         */
> >          ret = vfio_get_iommu_info(container, &info);
> > +        if (ret) {
> 
> Clearly untested,
> 
>                ret = -errno;
> 
> > +            error_setg_errno(errp, -ret, "Failed to get VFIO IOMMU info");
> > +            goto enable_discards_exit:;

There is a ":" in-between :)