Hi Peter,
On 9/23/19 9:51 AM, Peter Xu wrote:
> On Mon, Sep 23, 2019 at 08:55:51AM +0200, Eric Auger wrote:
>> The container error integer field is currently used to store
>> the first error potentially encountered during any
>> vfio_listener_region_add() call. However this fails to propagate
>> detailed error messages up to the vfio_connect_container caller.
>> Instead of using an integer, let's use an Error handle.
>>
>> Messages are slightly reworded to accomodate the propagation.
>
> Thanks for working on this. Mostly good at least to me, though I
> still have a few nitpickings below.
>
>> @@ -543,6 +545,9 @@ static void vfio_listener_region_add(MemoryListener
>> *listener,
>> hostwin->max_iova - hostwin->min_iova + 1,
>> section->offset_within_address_space,
>> int128_get64(section->size))) {
>> + error_setg(&err, "Overlap with existing IOMMU range "
>> + "[0x%"PRIx64",0x%"PRIx64"]",
>> hostwin->min_iova,
>> + hostwin->max_iova - hostwin->min_iova + 1);
>> ret = -1;
>
> This line seems to be useless now after we dropped the integer version
> of container->error and start to use Error*.
correct
>
>> goto fail;
>> }
>> @@ -550,6 +555,7 @@ static void vfio_listener_region_add(MemoryListener
>> *listener,
>>
>> ret = vfio_spapr_create_window(container, section, &pgsize);
>> if (ret) {
>> + error_setg_errno(&err, -ret, "Failed to create SPAPR window");
>> goto fail;
>> }
>>
>> @@ -559,7 +565,7 @@ static void vfio_listener_region_add(MemoryListener
>> *listener,
>> #ifdef CONFIG_KVM
>> if (kvm_enabled()) {
>> VFIOGroup *group;
>> - IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
>> + IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(mr);
>> struct kvm_vfio_spapr_tce param;
>> struct kvm_device_attr attr = {
>> .group = KVM_DEV_VFIO_GROUP,
>> @@ -594,18 +600,17 @@ static void vfio_listener_region_add(MemoryListener
>> *listener,
>> }
>>
>> if (!hostwin_found) {
>> - error_report("vfio: IOMMU container %p can't map guest IOVA region"
>> - " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx,
>> - container, iova, end);
>> + error_setg(&err, "Container %p can't map guest IOVA region"
>> + " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx, container, iova,
>> end);
>> ret = -EFAULT;
>
> Same here.
OK
>
>> goto fail;
>> }
>
> [...]
>
>> @@ -688,10 +694,14 @@ fail:
>> */
>> if (!container->initialized) {
>> if (!container->error) {
>> - container->error = ret;
>> + error_propagate_prepend(&container->error, err,
>> + "Region %s: ", memory_region_name(mr));
>> + } else {
>> + error_free(err);
>> }
>> } else {
>> - hw_error("vfio: DMA mapping failed, unable to continue");
>> + error_reportf_err(err,
>> + "vfio: DMA mapping failed, unable to continue: ");
>
> Probably need to keep hw_error() because it asserts inside. Maybe an
> error_report_err() before hw_error()?
that's correct.
>
>> }
>> }
>>
>> @@ -1251,6 +1261,7 @@ static int vfio_connect_container(VFIOGroup *group,
>> AddressSpace *as,
>> container = g_malloc0(sizeof(*container));
>> container->space = space;
>> container->fd = fd;
>> + container->error = NULL;
>> QLIST_INIT(&container->giommu_list);
>> QLIST_INIT(&container->hostwin_list);
>>
>> @@ -1308,9 +1319,9 @@ static int vfio_connect_container(VFIOGroup *group,
>> AddressSpace *as,
>> &address_space_memory);
>> if (container->error) {
>> memory_listener_unregister(&container->prereg_listener);
>> - ret = container->error;
>> - error_setg(errp,
>> - "RAM memory listener initialization failed for
>> container");
>> + ret = -1;
>> + error_propagate_prepend(errp, container->error,
>> + "RAM memory listener initialization failed: ");
>
> (I saw that we've got plenty of prepended prefixes for an error
> messages. For me I'll disgard quite a few of them because the errors
> will directly be delivered to the top level user, but this might be
> too personal as a comment)
That's true we have a lot of prefix messages.
The output message now is:
"vfio 0000:89:00.0: failed
to setup container for group 2: memory listener initialization failed:
Region smmuv3-iommu-memory-region-0-6: device 01.00.0 requires iommu MAP
notifier which is not currently supported"
Alex, any opinion?
Thanks
Eric
>
> Thanks,
>
>> goto free_container_exit;
>> }
>> }
>
