| Message ID | 20231026103104.1686921-28-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio: Adopt iommufd | expand |
On 10/26/23 12:30, Zhenzhong Duan wrote: > IOMMUFD supports auto allocated hwpt and manually allocated one. > Manually allocated hwpt has benefit that its life cycle is under > user's control, so it could be used as stage 2 page table by nested > feature in the future. Would an option be useful to switch from one mode to another ? > > Introduce two helpers __vfio_device_attach/detach_hwpt to facilitate > this change. I think merging this change with the previous patch makes sense. It doesn't add much to keep it as a standalone patch unless we want a feature toggle. Thanks, C. > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > hw/vfio/iommufd.c | 89 +++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 70 insertions(+), 19 deletions(-) > > diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c > index aee64d63f3..c1daaf1c39 100644 > --- a/hw/vfio/iommufd.c > +++ b/hw/vfio/iommufd.c > @@ -217,38 +217,91 @@ static VFIOIOASHwpt *vfio_container_get_hwpt(VFIOIOMMUFDContainer *container, > static void vfio_container_put_hwpt(IOMMUFDBackend *be, VFIOIOASHwpt *hwpt) > { > QLIST_REMOVE(hwpt, next); > + iommufd_backend_free_id(be->fd, hwpt->hwpt_id); > g_free(hwpt); > } > > -static int vfio_device_attach_container(VFIODevice *vbasedev, > - VFIOIOMMUFDContainer *container, > - Error **errp) > +static int __vfio_device_attach_hwpt(VFIODevice *vbasedev, uint32_t hwpt_id, > + Error **errp) > { > - int ret, iommufd = vbasedev->iommufd->fd; > - VFIOIOASHwpt *hwpt; > struct vfio_device_attach_iommufd_pt attach_data = { > .argsz = sizeof(attach_data), > .flags = 0, > - .pt_id = container->ioas_id, > + .pt_id = hwpt_id, > }; > + int ret; > > - /* Attach device to an ioas within iommufd */ > ret = ioctl(vbasedev->fd, VFIO_DEVICE_ATTACH_IOMMUFD_PT, &attach_data); > if (ret) { > error_setg_errno(errp, errno, > - "[iommufd=%d] error attach %s (%d) to ioasid=%d", > - container->be->fd, vbasedev->name, vbasedev->fd, > - attach_data.pt_id); > + "[iommufd=%d] error attach %s (%d) to hwpt_id=%d", > + vbasedev->iommufd->fd, vbasedev->name, vbasedev->fd, > + hwpt_id); > + } > + return ret; > +} > + > +static int __vfio_device_detach_hwpt(VFIODevice *vbasedev, Error **errp) > +{ > + struct vfio_device_detach_iommufd_pt detach_data = { > + .argsz = sizeof(detach_data), > + .flags = 0, > + }; > + int ret; > + > + ret = ioctl(vbasedev->fd, VFIO_DEVICE_DETACH_IOMMUFD_PT, &detach_data); > + if (ret) { > + error_setg_errno(errp, errno, "detach %s from ioas failed", > + vbasedev->name); > + } > + return ret; > +} > + > +static int vfio_device_attach_container(VFIODevice *vbasedev, > + VFIOIOMMUFDContainer *container, > + Error **errp) > +{ > + int ret, iommufd = vbasedev->iommufd->fd; > + VFIOIOASHwpt *hwpt; > + uint32_t hwpt_id; > + Error *err = NULL; > + > + /* try to attach to an existing hwpt in this container */ > + QLIST_FOREACH(hwpt, &container->hwpt_list, next) { > + ret = __vfio_device_attach_hwpt(vbasedev, hwpt->hwpt_id, &err); > + if (ret) { > + const char *msg = error_get_pretty(err); > + > + trace_vfio_iommufd_fail_attach_existing_hwpt(msg); > + error_free(err); > + err = NULL; > + } else { > + goto found_hwpt; > + } > + } > + > + ret = iommufd_backend_alloc_hwpt(iommufd, vbasedev->devid, > + container->ioas_id, &hwpt_id); > + > + if (ret) { > + error_setg_errno(errp, errno, "error alloc shadow hwpt"); > return ret; > + } > > + /* Attach device to an hwpt within iommufd */ > + ret = __vfio_device_attach_hwpt(vbasedev, hwpt_id, errp); > + if (ret) { > + iommufd_backend_free_id(iommufd, hwpt_id); > + return ret; > } > - hwpt = vfio_container_get_hwpt(container, attach_data.pt_id); > > + hwpt = vfio_container_get_hwpt(container, hwpt_id); > +found_hwpt: > QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, next); > vbasedev->hwpt = hwpt; > > trace_vfio_iommufd_attach_device(iommufd, vbasedev->name, vbasedev->fd, > - container->ioas_id, attach_data.pt_id); > + container->ioas_id, hwpt->hwpt_id); > return ret; > } > > @@ -256,14 +309,12 @@ static void vfio_device_detach_container(VFIODevice *vbasedev, > VFIOIOMMUFDContainer *container) > { > VFIOIOASHwpt *hwpt = vbasedev->hwpt; > - struct vfio_device_attach_iommufd_pt detach_data = { > - .argsz = sizeof(detach_data), > - .flags = 0, > - }; > + Error *err = NULL; > + int ret; > > - if (ioctl(vbasedev->fd, VFIO_DEVICE_DETACH_IOMMUFD_PT, &detach_data)) { > - error_report("detach %s from ioas id=%d failed: %m", vbasedev->name, > - container->ioas_id); > + ret = __vfio_device_detach_hwpt(vbasedev, &err); > + if (ret) { > + error_report_err(err); > } > > QLIST_REMOVE(vbasedev, next);
>-----Original Message----- >From: Cédric Le Goater <clg@redhat.com> >Sent: Monday, October 30, 2023 9:52 PM >Subject: Re: [PATCH v3 27/37] vfio/iommufd: Switch to manual hwpt allocation > >On 10/26/23 12:30, Zhenzhong Duan wrote: >> IOMMUFD supports auto allocated hwpt and manually allocated one. >> Manually allocated hwpt has benefit that its life cycle is under >> user's control, so it could be used as stage 2 page table by nested >> feature in the future. > >Would an option be useful to switch from one mode to another ? Looks unnecessary for me as we prefer manual allocation. The purpose of splitting is to make review easier. > >> >> Introduce two helpers __vfio_device_attach/detach_hwpt to facilitate >> this change. > >I think merging this change with the previous patch makes sense. >It doesn't add much to keep it as a standalone patch unless we >want a feature toggle. OK, will merge them in v4. Thanks Zhenzhong
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c index aee64d63f3..c1daaf1c39 100644 --- a/hw/vfio/iommufd.c +++ b/hw/vfio/iommufd.c @@ -217,38 +217,91 @@ static VFIOIOASHwpt *vfio_container_get_hwpt(VFIOIOMMUFDContainer *container, static void vfio_container_put_hwpt(IOMMUFDBackend *be, VFIOIOASHwpt *hwpt) { QLIST_REMOVE(hwpt, next); + iommufd_backend_free_id(be->fd, hwpt->hwpt_id); g_free(hwpt); } -static int vfio_device_attach_container(VFIODevice *vbasedev, - VFIOIOMMUFDContainer *container, - Error **errp) +static int __vfio_device_attach_hwpt(VFIODevice *vbasedev, uint32_t hwpt_id, + Error **errp) { - int ret, iommufd = vbasedev->iommufd->fd; - VFIOIOASHwpt *hwpt; struct vfio_device_attach_iommufd_pt attach_data = { .argsz = sizeof(attach_data), .flags = 0, - .pt_id = container->ioas_id, + .pt_id = hwpt_id, }; + int ret; - /* Attach device to an ioas within iommufd */ ret = ioctl(vbasedev->fd, VFIO_DEVICE_ATTACH_IOMMUFD_PT, &attach_data); if (ret) { error_setg_errno(errp, errno, - "[iommufd=%d] error attach %s (%d) to ioasid=%d", - container->be->fd, vbasedev->name, vbasedev->fd, - attach_data.pt_id); + "[iommufd=%d] error attach %s (%d) to hwpt_id=%d", + vbasedev->iommufd->fd, vbasedev->name, vbasedev->fd, + hwpt_id); + } + return ret; +} + +static int __vfio_device_detach_hwpt(VFIODevice *vbasedev, Error **errp) +{ + struct vfio_device_detach_iommufd_pt detach_data = { + .argsz = sizeof(detach_data), + .flags = 0, + }; + int ret; + + ret = ioctl(vbasedev->fd, VFIO_DEVICE_DETACH_IOMMUFD_PT, &detach_data); + if (ret) { + error_setg_errno(errp, errno, "detach %s from ioas failed", + vbasedev->name); + } + return ret; +} + +static int vfio_device_attach_container(VFIODevice *vbasedev, + VFIOIOMMUFDContainer *container, + Error **errp) +{ + int ret, iommufd = vbasedev->iommufd->fd; + VFIOIOASHwpt *hwpt; + uint32_t hwpt_id; + Error *err = NULL; + + /* try to attach to an existing hwpt in this container */ + QLIST_FOREACH(hwpt, &container->hwpt_list, next) { + ret = __vfio_device_attach_hwpt(vbasedev, hwpt->hwpt_id, &err); + if (ret) { + const char *msg = error_get_pretty(err); + + trace_vfio_iommufd_fail_attach_existing_hwpt(msg); + error_free(err); + err = NULL; + } else { + goto found_hwpt; + } + } + + ret = iommufd_backend_alloc_hwpt(iommufd, vbasedev->devid, + container->ioas_id, &hwpt_id); + + if (ret) { + error_setg_errno(errp, errno, "error alloc shadow hwpt"); return ret; + } + /* Attach device to an hwpt within iommufd */ + ret = __vfio_device_attach_hwpt(vbasedev, hwpt_id, errp); + if (ret) { + iommufd_backend_free_id(iommufd, hwpt_id); + return ret; } - hwpt = vfio_container_get_hwpt(container, attach_data.pt_id); + hwpt = vfio_container_get_hwpt(container, hwpt_id); +found_hwpt: QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, next); vbasedev->hwpt = hwpt; trace_vfio_iommufd_attach_device(iommufd, vbasedev->name, vbasedev->fd, - container->ioas_id, attach_data.pt_id); + container->ioas_id, hwpt->hwpt_id); return ret; } @@ -256,14 +309,12 @@ static void vfio_device_detach_container(VFIODevice *vbasedev, VFIOIOMMUFDContainer *container) { VFIOIOASHwpt *hwpt = vbasedev->hwpt; - struct vfio_device_attach_iommufd_pt detach_data = { - .argsz = sizeof(detach_data), - .flags = 0, - }; + Error *err = NULL; + int ret; - if (ioctl(vbasedev->fd, VFIO_DEVICE_DETACH_IOMMUFD_PT, &detach_data)) { - error_report("detach %s from ioas id=%d failed: %m", vbasedev->name, - container->ioas_id); + ret = __vfio_device_detach_hwpt(vbasedev, &err); + if (ret) { + error_report_err(err); } QLIST_REMOVE(vbasedev, next);
IOMMUFD supports auto allocated hwpt and manually allocated one. Manually allocated hwpt has benefit that its life cycle is under user's control, so it could be used as stage 2 page table by nested feature in the future. Introduce two helpers __vfio_device_attach/detach_hwpt to facilitate this change. Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- hw/vfio/iommufd.c | 89 +++++++++++++++++++++++++++++++++++++---------- 1 file changed, 70 insertions(+), 19 deletions(-)