[PATCH v3 02/22] include/xen/slr-table.h: Secure Launch Resource Table definitions

Sergii Dmytruk posted 22 patches 5 months ago
[PATCH v3 02/22] include/xen/slr-table.h: Secure Launch Resource Table definitions
Posted by Sergii Dmytruk 5 months ago
The file provides constants, structures and several helper functions for
parsing SLRT.

The data described by the structures is passed to Xen by a bootloader
which initiated DRTM.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
---
 xen/include/xen/slr-table.h | 276 ++++++++++++++++++++++++++++++++++++
 1 file changed, 276 insertions(+)
 create mode 100644 xen/include/xen/slr-table.h

diff --git a/xen/include/xen/slr-table.h b/xen/include/xen/slr-table.h
new file mode 100644
index 0000000000..fb36d06fa8
--- /dev/null
+++ b/xen/include/xen/slr-table.h
@@ -0,0 +1,276 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+/*
+ *  Copyright (c) 2025 Apertus Solutions, LLC
+ *  Copyright (c) 2025 Oracle and/or its affiliates.
+ *  Copyright (c) 2025 3mdeb Sp. z o.o
+ *
+ *  Secure Launch Resource Table definitions.  This table is passed to Xen by
+ *  a bootloader and contains information about pre-DRTM state necessary to
+ *  restore hardware configuration, where to find TPM event log, how to call
+ *  back into the bootloader (for EFI case) and what needs to be measured by
+ *  Xen.  In other words, this is similar to MBI in Multiboot Specification.
+ *
+ *  Specification:
+ *    https://trenchboot.org/specifications/Secure_Launch/
+ */
+
+#ifndef XEN_SLR_TABLE_H
+#define XEN_SLR_TABLE_H
+
+#include <xen/types.h>
+
+#define UEFI_SLR_TABLE_GUID \
+    { 0x877a9b2aU, 0x0385, 0x45d1, { 0xa0, 0x34, 0x9d, 0xac, 0x9c, 0x9e, 0x56, 0x5f } }
+
+/* SLR table header values */
+#define SLR_TABLE_MAGIC         0x4452544d
+#define SLR_TABLE_REVISION      1
+
+/* Current revisions for the policy and UEFI config */
+#define SLR_POLICY_REVISION         1
+#define SLR_UEFI_CONFIG_REVISION    1
+
+/* SLR defined architectures */
+#define SLR_INTEL_TXT   1
+#define SLR_AMD_SKINIT  2
+
+/* SLR defined bootloaders */
+#define SLR_BOOTLOADER_INVALID  0
+#define SLR_BOOTLOADER_GRUB     1
+
+/* Log formats */
+#define SLR_DRTM_TPM12_LOG      1
+#define SLR_DRTM_TPM20_LOG      2
+
+/* DRTM Policy Entry Flags */
+#define SLR_POLICY_FLAG_MEASURED    0x1
+#define SLR_POLICY_IMPLICIT_SIZE    0x2
+
+/* Array Lengths */
+#define TPM_EVENT_INFO_LENGTH       32
+#define TXT_VARIABLE_MTRRS_LENGTH   32
+
+/* Tags */
+#define SLR_ENTRY_INVALID       0x0000
+#define SLR_ENTRY_DL_INFO       0x0001
+#define SLR_ENTRY_LOG_INFO      0x0002
+#define SLR_ENTRY_DRTM_POLICY   0x0003
+#define SLR_ENTRY_INTEL_INFO    0x0004
+#define SLR_ENTRY_AMD_INFO      0x0005
+#define SLR_ENTRY_ARM_INFO      0x0006
+#define SLR_ENTRY_UEFI_INFO     0x0007
+#define SLR_ENTRY_UEFI_CONFIG   0x0008
+#define SLR_ENTRY_END           0xffff
+
+/* Entity Types */
+#define SLR_ET_UNSPECIFIED        0x0000
+#define SLR_ET_SLRT               0x0001
+#define SLR_ET_BOOT_PARAMS        0x0002
+#define SLR_ET_SETUP_DATA         0x0003
+#define SLR_ET_CMDLINE            0x0004
+#define SLR_ET_UEFI_MEMMAP        0x0005
+#define SLR_ET_RAMDISK            0x0006
+#define SLR_ET_MULTIBOOT2_INFO    0x0007
+#define SLR_ET_MULTIBOOT2_MODULE  0x0008
+#define SLR_ET_TXT_OS2MLE         0x0010
+#define SLR_ET_UNUSED             0xffff
+
+/*
+ * Primary SLR Table Header
+ */
+struct slr_table
+{
+    uint32_t magic;
+    uint16_t revision;
+    uint16_t architecture;
+    uint32_t size;
+    uint32_t max_size;
+    /* entries[] */
+} __packed;
+
+/*
+ * Common SLRT Table Header
+ */
+struct slr_entry_hdr
+{
+    uint32_t tag;
+    uint32_t size;
+} __packed;
+
+/*
+ * Boot loader context
+ */
+struct slr_bl_context
+{
+    uint16_t bootloader;
+    uint16_t reserved[3];
+    uint64_t context;
+} __packed;
+
+/*
+ * Prototype of a function pointed to by slr_entry_dl_info::dl_handler.
+ */
+typedef void (*dl_handler_func)(struct slr_bl_context *bl_context);
+
+/*
+ * DRTM Dynamic Launch Configuration
+ */
+struct slr_entry_dl_info
+{
+    struct slr_entry_hdr hdr;
+    uint64_t dce_size;
+    uint64_t dce_base;
+    uint64_t dlme_size;
+    uint64_t dlme_base;
+    uint64_t dlme_entry;
+    struct slr_bl_context bl_context;
+    uint64_t dl_handler;
+} __packed;
+
+/*
+ * TPM Log Information
+ */
+struct slr_entry_log_info
+{
+    struct slr_entry_hdr hdr;
+    uint16_t format;
+    uint16_t reserved;
+    uint32_t size;
+    uint64_t addr;
+} __packed;
+
+/*
+ * DRTM Measurement Entry
+ */
+struct slr_policy_entry
+{
+    uint16_t pcr;
+    uint16_t entity_type;
+    uint16_t flags;
+    uint16_t reserved;
+    uint64_t size;
+    uint64_t entity;
+    char evt_info[TPM_EVENT_INFO_LENGTH];
+} __packed;
+
+/*
+ * DRTM Measurement Policy
+ */
+struct slr_entry_policy
+{
+    struct slr_entry_hdr hdr;
+    uint16_t reserved[2];
+    uint16_t revision;
+    uint16_t nr_entries;
+    struct slr_policy_entry policy_entries[];
+} __packed;
+
+/*
+ * Secure Launch defined MTRR saving structures
+ */
+struct slr_txt_mtrr_pair
+{
+    uint64_t mtrr_physbase;
+    uint64_t mtrr_physmask;
+} __packed;
+
+struct slr_txt_mtrr_state
+{
+    uint64_t default_mem_type;
+    uint64_t mtrr_vcnt;
+    struct slr_txt_mtrr_pair mtrr_pair[TXT_VARIABLE_MTRRS_LENGTH];
+} __packed;
+
+/*
+ * Intel TXT Info table
+ */
+struct slr_entry_intel_info
+{
+    struct slr_entry_hdr hdr;
+    uint64_t boot_params_base;
+    uint64_t txt_heap;
+    uint64_t saved_misc_enable_msr;
+    struct slr_txt_mtrr_state saved_bsp_mtrrs;
+} __packed;
+
+/*
+ * AMD SKINIT Info table
+ */
+struct slr_entry_amd_info
+{
+    struct slr_entry_hdr hdr;
+    uint64_t next;
+    uint32_t type;
+    uint32_t len;
+    uint64_t slrt_size;
+    uint64_t slrt_base;
+    uint64_t boot_params_base;
+    uint16_t psp_version;
+    uint16_t reserved[3];
+} __packed;
+
+/*
+ * UEFI config measurement entry
+ */
+struct slr_uefi_cfg_entry
+{
+    uint16_t pcr;
+    uint16_t reserved;
+    uint32_t size;
+    uint64_t cfg; /* address or value */
+    char evt_info[TPM_EVENT_INFO_LENGTH];
+} __packed;
+
+struct slr_entry_uefi_config
+{
+    struct slr_entry_hdr hdr;
+    uint16_t reserved[2];
+    uint16_t revision;
+    uint16_t nr_entries;
+    struct slr_uefi_cfg_entry uefi_cfg_entries[];
+} __packed;
+
+static inline const void *
+slr_end_of_entries(const struct slr_table *table)
+{
+    return (const void *)table + table->size;
+}
+
+static inline const struct slr_entry_hdr *
+slr_next_entry(const struct slr_table *table, const struct slr_entry_hdr *curr)
+{
+    const struct slr_entry_hdr *next = (void *)curr + curr->size;
+
+    if ( (void *)next + sizeof(*next) > slr_end_of_entries(table) )
+        return NULL;
+    if ( next->tag == SLR_ENTRY_END )
+        return NULL;
+    if ( (void *)next + next->size > slr_end_of_entries(table) )
+        return NULL;
+
+    return next;
+}
+
+static inline const struct slr_entry_hdr *
+slr_next_entry_by_tag(const struct slr_table *table,
+                      const struct slr_entry_hdr *entry,
+                      uint16_t tag)
+{
+    if ( !entry ) /* Start from the beginning */
+        entry = (void *)table + sizeof(*table);
+
+    for ( ; ; )
+    {
+        if ( entry->tag == tag )
+            return entry;
+
+        entry = slr_next_entry(table, entry);
+        if ( !entry )
+            return NULL;
+    }
+
+    return NULL;
+}
+
+#endif /* XEN_SLR_TABLE_H */
-- 
2.49.0
Re: [PATCH v3 02/22] include/xen/slr-table.h: Secure Launch Resource Table definitions
Posted by Jan Beulich 4 months ago
On 30.05.2025 15:17, Sergii Dmytruk wrote:
> The file provides constants, structures and several helper functions for
> parsing SLRT.
> 
> The data described by the structures is passed to Xen by a bootloader
> which initiated DRTM.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
> Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
> ---
>  xen/include/xen/slr-table.h | 276 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 276 insertions(+)
>  create mode 100644 xen/include/xen/slr-table.h

