1. Patchew
  2. QEMU
  3. [PATCH v5 00/10] Enable shared device assignment
  4. View patch

[PATCH v5 08/10] memory: Change NotifyRamDiscard() definition to return the result

Chenyi Qiang posted 10 patches 5 months, 4 weeks ago
There is a newer version of this series
[PATCH v5 08/10] memory: Change NotifyRamDiscard() definition to return the result
Posted by Chenyi Qiang 5 months, 4 weeks ago 
So that the caller can check the result of NotifyRamDiscard() handler if
the operation fails.

Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
---
Changes in v5:
    - Revert to use of NotifyRamDiscard()

Changes in v4:
    - Newly added.
---
 hw/vfio/listener.c           | 6 ++++--
 include/system/memory.h      | 4 ++--
 system/ram-block-attribute.c | 3 +--
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
index bfacb3d8d9..06454e0584 100644
--- a/hw/vfio/listener.c
+++ b/hw/vfio/listener.c
@@ -190,8 +190,8 @@ out:
     rcu_read_unlock();
 }
 
-static void vfio_ram_discard_notify_discard(RamDiscardListener *rdl,
-                                            MemoryRegionSection *section)
+static int vfio_ram_discard_notify_discard(RamDiscardListener *rdl,
+                                           MemoryRegionSection *section)
 {
     VFIORamDiscardListener *vrdl = container_of(rdl, VFIORamDiscardListener,
                                                 listener);
@@ -206,6 +206,8 @@ static void vfio_ram_discard_notify_discard(RamDiscardListener *rdl,
         error_report("%s: vfio_container_dma_unmap() failed: %s", __func__,
                      strerror(-ret));
     }
+
+    return ret;
 }
 
 static int vfio_ram_discard_notify_populate(RamDiscardListener *rdl,
diff --git a/include/system/memory.h b/include/system/memory.h
index 83b28551c4..e5155120d9 100644
--- a/include/system/memory.h
+++ b/include/system/memory.h
@@ -518,8 +518,8 @@ struct IOMMUMemoryRegionClass {
 typedef struct RamDiscardListener RamDiscardListener;
 typedef int (*NotifyRamPopulate)(RamDiscardListener *rdl,
                                  MemoryRegionSection *section);
-typedef void (*NotifyRamDiscard)(RamDiscardListener *rdl,
-                                 MemoryRegionSection *section);
+typedef int (*NotifyRamDiscard)(RamDiscardListener *rdl,
+                                MemoryRegionSection *section);
 
 struct RamDiscardListener {
     /*
diff --git a/system/ram-block-attribute.c b/system/ram-block-attribute.c
index f12dd4b881..896c3d7543 100644
--- a/system/ram-block-attribute.c
+++ b/system/ram-block-attribute.c
@@ -66,8 +66,7 @@ static int ram_block_attribute_notify_discard_cb(MemoryRegionSection *section,
 {
     RamDiscardListener *rdl = arg;
 
-    rdl->notify_discard(rdl, section);
-    return 0;
+    return rdl->notify_discard(rdl, section);
 }
 
 static int
-- 
2.43.5
Re: [PATCH v5 08/10] memory: Change NotifyRamDiscard() definition to return the result
Posted by Cédric Le Goater 5 months, 3 weeks ago 
On 5/20/25 12:28, Chenyi Qiang wrote:
> So that the caller can check the result of NotifyRamDiscard() handler if
> the operation fails.
> 
> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
> ---
> Changes in v5:
>      - Revert to use of NotifyRamDiscard()
> 
> Changes in v4:
>      - Newly added.
> ---
>   hw/vfio/listener.c           | 6 ++++--
>   include/system/memory.h      | 4 ++--
>   system/ram-block-attribute.c | 3 +--
>   3 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
> index bfacb3d8d9..06454e0584 100644
> --- a/hw/vfio/listener.c
> +++ b/hw/vfio/listener.c
> @@ -190,8 +190,8 @@ out:
>       rcu_read_unlock();
>   }
>   
> -static void vfio_ram_discard_notify_discard(RamDiscardListener *rdl,
> -                                            MemoryRegionSection *section)
> +static int vfio_ram_discard_notify_discard(RamDiscardListener *rdl,
> +                                           MemoryRegionSection *section)
>   {
>       VFIORamDiscardListener *vrdl = container_of(rdl, VFIORamDiscardListener,
>                                                   listener);
> @@ -206,6 +206,8 @@ static void vfio_ram_discard_notify_discard(RamDiscardListener *rdl,
>           error_report("%s: vfio_container_dma_unmap() failed: %s", __func__,
>                        strerror(-ret));
>       }
> +
> +    return ret;
>   }

vfio_ram_discard_notify_populate() should also be modified
to return this value.


Thanks,

C.



>   static int vfio_ram_discard_notify_populate(RamDiscardListener *rdl,
> diff --git a/include/system/memory.h b/include/system/memory.h
> index 83b28551c4..e5155120d9 100644
> --- a/include/system/memory.h
> +++ b/include/system/memory.h
> @@ -518,8 +518,8 @@ struct IOMMUMemoryRegionClass {
>   typedef struct RamDiscardListener RamDiscardListener;
>   typedef int (*NotifyRamPopulate)(RamDiscardListener *rdl,
>                                    MemoryRegionSection *section);
> -typedef void (*NotifyRamDiscard)(RamDiscardListener *rdl,
> -                                 MemoryRegionSection *section);
> +typedef int (*NotifyRamDiscard)(RamDiscardListener *rdl,
> +                                MemoryRegionSection *section);
>   
>   struct RamDiscardListener {
>       /*
> diff --git a/system/ram-block-attribute.c b/system/ram-block-attribute.c
> index f12dd4b881..896c3d7543 100644
> --- a/system/ram-block-attribute.c
> +++ b/system/ram-block-attribute.c
> @@ -66,8 +66,7 @@ static int ram_block_attribute_notify_discard_cb(MemoryRegionSection *section,
>   {
>       RamDiscardListener *rdl = arg;
>   
> -    rdl->notify_discard(rdl, section);
> -    return 0;
> +    return rdl->notify_discard(rdl, section);
>   }
>   
>   static int
Re: [PATCH v5 08/10] memory: Change NotifyRamDiscard() definition to return the result
Posted by Cédric Le Goater 5 months, 3 weeks ago 
On 5/26/25 12:36, Cédric Le Goater wrote:
> On 5/20/25 12:28, Chenyi Qiang wrote:
>> So that the caller can check the result of NotifyRamDiscard() handler if
>> the operation fails.
>>
>> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
>> ---
>> Changes in v5:
>>      - Revert to use of NotifyRamDiscard()
>>
>> Changes in v4:
>>      - Newly added.
>> ---
>>   hw/vfio/listener.c           | 6 ++++--
>>   include/system/memory.h      | 4 ++--
>>   system/ram-block-attribute.c | 3 +--
>>   3 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
>> index bfacb3d8d9..06454e0584 100644
>> --- a/hw/vfio/listener.c
>> +++ b/hw/vfio/listener.c
>> @@ -190,8 +190,8 @@ out:
>>       rcu_read_unlock();
>>   }
>> -static void vfio_ram_discard_notify_discard(RamDiscardListener *rdl,
>> -                                            MemoryRegionSection *section)
>> +static int vfio_ram_discard_notify_discard(RamDiscardListener *rdl,
>> +                                           MemoryRegionSection *section)
>>   {
>>       VFIORamDiscardListener *vrdl = container_of(rdl, VFIORamDiscardListener,
>>                                                   listener);
>> @@ -206,6 +206,8 @@ static void vfio_ram_discard_notify_discard(RamDiscardListener *rdl,
>>           error_report("%s: vfio_container_dma_unmap() failed: %s", __func__,
>>                        strerror(-ret));
>>       }
>> +
>> +    return ret;
>>   }
> 
> vfio_ram_discard_notify_populate() should also be modified
> to return this value.

Nope. It should not. This is a rollback path in case of error. All good.

Thanks,

C.
Re: [PATCH v5 08/10] memory: Change NotifyRamDiscard() definition to return the result
Posted by Chenyi Qiang 5 months, 3 weeks ago 

On 5/26/2025 8:44 PM, Cédric Le Goater wrote:
> On 5/26/25 12:36, Cédric Le Goater wrote:
>> On 5/20/25 12:28, Chenyi Qiang wrote:
>>> So that the caller can check the result of NotifyRamDiscard() handler if
>>> the operation fails.
>>>
>>> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
>>> ---
>>> Changes in v5:
>>>      - Revert to use of NotifyRamDiscard()
>>>
>>> Changes in v4:
>>>      - Newly added.
>>> ---
>>>   hw/vfio/listener.c           | 6 ++++--
>>>   include/system/memory.h      | 4 ++--
>>>   system/ram-block-attribute.c | 3 +--
>>>   3 files changed, 7 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
>>> index bfacb3d8d9..06454e0584 100644
>>> --- a/hw/vfio/listener.c
>>> +++ b/hw/vfio/listener.c
>>> @@ -190,8 +190,8 @@ out:
>>>       rcu_read_unlock();
>>>   }
>>> -static void vfio_ram_discard_notify_discard(RamDiscardListener *rdl,
>>> -                                            MemoryRegionSection
>>> *section)
>>> +static int vfio_ram_discard_notify_discard(RamDiscardListener *rdl,
>>> +                                           MemoryRegionSection
>>> *section)
>>>   {
>>>       VFIORamDiscardListener *vrdl = container_of(rdl,
>>> VFIORamDiscardListener,
>>>                                                   listener);
>>> @@ -206,6 +206,8 @@ static void
>>> vfio_ram_discard_notify_discard(RamDiscardListener *rdl,
>>>           error_report("%s: vfio_container_dma_unmap() failed: %s",
>>> __func__,
>>>                        strerror(-ret));
>>>       }
>>> +
>>> +    return ret;
>>>   }
>>
>> vfio_ram_discard_notify_populate() should also be modified
>> to return this value.
> 
> Nope. It should not. This is a rollback path in case of error. All good.

Thanks for your review! Anyway, according to the discussion in patch
#10, I'll revert this patch in next version, since it is a future work
to consider the failure case of notifying discard.

> 
> Thanks,
> 
> C.
>
Re: [PATCH v5 08/10] memory: Change NotifyRamDiscard() definition to return the result
Posted by Philippe Mathieu-Daudé 5 months, 3 weeks ago 
On 20/5/25 12:28, Chenyi Qiang wrote:
> So that the caller can check the result of NotifyRamDiscard() handler if
> the operation fails.
> 
> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
> ---
> Changes in v5:
>      - Revert to use of NotifyRamDiscard()
> 
> Changes in v4:
>      - Newly added.
> ---
>   hw/vfio/listener.c           | 6 ++++--
>   include/system/memory.h      | 4 ++--
>   system/ram-block-attribute.c | 3 +--
>   3 files changed, 7 insertions(+), 6 deletions(-)


> diff --git a/include/system/memory.h b/include/system/memory.h
> index 83b28551c4..e5155120d9 100644
> --- a/include/system/memory.h
> +++ b/include/system/memory.h
> @@ -518,8 +518,8 @@ struct IOMMUMemoryRegionClass {
>   typedef struct RamDiscardListener RamDiscardListener;
>   typedef int (*NotifyRamPopulate)(RamDiscardListener *rdl,
>                                    MemoryRegionSection *section);
> -typedef void (*NotifyRamDiscard)(RamDiscardListener *rdl,
> -                                 MemoryRegionSection *section);
> +typedef int (*NotifyRamDiscard)(RamDiscardListener *rdl,
> +                                MemoryRegionSection *section);

Please document the return value.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.