[RFC PATCH 0/6] xen/abi: On wide bitfields inside primitive types

[RFC PATCH 0/6] xen/abi: On wide bitfields inside primitive types

[Alejandro Vallejo]

Non-boolean bitfields in the hypercall ABI make it fairly inconvenient to
create bindings for any language because (a) they are always ad-hoc and are
subject to restrictions regular fields are not (b) require boilerplate that
regular fields do not and (c) might not even be part of the core language,
forcing avoidable external libraries into any sort of generic library.

This patch (it's a series merely to split roughly by maintainer) is one such
case that I happened to spot while playing around. It's the grant_version
field, buried under an otherwise empty grant_opts.

The invariant I'd like to (slowly) introduce and discuss is that fields may
have bitflags (e.g: a packed array of booleans indexed by some enumerated
type), but not be mixed with wider fields in the same primitive type. This
ensures any field containing an integer of any kind can be referred by pointer
and treated the same way as any other with regards to sizeof() and the like.

I'd like to have a certain consensus about this general point before going
establishing this restriction in the IDL system I'm working on.

My preference would be to fold everything into a single patch if we decide to
follow through with this particular case. As I said before, the split is
artificial for review.

Alejandro Vallejo (6):
  xen/domctl: Refine grant_opts into grant_version
  tools: Rename grant_opts to grant_version
  tools/ocaml: Rename grant_opts to grant_version
  xen/arm: Rename grant_opts to grant_version
  xen/x86: Rename grant_opts to grant_version
  xen/common: Rename grant_opts to grant_version

 tools/helpers/init-xenstore-domain.c             |  2 +-
 tools/libs/light/libxl_create.c                  |  2 +-
 tools/ocaml/libs/xc/xenctrl_stubs.c              |  3 +--
 tools/python/xen/lowlevel/xc/xc.c                |  2 +-
 tools/tests/paging-mempool/test-paging-mempool.c |  2 +-
 tools/tests/resource/test-resource.c             |  6 +++---
 tools/tests/tsx/test-tsx.c                       |  4 ++--
 xen/arch/arm/dom0less-build.c                    |  4 ++--
 xen/arch/arm/domain_build.c                      |  2 +-
 xen/arch/x86/setup.c                             |  2 +-
 xen/common/domain.c                              |  6 +++---
 xen/common/grant_table.c                         |  3 +--
 xen/include/public/domctl.h                      | 15 +++++++++++----
 xen/include/xen/grant_table.h                    |  4 ++--
 14 files changed, 31 insertions(+), 26 deletions(-)

-- 
2.47.0

[RFC PATCH 1/6] xen/domctl: Refine grant_opts into grant_version

[Alejandro Vallejo]

grant_opts is overoptimizing for space packing in a hypercall that
doesn't warrant the effort. Tweak the ABI without breaking it in order
to remove the bitfield by extending it to 8 bits.

Xen only supports little-endian systems, so the transformation from
uint32_t to uint8_t followed by 3 octets worth of padding is not an ABI
breakage.

No functional change

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 xen/include/public/domctl.h | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 353f831e402e..b3c8271e66ba 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -90,11 +90,18 @@ struct xen_domctl_createdomain {
     int32_t max_grant_frames;
     int32_t max_maptrack_frames;
 
-/* Grant version, use low 4 bits. */
-#define XEN_DOMCTL_GRANT_version_mask    0xf
-#define XEN_DOMCTL_GRANT_version(v)      ((v) & XEN_DOMCTL_GRANT_version_mask)
+    /*
+     * Maximum grant table version the domain can be configured with.
+     *
+     * Domains always start with v1 (if CONFIG_GRANT_TABLE) and can be bumped
+     * to use up to `max_grant_version` via GNTTABOP_set_version.
+     *
+     * Must be zero iff !CONFIG_GRANT_TABLE.
+     */
+    uint8_t max_grant_version;
 
-    uint32_t grant_opts;
+    /* Unused */
+    uint8_t rsvd0[3];
 
 /*
  * Enable altp2m mixed mode.
-- 
2.47.0

Re: [RFC PATCH 1/6] xen/domctl: Refine grant_opts into grant_version

[Jan Beulich]

On 29.10.2024 19:16, Alejandro Vallejo wrote:
> grant_opts is overoptimizing for space packing in a hypercall that
> doesn't warrant the effort. Tweak the ABI without breaking it in order
> to remove the bitfield by extending it to 8 bits.
> 
> Xen only supports little-endian systems, so the transformation from
> uint32_t to uint8_t followed by 3 octets worth of padding is not an ABI
> breakage.
> 
> No functional change
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
>  xen/include/public/domctl.h | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)

This isn't a complete patch, is it? I expect it'll break the build without
users of the field also adjusted.

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -90,11 +90,18 @@ struct xen_domctl_createdomain {
>      int32_t max_grant_frames;
>      int32_t max_maptrack_frames;
>  
> -/* Grant version, use low 4 bits. */
> -#define XEN_DOMCTL_GRANT_version_mask    0xf
> -#define XEN_DOMCTL_GRANT_version(v)      ((v) & XEN_DOMCTL_GRANT_version_mask)
> +    /*
> +     * Maximum grant table version the domain can be configured with.
> +     *
> +     * Domains always start with v1 (if CONFIG_GRANT_TABLE) and can be bumped
> +     * to use up to `max_grant_version` via GNTTABOP_set_version.
> +     *
> +     * Must be zero iff !CONFIG_GRANT_TABLE.
> +     */
> +    uint8_t max_grant_version;
>  
> -    uint32_t grant_opts;
> +    /* Unused */
> +    uint8_t rsvd0[3];
>  
>  /*
>   * Enable altp2m mixed mode.

Just to mention it: I think while binary compatible, this is still on the edge
of needing an interface version bump. We may get away without as users of the
removed identifiers will still notice by way of observing build failures.

Jan

Re: [RFC PATCH 1/6] xen/domctl: Refine grant_opts into grant_version

[Alejandro Vallejo]

Hi,

On Wed Oct 30, 2024 at 9:08 AM GMT, Jan Beulich wrote:
> On 29.10.2024 19:16, Alejandro Vallejo wrote:
> > grant_opts is overoptimizing for space packing in a hypercall that
> > doesn't warrant the effort. Tweak the ABI without breaking it in order
> > to remove the bitfield by extending it to 8 bits.
> > 
> > Xen only supports little-endian systems, so the transformation from
> > uint32_t to uint8_t followed by 3 octets worth of padding is not an ABI
> > breakage.
> > 
> > No functional change
> > 
> > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> > ---
> >  xen/include/public/domctl.h | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
>
> This isn't a complete patch, is it? I expect it'll break the build without
> users of the field also adjusted.

Indeed. The non-RFC version would have everything folded in one. I just wanted
to avoid Cc-ing everyone in MAINTAINERS for the same single RFC patch. It's
split by (rough) maintained area.

>
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -90,11 +90,18 @@ struct xen_domctl_createdomain {
> >      int32_t max_grant_frames;
> >      int32_t max_maptrack_frames;
> >  
> > -/* Grant version, use low 4 bits. */
> > -#define XEN_DOMCTL_GRANT_version_mask    0xf
> > -#define XEN_DOMCTL_GRANT_version(v)      ((v) & XEN_DOMCTL_GRANT_version_mask)
> > +    /*
> > +     * Maximum grant table version the domain can be configured with.
> > +     *
> > +     * Domains always start with v1 (if CONFIG_GRANT_TABLE) and can be bumped
> > +     * to use up to `max_grant_version` via GNTTABOP_set_version.
> > +     *
> > +     * Must be zero iff !CONFIG_GRANT_TABLE.
> > +     */
> > +    uint8_t max_grant_version;
> >  
> > -    uint32_t grant_opts;
> > +    /* Unused */
> > +    uint8_t rsvd0[3];
> >  
> >  /*
> >   * Enable altp2m mixed mode.
>
> Just to mention it: I think while binary compatible, this is still on the edge
> of needing an interface version bump. We may get away without as users of the
> removed identifiers will still notice by way of observing build failures.
>
> Jan

If users are forced to rebuild either way, might as well prevent existing
binaries from breaking. There ought to be a strict distinction between ABI and
API compatibility because, while they typically move in lockstep, they don't
always (and this is one such an example).

Regardless, this is a discussion for the final patch if we get there.

Cheers,
Alejandro

[RFC PATCH 2/6] tools: Rename grant_opts to grant_version

[Alejandro Vallejo]

... and remove the macros that no longer exist.

No functional change

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 tools/helpers/init-xenstore-domain.c             | 2 +-
 tools/libs/light/libxl_create.c                  | 2 +-
 tools/python/xen/lowlevel/xc/xc.c                | 2 +-
 tools/tests/paging-mempool/test-paging-mempool.c | 2 +-
 tools/tests/resource/test-resource.c             | 6 +++---
 tools/tests/tsx/test-tsx.c                       | 4 ++--
 6 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/tools/helpers/init-xenstore-domain.c b/tools/helpers/init-xenstore-domain.c
index 01ca667d25d1..25e41cf5175f 100644
--- a/tools/helpers/init-xenstore-domain.c
+++ b/tools/helpers/init-xenstore-domain.c
@@ -96,7 +96,7 @@ static int build(xc_interface *xch)
          */
         .max_grant_frames = 4,
         .max_maptrack_frames = 128,
-        .grant_opts = XEN_DOMCTL_GRANT_version(1),
+        .grant_version = 1,
     };
 
     xs_fd = open("/dev/xen/xenbus_backend", O_RDWR);
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index edeadd57ef5a..f952614b1f8d 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -646,7 +646,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
             .max_evtchn_port = b_info->event_channels,
             .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),
