[3/7] tools/xl: Add max_altp2m parameter

Commit Message

Petr Beneš April 24, 2024, 8:42 p.m. UTC

From: Petr Beneš <[e-mail]>

Introduce a new max_altp2m parameter to control the maximum number of altp2m
views a domain can use. By default, if max_altp2m is unspecified and altp2m is
enabled, the value is set to 10, reflecting the legacy behavior.

Signed-off-by: Petr Beneš <[e-mail]>
---
 tools/golang/xenlight/helpers.gen.go | 2 ++
 tools/golang/xenlight/types.gen.go   | 1 +
 tools/include/libxl.h                | 8 ++++++++
 tools/libs/light/libxl_create.c      | 9 +++++++++
 tools/libs/light/libxl_types.idl     | 1 +
 tools/xl/xl_parse.c                  | 4 ++++
 xen/include/public/domctl.h          | 1 +
 7 files changed, 26 insertions(+)

Comments

Jan Beulich April 25, 2024, 6:19 a.m. UTC | #1

On 24.04.2024 22:42, Petr Beneš wrote:
> Introduce a new max_altp2m parameter to control the maximum number of altp2m
> views a domain can use. By default, if max_altp2m is unspecified and altp2m is
> enabled, the value is set to 10, reflecting the legacy behavior.