Btw, please don't forget to Cc maintainers of code you're changing / adding.

> +/*
> + * Prototype of a function pointed to by slr_entry_dl_info::dl_handler.
> + */
> +typedef void (*dl_handler_func)(struct slr_bl_context *bl_context);

I keep wondering why this ...

> +/*
> + * DRTM Dynamic Launch Configuration
> + */
> +struct slr_entry_dl_info
> +{
> +    struct slr_entry_hdr hdr;
> +    uint64_t dce_size;
> +    uint64_t dce_base;
> +    uint64_t dlme_size;
> +    uint64_t dlme_base;
> +    uint64_t dlme_entry;
> +    struct slr_bl_context bl_context;
> +    uint64_t dl_handler;

... then isn't used right here, instead requiring a cast somewhere (presumably,
as code using this isn't visible in this patch).

> +} __packed;

I similarly keep wondering why there are all these packed attributes here, when
(afaics) all of the structures are defined in a way that any padding is explicit
anyway.

> +static inline const struct slr_entry_hdr *
> +slr_next_entry(const struct slr_table *table, const struct slr_entry_hdr *curr)
> +{
> +    const struct slr_entry_hdr *next = (void *)curr + curr->size;
> +
> +    if ( (void *)next + sizeof(*next) > slr_end_of_entries(table) )
> +        return NULL;
> +    if ( next->tag == SLR_ENTRY_END )
> +        return NULL;
> +    if ( (void *)next + next->size > slr_end_of_entries(table) )
> +        return NULL;
> +
> +    return next;
> +}
> +
> +static inline const struct slr_entry_hdr *
> +slr_next_entry_by_tag(const struct slr_table *table,
> +                      const struct slr_entry_hdr *entry,
> +                      uint16_t tag)
> +{
> +    if ( !entry ) /* Start from the beginning */
> +        entry = (void *)table + sizeof(*table);
> +
> +    for ( ; ; )
> +    {
> +        if ( entry->tag == tag )
> +            return entry;
> +
> +        entry = slr_next_entry(table, entry);
> +        if ( !entry )
> +            return NULL;
> +    }
> +
> +    return NULL;
> +}

