diff mbox series

[v5,01/10] vfio: Add Error** argument to .set_dirty_page_tracking() handler

Message ID 20240506092053.388578-2-clg@redhat.com (mailing list archive)
State New, archived
Headers show
Series vfio: Improve error reporting (part 2) | expand

Commit Message

Cédric Le Goater May 6, 2024, 9:20 a.m. UTC
We will use the Error object to improve error reporting in the
.log_global*() handlers of VFIO. Add documentation while at it.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---

 Changes in v5:

 - Fixed typo in set_dirty_page_tracking documentation
 
 include/hw/vfio/vfio-container-base.h | 18 ++++++++++++++++--
 hw/vfio/common.c                      |  4 ++--
 hw/vfio/container-base.c              |  4 ++--
 hw/vfio/container.c                   |  6 +++---
 4 files changed, 23 insertions(+), 9 deletions(-)

Comments

Avihai Horon May 13, 2024, 1:03 p.m. UTC | #1
Hi Cedric,

On 06/05/2024 12:20, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> We will use the Error object to improve error reporting in the
> .log_global*() handlers of VFIO. Add documentation while at it.

First of all, I think commit 3688fec8923 ("memory: Add Error** argument 
to .log_global_start() handler") forgot to set errp in 
vfio_listener_log_global_start() in case of error.
This causes a null pointer de-reference if DPT start fails.
Maybe add a fix for that in the beginning of this series, or as a 
stand-alone fix?

Back to this patch, personally, I found the split of patch #1 and #2 a 
bit confusing.
Maybe consider squashing patch #1 and #2 so container based and device 
based DPT start/stop are changed in the same patch? Like you did in 
patch #8?
Whatever you think is better.

In any case:
Reviewed-by: Avihai Horon <avihaih@nvidia.com>

>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>
>   Changes in v5:
>
>   - Fixed typo in set_dirty_page_tracking documentation
>
>   include/hw/vfio/vfio-container-base.h | 18 ++++++++++++++++--
>   hw/vfio/common.c                      |  4 ++--
>   hw/vfio/container-base.c              |  4 ++--
>   hw/vfio/container.c                   |  6 +++---
>   4 files changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
> index 3582d5f97a37877b2adfc0d0b06996c82403f8b7..326ceea52a2030eec9dad289a9845866c4a8c090 100644
> --- a/include/hw/vfio/vfio-container-base.h
> +++ b/include/hw/vfio/vfio-container-base.h
> @@ -82,7 +82,7 @@ int vfio_container_add_section_window(VFIOContainerBase *bcontainer,
>   void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
>                                          MemoryRegionSection *section);
>   int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
> -                                           bool start);
> +                                           bool start, Error **errp);
>   int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>                                         VFIOBitmap *vbmap,
>                                         hwaddr iova, hwaddr size);
> @@ -121,9 +121,23 @@ struct VFIOIOMMUClass {
>       int (*attach_device)(const char *name, VFIODevice *vbasedev,
>                            AddressSpace *as, Error **errp);
>       void (*detach_device)(VFIODevice *vbasedev);
> +
>       /* migration feature */
> +
> +    /**
> +     * @set_dirty_page_tracking
> +     *
> +     * Start or stop dirty pages tracking on VFIO container
> +     *
> +     * @bcontainer: #VFIOContainerBase on which to de/activate dirty
> +     *              page tracking
> +     * @start: indicates whether to start or stop dirty pages tracking
> +     * @errp: pointer to Error*, to store an error if it happens.
> +     *
> +     * Returns zero to indicate success and negative for error
> +     */
>       int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
> -                                   bool start);
> +                                   bool start, Error **errp);
>       int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer,
>                                 VFIOBitmap *vbmap,
>                                 hwaddr iova, hwaddr size);
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 8f9cbdc0264044ce587877a7d19d14b28527291b..485e53916491f1164d29e739fb7106c0c77df737 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1076,7 +1076,7 @@ static bool vfio_listener_log_global_start(MemoryListener *listener,
>       if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>           ret = vfio_devices_dma_logging_start(bcontainer);
>       } else {
> -        ret = vfio_container_set_dirty_page_tracking(bcontainer, true);
> +        ret = vfio_container_set_dirty_page_tracking(bcontainer, true, NULL);
>       }
>
>       if (ret) {
> @@ -1096,7 +1096,7 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
>       if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>           vfio_devices_dma_logging_stop(bcontainer);
>       } else {
> -        ret = vfio_container_set_dirty_page_tracking(bcontainer, false);
> +        ret = vfio_container_set_dirty_page_tracking(bcontainer, false, NULL);
>       }
>
>       if (ret) {
> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
> index 913ae49077c4f09b7b27517c1231cfbe4befb7fb..7c0764121d24b02b6c4e66e368d7dff78a6d65aa 100644
> --- a/hw/vfio/container-base.c
> +++ b/hw/vfio/container-base.c
> @@ -53,14 +53,14 @@ void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
>   }
>
>   int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
> -                                           bool start)
> +                                           bool start, Error **errp)
>   {
>       if (!bcontainer->dirty_pages_supported) {
>           return 0;
>       }
>
>       g_assert(bcontainer->ops->set_dirty_page_tracking);
> -    return bcontainer->ops->set_dirty_page_tracking(bcontainer, start);
> +    return bcontainer->ops->set_dirty_page_tracking(bcontainer, start, errp);
>   }
>
>   int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 77bdec276ec49cb9cd767c0de42ec801b4421572..c35221fbe7dc5453050f97cd186fc958e24f28f7 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -209,7 +209,7 @@ static int vfio_legacy_dma_map(const VFIOContainerBase *bcontainer, hwaddr iova,
>
>   static int
>   vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
> -                                    bool start)
> +                                    bool start, Error **errp)
>   {
>       const VFIOContainer *container = container_of(bcontainer, VFIOContainer,
>                                                     bcontainer);
> @@ -227,8 +227,8 @@ vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
>       ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, &dirty);
>       if (ret) {
>           ret = -errno;
> -        error_report("Failed to set dirty tracking flag 0x%x errno: %d",
> -                     dirty.flags, errno);
> +        error_setg_errno(errp, errno, "Failed to set dirty tracking flag 0x%x",
> +                         dirty.flags);
>       }
>
>       return ret;
> --
> 2.45.0
>
Cédric Le Goater May 13, 2024, 4:27 p.m. UTC | #2
On 5/13/24 15:03, Avihai Horon wrote:
> Hi Cedric,
> 
> On 06/05/2024 12:20, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> We will use the Error object to improve error reporting in the
>> .log_global*() handlers of VFIO. Add documentation while at it.
> 
> First of all, I think commit 3688fec8923 ("memory: Add Error** argument to .log_global_start() handler") forgot to set errp in vfio_listener_log_global_start() in case of error.

