[PATCH] tools/libxl: Remove page_size and page_shift from struct libxl_acpi_ctxt

[PATCH] tools/libxl: Remove page_size and page_shift from struct libxl_acpi_ctxt

[Kevin Stefanov]

As a result of recent work, two members of struct libxl_acpi_ctxt were
left with only one user. Thus, it becomes illogical for them to be
members of the struct at all.

Drop the two struct members and instead let the only function using
them have them as local variables.

Signed-off-by: Kevin Stefanov <kevin.stefanov@citrix.com>
---
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libs/light/libxl_x86_acpi.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/tools/libs/light/libxl_x86_acpi.c b/tools/libs/light/libxl_x86_acpi.c
index 57a6b63790..68902e7809 100644
--- a/tools/libs/light/libxl_x86_acpi.c
+++ b/tools/libs/light/libxl_x86_acpi.c
@@ -25,9 +25,6 @@
 struct libxl_acpi_ctxt {
     struct acpi_ctxt c;
 
-    unsigned int page_size;
-    unsigned int page_shift;
-
     /* Memory allocator */
     unsigned long guest_start;
     unsigned long guest_curr;
@@ -159,12 +156,13 @@ int libxl__dom_load_acpi(libxl__gc *gc,
     struct acpi_config config = {0};
     struct libxl_acpi_ctxt libxl_ctxt;
     int rc = 0, acpi_pages_num;
+    unsigned int page_size, page_shift;
 
     if (b_info->type != LIBXL_DOMAIN_TYPE_PVH)
         goto out;
 
-    libxl_ctxt.page_size = XC_DOM_PAGE_SIZE(dom);
-    libxl_ctxt.page_shift =  XC_DOM_PAGE_SHIFT(dom);
+    page_size = XC_DOM_PAGE_SIZE(dom);
+    page_shift = XC_DOM_PAGE_SHIFT(dom);
 
     libxl_ctxt.c.mem_ops.alloc = mem_alloc;
     libxl_ctxt.c.mem_ops.v2p = virt_to_phys;
@@ -176,20 +174,19 @@ int libxl__dom_load_acpi(libxl__gc *gc,
         goto out;
     }
 
-    config.rsdp = (unsigned long)libxl__malloc(gc, libxl_ctxt.page_size);
-    config.infop = (unsigned long)libxl__malloc(gc, libxl_ctxt.page_size);
+    config.rsdp = (unsigned long)libxl__malloc(gc, page_size);
+    config.infop = (unsigned long)libxl__malloc(gc, page_size);
     /* Pages to hold ACPI tables */
-    libxl_ctxt.buf = libxl__malloc(gc, NUM_ACPI_PAGES *
-                                   libxl_ctxt.page_size);
+    libxl_ctxt.buf = libxl__malloc(gc, NUM_ACPI_PAGES * page_size);
 
     /*
      * Set up allocator memory.
      * Start next to acpi_info page to avoid fracturing e820.
      */
     libxl_ctxt.guest_start = libxl_ctxt.guest_curr = libxl_ctxt.guest_end =
-        ACPI_INFO_PHYSICAL_ADDRESS + libxl_ctxt.page_size;
+        ACPI_INFO_PHYSICAL_ADDRESS + page_size;
 
-    libxl_ctxt.guest_end += NUM_ACPI_PAGES * libxl_ctxt.page_size;
+    libxl_ctxt.guest_end += NUM_ACPI_PAGES * page_size;
 
     /* Build the tables. */
     rc = acpi_build_tables(&libxl_ctxt.c, &config);
@@ -199,8 +196,8 @@ int libxl__dom_load_acpi(libxl__gc *gc,
     }
 
     /* Calculate how many pages are needed for the tables. */
-    acpi_pages_num = (ALIGN(libxl_ctxt.guest_curr, libxl_ctxt.page_size) -
-                      libxl_ctxt.guest_start) >> libxl_ctxt.page_shift;
+    acpi_pages_num = (ALIGN(libxl_ctxt.guest_curr, page_size) -
+                      libxl_ctxt.guest_start) >> page_shift;
 
     dom->acpi_modules[0].data = (void *)config.rsdp;
     dom->acpi_modules[0].length = 64;
@@ -212,7 +209,7 @@ int libxl__dom_load_acpi(libxl__gc *gc,
     if (strcmp(xc_dom_guest_os(dom), "linux") ||
         xc_dom_feature_get(dom, XENFEAT_linux_rsdp_unrestricted))
         dom->acpi_modules[0].guest_addr_out = ACPI_INFO_PHYSICAL_ADDRESS +
-            (1 + acpi_pages_num) * libxl_ctxt.page_size;
+            (1 + acpi_pages_num) * page_size;
     else
         dom->acpi_modules[0].guest_addr_out = 0x100000 - 64;
 
@@ -221,9 +218,9 @@ int libxl__dom_load_acpi(libxl__gc *gc,
     dom->acpi_modules[1].guest_addr_out = ACPI_INFO_PHYSICAL_ADDRESS;
 
     dom->acpi_modules[2].data = libxl_ctxt.buf;
-    dom->acpi_modules[2].length = acpi_pages_num  << libxl_ctxt.page_shift;
+    dom->acpi_modules[2].length = acpi_pages_num << page_shift;
     dom->acpi_modules[2].guest_addr_out = ACPI_INFO_PHYSICAL_ADDRESS +
-        libxl_ctxt.page_size;
+        page_size;
 
 out:
     return rc;
-- 
2.25.1

Re: [PATCH] tools/libxl: Remove page_size and page_shift from struct libxl_acpi_ctxt

[Jan Beulich]

On 24.09.2021 13:05, Kevin Stefanov wrote:
> As a result of recent work, two members of struct libxl_acpi_ctxt were
> left with only one user. Thus, it becomes illogical for them to be
> members of the struct at all.
> 
> Drop the two struct members and instead let the only function using
> them have them as local variables.
> 
> Signed-off-by: Kevin Stefanov <kevin.stefanov@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

I would like to suggest though to consider ...

> @@ -176,20 +174,19 @@ int libxl__dom_load_acpi(libxl__gc *gc,
>          goto out;
>      }
>  
> -    config.rsdp = (unsigned long)libxl__malloc(gc, libxl_ctxt.page_size);
> -    config.infop = (unsigned long)libxl__malloc(gc, libxl_ctxt.page_size);
> +    config.rsdp = (unsigned long)libxl__malloc(gc, page_size);
> +    config.infop = (unsigned long)libxl__malloc(gc, page_size);
>      /* Pages to hold ACPI tables */
> -    libxl_ctxt.buf = libxl__malloc(gc, NUM_ACPI_PAGES *
> -                                   libxl_ctxt.page_size);
> +    libxl_ctxt.buf = libxl__malloc(gc, NUM_ACPI_PAGES * page_size);

... using page_shift to replace all multiplications like the one here
at this occasion.

Jan

Re: [PATCH] tools/libxl: Remove page_size and page_shift from struct libxl_acpi_ctxt

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH] tools/libxl: Remove page_size and page_shift from struct libxl_acpi_ctxt"):
> On 24.09.2021 13:05, Kevin Stefanov wrote:
> > As a result of recent work, two members of struct libxl_acpi_ctxt were
> > left with only one user. Thus, it becomes illogical for them to be
> > members of the struct at all.
> > 
> > Drop the two struct members and instead let the only function using
> > them have them as local variables.
> > 
> > Signed-off-by: Kevin Stefanov <kevin.stefanov@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Ian Jackson <iwj@xenproject.org>

> I would like to suggest though to consider ...
> 
> > @@ -176,20 +174,19 @@ int libxl__dom_load_acpi(libxl__gc *gc,
> >          goto out;
> >      }
> >  
> > -    config.rsdp = (unsigned long)libxl__malloc(gc, libxl_ctxt.page_size);
> > -    config.infop = (unsigned long)libxl__malloc(gc, libxl_ctxt.page_size);
> > +    config.rsdp = (unsigned long)libxl__malloc(gc, page_size);
> > +    config.infop = (unsigned long)libxl__malloc(gc, page_size);
> >      /* Pages to hold ACPI tables */
> > -    libxl_ctxt.buf = libxl__malloc(gc, NUM_ACPI_PAGES *
> > -                                   libxl_ctxt.page_size);
> > +    libxl_ctxt.buf = libxl__malloc(gc, NUM_ACPI_PAGES * page_size);
> 
> ... using page_shift to replace all multiplications like the one here
> at this occasion.

I don't have an opinion about this; my tools ack can stand if this
change is made and reviewed.

Ian.