For both of the functions, again: Please don't cast away const-ness.

Jan
Re: [PATCH v3 02/22] include/xen/slr-table.h: Secure Launch Resource Table definitions
Posted by Sergii Dmytruk 3 months, 3 weeks ago
On Wed, Jul 02, 2025 at 04:36:27PM +0200, Jan Beulich wrote:
> On 30.05.2025 15:17, Sergii Dmytruk wrote:
> > The file provides constants, structures and several helper functions for
> > parsing SLRT.
> >
> > The data described by the structures is passed to Xen by a bootloader
> > which initiated DRTM.
> >
> > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> > Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
> > Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
> > ---
> >  xen/include/xen/slr-table.h | 276 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 276 insertions(+)
> >  create mode 100644 xen/include/xen/slr-table.h
>
> Btw, please don't forget to Cc maintainers of code you're changing / adding.

What do you mean?  I'm running add_maintainers.pl on the patches.

> > +/*
> > + * Prototype of a function pointed to by slr_entry_dl_info::dl_handler.
> > + */
> > +typedef void (*dl_handler_func)(struct slr_bl_context *bl_context);
>
> I keep wondering why this ...
>
> > +/*
> > + * DRTM Dynamic Launch Configuration
> > + */
> > +struct slr_entry_dl_info
> > +{
> > +    struct slr_entry_hdr hdr;
> > +    uint64_t dce_size;
> > +    uint64_t dce_base;
> > +    uint64_t dlme_size;
> > +    uint64_t dlme_base;
> > +    uint64_t dlme_entry;
> > +    struct slr_bl_context bl_context;
> > +    uint64_t dl_handler;
>
> ... then isn't used right here, instead requiring a cast somewhere (presumably,
> as code using this isn't visible in this patch).

As was mentioned earlier: because size of a pointer between Xen and a
bootloader doesn't necessarily match.  What you're proposing can break
under certain conditions.

> > +} __packed;
>
> I similarly keep wondering why there are all these packed attributes here, when
> (afaics) all of the structures are defined in a way that any padding is explicit
> anyway.

