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

[Markus Armbruster]

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?