yes. This is unfortunate. There has been a few respins, the series
was split and I was hoping to upstream this part sooner. My bad.

> This causes a null pointer de-reference if DPT start fails.
> Maybe add a fix for that in the beginning of this series, or as a stand-alone fix?

Since it is fixed by patch 1+2 of this series, we should be fine ?

> Back to this patch, personally, I found the split of patch #1 and #2 a bit confusing.
> Maybe consider squashing patch #1 and #2 so container based and device based DPT start/stop are changed in the same patch? Like you did in patch #8?
> Whatever you think is better.

ok. Let's see how v5 goes. I might just send a PR with it if
no major changes are requested.

> 
> In any case:
> Reviewed-by: Avihai Horon <avihaih@nvidia.com>


Thanks,

C.


> 
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>
>>   Changes in v5:
>>
>>   - Fixed typo in set_dirty_page_tracking documentation
>>
>>   include/hw/vfio/vfio-container-base.h | 18 ++++++++++++++++--
>>   hw/vfio/common.c                      |  4 ++--
>>   hw/vfio/container-base.c              |  4 ++--
>>   hw/vfio/container.c                   |  6 +++---
>>   4 files changed, 23 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
>> index 3582d5f97a37877b2adfc0d0b06996c82403f8b7..326ceea52a2030eec9dad289a9845866c4a8c090 100644
>> --- a/include/hw/vfio/vfio-container-base.h
>> +++ b/include/hw/vfio/vfio-container-base.h
>> @@ -82,7 +82,7 @@ int vfio_container_add_section_window(VFIOContainerBase *bcontainer,
>>   void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
>>                                          MemoryRegionSection *section);
>>   int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
>> -                                           bool start);
>> +                                           bool start, Error **errp);
>>   int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>>                                         VFIOBitmap *vbmap,
>>                                         hwaddr iova, hwaddr size);
>> @@ -121,9 +121,23 @@ struct VFIOIOMMUClass {
>>       int (*attach_device)(const char *name, VFIODevice *vbasedev,
>>                            AddressSpace *as, Error **errp);
>>       void (*detach_device)(VFIODevice *vbasedev);
>> +
>>       /* migration feature */
>> +
>> +    /**
>> +     * @set_dirty_page_tracking
>> +     *
>> +     * Start or stop dirty pages tracking on VFIO container
>> +     *
>> +     * @bcontainer: #VFIOContainerBase on which to de/activate dirty
>> +     *              page tracking
>> +     * @start: indicates whether to start or stop dirty pages tracking
>> +     * @errp: pointer to Error*, to store an error if it happens.
>> +     *
>> +     * Returns zero to indicate success and negative for error
>> +     */
>>       int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
>> -                                   bool start);
>> +                                   bool start, Error **errp);
>>       int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer,
>>                                 VFIOBitmap *vbmap,
>>                                 hwaddr iova, hwaddr size);
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 8f9cbdc0264044ce587877a7d19d14b28527291b..485e53916491f1164d29e739fb7106c0c77df737 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1076,7 +1076,7 @@ static bool vfio_listener_log_global_start(MemoryListener *listener,
>>       if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>>           ret = vfio_devices_dma_logging_start(bcontainer);
>>       } else {
>> -        ret = vfio_container_set_dirty_page_tracking(bcontainer, true);
>> +        ret = vfio_container_set_dirty_page_tracking(bcontainer, true, NULL);
>>       }
>>
>>       if (ret) {
>> @@ -1096,7 +1096,7 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
>>       if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>>           vfio_devices_dma_logging_stop(bcontainer);
>>       } else {
>> -        ret = vfio_container_set_dirty_page_tracking(bcontainer, false);
>> +        ret = vfio_container_set_dirty_page_tracking(bcontainer, false, NULL);
>>       }
>>
>>       if (ret) {
>> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
>> index 913ae49077c4f09b7b27517c1231cfbe4befb7fb..7c0764121d24b02b6c4e66e368d7dff78a6d65aa 100644
>> --- a/hw/vfio/container-base.c
>> +++ b/hw/vfio/container-base.c
>> @@ -53,14 +53,14 @@ void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
>>   }
>>
>>   int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
>> -                                           bool start)
>> +                                           bool start, Error **errp)
>>   {
>>       if (!bcontainer->dirty_pages_supported) {
>>           return 0;
>>       }
>>
>>       g_assert(bcontainer->ops->set_dirty_page_tracking);
>> -    return bcontainer->ops->set_dirty_page_tracking(bcontainer, start);
>> +    return bcontainer->ops->set_dirty_page_tracking(bcontainer, start, errp);
>>   }
>>
>>   int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>> index 77bdec276ec49cb9cd767c0de42ec801b4421572..c35221fbe7dc5453050f97cd186fc958e24f28f7 100644
>> --- a/hw/vfio/container.c
>> +++ b/hw/vfio/container.c
>> @@ -209,7 +209,7 @@ static int vfio_legacy_dma_map(const VFIOContainerBase *bcontainer, hwaddr iova,
>>
>>   static int
>>   vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
>> -                                    bool start)
>> +                                    bool start, Error **errp)
>>   {
>>       const VFIOContainer *container = container_of(bcontainer, VFIOContainer,
>>                                                     bcontainer);
>> @@ -227,8 +227,8 @@ vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
>>       ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, &dirty);
>>       if (ret) {
>>           ret = -errno;
>> -        error_report("Failed to set dirty tracking flag 0x%x errno: %d",
>> -                     dirty.flags, errno);
>> +        error_setg_errno(errp, errno, "Failed to set dirty tracking flag 0x%x",
>> +                         dirty.flags);
>>       }
>>
>>       return ret;
>> -- 
>> 2.45.0
>>
>
Avihai Horon May 15, 2024, 12:17 p.m. UTC | #3
On 13/05/2024 19:27, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> On 5/13/24 15:03, Avihai Horon wrote:
>> Hi Cedric,
>>
>> On 06/05/2024 12:20, Cédric Le Goater wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> We will use the Error object to improve error reporting in the
>>> .log_global*() handlers of VFIO. Add documentation while at it.
>>
>> First of all, I think commit 3688fec8923 ("memory: Add Error** 
>> argument to .log_global_start() handler") forgot to set errp in 
>> vfio_listener_log_global_start() in case of error.
>
> yes. This is unfortunate. There has been a few respins, the series
> was split and I was hoping to upstream this part sooner. My bad.
>
>> This causes a null pointer de-reference if DPT start fails.
>> Maybe add a fix for that in the beginning of this series, or as a 
>> stand-alone fix?
>
> Since it is fixed by patch 1+2 of this series, we should be fine ?

A fix could be useful to be backported to QEMU 9.0, no?

>
>> Back to this patch, personally, I found the split of patch #1 and #2 
>> a bit confusing.
>> Maybe consider squashing patch #1 and #2 so container based and 
>> device based DPT start/stop are changed in the same patch? Like you 
>> did in patch #8?
>> Whatever you think is better.
>
> ok. Let's see how v5 goes. I might just send a PR with it if
> no major changes are requested.
>
>>
>> In any case:
>> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
>
>
> Thanks,
>
> C.
>
>
>>
>>>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>> ---
>>>
>>>   Changes in v5:
>>>
>>>   - Fixed typo in set_dirty_page_tracking documentation
>>>
>>>   include/hw/vfio/vfio-container-base.h | 18 ++++++++++++++++--
>>>   hw/vfio/common.c                      |  4 ++--
>>>   hw/vfio/container-base.c              |  4 ++--
>>>   hw/vfio/container.c                   |  6 +++---
>>>   4 files changed, 23 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/include/hw/vfio/vfio-container-base.h 
>>> b/include/hw/vfio/vfio-container-base.h
>>> index 
>>> 3582d5f97a37877b2adfc0d0b06996c82403f8b7..326ceea52a2030eec9dad289a9845866c4a8c090 
>>> 100644
>>> --- a/include/hw/vfio/vfio-container-base.h
>>> +++ b/include/hw/vfio/vfio-container-base.h
>>> @@ -82,7 +82,7 @@ int 
>>> vfio_container_add_section_window(VFIOContainerBase *bcontainer,
>>>   void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
>>>                                          MemoryRegionSection *section);
>>>   int vfio_container_set_dirty_page_tracking(VFIOContainerBase 
>>> *bcontainer,
>>> -                                           bool start);
>>> +                                           bool start, Error **errp);
>>>   int vfio_container_query_dirty_bitmap(const VFIOContainerBase 
>>> *bcontainer,
>>>                                         VFIOBitmap *vbmap,
>>>                                         hwaddr iova, hwaddr size);
>>> @@ -121,9 +121,23 @@ struct VFIOIOMMUClass {
>>>       int (*attach_device)(const char *name, VFIODevice *vbasedev,
>>>                            AddressSpace *as, Error **errp);
>>>       void (*detach_device)(VFIODevice *vbasedev);
>>> +
>>>       /* migration feature */
>>> +
>>> +    /**
>>> +     * @set_dirty_page_tracking
>>> +     *
>>> +     * Start or stop dirty pages tracking on VFIO container
>>> +     *
>>> +     * @bcontainer: #VFIOContainerBase on which to de/activate dirty
>>> +     *              page tracking
>>> +     * @start: indicates whether to start or stop dirty pages tracking
>>> +     * @errp: pointer to Error*, to store an error if it happens.
>>> +     *
>>> +     * Returns zero to indicate success and negative for error
>>> +     */
>>>       int (*set_dirty_page_tracking)(const VFIOContainerBase 
>>> *bcontainer,
>>> -                                   bool start);
>>> +                                   bool start, Error **errp);
>>>       int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer,
>>>                                 VFIOBitmap *vbmap,
>>>                                 hwaddr iova, hwaddr size);
>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>> index 
>>> 8f9cbdc0264044ce587877a7d19d14b28527291b..485e53916491f1164d29e739fb7106c0c77df737 
>>> 100644
>>> --- a/hw/vfio/common.c
>>> +++ b/hw/vfio/common.c
>>> @@ -1076,7 +1076,7 @@ static bool 
>>> vfio_listener_log_global_start(MemoryListener *listener,
>>>       if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>>>           ret = vfio_devices_dma_logging_start(bcontainer);
>>>       } else {
>>> -        ret = vfio_container_set_dirty_page_tracking(bcontainer, 
>>> true);
>>> +        ret = vfio_container_set_dirty_page_tracking(bcontainer, 
>>> true, NULL);
>>>       }
>>>
>>>       if (ret) {
>>> @@ -1096,7 +1096,7 @@ static void 
>>> vfio_listener_log_global_stop(MemoryListener *listener)
>>>       if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>>>           vfio_devices_dma_logging_stop(bcontainer);
>>>       } else {
>>> -        ret = vfio_container_set_dirty_page_tracking(bcontainer, 
>>> false);
>>> +        ret = vfio_container_set_dirty_page_tracking(bcontainer, 
>>> false, NULL);
>>>       }
>>>
>>>       if (ret) {
>>> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
>>> index 
>>> 913ae49077c4f09b7b27517c1231cfbe4befb7fb..7c0764121d24b02b6c4e66e368d7dff78a6d65aa 
>>> 100644
>>> --- a/hw/vfio/container-base.c
>>> +++ b/hw/vfio/container-base.c
>>> @@ -53,14 +53,14 @@ void 
>>> vfio_container_del_section_window(VFIOContainerBase *bcontainer,
>>>   }
>>>
>>>   int vfio_container_set_dirty_page_tracking(VFIOContainerBase 
>>> *bcontainer,
>>> -                                           bool start)
>>> +                                           bool start, Error **errp)
>>>   {
>>>       if (!bcontainer->dirty_pages_supported) {
>>>           return 0;
>>>       }
>>>
>>> g_assert(bcontainer->ops->set_dirty_page_tracking);
>>> -    return bcontainer->ops->set_dirty_page_tracking(bcontainer, 
>>> start);
>>> +    return bcontainer->ops->set_dirty_page_tracking(bcontainer, 
>>> start, errp);
>>>   }
>>>
>>>   int vfio_container_query_dirty_bitmap(const VFIOContainerBase 
>>> *bcontainer,
>>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>>> index 
>>> 77bdec276ec49cb9cd767c0de42ec801b4421572..c35221fbe7dc5453050f97cd186fc958e24f28f7 
>>> 100644
>>> --- a/hw/vfio/container.c
>>> +++ b/hw/vfio/container.c
>>> @@ -209,7 +209,7 @@ static int vfio_legacy_dma_map(const 
>>> VFIOContainerBase *bcontainer, hwaddr iova,
>>>
>>>   static int
>>>   vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase 
>>> *bcontainer,
>>> -                                    bool start)
>>> +                                    bool start, Error **errp)
>>>   {
>>>       const VFIOContainer *container = container_of(bcontainer, 
>>> VFIOContainer,
>>> bcontainer);
>>> @@ -227,8 +227,8 @@ vfio_legacy_set_dirty_page_tracking(const 
>>> VFIOContainerBase *bcontainer,
>>>       ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, &dirty);
>>>       if (ret) {
>>>           ret = -errno;
>>> -        error_report("Failed to set dirty tracking flag 0x%x errno: 
>>> %d",
>>> -                     dirty.flags, errno);
>>> +        error_setg_errno(errp, errno, "Failed to set dirty tracking 
>>> flag 0x%x",
>>> +                         dirty.flags);
>>>       }
>>>
>>>       return ret;
>>> -- 
>>> 2.45.0
>>>
>>
>
Cédric Le Goater May 15, 2024, 12:25 p.m. UTC | #4
On 5/15/24 14:17, Avihai Horon wrote:
> 
> On 13/05/2024 19:27, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 5/13/24 15:03, Avihai Horon wrote:
>>> Hi Cedric,
>>>
>>> On 06/05/2024 12:20, Cédric Le Goater wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> We will use the Error object to improve error reporting in the
>>>> .log_global*() handlers of VFIO. Add documentation while at it.
>>>
>>> First of all, I think commit 3688fec8923 ("memory: Add Error** argument to .log_global_start() handler") forgot to set errp in vfio_listener_log_global_start() in case of error.
>>
>> yes. This is unfortunate. There has been a few respins, the series
>> was split and I was hoping to upstream this part sooner. My bad.
>>
>>> This causes a null pointer de-reference if DPT start fails.
>>> Maybe add a fix for that in the beginning of this series, or as a stand-alone fix?
>>
>> Since it is fixed by patch 1+2 of this series, we should be fine ?
> 
> A fix could be useful to be backported to QEMU 9.0, no?

These changes were only merged in 9.1 with the migration PR.
Am I missing something ?

Thanks,

C.


> 
>>
>>> Back to this patch, personally, I found the split of patch #1 and #2 a bit confusing.
>>> Maybe consider squashing patch #1 and #2 so container based and device based DPT start/stop are changed in the same patch? Like you did in patch #8?
>>> Whatever you think is better.
>>
>> ok. Let's see how v5 goes. I might just send a PR with it if
>> no major changes are requested.
>>
>>>
>>> In any case:
>>> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
>>
>>
>> Thanks,
>>
>> C.
>>
>>
>>>
>>>>
>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>>> ---
>>>>
>>>>   Changes in v5:
>>>>
>>>>   - Fixed typo in set_dirty_page_tracking documentation
>>>>
>>>>   include/hw/vfio/vfio-container-base.h | 18 ++++++++++++++++--
>>>>   hw/vfio/common.c                      |  4 ++--
>>>>   hw/vfio/container-base.c              |  4 ++--
>>>>   hw/vfio/container.c                   |  6 +++---
>>>>   4 files changed, 23 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
>>>> index 3582d5f97a37877b2adfc0d0b06996c82403f8b7..326ceea52a2030eec9dad289a9845866c4a8c090 100644
>>>> --- a/include/hw/vfio/vfio-container-base.h
>>>> +++ b/include/hw/vfio/vfio-container-base.h
>>>> @@ -82,7 +82,7 @@ int vfio_container_add_section_window(VFIOContainerBase *bcontainer,
>>>>   void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
>>>>                                          MemoryRegionSection *section);
>>>>   int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
>>>> -                                           bool start);
>>>> +                                           bool start, Error **errp);
>>>>   int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>>>>                                         VFIOBitmap *vbmap,
>>>>                                         hwaddr iova, hwaddr size);
>>>> @@ -121,9 +121,23 @@ struct VFIOIOMMUClass {
>>>>       int (*attach_device)(const char *name, VFIODevice *vbasedev,
>>>>                            AddressSpace *as, Error **errp);
>>>>       void (*detach_device)(VFIODevice *vbasedev);
>>>> +
>>>>       /* migration feature */
>>>> +
>>>> +    /**
>>>> +     * @set_dirty_page_tracking
>>>> +     *
>>>> +     * Start or stop dirty pages tracking on VFIO container
>>>> +     *
>>>> +     * @bcontainer: #VFIOContainerBase on which to de/activate dirty
>>>> +     *              page tracking
>>>> +     * @start: indicates whether to start or stop dirty pages tracking
>>>> +     * @errp: pointer to Error*, to store an error if it happens.
>>>> +     *
>>>> +     * Returns zero to indicate success and negative for error
>>>> +     */
>>>>       int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
>>>> -                                   bool start);
>>>> +                                   bool start, Error **errp);
>>>>       int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer,
>>>>                                 VFIOBitmap *vbmap,
>>>>                                 hwaddr iova, hwaddr size);
>>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>>> index 8f9cbdc0264044ce587877a7d19d14b28527291b..485e53916491f1164d29e739fb7106c0c77df737 100644
>>>> --- a/hw/vfio/common.c
>>>> +++ b/hw/vfio/common.c
>>>> @@ -1076,7 +1076,7 @@ static bool vfio_listener_log_global_start(MemoryListener *listener,
>>>>       if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>>>>           ret = vfio_devices_dma_logging_start(bcontainer);
>>>>       } else {
>>>> -        ret = vfio_container_set_dirty_page_tracking(bcontainer, true);
>>>> +        ret = vfio_container_set_dirty_page_tracking(bcontainer, true, NULL);
>>>>       }
>>>>
>>>>       if (ret) {
>>>> @@ -1096,7 +1096,7 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
>>>>       if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>>>>           vfio_devices_dma_logging_stop(bcontainer);
>>>>       } else {
>>>> -        ret = vfio_container_set_dirty_page_tracking(bcontainer, false);
>>>> +        ret = vfio_container_set_dirty_page_tracking(bcontainer, false, NULL);
>>>>       }
>>>>
>>>>       if (ret) {
>>>> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
>>>> index 913ae49077c4f09b7b27517c1231cfbe4befb7fb..7c0764121d24b02b6c4e66e368d7dff78a6d65aa 100644
>>>> --- a/hw/vfio/container-base.c
>>>> +++ b/hw/vfio/container-base.c
>>>> @@ -53,14 +53,14 @@ void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
>>>>   }
>>>>
>>>>   int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
>>>> -                                           bool start)
>>>> +                                           bool start, Error **errp)
>>>>   {
>>>>       if (!bcontainer->dirty_pages_supported) {
>>>>           return 0;
>>>>       }
>>>>
>>>> g_assert(bcontainer->ops->set_dirty_page_tracking);
>>>> -    return bcontainer->ops->set_dirty_page_tracking(bcontainer, start);
>>>> +    return bcontainer->ops->set_dirty_page_tracking(bcontainer, start, errp);
>>>>   }
>>>>
>>>>   int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>>>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>>>> index 77bdec276ec49cb9cd767c0de42ec801b4421572..c35221fbe7dc5453050f97cd186fc958e24f28f7 100644
>>>> --- a/hw/vfio/container.c
>>>> +++ b/hw/vfio/container.c
>>>> @@ -209,7 +209,7 @@ static int vfio_legacy_dma_map(const VFIOContainerBase *bcontainer, hwaddr iova,
>>>>
>>>>   static int
>>>>   vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
>>>> -                                    bool start)
>>>> +                                    bool start, Error **errp)
>>>>   {
>>>>       const VFIOContainer *container = container_of(bcontainer, VFIOContainer,
>>>> bcontainer);
>>>> @@ -227,8 +227,8 @@ vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
>>>>       ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, &dirty);
>>>>       if (ret) {
>>>>           ret = -errno;
>>>> -        error_report("Failed to set dirty tracking flag 0x%x errno: %d",
>>>> -                     dirty.flags, errno);
>>>> +        error_setg_errno(errp, errno, "Failed to set dirty tracking flag 0x%x",
>>>> +                         dirty.flags);
>>>>       }
>>>>
>>>>       return ret;
>>>> -- 
>>>> 2.45.0
>>>>
>>>
>>
>
Avihai Horon May 15, 2024, 12:29 p.m. UTC | #5
On 15/05/2024 15:25, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> On 5/15/24 14:17, Avihai Horon wrote:
>>
>> On 13/05/2024 19:27, Cédric Le Goater wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 5/13/24 15:03, Avihai Horon wrote:
>>>> Hi Cedric,
>>>>
>>>> On 06/05/2024 12:20, Cédric Le Goater wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> We will use the Error object to improve error reporting in the
>>>>> .log_global*() handlers of VFIO. Add documentation while at it.
>>>>
>>>> First of all, I think commit 3688fec8923 ("memory: Add Error** 
>>>> argument to .log_global_start() handler") forgot to set errp in 
>>>> vfio_listener_log_global_start() in case of error.
>>>
>>> yes. This is unfortunate. There has been a few respins, the series
>>> was split and I was hoping to upstream this part sooner. My bad.
>>>
>>>> This causes a null pointer de-reference if DPT start fails.
>>>> Maybe add a fix for that in the beginning of this series, or as a 
>>>> stand-alone fix?
>>>
>>> Since it is fixed by patch 1+2 of this series, we should be fine ?
>>
>> A fix could be useful to be backported to QEMU 9.0, no?
>
> These changes were only merged in 9.1 with the migration PR.
> Am I missing something ?

Oh, my bad. I thought they were merged in 9.0.
So patches 1+2 should cover it.

Thanks.

>
> Thanks,
>
> C.
>
>
>>
>>>
>>>> Back to this patch, personally, I found the split of patch #1 and 
>>>> #2 a bit confusing.
>>>> Maybe consider squashing patch #1 and #2 so container based and 
>>>> device based DPT start/stop are changed in the same patch? Like you 
>>>> did in patch #8?
>>>> Whatever you think is better.
>>>
>>> ok. Let's see how v5 goes. I might just send a PR with it if
>>> no major changes are requested.
>>>
>>>>
>>>> In any case:
>>>> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
>>>
>>>
>>> Thanks,
>>>
>>> C.
>>>
>>>
>>>>
>>>>>
>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>>>> ---
>>>>>
>>>>>   Changes in v5:
>>>>>
>>>>>   - Fixed typo in set_dirty_page_tracking documentation
>>>>>
>>>>>   include/hw/vfio/vfio-container-base.h | 18 ++++++++++++++++--
>>>>>   hw/vfio/common.c                      |  4 ++--
>>>>>   hw/vfio/container-base.c              |  4 ++--
>>>>>   hw/vfio/container.c                   |  6 +++---
>>>>>   4 files changed, 23 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/include/hw/vfio/vfio-container-base.h 
>>>>> b/include/hw/vfio/vfio-container-base.h
>>>>> index 
>>>>> 3582d5f97a37877b2adfc0d0b06996c82403f8b7..326ceea52a2030eec9dad289a9845866c4a8c090 
>>>>> 100644
>>>>> --- a/include/hw/vfio/vfio-container-base.h
>>>>> +++ b/include/hw/vfio/vfio-container-base.h
>>>>> @@ -82,7 +82,7 @@ int 
>>>>> vfio_container_add_section_window(VFIOContainerBase *bcontainer,
>>>>>   void vfio_container_del_section_window(VFIOContainerBase 
>>>>> *bcontainer,
>>>>> MemoryRegionSection *section);
>>>>>   int vfio_container_set_dirty_page_tracking(VFIOContainerBase 
>>>>> *bcontainer,
>>>>> -                                           bool start);
>>>>> +                                           bool start, Error 
>>>>> **errp);
>>>>>   int vfio_container_query_dirty_bitmap(const VFIOContainerBase 
>>>>> *bcontainer,
>>>>>                                         VFIOBitmap *vbmap,
>>>>>                                         hwaddr iova, hwaddr size);
>>>>> @@ -121,9 +121,23 @@ struct VFIOIOMMUClass {
>>>>>       int (*attach_device)(const char *name, VFIODevice *vbasedev,
>>>>>                            AddressSpace *as, Error **errp);
>>>>>       void (*detach_device)(VFIODevice *vbasedev);
>>>>> +
>>>>>       /* migration feature */
>>>>> +
>>>>> +    /**
>>>>> +     * @set_dirty_page_tracking
>>>>> +     *
>>>>> +     * Start or stop dirty pages tracking on VFIO container
>>>>> +     *
>>>>> +     * @bcontainer: #VFIOContainerBase on which to de/activate dirty
>>>>> +     *              page tracking
>>>>> +     * @start: indicates whether to start or stop dirty pages 
>>>>> tracking
>>>>> +     * @errp: pointer to Error*, to store an error if it happens.
>>>>> +     *
>>>>> +     * Returns zero to indicate success and negative for error
>>>>> +     */
>>>>>       int (*set_dirty_page_tracking)(const VFIOContainerBase 
>>>>> *bcontainer,
>>>>> -                                   bool start);
>>>>> +                                   bool start, Error **errp);
>>>>>       int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer,
>>>>>                                 VFIOBitmap *vbmap,
>>>>>                                 hwaddr iova, hwaddr size);
>>>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>>>> index 
>>>>> 8f9cbdc0264044ce587877a7d19d14b28527291b..485e53916491f1164d29e739fb7106c0c77df737 
>>>>> 100644
>>>>> --- a/hw/vfio/common.c
>>>>> +++ b/hw/vfio/common.c
>>>>> @@ -1076,7 +1076,7 @@ static bool 
>>>>> vfio_listener_log_global_start(MemoryListener *listener,
>>>>>       if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>>>>>           ret = vfio_devices_dma_logging_start(bcontainer);
>>>>>       } else {
>>>>> -        ret = vfio_container_set_dirty_page_tracking(bcontainer, 
>>>>> true);
>>>>> +        ret = vfio_container_set_dirty_page_tracking(bcontainer, 
>>>>> true, NULL);
>>>>>       }
>>>>>
>>>>>       if (ret) {
>>>>> @@ -1096,7 +1096,7 @@ static void 
>>>>> vfio_listener_log_global_stop(MemoryListener *listener)
>>>>>       if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>>>>>           vfio_devices_dma_logging_stop(bcontainer);
>>>>>       } else {
>>>>> -        ret = vfio_container_set_dirty_page_tracking(bcontainer, 
>>>>> false);
>>>>> +        ret = vfio_container_set_dirty_page_tracking(bcontainer, 
>>>>> false, NULL);
>>>>>       }
>>>>>
>>>>>       if (ret) {
>>>>> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
>>>>> index 
>>>>> 913ae49077c4f09b7b27517c1231cfbe4befb7fb..7c0764121d24b02b6c4e66e368d7dff78a6d65aa 
>>>>> 100644
>>>>> --- a/hw/vfio/container-base.c
>>>>> +++ b/hw/vfio/container-base.c
>>>>> @@ -53,14 +53,14 @@ void 
>>>>> vfio_container_del_section_window(VFIOContainerBase *bcontainer,
>>>>>   }
>>>>>
>>>>>   int vfio_container_set_dirty_page_tracking(VFIOContainerBase 
>>>>> *bcontainer,
>>>>> -                                           bool start)
>>>>> +                                           bool start, Error **errp)
>>>>>   {
>>>>>       if (!bcontainer->dirty_pages_supported) {
>>>>>           return 0;
>>>>>       }
>>>>>
>>>>> g_assert(bcontainer->ops->set_dirty_page_tracking);
>>>>> -    return bcontainer->ops->set_dirty_page_tracking(bcontainer, 
>>>>> start);
>>>>> +    return bcontainer->ops->set_dirty_page_tracking(bcontainer, 
>>>>> start, errp);
>>>>>   }
>>>>>
>>>>>   int vfio_container_query_dirty_bitmap(const VFIOContainerBase 
>>>>> *bcontainer,
>>>>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>>>>> index 
>>>>> 77bdec276ec49cb9cd767c0de42ec801b4421572..c35221fbe7dc5453050f97cd186fc958e24f28f7 
>>>>> 100644
>>>>> --- a/hw/vfio/container.c
>>>>> +++ b/hw/vfio/container.c
>>>>> @@ -209,7 +209,7 @@ static int vfio_legacy_dma_map(const 
>>>>> VFIOContainerBase *bcontainer, hwaddr iova,
>>>>>
>>>>>   static int
>>>>>   vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase 
>>>>> *bcontainer,
>>>>> -                                    bool start)
>>>>> +                                    bool start, Error **errp)
>>>>>   {
>>>>>       const VFIOContainer *container = container_of(bcontainer, 
>>>>> VFIOContainer,
>>>>> bcontainer);
>>>>> @@ -227,8 +227,8 @@ vfio_legacy_set_dirty_page_tracking(const 
>>>>> VFIOContainerBase *bcontainer,
>>>>>       ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, &dirty);
>>>>>       if (ret) {
>>>>>           ret = -errno;
>>>>> -        error_report("Failed to set dirty tracking flag 0x%x 
>>>>> errno: %d",
>>>>> -                     dirty.flags, errno);
>>>>> +        error_setg_errno(errp, errno, "Failed to set dirty 
>>>>> tracking flag 0x%x",
>>>>> +                         dirty.flags);
>>>>>       }
>>>>>
>>>>>       return ret;
>>>>> -- 
>>>>> 2.45.0
>>>>>
>>>>
>>>
>>
>
Cédric Le Goater May 15, 2024, 12:36 p.m. UTC | #6
On 5/15/24 14:29, Avihai Horon wrote:
> 
> On 15/05/2024 15:25, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 5/15/24 14:17, Avihai Horon wrote:
>>>
>>> On 13/05/2024 19:27, Cédric Le Goater wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On 5/13/24 15:03, Avihai Horon wrote:
>>>>> Hi Cedric,
>>>>>
>>>>> On 06/05/2024 12:20, Cédric Le Goater wrote:
>>>>>> External email: Use caution opening links or attachments
>>>>>>
>>>>>>
>>>>>> We will use the Error object to improve error reporting in the
>>>>>> .log_global*() handlers of VFIO. Add documentation while at it.
>>>>>
>>>>> First of all, I think commit 3688fec8923 ("memory: Add Error** argument to .log_global_start() handler") forgot to set errp in vfio_listener_log_global_start() in case of error.
>>>>
>>>> yes. This is unfortunate. There has been a few respins, the series
>>>> was split and I was hoping to upstream this part sooner. My bad.
>>>>
>>>>> This causes a null pointer de-reference if DPT start fails.
>>>>> Maybe add a fix for that in the beginning of this series, or as a stand-alone fix?
>>>>
>>>> Since it is fixed by patch 1+2 of this series, we should be fine ?
>>>
>>> A fix could be useful to be backported to QEMU 9.0, no?
>>
>> These changes were only merged in 9.1 with the migration PR.
>> Am I missing something ?
> 
> Oh, my bad. I thought they were merged in 9.0.
> So patches 1+2 should cover it.

yeah. I still would like to merge them asap, with your little series
possibly, the one adding the event, plus the 2 cleanup series from
ZhenZhong. I will update vfio-next when they are sufficiently reviewed.


Thanks,

C.



> 
> Thanks.
> 
>>
>> Thanks,
>>
>> C.
>>
>>
>>>
>>>>
>>>>> Back to this patch, personally, I found the split of patch #1 and #2 a bit confusing.
>>>>> Maybe consider squashing patch #1 and #2 so container based and device based DPT start/stop are changed in the same patch? Like you did in patch #8?
>>>>> Whatever you think is better.
>>>>
>>>> ok. Let's see how v5 goes. I might just send a PR with it if
>>>> no major changes are requested.
>>>>
>>>>>
>>>>> In any case:
>>>>> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> C.
>>>>
>>>>
>>>>>
>>>>>>
>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>>>>> ---
>>>>>>
>>>>>>   Changes in v5:
>>>>>>
>>>>>>   - Fixed typo in set_dirty_page_tracking documentation
>>>>>>
>>>>>>   include/hw/vfio/vfio-container-base.h | 18 ++++++++++++++++--
>>>>>>   hw/vfio/common.c                      |  4 ++--
>>>>>>   hw/vfio/container-base.c              |  4 ++--
>>>>>>   hw/vfio/container.c                   |  6 +++---
>>>>>>   4 files changed, 23 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
>>>>>> index 3582d5f97a37877b2adfc0d0b06996c82403f8b7..326ceea52a2030eec9dad289a9845866c4a8c090 100644
>>>>>> --- a/include/hw/vfio/vfio-container-base.h
>>>>>> +++ b/include/hw/vfio/vfio-container-base.h
>>>>>> @@ -82,7 +82,7 @@ int vfio_container_add_section_window(VFIOContainerBase *bcontainer,
>>>>>>   void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
>>>>>> MemoryRegionSection *section);
>>>>>>   int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
>>>>>> -                                           bool start);
>>>>>> +                                           bool start, Error **errp);
>>>>>>   int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>>>>>>                                         VFIOBitmap *vbmap,
>>>>>>                                         hwaddr iova, hwaddr size);
>>>>>> @@ -121,9 +121,23 @@ struct VFIOIOMMUClass {
>>>>>>       int (*attach_device)(const char *name, VFIODevice *vbasedev,
>>>>>>                            AddressSpace *as, Error **errp);
>>>>>>       void (*detach_device)(VFIODevice *vbasedev);
>>>>>> +
>>>>>>       /* migration feature */
>>>>>> +
>>>>>> +    /**
>>>>>> +     * @set_dirty_page_tracking
>>>>>> +     *
>>>>>> +     * Start or stop dirty pages tracking on VFIO container
>>>>>> +     *
>>>>>> +     * @bcontainer: #VFIOContainerBase on which to de/activate dirty
>>>>>> +     *              page tracking
>>>>>> +     * @start: indicates whether to start or stop dirty pages tracking
>>>>>> +     * @errp: pointer to Error*, to store an error if it happens.
>>>>>> +     *
>>>>>> +     * Returns zero to indicate success and negative for error
>>>>>> +     */
>>>>>>       int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
>>>>>> -                                   bool start);
>>>>>> +                                   bool start, Error **errp);
>>>>>>       int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer,
>>>>>>                                 VFIOBitmap *vbmap,
>>>>>>                                 hwaddr iova, hwaddr size);
>>>>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>>>>> index 8f9cbdc0264044ce587877a7d19d14b28527291b..485e53916491f1164d29e739fb7106c0c77df737 100644
>>>>>> --- a/hw/vfio/common.c
>>>>>> +++ b/hw/vfio/common.c
>>>>>> @@ -1076,7 +1076,7 @@ static bool vfio_listener_log_global_start(MemoryListener *listener,
>>>>>>       if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>>>>>>           ret = vfio_devices_dma_logging_start(bcontainer);
>>>>>>       } else {
>>>>>> -        ret = vfio_container_set_dirty_page_tracking(bcontainer, true);
>>>>>> +        ret = vfio_container_set_dirty_page_tracking(bcontainer, true, NULL);
>>>>>>       }
>>>>>>
>>>>>>       if (ret) {
>>>>>> @@ -1096,7 +1096,7 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
>>>>>>       if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>>>>>>           vfio_devices_dma_logging_stop(bcontainer);
>>>>>>       } else {
>>>>>> -        ret = vfio_container_set_dirty_page_tracking(bcontainer, false);
>>>>>> +        ret = vfio_container_set_dirty_page_tracking(bcontainer, false, NULL);
>>>>>>       }
>>>>>>
>>>>>>       if (ret) {
>>>>>> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
>>>>>> index 913ae49077c4f09b7b27517c1231cfbe4befb7fb..7c0764121d24b02b6c4e66e368d7dff78a6d65aa 100644
>>>>>> --- a/hw/vfio/container-base.c
>>>>>> +++ b/hw/vfio/container-base.c
>>>>>> @@ -53,14 +53,14 @@ void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
>>>>>>   }
>>>>>>
>>>>>>   int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
>>>>>> -                                           bool start)
>>>>>> +                                           bool start, Error **errp)
>>>>>>   {
>>>>>>       if (!bcontainer->dirty_pages_supported) {
>>>>>>           return 0;
>>>>>>       }
>>>>>>
>>>>>> g_assert(bcontainer->ops->set_dirty_page_tracking);
>>>>>> -    return bcontainer->ops->set_dirty_page_tracking(bcontainer, start);
>>>>>> +    return bcontainer->ops->set_dirty_page_tracking(bcontainer, start, errp);
>>>>>>   }
>>>>>>
>>>>>>   int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>>>>>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>>>>>> index 77bdec276ec49cb9cd767c0de42ec801b4421572..c35221fbe7dc5453050f97cd186fc958e24f28f7 100644
>>>>>> --- a/hw/vfio/container.c
>>>>>> +++ b/hw/vfio/container.c
>>>>>> @@ -209,7 +209,7 @@ static int vfio_legacy_dma_map(const VFIOContainerBase *bcontainer, hwaddr iova,
>>>>>>
>>>>>>   static int
>>>>>>   vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
>>>>>> -                                    bool start)
>>>>>> +                                    bool start, Error **errp)
>>>>>>   {
>>>>>>       const VFIOContainer *container = container_of(bcontainer, VFIOContainer,
>>>>>> bcontainer);
>>>>>> @@ -227,8 +227,8 @@ vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
>>>>>>       ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, &dirty);
>>>>>>       if (ret) {
>>>>>>           ret = -errno;
>>>>>> -        error_report("Failed to set dirty tracking flag 0x%x errno: %d",
>>>>>> -                     dirty.flags, errno);
>>>>>> +        error_setg_errno(errp, errno, "Failed to set dirty tracking flag 0x%x",
>>>>>> +                         dirty.flags);
>>>>>>       }
>>>>>>
>>>>>>       return ret;
>>>>>> -- 
>>>>>> 2.45.0
>>>>>>
>>>>>
>>>>
>>>
>>
>
Avihai Horon May 15, 2024, 12:49 p.m. UTC | #7
On 15/05/2024 15:36, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> On 5/15/24 14:29, Avihai Horon wrote:
>>
>> On 15/05/2024 15:25, Cédric Le Goater wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 5/15/24 14:17, Avihai Horon wrote:
>>>>
>>>> On 13/05/2024 19:27, Cédric Le Goater wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> On 5/13/24 15:03, Avihai Horon wrote:
>>>>>> Hi Cedric,
>>>>>>
>>>>>> On 06/05/2024 12:20, Cédric Le Goater wrote:
>>>>>>> External email: Use caution opening links or attachments
>>>>>>>
>>>>>>>
>>>>>>> We will use the Error object to improve error reporting in the
>>>>>>> .log_global*() handlers of VFIO. Add documentation while at it.
>>>>>>
>>>>>> First of all, I think commit 3688fec8923 ("memory: Add Error** 
>>>>>> argument to .log_global_start() handler") forgot to set errp in 
>>>>>> vfio_listener_log_global_start() in case of error.
>>>>>
>>>>> yes. This is unfortunate. There has been a few respins, the series
>>>>> was split and I was hoping to upstream this part sooner. My bad.
>>>>>
>>>>>> This causes a null pointer de-reference if DPT start fails.
>>>>>> Maybe add a fix for that in the beginning of this series, or as a 
>>>>>> stand-alone fix?
>>>>>
>>>>> Since it is fixed by patch 1+2 of this series, we should be fine ?
>>>>
>>>> A fix could be useful to be backported to QEMU 9.0, no?
>>>
>>> These changes were only merged in 9.1 with the migration PR.
>>> Am I missing something ?
>>
>> Oh, my bad. I thought they were merged in 9.0.
>> So patches 1+2 should cover it.
>
> yeah. I still would like to merge them asap, with your little series
> possibly, the one adding the event, plus the 2 cleanup series from
> ZhenZhong. I will update vfio-next when they are sufficiently reviewed.
>
Sure, I am going to post v3 of my series shortly and then review your v6.
diff mbox series

Patch

diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
index 3582d5f97a37877b2adfc0d0b06996c82403f8b7..326ceea52a2030eec9dad289a9845866c4a8c090 100644
--- a/include/hw/vfio/vfio-container-base.h
+++ b/include/hw/vfio/vfio-container-base.h
@@ -82,7 +82,7 @@  int vfio_container_add_section_window(VFIOContainerBase *bcontainer,
 void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
                                        MemoryRegionSection *section);
 int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
-                                           bool start);
+                                           bool start, Error **errp);
 int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
                                       VFIOBitmap *vbmap,
                                       hwaddr iova, hwaddr size);
