Re: [PATCH v6 5/7] init-dom0less: allocate xenstore page is not already allocated

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 7 Feb 2025, Orzel, Michal wrote:
> NIT: commit title: s/is/if/
> 
> On 07/02/2025 02:53, Stefano Stabellini wrote:
> > We check if the xenstore page is already allocated. If yes, there is
> > nothing to do. If no, we proceed allocating it.
> The commit message lacks justification which is to support old 
> unpatched/unfixed kernels.
> 
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> > ---
> > Changes in v6:
> > - remove double blank lines
> > 
> >  tools/helpers/init-dom0less.c | 53 +++++++++++++++++++++++++++++++++--
> >  1 file changed, 50 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
> > index 2b51965fa7..78c59ec5e7 100644
> > --- a/tools/helpers/init-dom0less.c
> > +++ b/tools/helpers/init-dom0less.c
> > @@ -16,8 +16,34 @@
> >  
> >  #include "init-dom-json.h"
> >  
> > +#define XENSTORE_PFN_OFFSET 1
> >  #define STR_MAX_LENGTH 128
> >  
> > +static int alloc_xs_page(struct xc_interface_core *xch,
> > +                         libxl_dominfo *info,
> > +                         uint64_t *xenstore_pfn)
> > +{
> > +    int rc;
> > +    const xen_pfn_t base = GUEST_MAGIC_BASE >> XC_PAGE_SHIFT;
> > +    xen_pfn_t p2m = (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + 
> > XENSTORE_PFN_OFFSET;
> base already contains shifted value so why not use it?
> 
> > +
> > +    rc = xc_domain_setmaxmem(xch, info->domid,
> > +                             info->max_memkb + (XC_PAGE_SIZE/1024));
> > +    if (rc < 0)
> > +        return rc;
> > +
> > +    rc = xc_domain_populate_physmap_exact(xch, info->domid, 1, 0, 0, &p2m);
> > +    if (rc < 0)
> > +        return rc;
> > +
> > +    *xenstore_pfn = base + XENSTORE_PFN_OFFSET;
> > +    rc = xc_clear_domain_page(xch, info->domid, *xenstore_pfn);
> > +    if (rc < 0)
> > +        return rc;
> > +
> > +    return 0;
> > +}
> > +
> >  static int get_xs_page(struct xc_interface_core *xch, libxl_dominfo *info,
> >                         uint64_t *xenstore_pfn)
> >  {
> > @@ -233,9 +259,30 @@ static int init_domain(struct xs_handle *xsh,
> >          return 0;
> >  
> >      /* Get xenstore page */
> > -    if (get_xs_page(xch, info, &xenstore_pfn) != 0) {
> > -        printf("Error on getting xenstore page\n");
> > -        return 1;
> > +    if (get_xs_page(xch, info, &xenstore_pfn) != 0 || xenstore_pfn == 
> > ~0ULL) {
> If get_xs_page() returns != 0, then something is wrong and we definitiely 
> should not try
> to allocate a page. The only reason the script should allocate a page is if 
> xenstore_pfn is
> invalid i.e. ~0ULL or not set i.e. 0. At this point we already validated that 
> guest is xenstore enhanced
> so the only possibility is ~0ULL. So the code should be:
> 
> if (get_xs_page(xch, info, &xenstore_pfn) != 0) {
>     return 1;
> }
> 
> if (xenstore_pfn == ~0ULL) {
> ...
> 
> Other than that:
> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

Thanks Michal, great catch! I made this change and all the other changes
you suggested and validated with a successful pipeline again. I'll
queue it for 4.21.