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

[Alex Bennée <alex.bennee@xxxxxxxxxx>]

"Edgar E. Iglesias" <edgar.iglesias@xxxxxxxxx> writes:

> On Thu, Jul 4, 2024 at 1:26 PM Alex Bennée <alex.bennee@xxxxxxxxxx> wrote:
>
>  "Edgar E. Iglesias" <edgar.iglesias@xxxxxxxxx> writes:
>
>  > From: "Edgar E. Iglesias" <edgar.iglesias@xxxxxxx>
>  >
>  > 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@xxxxxxx>
>  > ---
>  >  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@xxxxxxxxxx>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro