[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 08/17] vfio: Pass an Error object to vfio_con
From: |
David Gibson |
Subject: |
Re: [Qemu-devel] [PATCH v5 08/17] vfio: Pass an Error object to vfio_connect_container |
Date: |
Tue, 11 Oct 2016 14:04:56 +1100 |
User-agent: |
Mutt/1.7.0 (2016-08-17) |
On Mon, Oct 10, 2016 at 02:36:28PM +0200, 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.
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?
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
- [Qemu-devel] [PATCH v5 08/17] vfio: Pass an Error object to vfio_connect_container, (continued)
- [Qemu-devel] [PATCH v5 08/17] vfio: Pass an Error object to vfio_connect_container, Eric Auger, 2016/10/06
- Re: [Qemu-devel] [PATCH v5 08/17] vfio: Pass an Error object to vfio_connect_container, Markus Armbruster, 2016/10/07
- Re: [Qemu-devel] [PATCH v5 08/17] vfio: Pass an Error object to vfio_connect_container, Auger Eric, 2016/10/07
- Re: [Qemu-devel] [PATCH v5 08/17] vfio: Pass an Error object to vfio_connect_container, Alexey Kardashevskiy, 2016/10/09
- Re: [Qemu-devel] [PATCH v5 08/17] vfio: Pass an Error object to vfio_connect_container, David Gibson, 2016/10/10
- Re: [Qemu-devel] [PATCH v5 08/17] vfio: Pass an Error object to vfio_connect_container, Auger Eric, 2016/10/10
- Re: [Qemu-devel] [PATCH v5 08/17] vfio: Pass an Error object to vfio_connect_container, Markus Armbruster, 2016/10/10
- Re: [Qemu-devel] [PATCH v5 08/17] vfio: Pass an Error object to vfio_connect_container, Auger Eric, 2016/10/10
- Re: [Qemu-devel] [PATCH v5 08/17] vfio: Pass an Error object to vfio_connect_container, David Gibson, 2016/10/10
- Re: [Qemu-devel] [PATCH v5 08/17] vfio: Pass an Error object to vfio_connect_container, Markus Armbruster, 2016/10/10
- Re: [Qemu-devel] [PATCH v5 08/17] vfio: Pass an Error object to vfio_connect_container,
David Gibson <=
[Qemu-devel] [PATCH v5 06/17] vfio/pci: Pass an error object to vfio_add_capabilities, Eric Auger, 2016/10/06
[Qemu-devel] [PATCH v5 10/17] vfio: Pass an error object to vfio_get_device, Eric Auger, 2016/10/06
[Qemu-devel] [PATCH v5 09/17] vfio: Pass an error object to vfio_get_group, Eric Auger, 2016/10/06
[Qemu-devel] [PATCH v5 12/17] vfio/platform: fix a wrong returned value in vfio_populate_device, Eric Auger, 2016/10/06
[Qemu-devel] [PATCH v5 11/17] vfio/platform: Pass an error object to vfio_populate_device, Eric Auger, 2016/10/06
[Qemu-devel] [PATCH v5 14/17] vfio/pci: Conversion to realize, Eric Auger, 2016/10/06
[Qemu-devel] [PATCH v5 13/17] vfio/platform: Pass an error object to vfio_base_device_init, Eric Auger, 2016/10/06
[Qemu-devel] [PATCH v5 15/17] vfio/pci: Remove vfio_msix_early_setup returned value, Eric Auger, 2016/10/06
[Qemu-devel] [PATCH v5 16/17] vfio/pci: Remove vfio_populate_device returned value, Eric Auger, 2016/10/06
[Qemu-devel] [PATCH v5 17/17] vfio/pci: Handle host oversight, Eric Auger, 2016/10/06