Re: [PATCH v4 14/17] xen: Add xen_mr_is_memory()
On Mon, May 6, 2024 at 11:59 AM Philippe Mathieu-Daudé
wrote:
>
> On 2/5/24 09:26, David Hildenbrand wrote:
> > On 30.04.24 18:49, Edgar E. Iglesias wrote:
> >> From: "Edgar E. Iglesias"
> >>
> >> Add xen_mr_is_memory() to abstract away tests for the
> >> xen_memory MR.
> >>
> >> Signed-off-by: Edgar E. Iglesias
> >> ---
> >
> > [...]
> >
> >> #endif
> >> diff --git a/system/physmem.c b/system/physmem.c
> >> index ad7a8c7d95..1a5ffcba2a 100644
> >> --- a/system/physmem.c
> >> +++ b/system/physmem.c
> >> @@ -2227,7 +2227,7 @@ static void *qemu_ram_ptr_length(RAMBlock
> >> *block, ram_addr_t addr,
> >>* because we don't want to map the entire memory in QEMU.
> >>* In that case just map the requested area.
> >>*/
> >> -if (block->offset == 0) {
> >> +if (xen_mr_is_memory(block->mr)) {
> >> return xen_map_cache(block->mr, addr, len, lock, lock,
> >>is_write);
> >> }
> >
> > I'd have moved that into a separate patch, because this is not a simple
> > abstraction here.
>
> Yes please, maybe using Stefano review comment in the description.
>
Thanks, for v5 I've split out this particular change into a separate patch:
softmmu: Replace check for RAMBlock offset 0 with xen_mr_is_memory
For xen, when checking for the first RAM (xen_memory), use
xen_mr_is_memory() rather than checking for a RAMBlock with
offset 0.
All Xen machines create xen_memory first so this has no
functional change for existing machines.
Signed-off-by: Edgar E. Iglesias
diff --git a/system/physmem.c b/system/physmem.c
index ad7a8c7d95..1a5ffcba2a 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -2227,7 +2227,7 @@ static void *qemu_ram_ptr_length(RAMBlock
*block, ram_addr_t addr,
* because we don't want to map the entire memory in QEMU.
* In that case just map the requested area.
*/
-if (block->offset == 0) {
+if (xen_mr_is_memory(block->mr)) {
return xen_map_cache(block->mr, addr, len, lock, lock,
is_write);
}
> >
> > Acked-by: David Hildenbrand
> >
>
Re: [PATCH v4 14/17] xen: Add xen_mr_is_memory()
On 2/5/24 09:26, David Hildenbrand wrote:
On 30.04.24 18:49, Edgar E. Iglesias wrote:
From: "Edgar E. Iglesias"
Add xen_mr_is_memory() to abstract away tests for the
xen_memory MR.
Signed-off-by: Edgar E. Iglesias
---
[...]
#endif
diff --git a/system/physmem.c b/system/physmem.c
index ad7a8c7d95..1a5ffcba2a 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -2227,7 +2227,7 @@ static void *qemu_ram_ptr_length(RAMBlock
*block, ram_addr_t addr,
* because we don't want to map the entire memory in QEMU.
* In that case just map the requested area.
*/
- if (block->offset == 0) {
+ if (xen_mr_is_memory(block->mr)) {
return xen_map_cache(block->mr, addr, len, lock, lock,
is_write);
}
I'd have moved that into a separate patch, because this is not a simple
abstraction here.
Yes please, maybe using Stefano review comment in the description.
Acked-by: David Hildenbrand
Re: [PATCH v4 14/17] xen: Add xen_mr_is_memory()
On 30.04.24 18:49, Edgar E. Iglesias wrote:
From: "Edgar E. Iglesias"
Add xen_mr_is_memory() to abstract away tests for the
xen_memory MR.
Signed-off-by: Edgar E. Iglesias
---
[...]
#endif
diff --git a/system/physmem.c b/system/physmem.c
index ad7a8c7d95..1a5ffcba2a 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -2227,7 +2227,7 @@ static void *qemu_ram_ptr_length(RAMBlock *block,
ram_addr_t addr,
* because we don't want to map the entire memory in QEMU.
* In that case just map the requested area.
*/
-if (block->offset == 0) {
+if (xen_mr_is_memory(block->mr)) {
return xen_map_cache(block->mr, addr, len, lock, lock,
is_write);
}
I'd have moved that into a separate patch, because this is not a simple
abstraction here.
Acked-by: David Hildenbrand
--
Cheers,
David / dhildenb
Re: [PATCH v4 14/17] xen: Add xen_mr_is_memory()
On Tue, 30 Apr 2024, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias"
>
> Add xen_mr_is_memory() to abstract away tests for the
> xen_memory MR.
>
> Signed-off-by: Edgar E. Iglesias
There is an important change in this patch below
> ---
> hw/xen/xen-hvm-common.c | 8 +++-
> include/sysemu/xen.h| 8
> system/physmem.c| 2 +-
> 3 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
> index 1627da7398..0267b88d26 100644
> --- a/hw/xen/xen-hvm-common.c
> +++ b/hw/xen/xen-hvm-common.c
> @@ -12,6 +12,12 @@
>
> MemoryRegion xen_memory;
>
> +/* Check for xen memory. */
> +bool xen_mr_is_memory(MemoryRegion *mr)
> +{
> +return mr == _memory;
> +}
> +
> void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr,
> Error **errp)
> {
> @@ -28,7 +34,7 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
> MemoryRegion *mr,
> return;
> }
>
> -if (mr == _memory) {
> +if (xen_mr_is_memory(mr)) {
> return;
> }
>
> diff --git a/include/sysemu/xen.h b/include/sysemu/xen.h
> index 754ec2e6cb..dc72f83bcb 100644
> --- a/include/sysemu/xen.h
> +++ b/include/sysemu/xen.h
> @@ -34,6 +34,8 @@ 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);
>
> +bool xen_mr_is_memory(MemoryRegion *mr);
> +
> #else /* !CONFIG_XEN_IS_POSSIBLE */
>
> #define xen_enabled() 0
> @@ -47,6 +49,12 @@ static inline void xen_ram_alloc(ram_addr_t ram_addr,
> ram_addr_t size,
> g_assert_not_reached();
> }
>
> +static inline bool xen_mr_is_memory(MemoryRegion *mr)
> +{
> +g_assert_not_reached();
> +return false;
> +}
> +
> #endif /* CONFIG_XEN_IS_POSSIBLE */
>
> #endif
> diff --git a/system/physmem.c b/system/physmem.c
> index ad7a8c7d95..1a5ffcba2a 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -2227,7 +2227,7 @@ static void *qemu_ram_ptr_length(RAMBlock *block,
> ram_addr_t addr,
> * because we don't want to map the entire memory in QEMU.
> * In that case just map the requested area.
> */
> -if (block->offset == 0) {
> +if (xen_mr_is_memory(block->mr)) {
This changes this check from block->offset == 0 to block->mr ==
_memory. I think that's correct in all cases (x86 machines, ARM
machines) but I wanted to highlight it.
Reviewed-by: Stefano Stabellini
> return xen_map_cache(block->mr, addr, len, lock, lock,
> is_write);
> }
> --
> 2.40.1
>
[PATCH v4 14/17] xen: Add xen_mr_is_memory()
From: "Edgar E. Iglesias"
Add xen_mr_is_memory() to abstract away tests for the
xen_memory MR.
Signed-off-by: Edgar E. Iglesias
---
hw/xen/xen-hvm-common.c | 8 +++-
include/sysemu/xen.h| 8
system/physmem.c| 2 +-
3 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
index 1627da7398..0267b88d26 100644
--- a/hw/xen/xen-hvm-common.c
+++ b/hw/xen/xen-hvm-common.c
@@ -12,6 +12,12 @@
MemoryRegion xen_memory;
+/* Check for xen memory. */
+bool xen_mr_is_memory(MemoryRegion *mr)
+{
+return mr == _memory;
+}
+
void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr,
Error **errp)
{
@@ -28,7 +34,7 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
MemoryRegion *mr,
return;
}
-if (mr == _memory) {
+if (xen_mr_is_memory(mr)) {
return;
}
diff --git a/include/sysemu/xen.h b/include/sysemu/xen.h
index 754ec2e6cb..dc72f83bcb 100644
--- a/include/sysemu/xen.h
+++ b/include/sysemu/xen.h
@@ -34,6 +34,8 @@ 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);
+bool xen_mr_is_memory(MemoryRegion *mr);
+
#else /* !CONFIG_XEN_IS_POSSIBLE */
#define xen_enabled() 0
@@ -47,6 +49,12 @@ static inline void xen_ram_alloc(ram_addr_t ram_addr,
ram_addr_t size,
g_assert_not_reached();
}
+static inline bool xen_mr_is_memory(MemoryRegion *mr)
+{
+g_assert_not_reached();
+return false;
+}
+
#endif /* CONFIG_XEN_IS_POSSIBLE */
#endif
diff --git a/system/physmem.c b/system/physmem.c
index ad7a8c7d95..1a5ffcba2a 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -2227,7 +2227,7 @@ static void *qemu_ram_ptr_length(RAMBlock *block,
ram_addr_t addr,
* because we don't want to map the entire memory in QEMU.
* In that case just map the requested area.
*/
-if (block->offset == 0) {
+if (xen_mr_is_memory(block->mr)) {
return xen_map_cache(block->mr, addr, len, lock, lock,
is_write);
}
--
2.40.1
