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

["Orzel, Michal" <Michal.Orzel@xxxxxxx>]

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>

~Michal