[PATCH RFC] x86/boot: Update construct_dom0() to take a const char *cmdline

Andrew Cooper posted 1 patch 9 months, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20230719131802.4078609-1-andrew.cooper3@citrix.com
xen/arch/x86/dom0_build.c             | 2 +-
xen/arch/x86/hvm/dom0_build.c         | 4 ++--
xen/arch/x86/include/asm/dom0_build.h | 4 ++--
xen/arch/x86/include/asm/setup.h      | 2 +-
xen/arch/x86/pv/dom0_build.c          | 2 +-
5 files changed, 7 insertions(+), 7 deletions(-)
[PATCH RFC] x86/boot: Update construct_dom0() to take a const char *cmdline
Posted by Andrew Cooper 9 months, 2 weeks ago
With hvm_copy_to_guest_*() able to use const sources, update construct_dom0()
and friends to pass a const cmdline pointer.  Nothing in these paths have a
reason to be modifying the command line passed in.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Daniel Smith <dpsmith@apertussolutions.com>
CC: Christopher Clark <christopher@nexfir.com>

Slightly RFC.

I'm confused as to why image is const, but the initrd isn't.

Also, I suspect this will interfere with the Hyperlauch work, and I'd be happy
to leave it alone if all of this is being fixed differently anyway.

This is necessary to make the -Wwrite-strings bodge compile, but I'm hoping
that a less-bad solution to the cmdline literals problem would avoid the need
to propagate const through this callpath.
---
 xen/arch/x86/dom0_build.c             | 2 +-
 xen/arch/x86/hvm/dom0_build.c         | 4 ++--
 xen/arch/x86/include/asm/dom0_build.h | 4 ++--
 xen/arch/x86/include/asm/setup.h      | 2 +-
 xen/arch/x86/pv/dom0_build.c          | 2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 9f5300a3efbb..8b1fcc6471d8 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -564,7 +564,7 @@ int __init dom0_setup_permissions(struct domain *d)
 
 int __init construct_dom0(struct domain *d, const module_t *image,
                           unsigned long image_headroom, module_t *initrd,
-                          char *cmdline)
+                          const char *cmdline)
 {
     int rc;
 
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index fd2cbf68bc62..a7ae9c3b046e 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -532,7 +532,7 @@ static paddr_t __init find_memory(
 static int __init pvh_load_kernel(struct domain *d, const module_t *image,
                                   unsigned long image_headroom,
                                   module_t *initrd, void *image_base,
-                                  char *cmdline, paddr_t *entry,
+                                  const char *cmdline, paddr_t *entry,
                                   paddr_t *start_info_addr)
 {
     void *image_start = image_base + image_headroom;
@@ -1177,7 +1177,7 @@ static void __hwdom_init pvh_setup_mmcfg(struct domain *d)
 int __init dom0_construct_pvh(struct domain *d, const module_t *image,
                               unsigned long image_headroom,
                               module_t *initrd,
-                              char *cmdline)
+                              const char *cmdline)
 {
     paddr_t entry, start_info;
     int rc;
diff --git a/xen/arch/x86/include/asm/dom0_build.h b/xen/arch/x86/include/asm/dom0_build.h
index a5f8c9e67f68..107c1ff98367 100644
--- a/xen/arch/x86/include/asm/dom0_build.h
+++ b/xen/arch/x86/include/asm/dom0_build.h
@@ -16,12 +16,12 @@ int dom0_setup_permissions(struct domain *d);
 int dom0_construct_pv(struct domain *d, const module_t *image,
                       unsigned long image_headroom,
                       module_t *initrd,
-                      char *cmdline);
+                      const char *cmdline);
 
 int dom0_construct_pvh(struct domain *d, const module_t *image,
                        unsigned long image_headroom,
                        module_t *initrd,
-                       char *cmdline);
+                       const char *cmdline);
 
 unsigned long dom0_paging_pages(const struct domain *d,
                                 unsigned long nr_pages);
diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h
index ae0dd3915a61..51fce66607dc 100644
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -35,7 +35,7 @@ int construct_dom0(
     struct domain *d,
     const module_t *kernel, unsigned long kernel_headroom,
     module_t *initrd,
-    char *cmdline);
+    const char *cmdline);
 void setup_io_bitmap(struct domain *d);
 
 unsigned long initial_images_nrpages(nodeid_t node);
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index c99135a5522f..909ee9a899a4 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -358,7 +358,7 @@ int __init dom0_construct_pv(struct domain *d,
                              const module_t *image,
                              unsigned long image_headroom,
                              module_t *initrd,
-                             char *cmdline)
+                             const char *cmdline)
 {
     int i, rc, order, machine;
     bool compatible, compat;
-- 
2.30.2


Re: [PATCH RFC] x86/boot: Update construct_dom0() to take a const char *cmdline
Posted by Daniel P. Smith 9 months, 1 week ago
On 7/19/23 09:18, Andrew Cooper wrote:
> With hvm_copy_to_guest_*() able to use const sources, update construct_dom0()
> and friends to pass a const cmdline pointer.  Nothing in these paths have a
> reason to be modifying the command line passed in.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Daniel Smith <dpsmith@apertussolutions.com>
> CC: Christopher Clark <christopher@nexfir.com>
> 
> Slightly RFC.
> 
> I'm confused as to why image is const, but the initrd isn't.
> 
> Also, I suspect this will interfere with the Hyperlauch work, and I'd be happy
> to leave it alone if all of this is being fixed differently anyway.

This is overall a good change and honestly I don't see this having any 
significant impact on HL. And if it does, it would be better to fix HL 
then block this positive change.

> This is necessary to make the -Wwrite-strings bodge compile, but I'm hoping
> that a less-bad solution to the cmdline literals problem would avoid the need
> to propagate const through this callpath.
> ---
>   xen/arch/x86/dom0_build.c             | 2 +-
>   xen/arch/x86/hvm/dom0_build.c         | 4 ++--
>   xen/arch/x86/include/asm/dom0_build.h | 4 ++--
>   xen/arch/x86/include/asm/setup.h      | 2 +-
>   xen/arch/x86/pv/dom0_build.c          | 2 +-
>   5 files changed, 7 insertions(+), 7 deletions(-)
> 

Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>

Re: [PATCH RFC] x86/boot: Update construct_dom0() to take a const char *cmdline
Posted by Jan Beulich 9 months, 2 weeks ago
On 19.07.2023 15:18, Andrew Cooper wrote:
> With hvm_copy_to_guest_*() able to use const sources, update construct_dom0()
> and friends to pass a const cmdline pointer.  Nothing in these paths have a
> reason to be modifying the command line passed in.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Daniel Smith <dpsmith@apertussolutions.com>
> CC: Christopher Clark <christopher@nexfir.com>
> 
> Slightly RFC.
> 
> I'm confused as to why image is const, but the initrd isn't.

dom0_construct_pv() has

            initrd->mod_start = initrd_mfn = mfn_x(page_to_mfn(page));

Looks like dom0_construct_pvh() and pvh_load_kernel() could have
it const-ified.

> This is necessary to make the -Wwrite-strings bodge compile, but I'm hoping
> that a less-bad solution to the cmdline literals problem would avoid the need
> to propagate const through this callpath.

But propagating const through this, like any other, path is a good
thing, isn't it?

If you want to keep it (you appear to be uncertain)
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan