Re: [PATCH V2 05/45] vfio/container: ram discard disable helper

["Cédric Le Goater" <clg@redhat.com>]

On 2/14/25 15:13, Steve Sistare wrote:
> Define a helper to set ram discard disable, generate error messages,
> and cleanup on failure.  The second vfio_ram_block_discard_disable
> call site now performs VFIO_GROUP_UNSET_CONTAINER immediately on failure,
> instead of relying on the close of the container fd to do so in the kernel,
> but this is equivalent.

should be ok.



Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>   hw/vfio/container.c | 48 +++++++++++++++++++++++++++---------------------
>   1 file changed, 27 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 7c57bdd2..0f17d53 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -535,16 +535,10 @@ static bool vfio_legacy_setup(VFIOContainerBase *bcontainer, Error **errp)
>       return true;
>   }
>   
> -static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
> -                                   Error **errp)
> +static bool vfio_attach_discard_disable(VFIOContainer *container,
> +                                        VFIOGroup *group, Error **errp)
>   {
> -    VFIOContainer *container;
> -    VFIOContainerBase *bcontainer;
> -    int ret, fd;
> -    VFIOAddressSpace *space;
> -    VFIOIOMMUClass *vioc;
> -
> -    space = vfio_get_address_space(as);
> +    int ret;
>   
>       /*
>        * VFIO is currently incompatible with discarding of RAM insofar as the
> @@ -577,18 +571,32 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>        * details once we know which type of IOMMU we are using.
>        */
>   
> +    ret = vfio_ram_block_discard_disable(container, true);
> +    if (ret) {
> +        error_setg_errno(errp, -ret, "Cannot set discarding of RAM broken");
> +        if (ioctl(group->fd, VFIO_GROUP_UNSET_CONTAINER, &container->fd)) {
> +            error_report("vfio: error disconnecting group %d from"
> +                         " container", group->groupid);
> +        }
> +    }
> +    return !ret;
> +}
> +
> +static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
> +                                   Error **errp)
> +{
> +    VFIOContainer *container;
> +    VFIOContainerBase *bcontainer;
> +    int ret, fd;
> +    VFIOAddressSpace *space;
> +    VFIOIOMMUClass *vioc;
> +
> +    space = vfio_get_address_space(as);
> +
>       QLIST_FOREACH(bcontainer, &space->containers, next) {
>           container = container_of(bcontainer, VFIOContainer, bcontainer);
>           if (!ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &container->fd)) {
> -            ret = vfio_ram_block_discard_disable(container, true);
> -            if (ret) {
> -                error_setg_errno(errp, -ret,
> -                                 "Cannot set discarding of RAM broken");
> -                if (ioctl(group->fd, VFIO_GROUP_UNSET_CONTAINER,
> -                          &container->fd)) {
> -                    error_report("vfio: error disconnecting group %d from"
> -                                 " container", group->groupid);
> -                }
> +            if (!vfio_attach_discard_disable(container, group, errp)) {
>                   return false;
>               }
>               group->container = container;
> @@ -620,9 +628,7 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>           goto free_container_exit;
>       }
>   
> -    ret = vfio_ram_block_discard_disable(container, true);
> -    if (ret) {
> -        error_setg_errno(errp, -ret, "Cannot set discarding of RAM broken");
> +    if (!vfio_attach_discard_disable(container, group, errp)) {
>           goto unregister_container_exit;
>       }
>