+            .grant_version = b_info->max_grant_version,
             .vmtrace_size = ROUNDUP(b_info->vmtrace_buf_kb << 10, XC_PAGE_SHIFT),
             .cpupool_id = info->poolid,
         };
diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
index 9feb12ae2b16..b3bbda6d955d 100644
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -167,7 +167,7 @@ static PyObject *pyxc_domain_create(XcObject *self,
 #else
 #error Architecture not supported
 #endif
-    config.grant_opts = XEN_DOMCTL_GRANT_version(max_grant_version);
+    config.grant_version = max_grant_version;
 
     if ( (ret = xc_domain_create(self->xc_handle, &dom, &config)) < 0 )
         return pyxc_error_to_exception(self->xc_handle);
diff --git a/tools/tests/paging-mempool/test-paging-mempool.c b/tools/tests/paging-mempool/test-paging-mempool.c
index 1ebc13455ac2..dc90b3b41793 100644
--- a/tools/tests/paging-mempool/test-paging-mempool.c
+++ b/tools/tests/paging-mempool/test-paging-mempool.c
@@ -24,7 +24,7 @@ static struct xen_domctl_createdomain create = {
     .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
     .max_vcpus = 1,
     .max_grant_frames = 1,
-    .grant_opts = XEN_DOMCTL_GRANT_version(1),
+    .grant_version = 1,
 
     .arch = {
 #if defined(__x86_64__) || defined(__i386__)
diff --git a/tools/tests/resource/test-resource.c b/tools/tests/resource/test-resource.c
index 1b10be16a6b4..33bdb3113d85 100644
--- a/tools/tests/resource/test-resource.c
+++ b/tools/tests/resource/test-resource.c
@@ -137,7 +137,7 @@ static void test_domain_configurations(void)
             .create = {
                 .max_vcpus = 2,
                 .max_grant_frames = 40,
-                .grant_opts = XEN_DOMCTL_GRANT_version(1),
+                .grant_version = 1,
             },
         },
         {
@@ -146,7 +146,7 @@ static void test_domain_configurations(void)
                 .flags = XEN_DOMCTL_CDF_hvm,
                 .max_vcpus = 2,
                 .max_grant_frames = 40,
-                .grant_opts = XEN_DOMCTL_GRANT_version(1),
+                .grant_version = 1,
                 .arch = {
                     .emulation_flags = XEN_X86_EMU_LAPIC,
                 },
@@ -159,7 +159,7 @@ static void test_domain_configurations(void)
                 .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
                 .max_vcpus = 2,
                 .max_grant_frames = 40,
-                .grant_opts = XEN_DOMCTL_GRANT_version(1),
+                .grant_version = 1,
             },
         },
 #endif
diff --git a/tools/tests/tsx/test-tsx.c b/tools/tests/tsx/test-tsx.c
index 5af04953f340..86608c95d627 100644
--- a/tools/tests/tsx/test-tsx.c
+++ b/tools/tests/tsx/test-tsx.c
@@ -457,7 +457,7 @@ static void test_guests(void)
         struct xen_domctl_createdomain c = {
             .max_vcpus = 1,
             .max_grant_frames = 1,
-            .grant_opts = XEN_DOMCTL_GRANT_version(1),
+            .grant_version = 1,
         };
 
         printf("Testing PV guest\n");
@@ -470,7 +470,7 @@ static void test_guests(void)
             .flags = XEN_DOMCTL_CDF_hvm,
             .max_vcpus = 1,
             .max_grant_frames = 1,
-            .grant_opts = XEN_DOMCTL_GRANT_version(1),
+            .grant_version = 1,
             .arch = {
                 .emulation_flags = XEN_X86_EMU_LAPIC,
             },
-- 
2.47.0

[RFC PATCH 3/6] tools/ocaml: Rename grant_opts to grant_version

[Alejandro Vallejo]

... and remove the macros that no longer exist.

No functional change.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 tools/ocaml/libs/xc/xenctrl_stubs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index c78191f95abc..c4d34ca48753 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -223,8 +223,7 @@ CAMLprim value stub_xc_domain_create(value xch_val, value wanted_domid, value co
 		.max_evtchn_port = Int_val(VAL_MAX_EVTCHN_PORT),
 		.max_grant_frames = Int_val(VAL_MAX_GRANT_FRAMES),
 		.max_maptrack_frames = Int_val(VAL_MAX_MAPTRACK_FRAMES),
-		.grant_opts =
-		    XEN_DOMCTL_GRANT_version(Int_val(VAL_MAX_GRANT_VERSION)),
+		.grant_version = Int_val(VAL_MAX_GRANT_VERSION),
 		.altp2m_opts = Int32_val(VAL_ALTP2M_OPTS),
 		.vmtrace_size = vmtrace_size,
 		.cpupool_id = Int32_val(VAL_CPUPOOL_ID),
-- 
2.47.0

[RFC PATCH 4/6] xen/arm: Rename grant_opts to grant_version

[Alejandro Vallejo]

... and remove the macros that no longer exist.

No functional change.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 xen/arch/arm/dom0less-build.c | 4 ++--
 xen/arch/arm/domain_build.c   | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index f328a044e9d3..1c6219c7cc82 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -877,7 +877,7 @@ void __init create_domUs(void)
             .max_evtchn_port = 1023,
             .max_grant_frames = -1,
             .max_maptrack_frames = -1,
-            .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
+            .max_grant_version = opt_gnttab_max_version,
         };
         unsigned int flags = 0U;
         uint32_t val;
@@ -959,7 +959,7 @@ void __init create_domUs(void)
         }
 
         if ( dt_property_read_u32(node, "max_grant_version", &val) )
