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

[Henry Wang <xin.wang2@xxxxxxx>]

Hi Jan,

On 4/18/2024 8:37 PM, Jan Beulich wrote:
> On 09.04.2024 06:53, Henry Wang wrote:
> > --- a/tools/libs/ctrl/xc_domain.c
> > +++ b/tools/libs/ctrl/xc_domain.c
> > @@ -697,6 +697,43 @@ int xc_domain_setmaxmem(xc_interface *xch,
> >       return do_domctl(xch, &domctl);
> >   }
> >   
> > +int xc_get_domain_mem_map(xc_interface *xch, uint32_t domid,
> > +                          struct xen_mem_region mem_regions[],
> > +                          uint32_t *nr_regions)
> > +{
> > +    int rc;
> > +    uint32_t nr = *nr_regions;
> > +    struct xen_domctl domctl = {
> > +        .cmd         = XEN_DOMCTL_get_mem_map,
> > +        .domain      = domid,
> > +        .u.mem_map = {
> > +            .nr_mem_regions = nr,
> > +        },
> > +    };
> > +
> > +    DECLARE_HYPERCALL_BOUNCE(mem_regions, sizeof(xen_mem_region_t) * nr,
> > +                             XC_HYPERCALL_BUFFER_BOUNCE_OUT);
> > +
> > +    if ( !mem_regions || xc_hypercall_bounce_pre(xch, mem_regions) || nr < 1 )
> Why the nr < 1 part? For a caller to size the necessary buffer, it may want
> to pass in 0 (and a NULL buffer pointer) first.
I will drop this nr < 1 part.

> > @@ -176,6 +175,33 @@ long arch_do_domctl(struct xen_domctl *domctl, struct 
> > domain *d,
> >   
> >           return rc;
> >       }
> > +    case XEN_DOMCTL_get_mem_map:
> > +    {
> > +        int rc = 0;
> > +        uint32_t nr_regions;
> unsigned int (see ./CODING_STYLE)?
Ok, I will use unsigned int.

> > --- a/xen/include/public/arch-arm.h
> > +++ b/xen/include/public/arch-arm.h
> > @@ -223,6 +223,13 @@ typedef uint64_t xen_pfn_t;
> >    */
> >   #define XEN_LEGACY_MAX_VCPUS 1
> >   
> > +/*
> > + * Maximum number of memory map regions for guest memory layout.
> > + * Used by XEN_DOMCTL_get_mem_map, currently there is only one region
> > + * for the guest magic pages.
> > + */
> > +#define XEN_MAX_MEM_REGIONS 1
> Why is this in the public header? I can only find Xen-internal uses.
It will also be used in the init-dom0less app which is the toolstack side.

> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -946,6 +946,33 @@ struct xen_domctl_paging_mempool {
> >       uint64_aligned_t size; /* Size in bytes. */
> >   };
> >   
> > +#ifndef XEN_MAX_MEM_REGIONS
> > +#define XEN_MAX_MEM_REGIONS 1
> > +#endif
> > +
> > +struct xen_mem_region {
> > +    uint64_aligned_t start;
> > +    uint64_aligned_t size;
> > +#define XEN_MEM_REGION_DEFAULT    0
> I can't spot any use of this. What's its purpose?
I can drop it. My original intention is to define a default type since 
the struct arch_domain should be zalloc-ed.
> > +#define XEN_MEM_REGION_MAGIC      1
> > +    uint32_t         type;
> > +    /* Must be zero */
> > +    uint32_t         pad;
> This being OUT only, I don't think the comment makes sense. I'd omit it
> completely; if you absolutely want one, please say "will" instead of "must".
Sure, I will follow your suggestion. Thanks.

Kind regards,
Henry


> Jan