Re: [Qemu-devel] [PATCH v5 08/17] vfio: Pass an Error object to vfio_connect_container

[David Gibson]

On Mon, Oct 10, 2016 at 03:21:24PM +0200, Auger Eric wrote:
> Hi Markus,
> On 10/10/2016 14:36, Markus Armbruster wrote:
> > Auger Eric <address@hidden> writes:
> > 
> >> Hi,
> >>
> >> On 10/10/2016 07:34, David Gibson wrote:
> >>> On Fri, Oct 07, 2016 at 09:36:09AM +0200, Auger Eric wrote:
> >>>> Hi,
> >>>>
> >>>> On 07/10/2016 09:01, Markus Armbruster wrote:
> >>>>> Eric Auger <address@hidden> writes:
> >>>>>
> >>>>>> The error is currently simply reported in vfio_get_group. Don't
> >>>>>> bother too much with the prefix which will be handled at upper level,
> >>>>>> later on.
> >>>>>>
> >>>>>> Also return an error value in case container->error is not 0 and
> >>>>>> the container is teared down.
> >>>>>
> >>>>> "torn down", I think.
> >>>>
> >>>> Sure. I had a wrong feeling when writing this ...
> >>>>>
> >>>>> Is this a bug fix?  See also below.
> >>>>>
> >>>>>> On vfio_spapr_remove_window failure, we also report an error whereas
> >>>>>> it was silent before.
> >>>>>>
> >>>>>> Signed-off-by: Eric Auger <address@hidden>
> >>>>>> Reviewed-by: Markus Armbruster <address@hidden>
> >>>>>>
> >>>>>> ---
> >>>>>>
> >>>>>> v4 -> v5:
> >>>>>> - set ret to container->error
> >>>>>> - mention error report on vfio_spapr_remove_window failure in the 
> >>>>>> commit
> >>>>>>   message
> >>>>>> ---
> >>>>>>  hw/vfio/common.c | 40 +++++++++++++++++++++++++---------------
> >>>>>>  1 file changed, 25 insertions(+), 15 deletions(-)
> >>>>>>
> >>>>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> >>>>>> index 29188a1..85a7759 100644
> >>>>>> --- a/hw/vfio/common.c
> >>>>>> +++ b/hw/vfio/common.c
> > [...]
> >>>>>> @@ -1008,7 +1010,9 @@ static int vfio_connect_container(VFIOGroup 
> >>>>>> *group, AddressSpace *as)
> >            container = g_malloc0(sizeof(*container));
> >            container->space = space;
> >            container->fd = fd;
> >            if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_TYPE1_IOMMU) ||
> >                ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_TYPE1v2_IOMMU)) {
> > [...]
> >            } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU) 
> > ||
> >                       ioctl(fd, VFIO_CHECK_EXTENSION, 
> > VFIO_SPAPR_TCE_v2_IOMMU)) {
> >                struct vfio_iommu_spapr_tce_info info;
> >                bool v2 = !!ioctl(fd, VFIO_CHECK_EXTENSION, 
> > VFIO_SPAPR_TCE_v2_IOMMU);
> > 
> >                ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd);
> >                if (ret) {
> >                    error_setg_errno(errp, errno, "failed to set group 
> > container");
> >                    ret = -errno;
> >                    goto free_container_exit;
> >                }
> >                container->iommu_type =
> >                    v2 ? VFIO_SPAPR_TCE_v2_IOMMU : VFIO_SPAPR_TCE_IOMMU;
> >                ret = ioctl(fd, VFIO_SET_IOMMU, container->iommu_type);
> >                if (ret) {
> >                    error_setg_errno(errp, errno, "failed to set iommu for 
> > container");
> >                    ret = -errno;
> >                    goto free_container_exit;
> >                }
> > 
> >                /*
> >                 * The host kernel code implementing VFIO_IOMMU_DISABLE is 
> > called
> >                 * when container fd is closed so we do not call it 
> > explicitly
> >                 * in this file.
> >                 */
> >                if (!v2) {
> >                    ret = ioctl(fd, VFIO_IOMMU_ENABLE);
> >                    if (ret) {
> >                        error_setg_errno(errp, errno, "failed to enable 
> > container");
> >                        ret = -errno;
> >                        goto free_container_exit;
> >                    }
> >                } else {
> >                    container->prereg_listener = vfio_prereg_listener;
> > 
> >                    memory_listener_register(&container->prereg_listener,
> >>>>>>                                       &address_space_memory);
> >>>>>>              if (container->error) {
> >>>>>
> >>>>> I tried to see where non-zero container->error comes from, but failed.
> >>>>> Can you help?
> >>>>
> >>>> Added Alexey in CC
> >>>>
> >>>> It is set in vfio_prereg_listener_region_add (spapr.c)
> >>>> There is a comment there saying:
> >>>> /*
> >>>>  * On the initfn path, store the first error in the container so we
> >>>>  * can gracefully fail.  Runtime, there's not much we can do other
> >>>>  * than throw a hardware error.
> >>>>  */
> >>>> 1) by the way I should also s/initfn/realize now.
> >>>> 2) by gracefully fail I understand the error should be properly
> >>>> cascaded. Also when looking at the other vfio_memory_listener
> >>>> registration below, ret is set to container->error.
> >>>> 3) I could use error_setg_errno ...
> >>>>
> >>>> David, Alexey, could you confirm we should set the returned value to the
> >>>> container->error below?
> >>>
> >>> I think the right approach is to change container->error from an int
> >>> to an Error *.  As now, we stash the first error from the listener in
> >>> there.
> >>>
> >>> realize() would check for a non-NULL error in the container after
> >>> registering the listener, and if present, propagate it up to the
> >>> caller.
> >>>
> >>>>
> >>>> Thanks
> >>>>
> >>>> Eric
> >>>>
> >>>>
> >>>>>
> >>>>>>                  
> >>>>>> memory_listener_unregister(&container->prereg_listener);
> >>>>>> -                error_report("vfio: RAM memory listener 
> >>>>>> initialization failed for container");
> >>>>>> +                ret = container->error;
> >> Thank you for your answers. OK to change container->error from an int
> >> to an Error *.
> >>
> >> So I understand the fix just above is correct, ie. consider a non-NULL
> >> container->error as an error that should be cascaded to the caller.
> >> Currently I understand it is not since ret was left to 0.
> > 
> > If whatever sets container->error now can provide more useful error
> > information by setting an Error, then replacing VFIOContainer member int
> > error by Error *err makes sense.  Else, I recommend to keep it simple
> > and stick to errno codes.
> > 
> > My original question was about something else: I can't see what could
> > have set container->error here.  Have a look at the additional context I
> > quoted above.  Initially, container->error is zero.  The ioctl()'s can't
> > change it.  That leaves memory_listener_register().  How can
> > container->error be set?
> My understanding is on
> memory_listener_register(&container->prereg_listener,
>                                      &address_space_memory);
> 
> you get the vfio_prereg_listener_region_add called which is likely to
> set container->error with the returned value of
> VFIO_IOMMU_SPAPR_REGISTER_MEMORY ioctl.
> 
> Do I miss something?

That's correct.  memory_listener_register() will call the registered
listener on all existing memory regions.  The listener we've
registered can set container->error in some circumstances, so that's
how it can be set here.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature