[v5,08/17] vfio: Pass an Error object to vfio_connect_container
diff mbox

Message ID 1475770058-20409-9-git-send-email-eric.auger@redhat.com
State New
Headers show

Commit Message

Eric Auger Oct. 6, 2016, 4:07 p.m. UTC
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.

On vfio_spapr_remove_window failure, we also report an error whereas
it was silent before.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>

---

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(-)

Comments

Markus Armbruster Oct. 7, 2016, 7:01 a.m. UTC | #1
Eric Auger <eric.auger@redhat.com> 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.

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 <eric.auger@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> ---
>
> 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?

>                  memory_listener_unregister(&container->prereg_listener);
> -                error_report("vfio: RAM memory listener initialization failed for container");
> +                ret = container->error;
> +                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;
>      }
Eric Auger Oct. 7, 2016, 7:36 a.m. UTC | #2
Hi,

On 07/10/2016 09:01, Markus Armbruster wrote:
> Eric Auger <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>
>> ---
>>
>> 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?

Thanks

Eric


> 
>>                  memory_listener_unregister(&container->prereg_listener);
>> -                error_report("vfio: RAM memory listener initialization failed for container");
>> +                ret = container->error;
>> +                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;
>>      }
>
Alexey Kardashevskiy Oct. 10, 2016, 2:02 a.m. UTC | #3
On 07/10/16 18:36, Auger Eric wrote:
> Hi,
> 
> On 07/10/2016 09:01, Markus Armbruster wrote:
>> Eric Auger <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
>>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>>
>>> ---
>>>
>>> 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 ...

Use on what Error* exactly? It is a listener, no return codes from there...


> David, Alexey, could you confirm we should set the returned value to the
> container->error below?

The hw/vfio/common.c's ret is -errno but the hw/vfio/spapr.c's ret is
ioctl()'s ret which is not a huge difference (as that specific ioctl() does
not return successful non-zero codes) but yes, I should have used -errno in
hw/vfio/spapr.c, not just ret.




> 
> Thanks
> 
> Eric
> 
> 
>>
>>>                  memory_listener_unregister(&container->prereg_listener);
>>> -                error_report("vfio: RAM memory listener initialization failed for container");
>>> +                ret = container->error;
>>> +                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;
>>>      }
>>
David Gibson Oct. 10, 2016, 5:34 a.m. UTC | #4
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 <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
> >> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> >>
> >> ---
> >>
> >> 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;
> >> +                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;
> >>      }
> > 
>
Eric Auger Oct. 10, 2016, 7:44 a.m. UTC | #5
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 <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
>>>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>>>
>>>> ---
>>>>
>>>> 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;
>>>>      }
>>>
>>
>
Markus Armbruster Oct. 10, 2016, 12:36 p.m. UTC | #6
Auger Eric <eric.auger@redhat.com> 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 <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
>>>>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>>>>
>>>>> ---
>>>>>
>>>>> 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?
Eric Auger Oct. 10, 2016, 1:21 p.m. UTC | #7
Hi Markus,
On 10/10/2016 14:36, Markus Armbruster wrote:
> Auger Eric <eric.auger@redhat.com> 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 <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
>>>>>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> 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?

Thanks

Eric
>
David Gibson Oct. 10, 2016, 1:36 p.m. UTC | #8
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 <eric.auger@redhat.com> 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 <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
> >>>>>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> >>>>>>
> >>>>>> ---
> >>>>>>
> >>>>>> 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.
Markus Armbruster Oct. 10, 2016, 3:27 p.m. UTC | #9
David Gibson <david@gibson.dropbear.id.au> writes:

> 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 <eric.auger@redhat.com> 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 <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
>> >>>>>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> >>>>>>
>> >>>>>> ---
>> >>>>>>
>> >>>>>> 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.

Okay.  I think this series is fine as it is.  If you want to change
container->error, you can do that on top.
David Gibson Oct. 11, 2016, 3:04 a.m. UTC | #10
On Mon, Oct 10, 2016 at 02:36:28PM +0200, Markus Armbruster wrote:
> Auger Eric <eric.auger@redhat.com> 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 <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
> >>>>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> >>>>>
> >>>>> ---
> >>>>>
> >>>>> 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.

So, the only thing that uses container->error right now is for the
return code from the VFIO_IOMMU_SPAPR_REGISTER_MEMORY ioctl().
However in vfio_preref_listener_region_add() there's also one
error_report() that would be a little more graceful to propagate up by
the same mechanism.

It probably makes more sense as a follow up change, though.

> 
> 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?
>

Patch
diff mbox

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) {
                 memory_listener_unregister(&container->prereg_listener);
-                error_report("vfio: RAM memory listener initialization failed for container");
+                ret = container->error;
+                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;
     }