On Mon, 15 Apr 2024, Andrew Cooper wrote:
> RFC. In theory this is a great way to avoid some of the spiketraps involved
> with C being the official representation.
>
> However, this doesn't build. gnttab_transfer has a layout that requires a
> CONFIG_COMPAT if we want to satisfy -Wpadding for both forms of the structure.
>
> Thoughts on whether this cross-check is worthwhile-enough to warrant the
> ifdefary?
I like this patch and I think we have no choice but going in this
direction and adding all the padding explicitly with any related
necessary ifdefary.
The only question for me is whether to:
1) add -Wpadding
2) add __packed__
3) do both
I think it is important to add __packed__ to the headers to clear out
any misconceptions about possible hidden paddings and get a
correct-by-default behavior for anyone that would import the headers
into their own projects.
The only issue is that __packed__ makes -Wpadding not useful. We could
technically add both if we disable __packed__ for the -Wpadding build.
For instance we could use __packed which is defined by Xen, and change
the definition of __packed to nothing for the -Wpadding build.
That way we get both the nice -Wpadding checks and also the nice
obvious-by-default __packed__.
> ~Andrew
> ---
> CC: George Dunlap <george.dun...@citrix.com>
> CC: Jan Beulich <jbeul...@suse.com>
> CC: Stefano Stabellini <sstabell...@kernel.org>
> CC: Julien Grall <jul...@xen.org>
> ---
> xen/common/Makefile | 1 +
> xen/common/hdr-chk.c | 13 +++++++++++++
> xen/include/public/grant_table.h | 7 +++++++
> 3 files changed, 21 insertions(+)
> create mode 100644 xen/common/hdr-chk.c
>
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index d512cad5243f..dbbda70829f1 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -15,6 +15,7 @@ obj-y += event_fifo.o
> obj-$(CONFIG_GRANT_TABLE) += grant_table.o
> obj-y += guestcopy.o
> obj-y += gzip/
> +obj-y += hdr-chk.o
> obj-$(CONFIG_HYPFS) += hypfs.o
> obj-$(CONFIG_IOREQ_SERVER) += ioreq.o
> obj-y += irq.o
> diff --git a/xen/common/hdr-chk.c b/xen/common/hdr-chk.c
> new file mode 100644
> index 000000000000..1c7a509dcd06
> --- /dev/null
> +++ b/xen/common/hdr-chk.c
> @@ -0,0 +1,13 @@
> +#include <xen/stdint.h>
> +
> +#include <public/xen.h>
> +
> +#pragma GCC diagnostic error "-Wpadded"
> +
> +#include <public/grant_table.h>
> +
> +#ifdef CONFIG_COMPAT
> +
> +#include <compat/grant_table.h>
> +
> +#endif /* CONFIG_COMPAT */
> diff --git a/xen/include/public/grant_table.h
> b/xen/include/public/grant_table.h
> index 1dfa17a6d02a..a66c77d0166c 100644
> --- a/xen/include/public/grant_table.h
> +++ b/xen/include/public/grant_table.h
> @@ -355,6 +355,7 @@ struct gnttab_unmap_grant_ref {
> grant_handle_t handle;
> /* OUT parameters. */
> int16_t status; /* => enum grant_status */
> + uint16_t _pad0;
> };
> typedef struct gnttab_unmap_grant_ref gnttab_unmap_grant_ref_t;
> DEFINE_XEN_GUEST_HANDLE(gnttab_unmap_grant_ref_t);
> @@ -371,6 +372,7 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_unmap_grant_ref_t);
> struct gnttab_setup_table {
> /* IN parameters. */
> domid_t dom;
> + uint16_t _pad0;
> uint32_t nr_frames;
> /* OUT parameters. */
> int16_t status; /* => enum grant_status */
> @@ -409,9 +411,12 @@ struct gnttab_transfer {
> /* IN parameters. */
> xen_pfn_t mfn;
> domid_t domid;
> + uint16_t _pad0;
> grant_ref_t ref;
> /* OUT parameters. */
> int16_t status;
> + uint16_t _pad1;
> + /* XXX compat-dependent padding. */
> };
> typedef struct gnttab_transfer gnttab_transfer_t;
> DEFINE_XEN_GUEST_HANDLE(gnttab_transfer_t);
> @@ -468,10 +473,12 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_copy_t);
> struct gnttab_query_size {
> /* IN parameters. */
> domid_t dom;
> + uint16_t _ign1;
> /* OUT parameters. */
> uint32_t nr_frames;
> uint32_t max_nr_frames;
> int16_t status; /* => enum grant_status */
> + uint16_t _ign2;
> };
> typedef struct gnttab_query_size gnttab_query_size_t;
> DEFINE_XEN_GUEST_HANDLE(gnttab_query_size_t);
> --
> 2.30.2
>
