[PATCH 0/3] sysemu/accel: Simplify sysemu/xen.h

[PATCH 0/3] sysemu/accel: Simplify sysemu/xen.h

[Philippe Mathieu-Daudé]

Trivial cleanups which simplify "sysemu/xen.h".

Philippe Mathieu-Daudé (3):
  sysemu/xen: Remove unuseful CONFIG_USER_ONLY header guard
  sysemu/xen: Remove unreachable xen_ram_alloc() code
  sysemu/xen: Allow elision of xen_hvm_modified_memory()

 include/exec/ram_addr.h |  8 ++++++--
 include/sysemu/xen.h    | 24 +++---------------------
 2 files changed, 9 insertions(+), 23 deletions(-)

-- 
2.41.0

[PATCH 1/3] sysemu/xen: Remove unuseful CONFIG_USER_ONLY header guard

[Philippe Mathieu-Daudé]

In commit da278d58a0 ("accel: Move Xen accelerator code under
accel/xen/") we used include/sysemu/kvm.h as a template for
include/sysemu/xen.h, using the CONFIG_USER_ONLY header guard.
In retrospective, it is not used / useful, so remove it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/sysemu/xen.h | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/include/sysemu/xen.h b/include/sysemu/xen.h
index bc13ad5692..9b2d0b21ff 100644
--- a/include/sysemu/xen.h
+++ b/include/sysemu/xen.h
@@ -26,16 +26,14 @@ extern bool xen_allowed;
 
 #define xen_enabled()           (xen_allowed)
 
-#ifndef CONFIG_USER_ONLY
 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);
-#endif
 
 #else /* !CONFIG_XEN_IS_POSSIBLE */
 
 #define xen_enabled() 0
-#ifndef CONFIG_USER_ONLY
+
 static inline void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t length)
 {
     /* nothing */
@@ -45,7 +43,6 @@ static inline void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
 {
     g_assert_not_reached();
 }
-#endif
 
 #endif /* CONFIG_XEN_IS_POSSIBLE */
 
-- 
2.41.0

[PATCH 2/3] sysemu/xen: Remove unreachable xen_ram_alloc() code

[Philippe Mathieu-Daudé]

The xen_ram_alloc() call in softmmu/physmem.c is guarded
by checking for xen_enabled(), which evaluate to 'false'
when XEN is not built in. The compiler elide the function
call, and thus the inlined function is not used. Remove it.

Inspired-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/sysemu/xen.h | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/include/sysemu/xen.h b/include/sysemu/xen.h
index 9b2d0b21ff..1f797a9abe 100644
--- a/include/sysemu/xen.h
+++ b/include/sysemu/xen.h
@@ -27,8 +27,6 @@ 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 */
 
@@ -38,12 +36,10 @@ 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_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
+                   struct MemoryRegion *mr, Error **errp);
+
 #endif
-- 
2.41.0

[PATCH 3/3] sysemu/xen: Allow elision of xen_hvm_modified_memory()

[Philippe Mathieu-Daudé]

Call xen_enabled() before xen_hvm_modified_memory() to let
the compiler elide its call.

Have xen_enabled() return a boolean to match its declaration
in the CONFIG_XEN_IS_POSSIBLE case.

Suggested-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/exec/ram_addr.h |  8 ++++++--
 include/sysemu/xen.h    | 15 ++-------------
 2 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 9f2e3893f5..66e849ac4e 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -330,7 +330,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);
+    }
 }
 
 #if !defined(_WIN32)
@@ -406,7 +408,9 @@ uint64_t cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
             }
         }
 
-        xen_hvm_modified_memory(start, pages << TARGET_PAGE_BITS);
+        if (xen_enabled()) {
+            xen_hvm_modified_memory(start, pages << TARGET_PAGE_BITS);
+        }
     } else {
         uint8_t clients = tcg_enabled() ? DIRTY_CLIENTS_ALL : DIRTY_CLIENTS_NOCODE;
 
diff --git a/include/sysemu/xen.h b/include/sysemu/xen.h
index 1f797a9abe..d84a5f3551 100644
--- a/include/sysemu/xen.h
+++ b/include/sysemu/xen.h
@@ -21,24 +21,13 @@
 #endif
 
 #ifdef CONFIG_XEN_IS_POSSIBLE
-
 extern bool xen_allowed;
-
 #define xen_enabled()           (xen_allowed)
-
-void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t length);
-
 #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 */
-}
-
+#define xen_enabled()           false
 #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,
                    struct MemoryRegion *mr, Error **errp);
 
-- 
2.41.0

Re: [PATCH 3/3] sysemu/xen: Allow elision of xen_hvm_modified_memory()

[David Hildenbrand]

On 05.09.23 14:21, Philippe Mathieu-Daudé wrote:
> Call xen_enabled() before xen_hvm_modified_memory() to let
> the compiler elide its call.
> 
> Have xen_enabled() return a boolean to match its declaration
> in the CONFIG_XEN_IS_POSSIBLE case.
> 
> Suggested-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 0/3] sysemu/accel: Simplify sysemu/xen.h

[Philippe Mathieu-Daudé]

On 5/9/23 14:21, Philippe Mathieu-Daudé wrote:
> Trivial cleanups which simplify "sysemu/xen.h".
> 
> Philippe Mathieu-Daudé (3):
>    sysemu/xen: Remove unuseful CONFIG_USER_ONLY header guard
>    sysemu/xen: Remove unreachable xen_ram_alloc() code
>    sysemu/xen: Allow elision of xen_hvm_modified_memory()
> 
>   include/exec/ram_addr.h |  8 ++++++--
>   include/sysemu/xen.h    | 24 +++---------------------
>   2 files changed, 9 insertions(+), 23 deletions(-)
> 

ping for trivial patches 1 & 2?

Re: [PATCH 2/3] sysemu/xen: Remove unreachable xen_ram_alloc() code

[Michael Tokarev]

05.09.2023 15:21, Philippe Mathieu-Daudé wrote:
> The xen_ram_alloc() call in softmmu/physmem.c is guarded
> by checking for xen_enabled(), which evaluate to 'false'
> when XEN is not built in. The compiler elide the function
> call, and thus the inlined function is not used. Remove it.

I still don't think it is a good way to just eliminate the
function (stub) in a hope compiler will elide the call.  It's
definitely not guaranteed by any standard, and compiler itself
can produce varying results (eg building with -O0 to make gdb
debugging easier).

static inline function costs nothing but keeps whole thing
manageable. IMHO anyway.

/mjt

> Inspired-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/sysemu/xen.h | 10 +++-------
>   1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/include/sysemu/xen.h b/include/sysemu/xen.h
> index 9b2d0b21ff..1f797a9abe 100644
> --- a/include/sysemu/xen.h
> +++ b/include/sysemu/xen.h
> @@ -27,8 +27,6 @@ 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 */
>   
> @@ -38,12 +36,10 @@ 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_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
> +                   struct MemoryRegion *mr, Error **errp);
> +
>   #endif