| Message ID | 20250207015341.1208429-5-stefano.stabellini@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Guest XenStore page allocation for 11 Dom0less domUs | expand |
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@amd.com> > --- > 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@amd.com> ~Michal
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@amd.com> > > --- > > 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@amd.com> 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.
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; + + 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) { + struct xenstore_domain_interface *intf; + + rc = alloc_xs_page(xch, info, &xenstore_pfn); + if (rc != 0) { + printf("Error on getting xenstore page\n"); + return 1; + } + + intf = xenforeignmemory_map(xfh, info->domid, PROT_READ | PROT_WRITE, 1, + &xenstore_pfn, NULL); + if (!intf) { + printf("Error mapping xenstore page\n"); + return 1; + } + + intf->connection = XENSTORE_RECONNECT; + xenforeignmemory_unmap(xfh, intf, 1); + + /* Now everything is ready: set HVM_PARAM_STORE_PFN */ + rc = xc_hvm_param_set(xch, info->domid, HVM_PARAM_STORE_PFN, + xenstore_pfn); + if (rc < 0) + return rc; } rc = xc_dom_gnttab_seed(xch, info->domid, true,
We check if the xenstore page is already allocated. If yes, there is nothing to do. If no, we proceed allocating it. Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> --- Changes in v6: - remove double blank lines tools/helpers/init-dom0less.c | 53 +++++++++++++++++++++++++++++++++-- 1 file changed, 50 insertions(+), 3 deletions(-)