Re: [QEMU][PATCH v1 2/7] xen: add pseudo RAM region for grant mappings

[Stefano Stabellini]

On Tue, 10 Oct 2023, Vikram Garhwal wrote:
> On Mon, Oct 09, 2023 at 05:02:14PM -0700, Stefano Stabellini wrote:
> > On Thu, 5 Oct 2023, Vikram Garhwal wrote:
> > > From: Juergen Gross <jgross@suse.com>
> > > 
> > > Add a memory region which can be used to automatically map granted
> > > memory. It is starting at 0x8000000000000000ULL in order to be able to
> > > distinguish it from normal RAM.
> > > 
> > > For this reason the xen.ram memory region is expanded, which has no
> > > further impact as it is used just as a container of the real RAM
> > > regions and now the grant region.
> > > 
> > > Signed-off-by: Juergen Gross <jgross@suse.com>
> > > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> > 
> > This patch doesn't apply to staging anymore
> Will re-base it. I rebased it against master branch.
> > 
> > 
> > > ---
> > >  hw/i386/xen/xen-hvm.c           |  3 ++
> > >  hw/xen/xen-hvm-common.c         |  4 +--
> > >  hw/xen/xen-mapcache.c           | 27 ++++++++++++++
> > >  include/exec/ram_addr.h         |  1 +
> > >  include/hw/xen/xen-hvm-common.h |  2 ++
> > >  include/hw/xen/xen_pvdev.h      |  3 ++
> > >  include/sysemu/xen-mapcache.h   |  3 ++
> > >  softmmu/physmem.c               | 62 +++++++++++++++++++++------------
> > >  8 files changed, 80 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> > > index f42621e674..67a55558a6 100644
> > > --- a/hw/i386/xen/xen-hvm.c
> > > +++ b/hw/i386/xen/xen-hvm.c
> > > @@ -172,6 +172,9 @@ static void xen_ram_init(PCMachineState *pcms,
> > >                                   x86ms->above_4g_mem_size);
> > >          memory_region_add_subregion(sysmem, 0x100000000ULL, &ram_hi);
> > >      }
> > > +
> > > +    /* Add grant mappings as a pseudo RAM region. */
> > > +    ram_grants = *xen_init_grant_ram();
> > >  }
> > >  
> > >  static XenPhysmap *get_physmapping(hwaddr start_addr, ram_addr_t size)
> > > diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
> > > index 565dc39c8f..b7255977a5 100644
> > > --- a/hw/xen/xen-hvm-common.c
> > > +++ b/hw/xen/xen-hvm-common.c
> > > @@ -9,7 +9,7 @@
> > >  #include "hw/boards.h"
> > >  #include "hw/xen/arch_hvm.h"
> > >  
> > > -MemoryRegion ram_memory;
> > > +MemoryRegion ram_memory, ram_grants;
> > >  
> > >  void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion 
> > > *mr,
> > >                     Error **errp)
> > > @@ -26,7 +26,7 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t 
> > > size, MemoryRegion *mr,
> > >          return;
> > >      }
> > >  
> > > -    if (mr == &ram_memory) {
> > > +    if (mr == &ram_memory || mr == &ram_grants) {
> > >          return;
> > >      }
> > >  
> > > diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c
> > > index f7d974677d..8115c44c00 100644
> > > --- a/hw/xen/xen-mapcache.c
> > > +++ b/hw/xen/xen-mapcache.c
> > > @@ -14,7 +14,9 @@
> > >  
> > >  #include <sys/resource.h>
> > >  
> > > +#include "hw/xen/xen-hvm-common.h"
> > >  #include "hw/xen/xen_native.h"
> > > +#include "hw/xen/xen_pvdev.h"
> > >  #include "qemu/bitmap.h"
> > >  
> > >  #include "sysemu/runstate.h"
> > > @@ -597,3 +599,28 @@ uint8_t *xen_replace_cache_entry(hwaddr 
> > > old_phys_addr,
> > >      mapcache_unlock();
> > >      return p;
> > >  }
> > > +
> > > +MemoryRegion *xen_init_grant_ram(void)
> > > +{
> > > +    RAMBlock *block;
> > > +
> > > +    memory_region_init(&ram_grants, NULL, "xen.grants",
> > > +                       XEN_MAX_VIRTIO_GRANTS * XC_PAGE_SIZE);
> > > +    block = g_malloc0(sizeof(*block));
> > > +    block->mr = &ram_grants;
> > > +    block->used_length = XEN_MAX_VIRTIO_GRANTS * XC_PAGE_SIZE;
> > > +    block->max_length = XEN_MAX_VIRTIO_GRANTS * XC_PAGE_SIZE;
> > > +    block->fd = -1;
> > > +    block->page_size = XC_PAGE_SIZE;
> > > +    block->host = (void *)XEN_GRANT_ADDR_OFF;
> > > +    block->offset = XEN_GRANT_ADDR_OFF;
> > > +    block->flags = RAM_PREALLOC;
> > > +    ram_grants.ram_block = block;
> > > +    ram_grants.ram = true;
> > > +    ram_grants.terminates = true;
> > > +    ram_block_add_list(block);
> > > +    memory_region_add_subregion(get_system_memory(), XEN_GRANT_ADDR_OFF,
> > > +                                &ram_grants);
> > > +
> > > +    return &ram_grants;
> > 
> > It doesn't look like xen_init_grant_ram has anything to do with the
> > mapcache. It should be in another file. Maybe ./hw/xen/xen-hvm-common.c
> > or ./hw/i386/xen/xen-hvm.c (but this is x86 specific and we need grants
> > on ARM too)
> Do you mean to move all grant related functions? As moving this alone will not
> be sufficient. There are lot of new grant related function added in later 
> patches.
> 
> I am okay with moving all to xen-hvm-common.c
> 
> Following code movement will happen in this case:
> 1. All grant related static function to xen-hvm-common.c.
>     xen_ram_addr_from_grant_cache(), xen_ram_addr_from_mapcache(),
>     xen_map_grant_dyn(), xen_unmap_grant_dyn and xen_init_grant_ram().
> 2. Remove static from xen_ram_addr_from_mapcache_try().
> 
> Does these changes looks good?

After reading all the patches, I think it is also OK to leave the code
here.