Apart from the public header you don't even touch hypervisor code here. What
you say above therefore is limited in scope to the tool stack. I question
though that adding a new field without respecting it constitutes an overall
consistent change. In particular I'm curious how you mean to deal with this
for other than x86, where altp2m as a concept doesn't exist (yet).

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -77,6 +77,7 @@ struct xen_domctl_createdomain {
>       */
>      uint32_t max_vcpus;
>      uint32_t max_evtchn_port;
> +    uint32_t max_altp2m;
>      int32_t max_grant_frames;
>      int32_t max_maptrack_frames;

Such a struct layout change needs to be accompanied by an interface version
bump (which hasn't happened so far in this release cycle, afaics).

Jan

Petr Beneš April 25, 2024, 8:51 a.m. UTC | #2

On Thu, Apr 25, 2024 at 8:19 AM Jan Beulich <[e-mail]> wrote:
>
> On 24.04.2024 22:42, Petr Beneš wrote:
> > Introduce a new max_altp2m parameter to control the maximum number of altp2m
> > views a domain can use. By default, if max_altp2m is unspecified and altp2m is
> > enabled, the value is set to 10, reflecting the legacy behavior.
>
> Apart from the public header you don't even touch hypervisor code here. What
> you say above therefore is limited in scope to the tool stack. I question
> though that adding a new field without respecting it constitutes an overall
> consistent change. In particular I'm curious how you mean to deal with this
> for other than x86, where altp2m as a concept doesn't exist (yet).
>
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -77,6 +77,7 @@ struct xen_domctl_createdomain {
> >       */
> >      uint32_t max_vcpus;
> >      uint32_t max_evtchn_port;
> > +    uint32_t max_altp2m;
> >      int32_t max_grant_frames;
> >      int32_t max_maptrack_frames;
>
> Such a struct layout change needs to be accompanied by an interface version
> bump (which hasn't happened so far in this release cycle, afaics).
>
> Jan

So I should not include domctl.h changes in this commit and instead
edit the commit message that it's merely a preparation for the
following commit.
Then, move the xen_domctl_createdomain field creation to the commit
with the hypervisor changes (+ include interface version bump) and
then create an additional commit that ties xl and xen together.

Do I understand it correctly?

Jan Beulich April 25, 2024, 9:56 a.m. UTC | #3

On 25.04.2024 10:51, Petr Beneš wrote:
> On Thu, Apr 25, 2024 at 8:19 AM Jan Beulich <[e-mail]> wrote:
>>
>> On 24.04.2024 22:42, Petr Beneš wrote:
>>> Introduce a new max_altp2m parameter to control the maximum number of altp2m
>>> views a domain can use. By default, if max_altp2m is unspecified and altp2m is
>>> enabled, the value is set to 10, reflecting the legacy behavior.
>>
>> Apart from the public header you don't even touch hypervisor code here. What
>> you say above therefore is limited in scope to the tool stack. I question
>> though that adding a new field without respecting it constitutes an overall
>> consistent change. In particular I'm curious how you mean to deal with this
>> for other than x86, where altp2m as a concept doesn't exist (yet).
>>
>>> --- a/xen/include/public/domctl.h
>>> +++ b/xen/include/public/domctl.h
>>> @@ -77,6 +77,7 @@ struct xen_domctl_createdomain {
>>>       */
>>>      uint32_t max_vcpus;
>>>      uint32_t max_evtchn_port;
>>> +    uint32_t max_altp2m;
>>>      int32_t max_grant_frames;
>>>      int32_t max_maptrack_frames;
>>
>> Such a struct layout change needs to be accompanied by an interface version
>> bump (which hasn't happened so far in this release cycle, afaics).
> 
> So I should not include domctl.h changes in this commit and instead
> edit the commit message that it's merely a preparation for the
> following commit.
> Then, move the xen_domctl_createdomain field creation to the commit
> with the hypervisor changes (+ include interface version bump) and
> then create an additional commit that ties xl and xen together.
> 
> Do I understand it correctly?

That's one way of approaching it, yes.

Jan

Patch

diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
index 78bdb08b15..4458d5bcbb 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -1158,6 +1158,7 @@  if err := x.ArchX86.MsrRelaxed.fromC(&xc.arch_x86.msr_relaxed);err != nil {
 return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
 }
 x.Altp2M = Altp2MMode(xc.altp2m)
+x.MaxAltp2M = uint32(xc.max_altp2m)
 x.VmtraceBufKb = int(xc.vmtrace_buf_kb)
 if err := x.Vpmu.fromC(&xc.vpmu);err != nil {
 return fmt.Errorf("converting field Vpmu: %v", err)
@@ -1674,6 +1675,7 @@  if err := x.ArchX86.MsrRelaxed.toC(&xc.arch_x86.msr_relaxed); err != nil {
 return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
 }
 xc.altp2m = C.libxl_altp2m_mode(x.Altp2M)
+xc.max_altp2m = C.uint32_t(x.MaxAltp2M)
 xc.vmtrace_buf_kb = C.int(x.VmtraceBufKb)
 if err := x.Vpmu.toC(&xc.vpmu); err != nil {
 return fmt.Errorf("converting field Vpmu: %v", err)
diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
index ccfe18019e..7139bcf324 100644
--- a/tools/golang/xenlight/types.gen.go
+++ b/tools/golang/xenlight/types.gen.go
@@ -602,6 +602,7 @@  ArchX86 struct {
 MsrRelaxed Defbool
 }
 Altp2M Altp2MMode
+MaxAltp2M uint32
 VmtraceBufKb int
 Vpmu Defbool
 }
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index 62cb07dea6..c73d9f2ff1 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -1239,6 +1239,14 @@  typedef struct libxl__ctx libxl_ctx;
  */
 #define LIBXL_HAVE_ALTP2M 1
 
+/*
+ * LIBXL_HAVE_MAX_ALTP2M
+ * If this is defined, then libxl supports setting the maximum number of
+ * alternate p2m tables.
+ */
+#define LIBXL_HAVE_MAX_ALTP2M 1
+#define LIBXL_MAX_ALTP2M_DEFAULT (~(uint32_t)0)
+
 /*
  * LIBXL_HAVE_REMUS
  * If this is defined, then libxl supports remus.
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index 41252ec553..6ccc1fa158 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -483,6 +483,14 @@  int libxl__domain_build_info_setdefault(libxl__gc *gc,
         return -ERROR_INVAL;
     }
 
+    if (b_info->max_altp2m == LIBXL_MAX_ALTP2M_DEFAULT) {
+        if ((libxl_defbool_val(b_info->u.hvm.altp2m) ||
+            b_info->altp2m != LIBXL_ALTP2M_MODE_DISABLED))
+            b_info->max_altp2m = 10;
+        else
+            b_info->max_altp2m = 0;
+    }
+
     /* Assume that providing a bootloader user implies enabling restrict. */
     libxl_defbool_setdefault(&b_info->bootloader_restrict,
                              !!b_info->bootloader_user);
@@ -645,6 +653,7 @@  int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
             .ssidref = info->ssidref,
             .max_vcpus = b_info->max_vcpus,
             .max_evtchn_port = b_info->event_channels,
+            .max_altp2m = b_info->max_altp2m,
             .max_grant_frames = b_info->max_grant_frames,
             .max_maptrack_frames = b_info->max_maptrack_frames,
             .grant_opts = XEN_DOMCTL_GRANT_version(b_info->max_grant_version),
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 470122e768..c887d8ea8c 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -728,6 +728,7 @@  libxl_domain_build_info = Struct("domain_build_info",[
     # Alternate p2m is not bound to any architecture or guest type, as it is
     # supported by x86 HVM and ARM support is planned.
     ("altp2m", libxl_altp2m_mode),
+    ("max_altp2m", uint32, {'init_val': 'LIBXL_MAX_ALTP2M_DEFAULT'}),
 
     # Size of preallocated vmtrace trace buffers (in KBYTES).
     # Use zero value to disable this feature.
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index ab09d0288b..741668e10a 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2061,6 +2061,10 @@  void parse_config_data(const char *config_source,
         }
     }
 
+    if (!xlu_cfg_get_long(config, "max_altp2m", &l, 1)) {
+        b_info->max_altp2m = l;
+    }
+
     if (!xlu_cfg_get_long(config, "vmtrace_buf_kb", &l, 1) && l) {
         b_info->vmtrace_buf_kb = l;
     }
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index a33f9ec32b..7a34465c21 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -77,6 +77,7 @@  struct xen_domctl_createdomain {
      */
     uint32_t max_vcpus;
     uint32_t max_evtchn_port;
+    uint32_t max_altp2m;
     int32_t max_grant_frames;
     int32_t max_maptrack_frames;