As before: it won't hurt, it's future-proof, just in case and to let
reader of the code know the structure must have no padding.  If you want
them gone, I can do that, but I think it will make the code harder to
maintain.

> > +static inline const struct slr_entry_hdr *
> > +slr_next_entry(const struct slr_table *table, const struct slr_entry_hdr *curr)
> > +{
> > +    const struct slr_entry_hdr *next = (void *)curr + curr->size;
> > +
> > +    if ( (void *)next + sizeof(*next) > slr_end_of_entries(table) )
> > +        return NULL;
> > +    if ( next->tag == SLR_ENTRY_END )
> > +        return NULL;
> > +    if ( (void *)next + next->size > slr_end_of_entries(table) )
> > +        return NULL;
> > +
> > +    return next;
> > +}
> > +
> > +static inline const struct slr_entry_hdr *
> > +slr_next_entry_by_tag(const struct slr_table *table,
> > +                      const struct slr_entry_hdr *entry,
> > +                      uint16_t tag)
> > +{
> > +    if ( !entry ) /* Start from the beginning */
> > +        entry = (void *)table + sizeof(*table);
> > +
> > +    for ( ; ; )
> > +    {
> > +        if ( entry->tag == tag )
> > +            return entry;
> > +
> > +        entry = slr_next_entry(table, entry);
> > +        if ( !entry )
> > +            return NULL;
> > +    }
> > +
> > +    return NULL;
> > +}
>
> For both of the functions, again: Please don't cast away const-ness.
>
> Jan

