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.au...@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.au...@redhat.com>
> >> Reviewed-by: Markus Armbruster <arm...@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;
> >> }
> >
>
--
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
