Re: [PATCH v6 1/9] vfio: Add Error** argument to .set_dirty_page_tracking() handler

2024-05-15 Thread Cédric Le Goater 

On 5/15/24 08:40, Eric Auger wrote:

Hi Cédric,

On 5/14/24 17:31, Cédric Le Goater wrote:

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é 
Reviewed-by: Avihai Horon 
Signed-off-by: Cédric Le Goater 
---
  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);

I am a bit confused now wrt [PATCH v2 03/11] vfio: Make
VFIOIOMMUClass::attach_device() and its wrapper return bool & co

Shall we return a bool or a int?


It would be better to follow the best practices described in qapi/error.h.

Zhenzhong excluded some files from his cleanup series to avoid conflicts
with this series of mine. And indeed, I would prefer to merge this one
first. It should be addressed later.




Looking at ./include/qapi/error.h I have not found any stringent requirement

  * - Whenever practical, also return a value that indicates success /
  *   failure.  This can make the error checking more concise, and can
  *   avoid useless error object creation and destruction.  Note that
  *   we still have many functions returning void.  We recommend
  *   • bool-valued functions return true on success / false on failure,
  *   • pointer-valued functions return non-null / null pointer, and
  *   • integer-valued functions return non-negative / negative.




  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

Re: [PATCH v6 1/9] vfio: Add Error** argument to .set_dirty_page_tracking() handler

2024-05-15 Thread Eric Auger 
Hi Cédric,

On 5/14/24 17:31, Cédric Le Goater wrote:
> 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é 
> Reviewed-by: Avihai Horon 
> Signed-off-by: Cédric Le Goater 
> ---
>  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);
I am a bit confused now wrt [PATCH v2 03/11] vfio: Make
VFIOIOMMUClass::attach_device() and its wrapper return bool & co

Shall we return a bool or a int?

Looking at ./include/qapi/error.h I have not found any stringent requirement

 * - Whenever practical, also return a value that indicates success /
 *   failure.  This can make the error checking more concise, and can
 *   avoid useless error object creation and destruction.  Note that
 *   we still have many functions returning void.  We recommend
 *   • bool-valued functions return true on success / false on failure,
 *   • pointer-valued functions return non-null / null pointer, and
 *   • integer-valued functions return non-negative / negative.



>  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)
> +