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