| Message ID | 20240701224421.1432654-2-edgar.iglesias@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen: mapcache: Fix unmapping of first the entry in a bucket | expand |
"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;
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 >
"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>
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 --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;