diff mbox series

[v1,1/2] physmem: Bail out qemu_ram_block_from_host() for invalid ram addrs

Message ID 20240701224421.1432654-2-edgar.iglesias@gmail.com
State New
Headers show
Series xen: mapcache: Fix unmapping of first the entry in a bucket | expand

Commit Message

Edgar E. Iglesias July 1, 2024, 10:44 p.m. UTC 
From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>

Bail out in qemu_ram_block_from_host() when
xen_ram_addr_from_mapcache() does not find an existing
mapping.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
---
 system/physmem.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Alex Bennée July 4, 2024, 10:26 a.m. UTC | #1 
"Edgar E. Iglesias" <edgar.iglesias@gmail.com> writes:

> From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
>
> Bail out in qemu_ram_block_from_host() when
> xen_ram_addr_from_mapcache() does not find an existing
> mapping.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> ---
>  system/physmem.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/system/physmem.c b/system/physmem.c
> index 33d09f7571..59d1576c2b 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -2277,6 +2277,10 @@ RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,
>          ram_addr_t ram_addr;
>          RCU_READ_LOCK_GUARD();
>          ram_addr = xen_ram_addr_from_mapcache(ptr);
> +        if (ram_addr == RAM_ADDR_INVALID) {
> +            return NULL;
> +        }
> +

Isn't this indicative of a failure? Should there at least be a trace
point for failed mappings?

>          block = qemu_get_ram_block(ram_addr);
>          if (block) {
>              *offset = ram_addr - block->offset;
Edgar E. Iglesias July 4, 2024, 11:42 a.m. UTC | #2 
On Thu, Jul 4, 2024 at 1:26 PM Alex Bennée <alex.bennee@linaro.org> wrote:

> "Edgar E. Iglesias" <edgar.iglesias@gmail.com> writes:
>
> > From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
> >
> > Bail out in qemu_ram_block_from_host() when
> > xen_ram_addr_from_mapcache() does not find an existing
> > mapping.
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> > ---
> >  system/physmem.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/system/physmem.c b/system/physmem.c
> > index 33d09f7571..59d1576c2b 100644
> > --- a/system/physmem.c
> > +++ b/system/physmem.c
> > @@ -2277,6 +2277,10 @@ RAMBlock *qemu_ram_block_from_host(void *ptr,
> bool round_offset,
> >          ram_addr_t ram_addr;
> >          RCU_READ_LOCK_GUARD();
> >          ram_addr = xen_ram_addr_from_mapcache(ptr);
> > +        if (ram_addr == RAM_ADDR_INVALID) {
> > +            return NULL;
> > +        }
> > +
>
> Isn't this indicative of a failure? Should there at least be a trace
> point for failed mappings?
>
>
Yes but there are already trace points for the failure cases inside
xen_ram_addr_from_mapcache().
Do those address your concerns or do you think we need additional trace
points?

Cheers,
Edgar


> >          block = qemu_get_ram_block(ram_addr);
> >          if (block) {
> >              *offset = ram_addr - block->offset;
>
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro
>
Alex Bennée July 4, 2024, 12:33 p.m. UTC | #3 
"Edgar E. Iglesias" <edgar.iglesias@gmail.com> writes:

> On Thu, Jul 4, 2024 at 1:26 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>  "Edgar E. Iglesias" <edgar.iglesias@gmail.com> writes:
>
>  > From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
>  >
>  > Bail out in qemu_ram_block_from_host() when
>  > xen_ram_addr_from_mapcache() does not find an existing
>  > mapping.
>  >
>  > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
>  > ---
>  >  system/physmem.c | 4 ++++
>  >  1 file changed, 4 insertions(+)
>  >
>  > diff --git a/system/physmem.c b/system/physmem.c
>  > index 33d09f7571..59d1576c2b 100644
>  > --- a/system/physmem.c
>  > +++ b/system/physmem.c
>  > @@ -2277,6 +2277,10 @@ RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,
>  >          ram_addr_t ram_addr;
>  >          RCU_READ_LOCK_GUARD();
>  >          ram_addr = xen_ram_addr_from_mapcache(ptr);
>  > +        if (ram_addr == RAM_ADDR_INVALID) {
>  > +            return NULL;
>  > +        }
>  > +
>
>  Isn't this indicative of a failure? Should there at least be a trace
>  point for failed mappings?
>
> Yes but there are already trace points for the failure cases inside xen_ram_addr_from_mapcache().
> Do those address your concerns or do you think we need additional
> trace points?

Ahh that will do.

I am curious for the reasons why we might not have an entry in the
mapcache. I guess the trace_xen_map_cache() covers all insertions into
the cache although you need to check trace_xen_map_cache_return() to see
if anything failed.

Anyway:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Stefano Stabellini July 8, 2024, 11:14 p.m. UTC | #4 
On Tue, 2 Jul 2024, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
> 
> Bail out in qemu_ram_block_from_host() when
> xen_ram_addr_from_mapcache() does not find an existing
> mapping.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  system/physmem.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/system/physmem.c b/system/physmem.c
> index 33d09f7571..59d1576c2b 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -2277,6 +2277,10 @@ RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,
>          ram_addr_t ram_addr;
>          RCU_READ_LOCK_GUARD();
>          ram_addr = xen_ram_addr_from_mapcache(ptr);
> +        if (ram_addr == RAM_ADDR_INVALID) {
> +            return NULL;
> +        }
> +
>          block = qemu_get_ram_block(ram_addr);
>          if (block) {
>              *offset = ram_addr - block->offset;
> -- 
> 2.43.0
>
diff mbox series

Patch

diff --git a/system/physmem.c b/system/physmem.c
index 33d09f7571..59d1576c2b 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -2277,6 +2277,10 @@  RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,
         ram_addr_t ram_addr;
         RCU_READ_LOCK_GUARD();
         ram_addr = xen_ram_addr_from_mapcache(ptr);
+        if (ram_addr == RAM_ADDR_INVALID) {
+            return NULL;
+        }
+
         block = qemu_get_ram_block(ram_addr);
         if (block) {
             *offset = ram_addr - block->offset;