Re: [QEMU][PATCH v3 3/7] softmmu: let qemu_map_ram_ptr() use qemu_ram_ptr_length()

[Vikram Garhwal]

On Fri, Mar 01, 2024 at 05:04:54PM +0000, Alex Bennée wrote:
> Vikram Garhwal <vikram.garhwal@amd.com> writes:
> 
> > From: Juergen Gross <jgross@suse.com>
> >
> > qemu_map_ram_ptr() and qemu_ram_ptr_length() share quite some code, so
> > modify qemu_ram_ptr_length() a little bit and use it for
> > qemu_map_ram_ptr(), too.
> >
> > Signed-off-by: Juergen Gross <jgross@suse.com>
> > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > ---
> >  system/physmem.c | 56 ++++++++++++++++++++----------------------------
> >  1 file changed, 23 insertions(+), 33 deletions(-)
> >
> <snip>
> > -
> > -/* Return a host pointer to guest's ram. Similar to qemu_map_ram_ptr
> > - * but takes a size argument.
> > +/*
> > + * Return a host pointer to guest's ram.
> >   *
> >   * Called within RCU critical section.
> >   */
> 
> If you end up re-spinning it would be nice to kdoc this function and at
> least call out size as a return by ref and optional. 
Will do if re-spinning is needed.
> 
> >  static void *qemu_ram_ptr_length(RAMBlock *block, ram_addr_t addr,
> >                                   hwaddr *size, bool lock)
> >  {
> > -    if (*size == 0) {
> > +    hwaddr len = 0;
> > +
> > +    if (size && *size == 0) {
> >          return NULL;
> >      }
> >  
> > @@ -2207,7 +2181,10 @@ static void *qemu_ram_ptr_length(RAMBlock *block, 
> > ram_addr_t addr,
> >          block = qemu_get_ram_block(addr);
> >          addr -= block->offset;
> >      }
> > -    *size = MIN(*size, block->max_length - addr);
> > +    if (size) {
> > +        *size = MIN(*size, block->max_length - addr);
> > +        len = *size;
> > +    }
> >  
> >      if (xen_enabled() && block->host == NULL) {
> >          /* We need to check if the requested address is in the RAM
> > @@ -2215,7 +2192,7 @@ static void *qemu_ram_ptr_length(RAMBlock *block, 
> > ram_addr_t addr,
> >           * In that case just map the requested area.
> >           */
> >          if (block->offset == 0) {
> > -            return xen_map_cache(addr, *size, lock, lock);
> > +            return xen_map_cache(addr, len, lock, lock);
> 
> I did wonder if len == 0 will confuse things but it seems xen_map_cache
> will default to XC_PAGE_SIZE in that case.
> 
> Anyway:
> 
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> 
> -- 
> Alex Bennée
> Virtualisation Tech Lead @ Linaro