You had commented on v2 after v3 was already sent.

Regards
Re: [PATCH v3 02/22] include/xen/slr-table.h: Secure Launch Resource Table definitions
Posted by Jan Beulich 3 months, 3 weeks ago
On 06.07.2025 18:55, Sergii Dmytruk wrote:
> On Wed, Jul 02, 2025 at 04:36:27PM +0200, Jan Beulich wrote:
>> On 30.05.2025 15:17, Sergii Dmytruk wrote:
>>> The file provides constants, structures and several helper functions for
>>> parsing SLRT.
>>>
>>> The data described by the structures is passed to Xen by a bootloader
>>> which initiated DRTM.
>>>
>>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>>> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
>>> Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
>>> ---
>>>  xen/include/xen/slr-table.h | 276 ++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 276 insertions(+)
>>>  create mode 100644 xen/include/xen/slr-table.h
>>
>> Btw, please don't forget to Cc maintainers of code you're changing / adding.
> 
> What do you mean?  I'm running add_maintainers.pl on the patches.

The Cc: list had none of the REST maintainers. (Whether there's a bug in the
script I can't tell.)

>>> +/*
>>> + * Prototype of a function pointed to by slr_entry_dl_info::dl_handler.
>>> + */
>>> +typedef void (*dl_handler_func)(struct slr_bl_context *bl_context);
>>
>> I keep wondering why this ...
>>
>>> +/*
>>> + * DRTM Dynamic Launch Configuration
>>> + */
>>> +struct slr_entry_dl_info
>>> +{
>>> +    struct slr_entry_hdr hdr;
>>> +    uint64_t dce_size;
>>> +    uint64_t dce_base;
>>> +    uint64_t dlme_size;
>>> +    uint64_t dlme_base;
>>> +    uint64_t dlme_entry;
>>> +    struct slr_bl_context bl_context;
>>> +    uint64_t dl_handler;
>>
>> ... then isn't used right here, instead requiring a cast somewhere (presumably,
>> as code using this isn't visible in this patch).
> 
> As was mentioned earlier: because size of a pointer between Xen and a
> bootloader doesn't necessarily match.  What you're proposing can break
> under certain conditions.

But the header isn't shared with a bootloader's code base. It's private to
Xen.

>>> +} __packed;
>>
>> I similarly keep wondering why there are all these packed attributes here, when
>> (afaics) all of the structures are defined in a way that any padding is explicit
>> anyway.
> 
> As before: it won't hurt, it's future-proof, just in case and to let
> reader of the code know the structure must have no padding.  If you want
> them gone, I can do that, but I think it will make the code harder to
> maintain.

Well, if there's a maintenance concern, then I would of course like to learn
about that. I could see such being the case if the file was imported from
somewhere. But with neither description nor any code comment not saying so,
that doesn't look to be the case.

Jan
Re: [PATCH v3 02/22] include/xen/slr-table.h: Secure Launch Resource Table definitions
Posted by Sergii Dmytruk 3 months, 3 weeks ago
On Mon, Jul 07, 2025 at 10:29:46AM +0200, Jan Beulich wrote:
> >> Btw, please don't forget to Cc maintainers of code you're changing / adding.
> >
> > What do you mean?  I'm running add_maintainers.pl on the patches.
>
> The Cc: list had none of the REST maintainers. (Whether there's a bug in the
> script I can't tell.)

Oh, looks like adding new entries to MAINTAINERS prevented application
of THE REST.  Will try running the script from master next time to
address this.

> >> ... then isn't used right here, instead requiring a cast somewhere (presumably,
> >> as code using this isn't visible in this patch).
> >
> > As was mentioned earlier: because size of a pointer between Xen and a
> > bootloader doesn't necessarily match.  What you're proposing can break
> > under certain conditions.
>
> But the header isn't shared with a bootloader's code base. It's private to
> Xen.

Yes, but sources of Xen be compiled with different size of a pointer
which messes up the interpretation of the data.  I tried using
BUILD_BUG_ON() to enforce the pointer is 64-bit and early code stopped
compiling.  The structures must not vary like that.

> >>> +} __packed;
> >>
> >> I similarly keep wondering why there are all these packed attributes here, when
> >> (afaics) all of the structures are defined in a way that any padding is explicit
> >> anyway.
> >
> > As before: it won't hurt, it's future-proof, just in case and to let
> > reader of the code know the structure must have no padding.  If you want
> > them gone, I can do that, but I think it will make the code harder to
> > maintain.
>
> Well, if there's a maintenance concern, then I would of course like to learn
> about that. I could see such being the case if the file was imported from
> somewhere. But with neither description nor any code comment not saying so,
> that doesn't look to be the case.
>
> Jan

While there is some synchronization to do, that's not what I meant, I
don't think it warrants writing the header in a special way.  __packed
just relieves anyone from checking for padding, while just to remove the
attribute one needs to go through the structures and make sure nothing
will change.

Regard
Re: [PATCH v3 02/22] include/xen/slr-table.h: Secure Launch Resource Table definitions
Posted by Jan Beulich 3 months, 3 weeks ago
On 07.07.2025 19:31, Sergii Dmytruk wrote:
> On Mon, Jul 07, 2025 at 10:29:46AM +0200, Jan Beulich wrote:
>>>> ... then isn't used right here, instead requiring a cast somewhere (presumably,
>>>> as code using this isn't visible in this patch).
>>>
>>> As was mentioned earlier: because size of a pointer between Xen and a
>>> bootloader doesn't necessarily match.  What you're proposing can break
>>> under certain conditions.
>>
>> But the header isn't shared with a bootloader's code base. It's private to
>> Xen.
> 
> Yes, but sources of Xen be compiled with different size of a pointer
> which messes up the interpretation of the data.  I tried using
> BUILD_BUG_ON() to enforce the pointer is 64-bit and early code stopped
> compiling.  The structures must not vary like that.

Hmm. Does early code actually need to have this struct exposed to it?

Jan
Re: [PATCH v3 02/22] include/xen/slr-table.h: Secure Launch Resource Table definitions
Posted by Sergii Dmytruk 3 months, 2 weeks ago
On Tue, Jul 08, 2025 at 08:52:36AM +0200, Jan Beulich wrote:
> On 07.07.2025 19:31, Sergii Dmytruk wrote:
> > On Mon, Jul 07, 2025 at 10:29:46AM +0200, Jan Beulich wrote:
> >>>> ... then isn't used right here, instead requiring a cast somewhere (presumably,
> >>>> as code using this isn't visible in this patch).
> >>>
> >>> As was mentioned earlier: because size of a pointer between Xen and a
> >>> bootloader doesn't necessarily match.  What you're proposing can break
> >>> under certain conditions.
> >>
> >> But the header isn't shared with a bootloader's code base. It's private to
> >> Xen.
> >
> > Yes, but sources of Xen be compiled with different size of a pointer
> > which messes up the interpretation of the data.  I tried using
> > BUILD_BUG_ON() to enforce the pointer is 64-bit and early code stopped
> > compiling.  The structures must not vary like that.
>
> Hmm. Does early code actually need to have this struct exposed to it?
>
> Jan

It doesn't use this particular structure, but it uses some other ones in
the header (also SLRT entries, but of different types).  Making a
separate header just to get rid of a cast doesn't seem to be worth it.

Regards
Re: [PATCH v3 02/22] include/xen/slr-table.h: Secure Launch Resource Table definitions
Posted by Jan Beulich 3 months, 2 weeks ago
On 13.07.2025 19:29, Sergii Dmytruk wrote:
> On Tue, Jul 08, 2025 at 08:52:36AM +0200, Jan Beulich wrote:
>> On 07.07.2025 19:31, Sergii Dmytruk wrote:
>>> On Mon, Jul 07, 2025 at 10:29:46AM +0200, Jan Beulich wrote:
>>>>>> ... then isn't used right here, instead requiring a cast somewhere (presumably,
>>>>>> as code using this isn't visible in this patch).
>>>>>
>>>>> As was mentioned earlier: because size of a pointer between Xen and a
>>>>> bootloader doesn't necessarily match.  What you're proposing can break
>>>>> under certain conditions.
>>>>
>>>> But the header isn't shared with a bootloader's code base. It's private to
>>>> Xen.
>>>
>>> Yes, but sources of Xen be compiled with different size of a pointer
>>> which messes up the interpretation of the data.  I tried using
>>> BUILD_BUG_ON() to enforce the pointer is 64-bit and early code stopped
>>> compiling.  The structures must not vary like that.
>>
>> Hmm. Does early code actually need to have this struct exposed to it?
> 
> It doesn't use this particular structure, but it uses some other ones in
> the header (also SLRT entries, but of different types).  Making a
> separate header just to get rid of a cast doesn't seem to be worth it.

Which I also didn't suggest. Didn't I see an EARLY_<something> #define-d
somewhere? Couldn't you key exposure of the structure to that not being
defined?

Jan
Re: [PATCH v3 02/22] include/xen/slr-table.h: Secure Launch Resource Table definitions
Posted by Sergii Dmytruk 3 months, 2 weeks ago
On Mon, Jul 14, 2025 at 09:33:09AM +0200, Jan Beulich wrote:
> On 13.07.2025 19:29, Sergii Dmytruk wrote:
> > On Tue, Jul 08, 2025 at 08:52:36AM +0200, Jan Beulich wrote:
> >> On 07.07.2025 19:31, Sergii Dmytruk wrote:
> >>> On Mon, Jul 07, 2025 at 10:29:46AM +0200, Jan Beulich wrote:
> >>>>>> ... then isn't used right here, instead requiring a cast somewhere (presumably,
> >>>>>> as code using this isn't visible in this patch).
> >>>>>
> >>>>> As was mentioned earlier: because size of a pointer between Xen and a
> >>>>> bootloader doesn't necessarily match.  What you're proposing can break
> >>>>> under certain conditions.
> >>>>
> >>>> But the header isn't shared with a bootloader's code base. It's private to
> >>>> Xen.
> >>>
> >>> Yes, but sources of Xen be compiled with different size of a pointer
> >>> which messes up the interpretation of the data.  I tried using
> >>> BUILD_BUG_ON() to enforce the pointer is 64-bit and early code stopped
> >>> compiling.  The structures must not vary like that.
> >>
> >> Hmm. Does early code actually need to have this struct exposed to it?
> >
> > It doesn't use this particular structure, but it uses some other ones in
> > the header (also SLRT entries, but of different types).  Making a
> > separate header just to get rid of a cast doesn't seem to be worth it.
>
> Which I also didn't suggest. Didn't I see an EARLY_<something> #define-d
> somewhere? Couldn't you key exposure of the structure to that not being
> defined?
>
> Jan

That's possible, but it seems rather adhoc to conditionally provide a
declaration which would also make normal and early sources differ a bit
more.  __EARLY_SLAUNCH__ was added out of necessity, I wouldn't use it
for anything new unless there is no better option.

Regards