Re: [PATCH v2 3/5] xen/domctl, tools: Introduce a new domctl to get guest memory map

[Henry Wang <xin.wang2@xxxxxxx>]

Hi Michal,

On 3/11/2024 5:10 PM, Michal Orzel wrote:
> Hi Henry,
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 5e7a7f3e7e..54f3601ab0 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -696,6 +696,7 @@ int arch_domain_create(struct domain *d,
>   {
>       unsigned int count = 0;
>       int rc;
> +    struct mem_map_domain *mem_map = &d->arch.mem_map;
>   
>       BUILD_BUG_ON(GUEST_MAX_VCPUS < MAX_VIRT_CPUS);
>   
> @@ -785,6 +786,11 @@ int arch_domain_create(struct domain *d,
>       d->arch.sve_vl = config->arch.sve_vl;
>   #endif
>   
> +    mem_map->regions[mem_map->nr_mem_regions].start = GUEST_MAGIC_BASE;
> You don't check for exceeding max number of regions. Is the expectation that 
> nr_mem_regions
> is 0 at this stage? Maybe add an ASSERT here.
Sure, I will add the checking.

> > +    mem_map->regions[mem_map->nr_mem_regions].size = GUEST_MAGIC_SIZE;
> > +    mem_map->regions[mem_map->nr_mem_regions].type = GUEST_MEM_REGION_MAGIC;
> > +    mem_map->nr_mem_regions++;
> > +
> >       return 0;
> >   
> >   fail:
> > diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> > index ad56efb0f5..92024bcaa0 100644
> > --- a/xen/arch/arm/domctl.c
> > +++ b/xen/arch/arm/domctl.c
> > @@ -148,7 +148,6 @@ long arch_do_domctl(struct xen_domctl *domctl, struct 
> > domain *d,
> >   
> >           return 0;
> >       }
> > -
> >       case XEN_DOMCTL_vuart_op:
> >       {
> >           int rc;
> > @@ -176,6 +175,24 @@ long arch_do_domctl(struct xen_domctl *domctl, struct 
> > domain *d,
> >   
> >           return rc;
> >       }
> > +    case XEN_DOMCTL_get_mem_map:
> > +    {
> > +        int rc;
> Without initialization, what will be the rc value on success?
Thanks for catching this (and the copy back issue below). I made a silly 
mistake here and didn't catch it as I also missed checking the rc in the 
toolstack side...I will fix both side.
> > +        /*
> > +         * Cap the number of regions to the minimum value between toolstack and
> > +         * hypervisor to avoid overflowing the buffer.
> > +         */
> > +        uint32_t nr_regions = min(d->arch.mem_map.nr_mem_regions,
> > +                                  domctl->u.mem_map.nr_mem_regions);
> > +
> > +        if ( copy_to_guest(domctl->u.mem_map.buffer,
> > +                           d->arch.mem_map.regions,
> > +                           nr_regions) ||
> > +             __copy_to_guest(u_domctl, domctl, 1) )
> In domctl.h, you wrote that nr_regions is IN/OUT but you don't seem to write 
> back the actual number
> of regions.
Thanks. Added "domctl->u.mem_map.nr_mem_regions = nr_regions;" locally.

> > +/* Guest memory region types */
> > +#define GUEST_MEM_REGION_DEFAULT    0
> What's the purpose of this default type? It seems unusued.
I added it because struct arch_domain (or we should say struct domain) 
is zalloc-ed. So the default type field in struct xen_mem_region is 0. 
Otherwise we may (mistakenly) define a region type as 0 and lead to 
mistakes.
> > +#define GUEST_MEM_REGION_MAGIC      1
> > +
> >   /* Physical Address Space */
> >   
> >   /* Virtio MMIO mappings */
> > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> > index a33f9ec32b..77bf999651 100644
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -946,6 +946,25 @@ struct xen_domctl_paging_mempool {
> >       uint64_aligned_t size; /* Size in bytes. */
> >   };
> >   
> > +#define XEN_MAX_MEM_REGIONS 1
> The max number of regions can differ between arches. How are you going to 
> handle it?
I think we can add
```
#ifndef XEN_MAX_MEM_REGIONS

#define XEN_MAX_MEM_REGIONS 1

#endif
```
here and define the arch specific XEN_MAX_MEM_REGIONS in 
public/arch-*.h. I will fix this in v3.
Kind regards,
Henry

> ~Michal