Re: [PATCH v5 11/17] exec/ram_addr: call xen_hvm_modified_memory only if xen is enabled

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 3/17/25 09:23, Philippe Mathieu-Daudé wrote:
> On 17/3/25 17:22, Philippe Mathieu-Daudé wrote:
>> On 17/3/25 17:07, Pierrick Bouvier wrote:
>>> On 3/17/25 08:50, Philippe Mathieu-Daudé wrote:
>>>> On 14/3/25 18:31, Pierrick Bouvier wrote:
>>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>>> ---
>>>>>     include/exec/ram_addr.h | 8 ++++++--
>>>>>     1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
>>>>> index f5d574261a3..92e8708af76 100644
>>>>> --- a/include/exec/ram_addr.h
>>>>> +++ b/include/exec/ram_addr.h
>>>>> @@ -339,7 +339,9 @@ static inline void
>>>>> cpu_physical_memory_set_dirty_range(ram_addr_t start,
>>>>>             }
>>>>>         }
>>>>> -    xen_hvm_modified_memory(start, length);
>>>>> +    if (xen_enabled()) {
>>>>> +        xen_hvm_modified_memory(start, length);
>>>>
>>>> Please remove the stub altogether.
>>>>
>>>
>>> We can eventually ifdef this code under CONFIG_XEN, but it may still
>>> be available or not. The matching stub for xen_hvm_modified_memory()
>>> will assert in case it is reached.
>>>
>>> Which change would you expect precisely?
>>
>> -- >8 --
>> diff --git a/include/system/xen-mapcache.h b/include/system/xen-mapcache.h
>> index b68f196ddd5..bb454a7c96c 100644
>> --- a/include/system/xen-mapcache.h
>> +++ b/include/system/xen-mapcache.h
>> @@ -14,8 +14,6 @@
>>
>>    typedef hwaddr (*phys_offset_to_gaddr_t)(hwaddr phys_offset,
>>                                             ram_addr_t size);
>> -#ifdef CONFIG_XEN_IS_POSSIBLE
>> -
>>    void xen_map_cache_init(phys_offset_to_gaddr_t f,
>>                            void *opaque);
>>    uint8_t *xen_map_cache(MemoryRegion *mr, hwaddr phys_addr, hwaddr size,
>> @@ -28,44 +26,5 @@ void xen_invalidate_map_cache(void);
>>    uint8_t *xen_replace_cache_entry(hwaddr old_phys_addr,
>>                                     hwaddr new_phys_addr,
>>                                     hwaddr size);
>> -#else
>> -
>> -static inline void xen_map_cache_init(phys_offset_to_gaddr_t f,
>> -                                      void *opaque)
>> -{
>> -}
>> -
>> -static inline uint8_t *xen_map_cache(MemoryRegion *mr,
>> -                                     hwaddr phys_addr,
>> -                                     hwaddr size,
>> -                                     ram_addr_t ram_addr_offset,
>> -                                     uint8_t lock,
>> -                                     bool dma,
>> -                                     bool is_write)
>> -{
>> -    abort();
>> -}
>> -
>> -static inline ram_addr_t xen_ram_addr_from_mapcache(void *ptr)
>> -{
>> -    abort();
>> -}
>> -
>> -static inline void xen_invalidate_map_cache_entry(uint8_t *buffer)
>> -{
>> -}
>> -
>> -static inline void xen_invalidate_map_cache(void)
>> -{
>> -}
>> -
>> -static inline uint8_t *xen_replace_cache_entry(hwaddr old_phys_addr,
>> -                                               hwaddr new_phys_addr,
>> -                                               hwaddr size)
>> -{
>> -    abort();
>> -}
>> -
>> -#endif
>>
>>    #endif /* XEN_MAPCACHE_H */
> 
> (sorry, the include/system/xen-mapcache.h change is for the next patch)
> 
>> diff --git a/include/system/xen.h b/include/system/xen.h
>> index 990c19a8ef0..04fe30cca50 100644
>> --- a/include/system/xen.h
>> +++ b/include/system/xen.h
>> @@ -30,25 +30,16 @@ extern bool xen_allowed;
>>
>>    #define xen_enabled()           (xen_allowed)
>>
>> -void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t length);
>> -void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
>> -                   struct MemoryRegion *mr, Error **errp);
>> -
>>    #else /* !CONFIG_XEN_IS_POSSIBLE */
>>
>>    #define xen_enabled() 0
>> -static inline void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t
>> length)
>> -{
>> -    /* nothing */
>> -}
>> -static inline void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
>> -                                 MemoryRegion *mr, Error **errp)
>> -{
>> -    g_assert_not_reached();
>> -}
>>
>>    #endif /* CONFIG_XEN_IS_POSSIBLE */
>>
>> +void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t length);
>> +void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
>> +                   MemoryRegion *mr, Error **errp);
>> +
>>    bool xen_mr_is_memory(MemoryRegion *mr);
>>    bool xen_mr_is_grants(MemoryRegion *mr);
>>    #endif
>> ---
> 

Sounds good!
I will include it in next version.