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

[Auger Eric]

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

you get the vfio_prereg_listener_region_add called which is likely to
set container->error with the returned value of
VFIO_IOMMU_SPAPR_REGISTER_MEMORY ioctl.

Do I miss something?

Thanks

Eric
>