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

[Auger Eric]

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
>>>> @@ -34,6 +34,7 @@
>>>>  #include "qemu/range.h"
>>>>  #include "sysemu/kvm.h"
>>>>  #include "trace.h"
>>>> +#include "qapi/error.h"
>>>>  
>>>>  struct vfio_group_head vfio_group_list =
>>>>      QLIST_HEAD_INITIALIZER(vfio_group_list);
>>>> @@ -900,7 +901,8 @@ static void vfio_put_address_space(VFIOAddressSpace 
>>>> *space)
>>>>      }
>>>>  }
>>>>  
>>>> -static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>>>> +static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>>>> +                                  Error **errp)
>>>>  {
>>>>      VFIOContainer *container;
>>>>      int ret, fd;
>>>> @@ -918,15 +920,15 @@ static int vfio_connect_container(VFIOGroup *group, 
>>>> AddressSpace *as)
>>>>  
>>>>      fd = qemu_open("/dev/vfio/vfio", O_RDWR);
>>>>      if (fd < 0) {
>>>> -        error_report("vfio: failed to open /dev/vfio/vfio: %m");
>>>> +        error_setg_errno(errp, errno, "failed to open /dev/vfio/vfio");
>>>>          ret = -errno;
>>>>          goto put_space_exit;
>>>>      }
>>>>  
>>>>      ret = ioctl(fd, VFIO_GET_API_VERSION);
>>>>      if (ret != VFIO_API_VERSION) {
>>>> -        error_report("vfio: supported vfio version: %d, "
>>>> -                     "reported version: %d", VFIO_API_VERSION, ret);
>>>> +        error_setg(errp, "supported vfio version: %d, "
>>>> +                   "reported version: %d", VFIO_API_VERSION, ret);
>>>>          ret = -EINVAL;
>>>>          goto close_fd_exit;
>>>>      }
>>>> @@ -941,7 +943,7 @@ static int vfio_connect_container(VFIOGroup *group, 
>>>> AddressSpace *as)
>>>>  
>>>>          ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd);
>>>>          if (ret) {
>>>> -            error_report("vfio: failed to set group container: %m");
>>>> +            error_setg_errno(errp, errno, "failed to set group 
>>>> container");
>>>>              ret = -errno;
>>>>              goto free_container_exit;
>>>>          }
>>>> @@ -949,7 +951,7 @@ static int vfio_connect_container(VFIOGroup *group, 
>>>> AddressSpace *as)
>>>>          container->iommu_type = v2 ? VFIO_TYPE1v2_IOMMU : 
>>>> VFIO_TYPE1_IOMMU;
>>>>          ret = ioctl(fd, VFIO_SET_IOMMU, container->iommu_type);
>>>>          if (ret) {
>>>> -            error_report("vfio: failed to set iommu for container: %m");
>>>> +            error_setg_errno(errp, errno, "failed to set iommu for 
>>>> container");
>>>>              ret = -errno;
>>>>              goto free_container_exit;
>>>>          }
>>>> @@ -976,7 +978,7 @@ static int vfio_connect_container(VFIOGroup *group, 
>>>> AddressSpace *as)
>>>>  
>>>>          ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd);
>>>>          if (ret) {
>>>> -            error_report("vfio: failed to set group container: %m");
>>>> +            error_setg_errno(errp, errno, "failed to set group 
>>>> container");
>>>>              ret = -errno;
>>>>              goto free_container_exit;
>>>>          }
>>>> @@ -984,7 +986,7 @@ static int vfio_connect_container(VFIOGroup *group, 
>>>> AddressSpace *as)
>>>>              v2 ? VFIO_SPAPR_TCE_v2_IOMMU : VFIO_SPAPR_TCE_IOMMU;
>>>>          ret = ioctl(fd, VFIO_SET_IOMMU, container->iommu_type);
>>>>          if (ret) {
>>>> -            error_report("vfio: failed to set iommu for container: %m");
>>>> +            error_setg_errno(errp, errno, "failed to set iommu for 
>>>> container");
>>>>              ret = -errno;
>>>>              goto free_container_exit;
>>>>          }
>>>> @@ -997,7 +999,7 @@ static int vfio_connect_container(VFIOGroup *group, 
>>>> AddressSpace *as)
>>>>          if (!v2) {
>>>>              ret = ioctl(fd, VFIO_IOMMU_ENABLE);
>>>>              if (ret) {
>>>> -                error_report("vfio: failed to enable container: %m");
>>>> +                error_setg_errno(errp, errno, "failed to enable 
>>>> container");
>>>>                  ret = -errno;
>>>>                  goto free_container_exit;
>>>>              }
>>>> @@ -1008,7 +1010,9 @@ static int vfio_connect_container(VFIOGroup *group, 
>>>> AddressSpace *as)
>>>>                                       &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.

Thanks

Eric
>>>> +                error_setg(errp,
>>>> +                    "RAM memory listener initialization failed for 
>>>> container");
>>>>                  goto free_container_exit;
>>>>              }
>>>>          }
>>>> @@ -1016,7 +1020,8 @@ static int vfio_connect_container(VFIOGroup *group, 
>>>> AddressSpace *as)
>>>>          info.argsz = sizeof(info);
>>>>          ret = ioctl(fd, VFIO_IOMMU_SPAPR_TCE_GET_INFO, &info);
>>>>          if (ret) {
>>>> -            error_report("vfio: VFIO_IOMMU_SPAPR_TCE_GET_INFO failed: 
>>>> %m");
>>>> +            error_setg_errno(errp, errno,
>>>> +                             "VFIO_IOMMU_SPAPR_TCE_GET_INFO failed");
>>>>              ret = -errno;
>>>>              if (v2) {
>>>>                  memory_listener_unregister(&container->prereg_listener);
>>>> @@ -1033,6 +1038,8 @@ static int vfio_connect_container(VFIOGroup *group, 
>>>> AddressSpace *as)
>>>>               */
>>>>              ret = vfio_spapr_remove_window(container, 
>>>> info.dma32_window_start);
>>>>              if (ret) {
>>>> +                error_setg_errno(errp, -ret,
>>>> +                                 "failed to remove existing window");
>>>>                  goto free_container_exit;
>>>>              }
>>>>          } else {
>>>> @@ -1043,7 +1050,7 @@ static int vfio_connect_container(VFIOGroup *group, 
>>>> AddressSpace *as)
>>>>                                0x1000);
>>>>          }
>>>>      } else {
>>>> -        error_report("vfio: No available IOMMU models");
>>>> +        error_setg(errp, "No available IOMMU models");
>>>>          ret = -EINVAL;
>>>>          goto free_container_exit;
>>>>      }
>>>> @@ -1054,7 +1061,8 @@ static int vfio_connect_container(VFIOGroup *group, 
>>>> AddressSpace *as)
>>>>  
>>>>      if (container->error) {
>>>>          ret = container->error;
>>>> -        error_report("vfio: memory listener initialization failed for 
>>>> container");
>>>> +        error_setg_errno(errp, -ret,
>>>> +                         "memory listener initialization failed for 
>>>> container");
>>>>          goto listener_release_exit;
>>>>      }
>>>>  
>>>> @@ -1120,6 +1128,7 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace 
>>>> *as)
>>>>      VFIOGroup *group;
>>>>      char path[32];
>>>>      struct vfio_group_status status = { .argsz = sizeof(status) };
>>>> +    Error *err = NULL;
>>>>  
>>>>      QLIST_FOREACH(group, &vfio_group_list, next) {
>>>>          if (group->groupid == groupid) {
>>>> @@ -1158,8 +1167,9 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace 
>>>> *as)
>>>>      group->groupid = groupid;
>>>>      QLIST_INIT(&group->device_list);
>>>>  
>>>> -    if (vfio_connect_container(group, as)) {
>>>> -        error_report("vfio: failed to setup container for group %d", 
>>>> groupid);
>>>> +    if (vfio_connect_container(group, as, &err)) {
>>>> +        error_reportf_err(err, "vfio: failed to setup container for group 
>>>> %d",
>>>> +                          groupid);
>>>>          goto close_fd_exit;
>>>>      }
>>>
>>
>