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

[Henry Wang <xin.wang2@xxxxxxx>]

Hi Jan,

On 4/4/2024 5:28 PM, Jan Beulich wrote:
> On 03.04.2024 10:16, Henry Wang wrote:
> > --- a/tools/libs/ctrl/xc_domain.c
> > +++ b/tools/libs/ctrl/xc_domain.c
> > @@ -697,6 +697,39 @@ 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;
> > +    struct xen_domctl domctl = {
> > +        .cmd         = XEN_DOMCTL_get_mem_map,
> > +        .domain      = domid,
> > +        .u.mem_map = {
> > +            .nr_mem_regions = *nr_regions,
> > +            .pad            = 0,
> This isn't needed: By there being an initializer for the struct, all
> unmentioned fields will be set to 0 anyway.
Ok, I can drop the initialization of the .pad field.

> > --- 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,20 @@ int arch_domain_create(struct domain *d,
> >       d->arch.sve_vl = config->arch.sve_vl;
> >   #endif
> >   
> > +    if ( mem_map->nr_mem_regions < XEN_MAX_MEM_REGIONS )
> > +    {
> > +        mem_map->regions[mem_map->nr_mem_regions].start = GUEST_MAGIC_BASE;
> > +        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++;
> > +    }
> > +    else
> > +    {
> > +        printk("Exceed max number of supported memory map regions\n");
> Debugging leftover?
Well, not really, I did this on purpose to print some info before exit. 
But now I realize other error paths in arch_domain_create() do not do 
that. I will drop this printk in v4.
> > --- 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;
> Why? Instead you want ...
Because there are no empty line between the other sub-ops in the arm 
version of arch_do_domctl(). Since there is no explicit guideline in 
CODING_STYLE, I was trying to take the opportunity to keep the coding 
style consistent within the file. However since you are asking, I 
realized that the x86 arch_do_domctl() is using the other way, i.e 
adding an empty line between the sub-ops, so...
> > @@ -176,6 +175,37 @@ long arch_do_domctl(struct xen_domctl *domctl, struct 
> > domain *d,
> >   
> >           return rc;
> >       }
> > +    case XEN_DOMCTL_get_mem_map:
> ... separating blank line above this line and ...
>
> > +    {
> > +        int rc = 0;
> > +        uint32_t nr_regions, i;
> > +
> > +        if ( domctl->u.mem_map.pad )
> > +            return -EINVAL;
> > +
> > +        /*
> > +         * Cap the number of regions to the minimum value between toolstack and
> > +         * hypervisor to avoid overflowing the buffer.
> > +         */
> > +        nr_regions = min(d->arch.mem_map.nr_mem_regions,
> > +                         domctl->u.mem_map.nr_mem_regions);
> > +
> > +        domctl->u.mem_map.nr_mem_regions = nr_regions;
> > +
> > +        for ( i = 0; i < nr_regions; i++ )
> > +        {
> > +            if ( d->arch.mem_map.regions[i].pad )
> > +                return -EINVAL;
> > +        }
> > +
> > +        if ( copy_to_guest(domctl->u.mem_map.buffer,
> > +                           d->arch.mem_map.regions,
> > +                           nr_regions) ||
> > +             __copy_to_guest(u_domctl, domctl, 1) )
> > +            rc = -EFAULT;
> > +
> > +        return rc;
> > +    }
> >       default:
> ... this one.
...personally I don't have strong opinions on the style as long as we 
keep consistent. I can switch the Arm one following the x86 style or 
just leave it as is.
> Further with the way you use min() above, how is the caller going to know
> whether it simply specified too small an array?
I am a bit unsure if we need to forbid caller to specify a smaller value 
than the max number of regions supported by the hypervisor, technically 
it is legal, although I agree it will lead to some issues in the 
toolstack side. It looks like the similar hypercall of e820 also does 
not forbid this (see get_mem_mapping_layout() and related 
XENMEM_memory_map). Do you have any suggestions?
> And then you check d->arch.mem_map.regions[i].pad. Why's that? And even
> if needed here for some reason, that's surely not EINVAL, but an internal
> error in Xen.
I did that under the impression that we need to check the value of 
padding field being 0. Also you mentioned in one of the comments below 
that Xen should guarantee that the padding field should be 0 before 
return. Apologize if I misunderstand your comment. The -EINVAL is taken 
from the same way of checking the padding field in XEN_DOMCTL_vuart_op 
above. Personally I would keep some consistency, but I am open to 
suggestions to make it better.
> Finally instead of __copy_to_guest() can't you use __copy_field_to_guest(),
> for just nr_regions?
You mean replacing __copy_to_guest(u_domctl, domctl, 1) with only the 
__copy_field_to_guest(u_domctl, domctl, u.mem_map.nr_mem_regions)? Ok I 
can do that in v4.
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -946,6 +946,31 @@ 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;
> > +    uint32_t         type;
> What is this field set to? I see no #define(s) in this header. If it's
> the GUEST_MEM_REGION_* in the Arm header, a connection needs to be made.
> Also note that GUEST_MEM_REGION_* violate name space requirements: New
> additions should have XEN_ / xen_ prefixes on their names.
Yeah it is the GUEST_MEM_REGION_* in the Arm header. The default value 
will be set to 0 when struct domain is created. I will switch to the 
XEN_* prefix in v4.
> > +    /* Must be zero */
> > +    uint32_t         pad;
> This, being OUT only, should not be required to be set by the caller. As
> long as no use appears, Xen merely ought to guarantee that it'll be 0 upon
> return.
See above.

Kind regards,
Henry

> Jan