-            d_cfg.grant_opts = XEN_DOMCTL_GRANT_version(val);
+            d_cfg.max_grant_version = val;
 
         if ( dt_property_read_u32(node, "max_grant_frames", &val) )
         {
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 2c30792de88b..773412ba2acb 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2194,7 +2194,7 @@ void __init create_dom0(void)
         .max_evtchn_port = -1,
         .max_grant_frames = gnttab_dom0_frames(),
         .max_maptrack_frames = -1,
-        .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
+        .max_grant_version = opt_gnttab_max_version,
     };
     int rc;
 
-- 
2.47.0

[RFC PATCH 5/6] xen/x86: Rename grant_opts to grant_version

[Alejandro Vallejo]

... and remove the macros that no longer exist.

No functional change.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 xen/arch/x86/setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 177f4024abca..a9130161969b 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -956,7 +956,7 @@ static struct domain *__init create_dom0(const module_t *image,
         .max_evtchn_port = -1,
         .max_grant_frames = -1,
         .max_maptrack_frames = -1,
-        .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
+        .max_grant_version = opt_gnttab_max_version,
         .max_vcpus = dom0_max_vcpus(),
         .arch = {
             .misc_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED : 0,
-- 
2.47.0

[RFC PATCH 6/6] xen/common: Rename grant_opts to grant_version

[Alejandro Vallejo]

... and remove the macros that no longer exist.

No functional change.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 xen/common/domain.c           | 6 +++---
 xen/common/grant_table.c      | 3 +--
 xen/include/xen/grant_table.h | 4 ++--
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 92263a4fbdc5..86f0e99e0d4a 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -579,9 +579,9 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
         return -EINVAL;
     }
 
-    if ( config->grant_opts & ~XEN_DOMCTL_GRANT_version_mask )
+    if ( config->rsvd0[0] | config->rsvd0[1] | config->rsvd0[2] )
     {
-        dprintk(XENLOG_INFO, "Unknown grant options %#x\n", config->grant_opts);
+        dprintk(XENLOG_INFO, "Rubble in rsvd0 padding\n");
         return -EINVAL;
     }
 
@@ -788,7 +788,7 @@ struct domain *domain_create(domid_t domid,
 
     if ( (err = grant_table_init(d, config->max_grant_frames,
                                  config->max_maptrack_frames,
-                                 config->grant_opts)) != 0 )
+                                 config->max_grant_version)) != 0 )
         goto fail;
     init_status |= INIT_gnttab;
 
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 6c77867f8cdd..51a3f72a9601 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1963,10 +1963,9 @@ active_alloc_failed:
 }
 
 int grant_table_init(struct domain *d, int max_grant_frames,
-                     int max_maptrack_frames, unsigned int options)
+                     int max_maptrack_frames, uint8_t max_grant_version)
 {
     struct grant_table *gt;
-    unsigned int max_grant_version = options & XEN_DOMCTL_GRANT_version_mask;
     int ret = -ENOMEM;
 
     if ( !max_grant_version )
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 50edfecfb62f..f3edbae3c974 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -73,9 +73,9 @@ int gnttab_acquire_resource(
 static inline int grant_table_init(struct domain *d,
                                    int max_grant_frames,
                                    int max_maptrack_frames,
-                                   unsigned int options)
+                                   uint8_t max_grant_version)
 {
-    if ( options )
+    if ( max_grant_version )
         return -EINVAL;
 
     return 0;
-- 
2.47.0

Re: [RFC PATCH 0/6] xen/abi: On wide bitfields inside primitive types

[Alejandro Vallejo]

On Tue Oct 29, 2024 at 6:16 PM GMT, Alejandro Vallejo wrote:
> Non-boolean bitfields in the hypercall ABI make it fairly inconvenient to
> create bindings for any language because (a) they are always ad-hoc and are
> subject to restrictions regular fields are not (b) require boilerplate that
> regular fields do not and (c) might not even be part of the core language,
> forcing avoidable external libraries into any sort of generic library.
>
> This patch (it's a series merely to split roughly by maintainer) is one such
> case that I happened to spot while playing around. It's the grant_version
> field, buried under an otherwise empty grant_opts.
>
> The invariant I'd like to (slowly) introduce and discuss is that fields may
> have bitflags (e.g: a packed array of booleans indexed by some enumerated
> type), but not be mixed with wider fields in the same primitive type. This
> ensures any field containing an integer of any kind can be referred by pointer
> and treated the same way as any other with regards to sizeof() and the like.
>
> I'd like to have a certain consensus about this general point before going
> establishing this restriction in the IDL system I'm working on.
>
> My preference would be to fold everything into a single patch if we decide to
> follow through with this particular case. As I said before, the split is
> artificial for review.
>
> Alejandro Vallejo (6):
>   xen/domctl: Refine grant_opts into grant_version
>   tools: Rename grant_opts to grant_version
>   tools/ocaml: Rename grant_opts to grant_version
>   xen/arm: Rename grant_opts to grant_version
>   xen/x86: Rename grant_opts to grant_version
>   xen/common: Rename grant_opts to grant_version
>
>  tools/helpers/init-xenstore-domain.c             |  2 +-
>  tools/libs/light/libxl_create.c                  |  2 +-
>  tools/ocaml/libs/xc/xenctrl_stubs.c              |  3 +--
>  tools/python/xen/lowlevel/xc/xc.c                |  2 +-
>  tools/tests/paging-mempool/test-paging-mempool.c |  2 +-
>  tools/tests/resource/test-resource.c             |  6 +++---
>  tools/tests/tsx/test-tsx.c                       |  4 ++--
>  xen/arch/arm/dom0less-build.c                    |  4 ++--
>  xen/arch/arm/domain_build.c                      |  2 +-
>  xen/arch/x86/setup.c                             |  2 +-
>  xen/common/domain.c                              |  6 +++---
>  xen/common/grant_table.c                         |  3 +--
>  xen/include/public/domctl.h                      | 15 +++++++++++----
>  xen/include/xen/grant_table.h                    |  4 ++--
>  14 files changed, 31 insertions(+), 26 deletions(-)

Bah. I sent it too early. The new field in patches 2-6 ought to be
max_grant_version. Regardless, the general point still holds, I hope.

Cheers,
Alejandro

Re: [RFC PATCH 0/6] xen/abi: On wide bitfields inside primitive types

[Christian Lindig]

> On 29 Oct 2024, at 18:16, Alejandro Vallejo <alejandro.vallejo@cloud.com> wrote:
> 
> 
> The invariant I'd like to (slowly) introduce and discuss is that fields may
> have bitflags (e.g: a packed array of booleans indexed by some enumerated
> type), but not be mixed with wider fields in the same primitive type. This
> ensures any field containing an integer of any kind can be referred by pointer
> and treated the same way as any other with regards to sizeof() and the like.

Acked-by: Christian Lindig <christian.lindig@cloud.com>


Fine with me but the OCaml part is not very exposed to this.

— C

Re: [RFC PATCH 0/6] xen/abi: On wide bitfields inside primitive types

[Alejandro Vallejo]

On Wed Oct 30, 2024 at 8:45 AM GMT, Christian Lindig wrote:
>
>
> > On 29 Oct 2024, at 18:16, Alejandro Vallejo <alejandro.vallejo@cloud.com> wrote:
> > 
> > 
> > The invariant I'd like to (slowly) introduce and discuss is that fields may
> > have bitflags (e.g: a packed array of booleans indexed by some enumerated
> > type), but not be mixed with wider fields in the same primitive type. This
> > ensures any field containing an integer of any kind can be referred by pointer
> > and treated the same way as any other with regards to sizeof() and the like.
>
> Acked-by: Christian Lindig <christian.lindig@cloud.com>

Thanks.

>
>
> Fine with me but the OCaml part is not very exposed to this.

Yeah, OCaml is pretty far from interacting with these details at all.
>
> — C

Cheers,
Alejandro

Re: [RFC PATCH 0/6] xen/abi: On wide bitfields inside primitive types

[Jan Beulich]

On 29.10.2024 19:16, Alejandro Vallejo wrote:
> Non-boolean bitfields in the hypercall ABI make it fairly inconvenient to
> create bindings for any language because (a) they are always ad-hoc and are
> subject to restrictions regular fields are not (b) require boilerplate that
> regular fields do not and (c) might not even be part of the core language,
> forcing avoidable external libraries into any sort of generic library.
> 
> This patch (it's a series merely to split roughly by maintainer) is one such
> case that I happened to spot while playing around. It's the grant_version
> field, buried under an otherwise empty grant_opts.
> 
> The invariant I'd like to (slowly) introduce and discuss is that fields may
> have bitflags (e.g: a packed array of booleans indexed by some enumerated
> type), but not be mixed with wider fields in the same primitive type. This
> ensures any field containing an integer of any kind can be referred by pointer
> and treated the same way as any other with regards to sizeof() and the like.

While I don't strictly mind, I'm also not really seeing why taking addresses
or applying sizeof() would be commonly necessary. Can you perhaps provide a
concrete example of where the present way of dealing with grant max version
is getting in the way? After all your use of the term "bitfield" doesn't
really mean C's understanding of it, so especially (c) above escapes me to a
fair degree.

> I'd like to have a certain consensus about this general point before going
> establishing this restriction in the IDL system I'm working on.
> 
> My preference would be to fold everything into a single patch if we decide to
> follow through with this particular case. As I said before, the split is
> artificial for review.

That's not just a preference, but a requirement, or else the build will break
in the middle of the series (harming bisection at the very least).

Jan

Re: [RFC PATCH 0/6] xen/abi: On wide bitfields inside primitive types

[Alejandro Vallejo]

In the course of preparing this answer I just noticed that altp2m_opts suffers
from the exact same annoyance, with the exact same fix. I just noticed while
rebasing my Rust branch.

On Wed Oct 30, 2024 at 9:14 AM GMT, Jan Beulich wrote:
> On 29.10.2024 19:16, Alejandro Vallejo wrote:
> > Non-boolean bitfields in the hypercall ABI make it fairly inconvenient to
> > create bindings for any language because (a) they are always ad-hoc and are
> > subject to restrictions regular fields are not (b) require boilerplate that
> > regular fields do not and (c) might not even be part of the core language,
> > forcing avoidable external libraries into any sort of generic library.
> > 
> > This patch (it's a series merely to split roughly by maintainer) is one such
> > case that I happened to spot while playing around. It's the grant_version
> > field, buried under an otherwise empty grant_opts.
> > 
> > The invariant I'd like to (slowly) introduce and discuss is that fields may
> > have bitflags (e.g: a packed array of booleans indexed by some enumerated
> > type), but not be mixed with wider fields in the same primitive type. This
> > ensures any field containing an integer of any kind can be referred by pointer
> > and treated the same way as any other with regards to sizeof() and the like.
>
> While I don't strictly mind, I'm also not really seeing why taking addresses
> or applying sizeof() would be commonly necessary. Can you perhaps provide a
> concrete example of where the present way of dealing with grant max version
> is getting in the way? After all your use of the term "bitfield" doesn't
> really mean C's understanding of it, so especially (c) above escapes me to a
> fair degree.

Wall of text ahead, but I'll try to stay on point. The rationale should become
a lot clearer after I send an RFC series with initial code to autogenerate some
hypercall payloads from markup. The biggest question is: Can I create a
definition language such that (a) it precisely represents the Xen ABI and (b)
is fully type-safe under modern strongly-typed languages?

I already have a backbone I can define the ABI in, so my options when I hit
some impedance mismatch are:

  1. Change the ABI so it matches better my means of defining it.
  2. Change the means to define so it captures the existing ABI better.

Most of the work I've done has moved in the (2) direction so far, but I found a
number of pain points when mapping the existing ABI to Rust that, while not
impossible to work around, are quite annoying for no clear benefit. If
possible, I'd like to simplify the cognitive load involved in defining, using
and updating hypercalls rather than bending over backwards to support a
construct that provides no real benefit. IOW: If I can define an ABI that is
_simpler_, it follows that it's also easier to not make mistakes and it's
easier to generate code for it.

The use of packed fields is one such case. Even in C, we create extra macros
for creating a field, modifying it, fetching it, etc. Patches 2-6 are strict
code removals. And even in the most extreme cases the space savings are largely
irrelevant because the hypercall has a fixed size. We do want to pack _flags_
as otherwise the payload size would explode pretty quickly on hypercalls with
tons of boolean options, but I'm not aware of that being problematic for wider
subfields (like the grant max version).

Now, being more concrete...

##################################################################
# IDL is simpler if the size is a property of the type
##################################################################

Consider the definition of the (new) max_grant_version type under the IDL I'm
working on (it's TOML, but I don't particularly care about which markup we end
up using).

  [[enums]]
  name = "xen_domaincreate_max_grant_version"
  description = "Content of the `max_grant_version` field of the domain creation hypercall."
  typ = { tag = "u8" }

  [[enums.variants]]
  name = "off"
  description = "Must be used with gnttab support compiled out"
  value = 0

  [[enums.variants]]
  name = "v1"
  description = "Allow the domain to use up to gnttab_v1"
  value = 1

  [[enums.variants]]
  name = "v2"
  description = "Allow the domain to use up to gnttab_v2"
  value = 2

Note that I can define a type being enumerated, can choose its specific
variants and its width is a property of the type itself. With bitfields you're
always in a weird position of the width not being part of the type that goes
into it.

Should I need it as a field somewhere, then...

  [[structs.fields]]
  name = "max_grant_version"
  description = "Maximum grant table version the domain may be bumped to"
  typ = { tag = "enum", args = "xen_domaincreate_max_grant_version" }

... at which point the size of the field is given by an intrinsic property of
the type (the typ property on the enums table) I previously defined. It's
extensible, composable and allows me to generate readable code in both C and
Rust.

Should I need to support full bitfields I would require a means of stating the
start and end bits of every field, which is very bad for the sanity of whoever
wants to maintain coherency in the ABI.

##################################################################
# Rust and Go don't like bitfields...
##################################################################

... and neither does C, even if for historic reasons they do exist in the
standard. On a slight tangent, neither Rust nor Go support bitfields in the
core language. This was a deliberate design decision of their respective
designers. I can't speak for Go as I'm not a Go developer, but Rust does have a
very well-known, well-supported and very common external crate ("bitflags")
that allows very ergonomic semantics for definition of packed booleans. As an
example here's the flags for domain create, as spitted out by the generator I
have.

(comments removed for brevity). This defines a bitmap indexed by the flags
type, represented by a 32bit primitive. It's type-safe from the PoV that I
can't just write 1 << 15 to a variable of this type and expect anything but
screams from the compiler.

  bitflags! {
      #[repr(C)]
      #[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
      pub struct XenDomaincreateFlags: u32 {
          const Hvm = 1 << 0;
          const Hap = 1 << 1;
          const S3Integrity = 1 << 2;
          const OosOff = 1 << 3;
          const XsDomain = 1 << 4;
          const Iommu = 1 << 5;
          const NestedVirt = 1 << 6;
          const Vpmu = 1 << 7;
      }
  }

This enables callers to have type-safe variables in a pretty ergonomic fashion:

  let flags = XenDomainCreateFlags::Hvm |
              XenDomainCreateFlags::Hap |
              XenDomainCreateFlags::Iommu;

and assignments to its related struct would follow regular assignment rules.

##################################################################
# IOW
##################################################################

Supporting general bitfields is annoying, even in C. Adding support in IDL for
them is a headache if we want the descriptions to help us catch mistakes and
language support is poor using this constructs (at best).

Can we please get rid of them?

>
> > I'd like to have a certain consensus about this general point before going
> > establishing this restriction in the IDL system I'm working on.
> > 
> > My preference would be to fold everything into a single patch if we decide to
> > follow through with this particular case. As I said before, the split is
> > artificial for review.
>
> That's not just a preference, but a requirement, or else the build will break
> in the middle of the series (harming bisection at the very least).
>
> Jan

Yes, indeed. As I said, I'm more interested in the discussion now rather than
the details of committing it.

Cheers,
Alejandro

Re: [RFC PATCH 0/6] xen/abi: On wide bitfields inside primitive types

[Jan Beulich]

On 30.10.2024 16:08, Alejandro Vallejo wrote:
> 
> In the course of preparing this answer I just noticed that altp2m_opts suffers
> from the exact same annoyance, with the exact same fix. I just noticed while
> rebasing my Rust branch.

Hardly the only other one. See GTF_type_mask or XEN_DOMCTL_PFINFO_LTAB_MASK.

> On Wed Oct 30, 2024 at 9:14 AM GMT, Jan Beulich wrote:
>> On 29.10.2024 19:16, Alejandro Vallejo wrote:
>>> Non-boolean bitfields in the hypercall ABI make it fairly inconvenient to
>>> create bindings for any language because (a) they are always ad-hoc and are
>>> subject to restrictions regular fields are not (b) require boilerplate that
>>> regular fields do not and (c) might not even be part of the core language,
>>> forcing avoidable external libraries into any sort of generic library.
>>>
>>> This patch (it's a series merely to split roughly by maintainer) is one such
>>> case that I happened to spot while playing around. It's the grant_version
>>> field, buried under an otherwise empty grant_opts.
>>>
>>> The invariant I'd like to (slowly) introduce and discuss is that fields may
>>> have bitflags (e.g: a packed array of booleans indexed by some enumerated
>>> type), but not be mixed with wider fields in the same primitive type. This
>>> ensures any field containing an integer of any kind can be referred by pointer
>>> and treated the same way as any other with regards to sizeof() and the like.
>>
>> While I don't strictly mind, I'm also not really seeing why taking addresses
>> or applying sizeof() would be commonly necessary. Can you perhaps provide a
>> concrete example of where the present way of dealing with grant max version
>> is getting in the way? After all your use of the term "bitfield" doesn't
>> really mean C's understanding of it, so especially (c) above escapes me to a
>> fair degree.
> 
> Wall of text ahead, but I'll try to stay on point. The rationale should become
> a lot clearer after I send an RFC series with initial code to autogenerate some
> hypercall payloads from markup. The biggest question is: Can I create a
> definition language such that (a) it precisely represents the Xen ABI and (b)
> is fully type-safe under modern strongly-typed languages?
> 
> I already have a backbone I can define the ABI in, so my options when I hit
> some impedance mismatch are:
> 
>   1. Change the ABI so it matches better my means of defining it.
>   2. Change the means to define so it captures the existing ABI better.
> 
> Most of the work I've done has moved in the (2) direction so far, but I found a
> number of pain points when mapping the existing ABI to Rust that, while not
> impossible to work around, are quite annoying for no clear benefit. If
> possible, I'd like to simplify the cognitive load involved in defining, using
> and updating hypercalls rather than bending over backwards to support a
> construct that provides no real benefit. IOW: If I can define an ABI that is
> _simpler_, it follows that it's also easier to not make mistakes and it's
> easier to generate code for it.
> 
> The use of packed fields is one such case. Even in C, we create extra macros
> for creating a field, modifying it, fetching it, etc. Patches 2-6 are strict
> code removals. And even in the most extreme cases the space savings are largely
> irrelevant because the hypercall has a fixed size. We do want to pack _flags_
> as otherwise the payload size would explode pretty quickly on hypercalls with
> tons of boolean options, but I'm not aware of that being problematic for wider
> subfields (like the grant max version).
> 
> Now, being more concrete...
> 
> ##################################################################
> # IDL is simpler if the size is a property of the type
> ##################################################################
> 
> Consider the definition of the (new) max_grant_version type under the IDL I'm
> working on (it's TOML, but I don't particularly care about which markup we end
> up using).
> 
>   [[enums]]
>   name = "xen_domaincreate_max_grant_version"
>   description = "Content of the `max_grant_version` field of the domain creation hypercall."
>   typ = { tag = "u8" }
> 
>   [[enums.variants]]
>   name = "off"
>   description = "Must be used with gnttab support compiled out"
>   value = 0
> 
>   [[enums.variants]]
>   name = "v1"
>   description = "Allow the domain to use up to gnttab_v1"
>   value = 1
> 
>   [[enums.variants]]
>   name = "v2"
>   description = "Allow the domain to use up to gnttab_v2"
>   value = 2
> 
> Note that I can define a type being enumerated, can choose its specific
> variants and its width is a property of the type itself. With bitfields you're
> always in a weird position of the width not being part of the type that goes
> into it.
> 
> Should I need it as a field somewhere, then...
> 
>   [[structs.fields]]
>   name = "max_grant_version"
>   description = "Maximum grant table version the domain may be bumped to"
>   typ = { tag = "enum", args = "xen_domaincreate_max_grant_version" }
> 
> ... at which point the size of the field is given by an intrinsic property of
> the type (the typ property on the enums table) I previously defined. It's
> extensible, composable and allows me to generate readable code in both C and
> Rust.
> 
> Should I need to support full bitfields I would require a means of stating the
> start and end bits of every field, which is very bad for the sanity of whoever
> wants to maintain coherency in the ABI.
> 
> ##################################################################
> # Rust and Go don't like bitfields...
> ##################################################################
> 
> ... and neither does C, even if for historic reasons they do exist in the
> standard.

I don't think that's just for historic reasons. To interface with hardware,
alternative approaches are often more cumbersome. See how we're (slowly)
moving to using bitfields more in Xen, in favor of tons of #define-s and
more or less open-coded masking operations.

> On a slight tangent, neither Rust nor Go support bitfields in the
> core language. This was a deliberate design decision of their respective
> designers. I can't speak for Go as I'm not a Go developer, but Rust does have a
> very well-known, well-supported and very common external crate ("bitflags")
> that allows very ergonomic semantics for definition of packed booleans. As an
> example here's the flags for domain create, as spitted out by the generator I
> have.
> 
> (comments removed for brevity). This defines a bitmap indexed by the flags
> type, represented by a 32bit primitive. It's type-safe from the PoV that I
> can't just write 1 << 15 to a variable of this type and expect anything but
> screams from the compiler.
> 
>   bitflags! {
>       #[repr(C)]
>       #[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
>       pub struct XenDomaincreateFlags: u32 {
>           const Hvm = 1 << 0;
>           const Hap = 1 << 1;
>           const S3Integrity = 1 << 2;
>           const OosOff = 1 << 3;
>           const XsDomain = 1 << 4;
>           const Iommu = 1 << 5;
>           const NestedVirt = 1 << 6;
>           const Vpmu = 1 << 7;
>       }
>   }
> 
> This enables callers to have type-safe variables in a pretty ergonomic fashion:
> 
>   let flags = XenDomainCreateFlags::Hvm |
>               XenDomainCreateFlags::Hap |
>               XenDomainCreateFlags::Iommu;
> 
> and assignments to its related struct would follow regular assignment rules.
> 
> ##################################################################
> # IOW
> ##################################################################
> 
> Supporting general bitfields is annoying, even in C. Adding support in IDL for
> them is a headache if we want the descriptions to help us catch mistakes and
> language support is poor using this constructs (at best).
> 
> Can we please get rid of them?

I remain undecided. If we set forth as a goal to avoid such in the new ABI
that has been mentioned for many years, that would be a more clear cut.

Just taking the example of the gnttab max version: By switching to

    uint8_t max_grant_version;
    uint8_t rsvd0[3];

you actively make it more cumbersome to re-use the rest of the 32-bit field
as (boolean) flags. It may reasonably work for the first 8, with early
provisions it may also work for 16, yet it'll be harder / uglier for more
than that. Allowing such is, after all, the idea behind the present
"packing". (The issue would similarly exist in the new ABI, yet we could
simply decide up front that we're willing to pay that price, perhaps even
outlining up front how to best limit undesirable effects when doing so.)

Just to mention it - you didn't really answer the sizeof() / address-taking
question I raised. You provided a lot of other _useful_ background though.

Jan

Re: [RFC PATCH 0/6] xen/abi: On wide bitfields inside primitive types

[Alejandro Vallejo]

On Thu Oct 31, 2024 at 7:57 AM GMT, Jan Beulich wrote:
> On 30.10.2024 16:08, Alejandro Vallejo wrote:
> > 
> > In the course of preparing this answer I just noticed that altp2m_opts suffers
> > from the exact same annoyance, with the exact same fix. I just noticed while
> > rebasing my Rust branch.
>
> Hardly the only other one. See GTF_type_mask or XEN_DOMCTL_PFINFO_LTAB_MASK.
>
> > On Wed Oct 30, 2024 at 9:14 AM GMT, Jan Beulich wrote:
> >> On 29.10.2024 19:16, Alejandro Vallejo wrote:
> >>> Non-boolean bitfields in the hypercall ABI make it fairly inconvenient to
> >>> create bindings for any language because (a) they are always ad-hoc and are
> >>> subject to restrictions regular fields are not (b) require boilerplate that
> >>> regular fields do not and (c) might not even be part of the core language,
> >>> forcing avoidable external libraries into any sort of generic library.
> >>>
> >>> This patch (it's a series merely to split roughly by maintainer) is one such
> >>> case that I happened to spot while playing around. It's the grant_version
> >>> field, buried under an otherwise empty grant_opts.
> >>>
> >>> The invariant I'd like to (slowly) introduce and discuss is that fields may
> >>> have bitflags (e.g: a packed array of booleans indexed by some enumerated
> >>> type), but not be mixed with wider fields in the same primitive type. This
> >>> ensures any field containing an integer of any kind can be referred by pointer
> >>> and treated the same way as any other with regards to sizeof() and the like.
> >>
> >> While I don't strictly mind, I'm also not really seeing why taking addresses
> >> or applying sizeof() would be commonly necessary. Can you perhaps provide a
> >> concrete example of where the present way of dealing with grant max version
> >> is getting in the way? After all your use of the term "bitfield" doesn't
> >> really mean C's understanding of it, so especially (c) above escapes me to a
> >> fair degree.
> > 
> > Wall of text ahead, but I'll try to stay on point. The rationale should become
> > a lot clearer after I send an RFC series with initial code to autogenerate some
> > hypercall payloads from markup. The biggest question is: Can I create a
> > definition language such that (a) it precisely represents the Xen ABI and (b)
> > is fully type-safe under modern strongly-typed languages?
> > 
> > I already have a backbone I can define the ABI in, so my options when I hit
> > some impedance mismatch are:
> > 
> >   1. Change the ABI so it matches better my means of defining it.
> >   2. Change the means to define so it captures the existing ABI better.
> > 
> > Most of the work I've done has moved in the (2) direction so far, but I found a
> > number of pain points when mapping the existing ABI to Rust that, while not
> > impossible to work around, are quite annoying for no clear benefit. If
> > possible, I'd like to simplify the cognitive load involved in defining, using
> > and updating hypercalls rather than bending over backwards to support a
> > construct that provides no real benefit. IOW: If I can define an ABI that is
> > _simpler_, it follows that it's also easier to not make mistakes and it's
> > easier to generate code for it.
> > 
> > The use of packed fields is one such case. Even in C, we create extra macros
> > for creating a field, modifying it, fetching it, etc. Patches 2-6 are strict
> > code removals. And even in the most extreme cases the space savings are largely
> > irrelevant because the hypercall has a fixed size. We do want to pack _flags_
> > as otherwise the payload size would explode pretty quickly on hypercalls with
> > tons of boolean options, but I'm not aware of that being problematic for wider
> > subfields (like the grant max version).
> > 
> > Now, being more concrete...
> > 
> > ##################################################################
> > # IDL is simpler if the size is a property of the type
> > ##################################################################
> > 
> > Consider the definition of the (new) max_grant_version type under the IDL I'm
> > working on (it's TOML, but I don't particularly care about which markup we end
> > up using).
> > 
> >   [[enums]]
> >   name = "xen_domaincreate_max_grant_version"
> >   description = "Content of the `max_grant_version` field of the domain creation hypercall."
> >   typ = { tag = "u8" }
> > 
> >   [[enums.variants]]
> >   name = "off"
> >   description = "Must be used with gnttab support compiled out"
> >   value = 0
> > 
> >   [[enums.variants]]
> >   name = "v1"
> >   description = "Allow the domain to use up to gnttab_v1"
> >   value = 1
> > 
> >   [[enums.variants]]
> >   name = "v2"
> >   description = "Allow the domain to use up to gnttab_v2"
> >   value = 2
> > 
> > Note that I can define a type being enumerated, can choose its specific
> > variants and its width is a property of the type itself. With bitfields you're
> > always in a weird position of the width not being part of the type that goes
> > into it.
> > 
> > Should I need it as a field somewhere, then...
> > 
> >   [[structs.fields]]
> >   name = "max_grant_version"
> >   description = "Maximum grant table version the domain may be bumped to"
> >   typ = { tag = "enum", args = "xen_domaincreate_max_grant_version" }
> > 
> > ... at which point the size of the field is given by an intrinsic property of
> > the type (the typ property on the enums table) I previously defined. It's
> > extensible, composable and allows me to generate readable code in both C and
> > Rust.
> > 
> > Should I need to support full bitfields I would require a means of stating the
> > start and end bits of every field, which is very bad for the sanity of whoever
> > wants to maintain coherency in the ABI.
> > 
> > ##################################################################
> > # Rust and Go don't like bitfields...
> > ##################################################################
> > 
> > ... and neither does C, even if for historic reasons they do exist in the
> > standard.
>
> I don't think that's just for historic reasons. To interface with hardware,
> alternative approaches are often more cumbersome. See how we're (slowly)
> moving to using bitfields more in Xen, in favor of tons of #define-s and
> more or less open-coded masking operations.
>
> > On a slight tangent, neither Rust nor Go support bitfields in the
> > core language. This was a deliberate design decision of their respective
> > designers. I can't speak for Go as I'm not a Go developer, but Rust does have a
> > very well-known, well-supported and very common external crate ("bitflags")
> > that allows very ergonomic semantics for definition of packed booleans. As an
> > example here's the flags for domain create, as spitted out by the generator I
> > have.
> > 
> > (comments removed for brevity). This defines a bitmap indexed by the flags
> > type, represented by a 32bit primitive. It's type-safe from the PoV that I
> > can't just write 1 << 15 to a variable of this type and expect anything but
> > screams from the compiler.
> > 
> >   bitflags! {
> >       #[repr(C)]
> >       #[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
> >       pub struct XenDomaincreateFlags: u32 {
> >           const Hvm = 1 << 0;
> >           const Hap = 1 << 1;
> >           const S3Integrity = 1 << 2;
> >           const OosOff = 1 << 3;
> >           const XsDomain = 1 << 4;
> >           const Iommu = 1 << 5;
> >           const NestedVirt = 1 << 6;
> >           const Vpmu = 1 << 7;
> >       }
> >   }
> > 
> > This enables callers to have type-safe variables in a pretty ergonomic fashion:
> > 
> >   let flags = XenDomainCreateFlags::Hvm |
> >               XenDomainCreateFlags::Hap |
> >               XenDomainCreateFlags::Iommu;
> > 
> > and assignments to its related struct would follow regular assignment rules.
> > 
> > ##################################################################
> > # IOW
> > ##################################################################
> > 
> > Supporting general bitfields is annoying, even in C. Adding support in IDL for
> > them is a headache if we want the descriptions to help us catch mistakes and
> > language support is poor using this constructs (at best).
> > 
> > Can we please get rid of them?
>
> I remain undecided. If we set forth as a goal to avoid such in the new ABI
> that has been mentioned for many years, that would be a more clear cut.

That's the direction I want to move on, but there's a chicken-and-egg
situation. I want to avoid the effort I'm putting in adding cross-language
support to the hypercall boundary to be necessarily gated on introducing a
whole new ABI that hasn't yet been openly discussed. And at the same time, I'd
like the IDL to provide comfortable means to statically validate the invariants
we impose on the new ABI. Simpler semantics make for simpler validation.

IOW, it's not modifying EVERYTHING so it fits in my little hole that I'm after,
but a general nod that for future changes this would be taken into account to
avoid taking one forward and two back every other fortnight

>
> Just taking the example of the gnttab max version: By switching to
>
>     uint8_t max_grant_version;
>     uint8_t rsvd0[3];
>
> you actively make it more cumbersome to re-use the rest of the 32-bit field
> as (boolean) flags.

Only the following 8 bits. The last 16 can become a uint16_t worth of flags
without issues. Note that this minor annoyance is becasuse of the redefinition
of a uint32_t as a composite, not because of having unpacked fields.

> It may reasonably work for the first 8, with early
> provisions it may also work for 16, yet it'll be harder / uglier for more
> than that.

But at that point you're discussing the effects of a binary-compatible
re-definition of an existing field, not the effects of new fields on any
hypercall. While stylistically desirable, nothing forces grant-related fields
to be adjacent in the struct.

You are right that should we need 20 new bits this reserved array wouldn't be
very adequate to hold them. But it doesn't have to. We're free to allocate a
new field, just as we would if we needed 31 new bits rather than 20. In
practice when we need a few bits in the future I'd expect to just allocate
the last 16bits out of the array, turn them into a uint16_t, and use that as a
flags field.

Or I can redefine the whole 32bits to be the max_grant_version/altp2m_mode.

Both options are binary compatible, and I went for this one in order to give
back some unused space.

> Allowing such is, after all, the idea behind the present
> "packing". (The issue would similarly exist in the new ABI, yet we could
> simply decide up front that we're willing to pay that price, perhaps even
> outlining up front how to best limit undesirable effects when doing so.)

Just a note that adding fields need not break the ABI. It happens to do so in
this hypercall because "arch" can itself change, but that wouldn't be an ABI
breakage if it was passed as a pointer instead (which we ought to do if we
happen to break the ABI again before new ABI lands).

>
> Just to mention it - you didn't really answer the sizeof() / address-taking
> question I raised. You provided a lot of other _useful_ background though.
>
> Jan

There's nothing major in those two. It's an existing restriction on packed
fields we could lift by unpacking them. The real meat is in the rest of the
background rationale.

Things are slightly more annoying in Rust because I might be forced to give a
mutable reference to the whole packed field (and hope the callee only mutates
what I want it to) in cases where I would rather give a single mutable
reference to one of its constituents instead. Not major, but annoying.

By far the biggest sigh would be having to describe hypercalls by bit offsets
rather than simpler primitive types.

Cheers,
Alejandro