@@ -121,9 +121,23 @@  struct VFIOIOMMUClass {
     int (*attach_device)(const char *name, VFIODevice *vbasedev,
                          AddressSpace *as, Error **errp);
     void (*detach_device)(VFIODevice *vbasedev);
+
     /* migration feature */
+
+    /**
+     * @set_dirty_page_tracking
+     *
+     * Start or stop dirty pages tracking on VFIO container
+     *
+     * @bcontainer: #VFIOContainerBase on which to de/activate dirty
+     *              page tracking
+     * @start: indicates whether to start or stop dirty pages tracking
+     * @errp: pointer to Error*, to store an error if it happens.
+     *
+     * Returns zero to indicate success and negative for error
+     */
     int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
-                                   bool start);
+                                   bool start, Error **errp);
     int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer,
                               VFIOBitmap *vbmap,
                               hwaddr iova, hwaddr size);
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 8f9cbdc0264044ce587877a7d19d14b28527291b..485e53916491f1164d29e739fb7106c0c77df737 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1076,7 +1076,7 @@  static bool vfio_listener_log_global_start(MemoryListener *listener,
     if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
         ret = vfio_devices_dma_logging_start(bcontainer);
     } else {
-        ret = vfio_container_set_dirty_page_tracking(bcontainer, true);
+        ret = vfio_container_set_dirty_page_tracking(bcontainer, true, NULL);
     }
 
     if (ret) {
@@ -1096,7 +1096,7 @@  static void vfio_listener_log_global_stop(MemoryListener *listener)
     if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
         vfio_devices_dma_logging_stop(bcontainer);
     } else {
-        ret = vfio_container_set_dirty_page_tracking(bcontainer, false);
+        ret = vfio_container_set_dirty_page_tracking(bcontainer, false, NULL);
     }
 
     if (ret) {
diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
index 913ae49077c4f09b7b27517c1231cfbe4befb7fb..7c0764121d24b02b6c4e66e368d7dff78a6d65aa 100644
--- a/hw/vfio/container-base.c
+++ b/hw/vfio/container-base.c
@@ -53,14 +53,14 @@  void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
 }
 
 int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
-                                           bool start)
+                                           bool start, Error **errp)
 {
     if (!bcontainer->dirty_pages_supported) {
         return 0;
     }
 
     g_assert(bcontainer->ops->set_dirty_page_tracking);
-    return bcontainer->ops->set_dirty_page_tracking(bcontainer, start);
+    return bcontainer->ops->set_dirty_page_tracking(bcontainer, start, errp);
 }
 
 int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 77bdec276ec49cb9cd767c0de42ec801b4421572..c35221fbe7dc5453050f97cd186fc958e24f28f7 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -209,7 +209,7 @@  static int vfio_legacy_dma_map(const VFIOContainerBase *bcontainer, hwaddr iova,
 
 static int
 vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
-                                    bool start)
+                                    bool start, Error **errp)
 {
     const VFIOContainer *container = container_of(bcontainer, VFIOContainer,
                                                   bcontainer);
@@ -227,8 +227,8 @@  vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
     ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, &dirty);
     if (ret) {
         ret = -errno;
-        error_report("Failed to set dirty tracking flag 0x%x errno: %d",
-                     dirty.flags, errno);
+        error_setg_errno(errp, errno, "Failed to set dirty tracking flag 0x%x",
+                         dirty.flags);
     }
 
     return ret;