From: Krystian Hebel <krystian.hebel@3mdeb.com>
The file contains TXT register spaces base address, registers offsets,
error codes and inline functions for accessing structures stored on
TXT heap.
Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>
Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
---
xen/arch/x86/include/asm/intel-txt.h | 277 +++++++++++++++++++++++++++
xen/arch/x86/tboot.c | 20 +-
2 files changed, 279 insertions(+), 18 deletions(-)
create mode 100644 xen/arch/x86/include/asm/intel-txt.h
diff --git a/xen/arch/x86/include/asm/intel-txt.h b/xen/arch/x86/include/asm/intel-txt.h
new file mode 100644
index 0000000000..07be43f557
--- /dev/null
+++ b/xen/arch/x86/include/asm/intel-txt.h
@@ -0,0 +1,277 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef ASM__X86__INTEL_TXT_H
+#define ASM__X86__INTEL_TXT_H
+
+/*
+ * TXT configuration registers (offsets from TXT_{PUB, PRIV}_CONFIG_REGS_BASE)
+ */
+#define TXT_PUB_CONFIG_REGS_BASE 0xfed30000U
+#define TXT_PRIV_CONFIG_REGS_BASE 0xfed20000U
+
+/*
+ * The same set of registers is exposed twice (with different permissions) and
+ * they are allocated continuously with page alignment.
+ */
+#define NR_TXT_CONFIG_SIZE \
+ (TXT_PUB_CONFIG_REGS_BASE - TXT_PRIV_CONFIG_REGS_BASE)
+
+/* Offsets from pub/priv config space. */
+#define TXTCR_STS 0x0000
+#define TXTCR_ESTS 0x0008
+#define TXTCR_ERRORCODE 0x0030
+#define TXTCR_CMD_RESET 0x0038
+#define TXTCR_CMD_CLOSE_PRIVATE 0x0048
+#define TXTCR_DIDVID 0x0110
+#define TXTCR_VER_EMIF 0x0200
+#define TXTCR_CMD_UNLOCK_MEM_CONFIG 0x0218
+#define TXTCR_SINIT_BASE 0x0270
+#define TXTCR_SINIT_SIZE 0x0278
+#define TXTCR_MLE_JOIN 0x0290
+#define TXTCR_HEAP_BASE 0x0300
+#define TXTCR_HEAP_SIZE 0x0308
+#define TXTCR_SCRATCHPAD 0x0378
+#define TXTCR_CMD_OPEN_LOCALITY1 0x0380
+#define TXTCR_CMD_CLOSE_LOCALITY1 0x0388
+#define TXTCR_CMD_OPEN_LOCALITY2 0x0390
+#define TXTCR_CMD_CLOSE_LOCALITY2 0x0398
+#define TXTCR_CMD_SECRETS 0x08e0
+#define TXTCR_CMD_NO_SECRETS 0x08e8
+#define TXTCR_E2STS 0x08f0
+
+/*
+ * Secure Launch Defined Error Codes used in MLE-initiated TXT resets.
+ *
+ * TXT Specification
+ * Appendix I ACM Error Codes
+ */
+#define SLAUNCH_ERROR_GENERIC 0xc0008001U
+#define SLAUNCH_ERROR_TPM_INIT 0xc0008002U
+#define SLAUNCH_ERROR_TPM_INVALID_LOG20 0xc0008003U
+#define SLAUNCH_ERROR_TPM_LOGGING_FAILED 0xc0008004U
+#define SLAUNCH_ERROR_REGION_STRADDLE_4GB 0xc0008005U
+#define SLAUNCH_ERROR_TPM_EXTEND 0xc0008006U
+#define SLAUNCH_ERROR_MTRR_INV_VCNT 0xc0008007U
+#define SLAUNCH_ERROR_MTRR_INV_DEF_TYPE 0xc0008008U
+#define SLAUNCH_ERROR_MTRR_INV_BASE 0xc0008009U
+#define SLAUNCH_ERROR_MTRR_INV_MASK 0xc000800aU
+#define SLAUNCH_ERROR_MSR_INV_MISC_EN 0xc000800bU
+#define SLAUNCH_ERROR_INV_AP_INTERRUPT 0xc000800cU
+#define SLAUNCH_ERROR_INTEGER_OVERFLOW 0xc000800dU
+#define SLAUNCH_ERROR_HEAP_WALK 0xc000800eU
+#define SLAUNCH_ERROR_HEAP_MAP 0xc000800fU
+#define SLAUNCH_ERROR_REGION_ABOVE_4GB 0xc0008010U
+#define SLAUNCH_ERROR_HEAP_INVALID_DMAR 0xc0008011U
+#define SLAUNCH_ERROR_HEAP_DMAR_SIZE 0xc0008012U
+#define SLAUNCH_ERROR_HEAP_DMAR_MAP 0xc0008013U
+#define SLAUNCH_ERROR_HI_PMR_BASE 0xc0008014U
+#define SLAUNCH_ERROR_HI_PMR_SIZE 0xc0008015U
+#define SLAUNCH_ERROR_LO_PMR_BASE 0xc0008016U
+#define SLAUNCH_ERROR_LO_PMR_SIZE 0xc0008017U
+#define SLAUNCH_ERROR_LO_PMR_MLE 0xc0008018U
+#define SLAUNCH_ERROR_INITRD_TOO_BIG 0xc0008019U
+#define SLAUNCH_ERROR_HEAP_ZERO_OFFSET 0xc000801aU
+#define SLAUNCH_ERROR_WAKE_BLOCK_TOO_SMALL 0xc000801bU
+#define SLAUNCH_ERROR_MLE_BUFFER_OVERLAP 0xc000801cU
+#define SLAUNCH_ERROR_BUFFER_BEYOND_PMR 0xc000801dU
+#define SLAUNCH_ERROR_OS_SINIT_BAD_VERSION 0xc000801eU
+#define SLAUNCH_ERROR_EVENTLOG_MAP 0xc000801fU
+#define SLAUNCH_ERROR_TPM_NUMBER_ALGS 0xc0008020U
+#define SLAUNCH_ERROR_TPM_UNKNOWN_DIGEST 0xc0008021U
+#define SLAUNCH_ERROR_TPM_INVALID_EVENT 0xc0008022U
+
+#define SLAUNCH_BOOTLOADER_MAGIC 0x4c534254
+
+#ifndef __ASSEMBLY__
+
+/* Need to differentiate between pre- and post paging enabled. */
+#ifdef __EARLY_SLAUNCH__
+#include <xen/macros.h>
+#define _txt(x) _p(x)
+#else
+#include <xen/types.h>
+#include <asm/page.h> // __va()
+#define _txt(x) __va(x)
+#endif
+
+/*
+ * Always use private space as some of registers are either read-only or not
+ * present in public space.
+ */
+static inline uint64_t read_txt_reg(int reg_no)
+{
+ volatile uint64_t *reg = _txt(TXT_PRIV_CONFIG_REGS_BASE + reg_no);
+ return *reg;
+}
+
+static inline void write_txt_reg(int reg_no, uint64_t val)
+{
+ volatile uint64_t *reg = _txt(TXT_PRIV_CONFIG_REGS_BASE + reg_no);
+ *reg = val;
+ /* This serves as TXT register barrier */
+ (void)read_txt_reg(TXTCR_ESTS);
+}
+
+static inline void txt_reset(uint32_t error)
+{
+ write_txt_reg(TXTCR_ERRORCODE, error);
+ write_txt_reg(TXTCR_CMD_NO_SECRETS, 1);
+ write_txt_reg(TXTCR_CMD_UNLOCK_MEM_CONFIG, 1);
+ write_txt_reg(TXTCR_CMD_RESET, 1);
+ while (1);
+}
+
+/*
+ * Secure Launch defined OS/MLE TXT Heap table
+ */
+struct txt_os_mle_data {
+ uint32_t version;
+ uint32_t reserved;
+ uint64_t slrt;
+ uint64_t txt_info;
+ uint32_t ap_wake_block;
+ uint32_t ap_wake_block_size;
+ uint8_t mle_scratch[64];
+} __packed;
+
+/*
+ * TXT specification defined BIOS data TXT Heap table
+ */
+struct txt_bios_data {
+ uint32_t version; /* Currently 5 for TPM 1.2 and 6 for TPM 2.0 */
+ uint32_t bios_sinit_size;
+ uint64_t reserved1;
+ uint64_t reserved2;
+ uint32_t num_logical_procs;
+ /* Versions >= 3 && < 5 */
+ uint32_t sinit_flags;
+ /* Versions >= 5 with updates in version 6 */
+ uint32_t mle_flags;
+ /* Versions >= 4 */
+ /* Ext Data Elements */
+} __packed;
+
+/*
+ * TXT specification defined OS/SINIT TXT Heap table
+ */
+struct txt_os_sinit_data {
+ uint32_t version; /* Currently 6 for TPM 1.2 and 7 for TPM 2.0 */
+ uint32_t flags; /* Reserved in version 6 */
+ uint64_t mle_ptab;
+ uint64_t mle_size;
+ uint64_t mle_hdr_base;
+ uint64_t vtd_pmr_lo_base;
+ uint64_t vtd_pmr_lo_size;
+ uint64_t vtd_pmr_hi_base;
+ uint64_t vtd_pmr_hi_size;
+ uint64_t lcp_po_base;
+ uint64_t lcp_po_size;
+ uint32_t capabilities;
+ /* Version = 5 */
+ uint64_t efi_rsdt_ptr; /* RSD*P* in versions >= 6 */
+ /* Versions >= 6 */
+ /* Ext Data Elements */
+} __packed;
+
+/*
+ * TXT specification defined SINIT/MLE TXT Heap table
+ */
+struct txt_sinit_mle_data {
+ uint32_t version; /* Current values are 6 through 9 */
+ /* Versions <= 8, fields until lcp_policy_control must be 0 for >= 9 */
+ uint8_t bios_acm_id[20];
+ uint32_t edx_senter_flags;
+ uint64_t mseg_valid;
+ uint8_t sinit_hash[20];
+ uint8_t mle_hash[20];
+ uint8_t stm_hash[20];
+ uint8_t lcp_policy_hash[20];
+ uint32_t lcp_policy_control;
+ /* Versions >= 7 */
+ uint32_t rlp_wakeup_addr;
+ uint32_t reserved;
+ uint32_t num_of_sinit_mdrs;
+ uint32_t sinit_mdrs_table_offset;
+ uint32_t sinit_vtd_dmar_table_size;
+ uint32_t sinit_vtd_dmar_table_offset;
+ /* Versions >= 8 */
+ uint32_t processor_scrtm_status;
+ /* Versions >= 9 */
+ /* Ext Data Elements */
+} __packed;
+
+/*
+ * Functions to extract data from the Intel TXT Heap Memory. The layout
+ * of the heap is as follows:
+ * +------------------------------------+
+ * | Size of Bios Data table (uint64_t) |
+ * +------------------------------------+
+ * | Bios Data table |
+ * +------------------------------------+
+ * | Size of OS MLE table (uint64_t) |
+ * +------------------------------------+
+ * | OS MLE table |
+ * +-------------------------------- +
+ * | Size of OS SINIT table (uint64_t) |
+ * +------------------------------------+
+ * | OS SINIT table |
+ * +------------------------------------+
+ * | Size of SINIT MLE table (uint64_t) |
+ * +------------------------------------+
+ * | SINIT MLE table |
+ * +------------------------------------+
+ *
+ * NOTE: the table size fields include the 8 byte size field itself.
+ */
+static inline uint64_t txt_bios_data_size(void *heap)
+{
+ return *((uint64_t *)heap) - sizeof(uint64_t);
+}
+
+static inline void *txt_bios_data_start(void *heap)
+{
+ return heap + sizeof(uint64_t);
+}
+
+static inline uint64_t txt_os_mle_data_size(void *heap)
+{
+ return *((uint64_t *)(txt_bios_data_start(heap) +
+ txt_bios_data_size(heap))) -
+ sizeof(uint64_t);
+}
+
+static inline void *txt_os_mle_data_start(void *heap)
+{
+ return txt_bios_data_start(heap) + txt_bios_data_size(heap) +
+ sizeof(uint64_t);
+}
+
+static inline uint64_t txt_os_sinit_data_size(void *heap)
+{
+ return *((uint64_t *)(txt_os_mle_data_start(heap) +
+ txt_os_mle_data_size(heap))) -
+ sizeof(uint64_t);
+}
+
+static inline void *txt_os_sinit_data_start(void *heap)
+{
+ return txt_os_mle_data_start(heap) + txt_os_mle_data_size(heap) +
+ sizeof(uint64_t);
+}
+
+static inline uint64_t txt_sinit_mle_data_size(void *heap)
+{
+ return *((uint64_t *)(txt_os_sinit_data_start(heap) +
+ txt_os_sinit_data_size(heap))) -
+ sizeof(uint64_t);
+}
+
+static inline void *txt_sinit_mle_data_start(void *heap)
+{
+ return txt_os_sinit_data_start(heap) + txt_os_sinit_data_size(heap) +
+ sizeof(uint64_t);
+}
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* ASM__X86__INTEL_TXT_H */
diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c
index d5db60d335..8a573d8c79 100644
--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -15,6 +15,7 @@
#include <asm/tboot.h>
#include <asm/setup.h>
#include <asm/trampoline.h>
+#include <asm/intel-txt.h>
#include <crypto/vmac.h>
@@ -35,23 +36,6 @@ static uint64_t __initdata sinit_base, __initdata sinit_size;
static bool __ro_after_init is_vtd;
-/*
- * TXT configuration registers (offsets from TXT_{PUB, PRIV}_CONFIG_REGS_BASE)
- */
-
-#define TXT_PUB_CONFIG_REGS_BASE 0xfed30000
-#define TXT_PRIV_CONFIG_REGS_BASE 0xfed20000
-
-/* # pages for each config regs space - used by fixmap */
-#define NR_TXT_CONFIG_PAGES ((TXT_PUB_CONFIG_REGS_BASE - \
- TXT_PRIV_CONFIG_REGS_BASE) >> PAGE_SHIFT)
-
-/* offsets from pub/priv config space */
-#define TXTCR_SINIT_BASE 0x0270
-#define TXTCR_SINIT_SIZE 0x0278
-#define TXTCR_HEAP_BASE 0x0300
-#define TXTCR_HEAP_SIZE 0x0308
-
#define SHA1_SIZE 20
typedef uint8_t sha1_hash_t[SHA1_SIZE];
@@ -409,7 +393,7 @@ int __init tboot_protect_mem_regions(void)
/* TXT Private Space */
rc = e820_change_range_type(&e820, TXT_PRIV_CONFIG_REGS_BASE,
- TXT_PRIV_CONFIG_REGS_BASE + NR_TXT_CONFIG_PAGES * PAGE_SIZE,
+ TXT_PRIV_CONFIG_REGS_BASE + NR_TXT_CONFIG_SIZE,
E820_RESERVED, E820_UNUSABLE);
if ( !rc )
return 0;
--
2.49.0On 13.05.2025 19:05, Sergii Dmytruk wrote:
> From: Krystian Hebel <krystian.hebel@3mdeb.com>
>
> The file contains TXT register spaces base address, registers offsets,
> error codes and inline functions for accessing structures stored on
> TXT heap.
>
> Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>
> Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
> ---
> xen/arch/x86/include/asm/intel-txt.h | 277 +++++++++++++++++++++++++++
> xen/arch/x86/tboot.c | 20 +-
> 2 files changed, 279 insertions(+), 18 deletions(-)
> create mode 100644 xen/arch/x86/include/asm/intel-txt.h
>
> diff --git a/xen/arch/x86/include/asm/intel-txt.h b/xen/arch/x86/include/asm/intel-txt.h
> new file mode 100644
> index 0000000000..07be43f557
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/intel-txt.h
> @@ -0,0 +1,277 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef ASM__X86__INTEL_TXT_H
> +#define ASM__X86__INTEL_TXT_H
> +
> +/*
> + * TXT configuration registers (offsets from TXT_{PUB, PRIV}_CONFIG_REGS_BASE)
> + */
> +#define TXT_PUB_CONFIG_REGS_BASE 0xfed30000U
> +#define TXT_PRIV_CONFIG_REGS_BASE 0xfed20000U
> +
> +/*
> + * The same set of registers is exposed twice (with different permissions) and
> + * they are allocated continuously with page alignment.
> + */
> +#define NR_TXT_CONFIG_SIZE \
> + (TXT_PUB_CONFIG_REGS_BASE - TXT_PRIV_CONFIG_REGS_BASE)
What does the NR_ in the identifier try to express?
> +/*
> + * Secure Launch defined OS/MLE TXT Heap table
> + */
> +struct txt_os_mle_data {
> + uint32_t version;
> + uint32_t reserved;
> + uint64_t slrt;
> + uint64_t txt_info;
> + uint32_t ap_wake_block;
> + uint32_t ap_wake_block_size;
> + uint8_t mle_scratch[64];
> +} __packed;
This being x86-specific, what's the __packed intended to achieve here?
> +/*
> + * TXT specification defined BIOS data TXT Heap table
> + */
> +struct txt_bios_data {
> + uint32_t version; /* Currently 5 for TPM 1.2 and 6 for TPM 2.0 */
> + uint32_t bios_sinit_size;
> + uint64_t reserved1;
> + uint64_t reserved2;
> + uint32_t num_logical_procs;
> + /* Versions >= 3 && < 5 */
> + uint32_t sinit_flags;
> + /* Versions >= 5 with updates in version 6 */
> + uint32_t mle_flags;
> + /* Versions >= 4 */
> + /* Ext Data Elements */
> +} __packed;
It does affect sizeof() here, which I'm unsure is going to matter.
> +/*
> + * TXT specification defined OS/SINIT TXT Heap table
> + */
> +struct txt_os_sinit_data {
> + uint32_t version; /* Currently 6 for TPM 1.2 and 7 for TPM 2.0 */
> + uint32_t flags; /* Reserved in version 6 */
> + uint64_t mle_ptab;
> + uint64_t mle_size;
> + uint64_t mle_hdr_base;
> + uint64_t vtd_pmr_lo_base;
> + uint64_t vtd_pmr_lo_size;
> + uint64_t vtd_pmr_hi_base;
> + uint64_t vtd_pmr_hi_size;
> + uint64_t lcp_po_base;
> + uint64_t lcp_po_size;
> + uint32_t capabilities;
> + /* Version = 5 */
> + uint64_t efi_rsdt_ptr; /* RSD*P* in versions >= 6 */
> + /* Versions >= 6 */
> + /* Ext Data Elements */
> +} __packed;
It does really affect the layout here, so can't be removed in this case.
> +/*
> + * Functions to extract data from the Intel TXT Heap Memory. The layout
> + * of the heap is as follows:
> + * +------------------------------------+
> + * | Size of Bios Data table (uint64_t) |
> + * +------------------------------------+
> + * | Bios Data table |
> + * +------------------------------------+
> + * | Size of OS MLE table (uint64_t) |
> + * +------------------------------------+
> + * | OS MLE table |
> + * +-------------------------------- +
> + * | Size of OS SINIT table (uint64_t) |
> + * +------------------------------------+
> + * | OS SINIT table |
> + * +------------------------------------+
> + * | Size of SINIT MLE table (uint64_t) |
> + * +------------------------------------+
> + * | SINIT MLE table |
> + * +------------------------------------+
> + *
> + * NOTE: the table size fields include the 8 byte size field itself.
> + */
> +static inline uint64_t txt_bios_data_size(void *heap)
Here, below, and in general: Please try to have code be const-correct, i.e.
use pointers-to-const wherever applicable.
> +{
> + return *((uint64_t *)heap) - sizeof(uint64_t);
For readability it generally helps if excess parentheses like the ones here
are omitted.
> +}
> +
> +static inline void *txt_bios_data_start(void *heap)
> +{
> + return heap + sizeof(uint64_t);
> +}
> +
> +static inline uint64_t txt_os_mle_data_size(void *heap)
> +{
> + return *((uint64_t *)(txt_bios_data_start(heap) +
> + txt_bios_data_size(heap))) -
> + sizeof(uint64_t);
This line wants indenting a little further, such that the"sizeof" aligns
with the start of the expression. (Same elsewhere below.)
Jan
On Wed, May 21, 2025 at 05:19:57PM +0200, Jan Beulich wrote:
> > +/*
> > + * The same set of registers is exposed twice (with different permissions) and
> > + * they are allocated continuously with page alignment.
> > + */
> > +#define NR_TXT_CONFIG_SIZE \
> > + (TXT_PUB_CONFIG_REGS_BASE - TXT_PRIV_CONFIG_REGS_BASE)
>
> What does the NR_ in the identifier try to express?
That's a leftover from the original name inside tboot.c, I'll rename it
to TXT_CONFIG_SPACE_SIZE.
> > +/*
> > + * Secure Launch defined OS/MLE TXT Heap table
> > + */
> > +struct txt_os_mle_data {
> > + uint32_t version;
> > + uint32_t reserved;
> > + uint64_t slrt;
> > + uint64_t txt_info;
> > + uint32_t ap_wake_block;
> > + uint32_t ap_wake_block_size;
> > + uint8_t mle_scratch[64];
> > +} __packed;
>
> This being x86-specific, what's the __packed intended to achieve here?
This structure is passed to Xen by a bootloader, __packed makes sure the
structure has a compatible layout.
> > +/*
> > + * TXT specification defined BIOS data TXT Heap table
> > + */
> > +struct txt_bios_data {
> > + uint32_t version; /* Currently 5 for TPM 1.2 and 6 for TPM 2.0 */
> > + uint32_t bios_sinit_size;
> > + uint64_t reserved1;
> > + uint64_t reserved2;
> > + uint32_t num_logical_procs;
> > + /* Versions >= 3 && < 5 */
> > + uint32_t sinit_flags;
> > + /* Versions >= 5 with updates in version 6 */
> > + uint32_t mle_flags;
> > + /* Versions >= 4 */
> > + /* Ext Data Elements */
> > +} __packed;
>
> It does affect sizeof() here, which I'm unsure is going to matter.
It doesn't hurt anything and makes sure offsets match those in the
specification.
> > +static inline uint64_t txt_bios_data_size(void *heap)
>
> Here, below, and in general: Please try to have code be const-correct, i.e.
> use pointers-to-const wherever applicable.
I assume this doesn't apply to functions returning `void *`. The
approach used in libc is to accept pointers-to-const but then cast the
constness away for the return value, but this header isn't a widely-used
code.
> > +{
> > + return *((uint64_t *)heap) - sizeof(uint64_t);
>
> For readability it generally helps if excess parentheses like the ones here
> are omitted.
OK.
> > +static inline uint64_t txt_os_mle_data_size(void *heap)
> > +{
> > + return *((uint64_t *)(txt_bios_data_start(heap) +
> > + txt_bios_data_size(heap))) -
> > + sizeof(uint64_t);
>
> This line wants indenting a little further, such that the"sizeof" aligns
> with the start of the expression. (Same elsewhere below.)
>
> Jan
Will update.
Regards
On 23.05.2025 21:51, Sergii Dmytruk wrote:
> On Wed, May 21, 2025 at 05:19:57PM +0200, Jan Beulich wrote:
>>> +/*
>>> + * Secure Launch defined OS/MLE TXT Heap table
>>> + */
>>> +struct txt_os_mle_data {
>>> + uint32_t version;
>>> + uint32_t reserved;
>>> + uint64_t slrt;
>>> + uint64_t txt_info;
>>> + uint32_t ap_wake_block;
>>> + uint32_t ap_wake_block_size;
>>> + uint8_t mle_scratch[64];
>>> +} __packed;
>>
>> This being x86-specific, what's the __packed intended to achieve here?
>
> This structure is passed to Xen by a bootloader, __packed makes sure the
> structure has a compatible layout.
And it won't have a compatible layout without the attribute?
>>> +/*
>>> + * TXT specification defined BIOS data TXT Heap table
>>> + */
>>> +struct txt_bios_data {
>>> + uint32_t version; /* Currently 5 for TPM 1.2 and 6 for TPM 2.0 */
>>> + uint32_t bios_sinit_size;
>>> + uint64_t reserved1;
>>> + uint64_t reserved2;
>>> + uint32_t num_logical_procs;
>>> + /* Versions >= 3 && < 5 */
>>> + uint32_t sinit_flags;
>>> + /* Versions >= 5 with updates in version 6 */
>>> + uint32_t mle_flags;
>>> + /* Versions >= 4 */
>>> + /* Ext Data Elements */
>>> +} __packed;
>>
>> It does affect sizeof() here, which I'm unsure is going to matter.
>
> It doesn't hurt anything and makes sure offsets match those in the
> specification.
It similarly doesn't appear to hurt anything if the attribute was omitted.
Imo we ought to use compiler extensions on when there is a need to do so.
>>> +static inline uint64_t txt_bios_data_size(void *heap)
>>
>> Here, below, and in general: Please try to have code be const-correct, i.e.
>> use pointers-to-const wherever applicable.
>
> I assume this doesn't apply to functions returning `void *`. The
> approach used in libc is to accept pointers-to-const but then cast the
> constness away for the return value, but this header isn't a widely-used
> code.
Which is, from all I know, bad practice not only by my own view.
Jan
On Mon, Jun 02, 2025 at 09:17:37AM +0200, Jan Beulich wrote:
> On 23.05.2025 21:51, Sergii Dmytruk wrote:
> > On Wed, May 21, 2025 at 05:19:57PM +0200, Jan Beulich wrote:
> >>> +/*
> >>> + * Secure Launch defined OS/MLE TXT Heap table
> >>> + */
> >>> +struct txt_os_mle_data {
> >>> + uint32_t version;
> >>> + uint32_t reserved;
> >>> + uint64_t slrt;
> >>> + uint64_t txt_info;
> >>> + uint32_t ap_wake_block;
> >>> + uint32_t ap_wake_block_size;
> >>> + uint8_t mle_scratch[64];
> >>> +} __packed;
> >>
> >> This being x86-specific, what's the __packed intended to achieve here?
> >
> > This structure is passed to Xen by a bootloader, __packed makes sure the
> > structure has a compatible layout.
>
> And it won't have a compatible layout without the attribute?
It will, but presence of __packed makes it trivial to see.
> >>> +/*
> >>> + * TXT specification defined BIOS data TXT Heap table
> >>> + */
> >>> +struct txt_bios_data {
> >>> + uint32_t version; /* Currently 5 for TPM 1.2 and 6 for TPM 2.0 */
> >>> + uint32_t bios_sinit_size;
> >>> + uint64_t reserved1;
> >>> + uint64_t reserved2;
> >>> + uint32_t num_logical_procs;
> >>> + /* Versions >= 3 && < 5 */
> >>> + uint32_t sinit_flags;
> >>> + /* Versions >= 5 with updates in version 6 */
> >>> + uint32_t mle_flags;
> >>> + /* Versions >= 4 */
> >>> + /* Ext Data Elements */
> >>> +} __packed;
> >>
> >> It does affect sizeof() here, which I'm unsure is going to matter.
> >
> > It doesn't hurt anything and makes sure offsets match those in the
> > specification.
>
> It similarly doesn't appear to hurt anything if the attribute was omitted.
> Imo we ought to use compiler extensions on when there is a need to do so.
I would argue that it hurts maintainability and code readability to some
extent:
* when the attribute is used, there is no need to verify compatibility
in any way (manually or using pahole) neither now nor on any future
modification
* when I see __packed, I immediately know the structure is defined
externally and can't be changed at will
* having the attribute only for some structures seems inconsistent
It would be nice if it was possible to verify the structure is packed
via a static assert using only standard C, but without such means I see
__packed as useful and harmless compiler extension.
I can of course drop unnecessary attributes if that's a standard
practice for Xen's sources, never thought it could be undesirable in
a context like this one.
> >>> +static inline uint64_t txt_bios_data_size(void *heap)
> >>
> >> Here, below, and in general: Please try to have code be const-correct, i.e.
> >> use pointers-to-const wherever applicable.
> >
> > I assume this doesn't apply to functions returning `void *`. The
> > approach used in libc is to accept pointers-to-const but then cast the
> > constness away for the return value, but this header isn't a widely-used
> > code.
>
> Which is, from all I know, bad practice not only by my own view.
>
> Jan
I actually ended up doing that to have const-correctness in v3. In the
absence of function overloads the casts have to be somewhere, can put
them in the calling code instead.
Regards
On 03.06.2025 00:00, Sergii Dmytruk wrote: > On Mon, Jun 02, 2025 at 09:17:37AM +0200, Jan Beulich wrote: >> On 23.05.2025 21:51, Sergii Dmytruk wrote: >>> On Wed, May 21, 2025 at 05:19:57PM +0200, Jan Beulich wrote: >>>>> +static inline uint64_t txt_bios_data_size(void *heap) >>>> >>>> Here, below, and in general: Please try to have code be const-correct, i.e. >>>> use pointers-to-const wherever applicable. >>> >>> I assume this doesn't apply to functions returning `void *`. The >>> approach used in libc is to accept pointers-to-const but then cast the >>> constness away for the return value, but this header isn't a widely-used >>> code. >> >> Which is, from all I know, bad practice not only by my own view. > > I actually ended up doing that to have const-correctness in v3. In the > absence of function overloads the casts have to be somewhere, can put > them in the calling code instead. Casts of which kind? For context: There shouldn't be any casting away of const-ness (or volatile-ness, for the sake of completeness). Jan
On Tue, Jun 03, 2025 at 09:06:53AM +0200, Jan Beulich wrote:
> On 03.06.2025 00:00, Sergii Dmytruk wrote:
> > On Mon, Jun 02, 2025 at 09:17:37AM +0200, Jan Beulich wrote:
> >> On 23.05.2025 21:51, Sergii Dmytruk wrote:
> >>> On Wed, May 21, 2025 at 05:19:57PM +0200, Jan Beulich wrote:
> >>>>> +static inline uint64_t txt_bios_data_size(void *heap)
> >>>>
> >>>> Here, below, and in general: Please try to have code be const-correct, i.e.
> >>>> use pointers-to-const wherever applicable.
> >>>
> >>> I assume this doesn't apply to functions returning `void *`. The
> >>> approach used in libc is to accept pointers-to-const but then cast the
> >>> constness away for the return value, but this header isn't a widely-used
> >>> code.
> >>
> >> Which is, from all I know, bad practice not only by my own view.
> >
> > I actually ended up doing that to have const-correctness in v3. In the
> > absence of function overloads the casts have to be somewhere, can put
> > them in the calling code instead.
>
> Casts of which kind? For context: There shouldn't be any casting away of
> const-ness (or volatile-ness, for the sake of completeness).
>
> Jan
Casting away const-ness inside of functions like
static inline void *txt_bios_data_start(const void *heap)
If a function accepts a const pointer and returns it, this turns a
non-const incoming pointer into a const one. Without duplicating the
code (either having const and non-const versions or repeating code in
other ways), nothing can be made const cleanly in here including
*_size() functions because they call *_start() functions:
static inline uint64_t txt_os_mle_data_size(const void *heap)
{
return *((const uint64_t *)(txt_bios_data_start(heap) +
// ^^^^ -- const
txt_bios_data_size(heap))) -
sizeof(uint64_t);
}
Regards
On 03.06.2025 10:50, Sergii Dmytruk wrote:
> On Tue, Jun 03, 2025 at 09:06:53AM +0200, Jan Beulich wrote:
>> On 03.06.2025 00:00, Sergii Dmytruk wrote:
>>> On Mon, Jun 02, 2025 at 09:17:37AM +0200, Jan Beulich wrote:
>>>> On 23.05.2025 21:51, Sergii Dmytruk wrote:
>>>>> On Wed, May 21, 2025 at 05:19:57PM +0200, Jan Beulich wrote:
>>>>>>> +static inline uint64_t txt_bios_data_size(void *heap)
>>>>>>
>>>>>> Here, below, and in general: Please try to have code be const-correct, i.e.
>>>>>> use pointers-to-const wherever applicable.
>>>>>
>>>>> I assume this doesn't apply to functions returning `void *`. The
>>>>> approach used in libc is to accept pointers-to-const but then cast the
>>>>> constness away for the return value, but this header isn't a widely-used
>>>>> code.
>>>>
>>>> Which is, from all I know, bad practice not only by my own view.
>>>
>>> I actually ended up doing that to have const-correctness in v3. In the
>>> absence of function overloads the casts have to be somewhere, can put
>>> them in the calling code instead.
>>
>> Casts of which kind? For context: There shouldn't be any casting away of
>> const-ness (or volatile-ness, for the sake of completeness).
>>
>> Jan
>
> Casting away const-ness inside of functions like
>
> static inline void *txt_bios_data_start(const void *heap)
>
> If a function accepts a const pointer and returns it, this turns a
> non-const incoming pointer into a const one. Without duplicating the
> code (either having const and non-const versions or repeating code in
> other ways), nothing can be made const cleanly in here including
> *_size() functions because they call *_start() functions:
>
> static inline uint64_t txt_os_mle_data_size(const void *heap)
> {
> return *((const uint64_t *)(txt_bios_data_start(heap) +
> // ^^^^ -- const
> txt_bios_data_size(heap))) -
> sizeof(uint64_t);
> }
Yet just to repeat: Besides myself (and maybe others), Misra objects to
the casting away of const-ness.
Jan
On Tue, Jun 03, 2025 at 10:52:09AM +0200, Jan Beulich wrote:
> On 03.06.2025 10:50, Sergii Dmytruk wrote:
> > On Tue, Jun 03, 2025 at 09:06:53AM +0200, Jan Beulich wrote:
> >> On 03.06.2025 00:00, Sergii Dmytruk wrote:
> >>> On Mon, Jun 02, 2025 at 09:17:37AM +0200, Jan Beulich wrote:
> >>>> On 23.05.2025 21:51, Sergii Dmytruk wrote:
> >>>>> On Wed, May 21, 2025 at 05:19:57PM +0200, Jan Beulich wrote:
> >>>>>>> +static inline uint64_t txt_bios_data_size(void *heap)
> >>>>>>
> >>>>>> Here, below, and in general: Please try to have code be const-correct, i.e.
> >>>>>> use pointers-to-const wherever applicable.
> >>>>>
> >>>>> I assume this doesn't apply to functions returning `void *`. The
> >>>>> approach used in libc is to accept pointers-to-const but then cast the
> >>>>> constness away for the return value, but this header isn't a widely-used
> >>>>> code.
> >>>>
> >>>> Which is, from all I know, bad practice not only by my own view.
> >>>
> >>> I actually ended up doing that to have const-correctness in v3. In the
> >>> absence of function overloads the casts have to be somewhere, can put
> >>> them in the calling code instead.
> >>
> >> Casts of which kind? For context: There shouldn't be any casting away of
> >> const-ness (or volatile-ness, for the sake of completeness).
> >>
> >> Jan
> >
> > Casting away const-ness inside of functions like
> >
> > static inline void *txt_bios_data_start(const void *heap)
> >
> > If a function accepts a const pointer and returns it, this turns a
> > non-const incoming pointer into a const one. Without duplicating the
> > code (either having const and non-const versions or repeating code in
> > other ways), nothing can be made const cleanly in here including
> > *_size() functions because they call *_start() functions:
> >
> > static inline uint64_t txt_os_mle_data_size(const void *heap)
> > {
> > return *((const uint64_t *)(txt_bios_data_start(heap) +
> > // ^^^^ -- const
> > txt_bios_data_size(heap))) -
> > sizeof(uint64_t);
> > }
>
> Yet just to repeat: Besides myself (and maybe others), Misra objects to
> the casting away of const-ness.
>
> Jan
OK, I'll change it back then unless I'll find a way to preserve some
const-ness without duplication.
Regards
On 13/05/2025 6:05 pm, Sergii Dmytruk wrote:
> diff --git a/xen/arch/x86/include/asm/intel-txt.h b/xen/arch/x86/include/asm/intel-txt.h
> new file mode 100644
> index 0000000000..07be43f557
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/intel-txt.h
> @@ -0,0 +1,277 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
Please have at least a one-liner introduction to what TXT is. Is there
a stable URL for the TXT spec? (I can't spot an obvious one, googling
around)
> +#ifndef ASM__X86__INTEL_TXT_H
> +#define ASM__X86__INTEL_TXT_H
I realise this is what the documentation currently says, but we're just
in the process of finalising some changes. Please make this
X86_INTEL_TXT_H.
> +
> +/*
> + * TXT configuration registers (offsets from TXT_{PUB, PRIV}_CONFIG_REGS_BASE)
> + */
> +#define TXT_PUB_CONFIG_REGS_BASE 0xfed30000U
> +#define TXT_PRIV_CONFIG_REGS_BASE 0xfed20000U
> +
> +/*
> + * The same set of registers is exposed twice (with different permissions) and
> + * they are allocated continuously with page alignment.
> + */
> +#define NR_TXT_CONFIG_SIZE \
> + (TXT_PUB_CONFIG_REGS_BASE - TXT_PRIV_CONFIG_REGS_BASE)
Commit message needs to note that you're swapping NR_TXT_CONFIG_PAGES
for NR_TXT_CONFIG_SIZE, hence the change in logic in
tboot_protect_mem_regions().
> +#ifndef __ASSEMBLY__
> +
> +/* Need to differentiate between pre- and post paging enabled. */
> +#ifdef __EARLY_SLAUNCH__
> +#include <xen/macros.h>
> +#define _txt(x) _p(x)
> +#else
> +#include <xen/types.h>
> +#include <asm/page.h> // __va()
> +#define _txt(x) __va(x)
> +#endif
I have to admit that I'm rather suspicious of this, but I'm going to
have to wait to see later patches before making a suggestion. (I highly
suspect we'll want a fixmap entry).
> +
> +/*
> + * Always use private space as some of registers are either read-only or not
> + * present in public space.
> + */
> +static inline uint64_t read_txt_reg(int reg_no)
unsigned int reg
I'd be tempted to name it simply txt_read(). I don't think the extra
"_reg" is a helpful suffix, and it makes the APIs consistent with
txt_reset(). But I expect others may have opinions here.
> +{
> + volatile uint64_t *reg = _txt(TXT_PRIV_CONFIG_REGS_BASE + reg_no);
> + return *reg;
> +}
> +
> +static inline void write_txt_reg(int reg_no, uint64_t val)
> +{
> + volatile uint64_t *reg = _txt(TXT_PRIV_CONFIG_REGS_BASE + reg_no);
> + *reg = val;
> + /* This serves as TXT register barrier */
> + (void)read_txt_reg(TXTCR_ESTS);
What is this barrier for?
It's not for anything in the CPU side of things, because UC accesses are
strictly ordered.
I presume it's for LPC bus ordering then, but making every write be a
write/read pair seems very expensive.
> +}
> +
> +static inline void txt_reset(uint32_t error)
> +{
> + write_txt_reg(TXTCR_ERRORCODE, error);
> + write_txt_reg(TXTCR_CMD_NO_SECRETS, 1);
> + write_txt_reg(TXTCR_CMD_UNLOCK_MEM_CONFIG, 1);
> + write_txt_reg(TXTCR_CMD_RESET, 1);
> + while (1);
for ( ;; )
cpu_relax();
please.
Will the write to CMD_RESET try to initiate a platform reset, or will we
just hang here forever?
> +/*
> + * Functions to extract data from the Intel TXT Heap Memory. The layout
> + * of the heap is as follows:
> + * +------------------------------------+
> + * | Size of Bios Data table (uint64_t) |
> + * +------------------------------------+
> + * | Bios Data table |
> + * +------------------------------------+
> + * | Size of OS MLE table (uint64_t) |
> + * +------------------------------------+
> + * | OS MLE table |
> + * +-------------------------------- +
> + * | Size of OS SINIT table (uint64_t) |
> + * +------------------------------------+
> + * | OS SINIT table |
> + * +------------------------------------+
> + * | Size of SINIT MLE table (uint64_t) |
> + * +------------------------------------+
> + * | SINIT MLE table |
> + * +------------------------------------+
> + *
> + * NOTE: the table size fields include the 8 byte size field itself.
> + */
> +static inline uint64_t txt_bios_data_size(void *heap)
> +{
> + return *((uint64_t *)heap) - sizeof(uint64_t);
> +}
This is quite horrible, but I presume this is as specified by TXT?
To confirm, it's a tightly packed data structure where each of the 4
blobs is variable size? Are there any alignment requirements on the
table sizes? I suspect not, given the __packed attributes.
> diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c
> index d5db60d335..8a573d8c79 100644
> --- a/xen/arch/x86/tboot.c
> +++ b/xen/arch/x86/tboot.c
> @@ -15,6 +15,7 @@
> #include <asm/tboot.h>
> #include <asm/setup.h>
> #include <asm/trampoline.h>
> +#include <asm/intel-txt.h>
>
> #include <crypto/vmac.h>
I'll send a prep patch to fix tboot.c, but we're trying to keep includes
sorted (for sanity of backports).
~Andrew
On 14.05.2025 16:55, Andrew Cooper wrote:
> On 13/05/2025 6:05 pm, Sergii Dmytruk wrote:
>> +/*
>> + * Always use private space as some of registers are either read-only or not
>> + * present in public space.
>> + */
>> +static inline uint64_t read_txt_reg(int reg_no)
>
> unsigned int reg
>
> I'd be tempted to name it simply txt_read(). I don't think the extra
> "_reg" is a helpful suffix, and it makes the APIs consistent with
> txt_reset(). But I expect others may have opinions here.
+1
>> +static inline void txt_reset(uint32_t error)
>> +{
>> + write_txt_reg(TXTCR_ERRORCODE, error);
>> + write_txt_reg(TXTCR_CMD_NO_SECRETS, 1);
>> + write_txt_reg(TXTCR_CMD_UNLOCK_MEM_CONFIG, 1);
>> + write_txt_reg(TXTCR_CMD_RESET, 1);
>> + while (1);
>
> for ( ;; )
> cpu_relax();
>
> please.
With (style nit) another blank between the semicolons.
Jan
On Wed, May 14, 2025 at 03:55:51PM +0100, Andrew Cooper wrote:
> Please have at least a one-liner introduction to what TXT is. Is there
> a stable URL for the TXT spec? (I can't spot an obvious one, googling
> around)
In addition to a short definition I'll add:
* https://www.intel.com/content/www/us/en/support/articles/000025873/processors.html
* unversioned link to Software Development Guide
* link to that unofficial archive (marked as such) mentioned by Krystian
> > +#ifndef ASM__X86__INTEL_TXT_H
> > +#define ASM__X86__INTEL_TXT_H
>
> I realise this is what the documentation currently says, but we're just
> in the process of finalising some changes. Please make this
> X86_INTEL_TXT_H.
Will fix.
> Commit message needs to note that you're swapping NR_TXT_CONFIG_PAGES
> for NR_TXT_CONFIG_SIZE, hence the change in logic in
> tboot_protect_mem_regions().
Will clarify that.
> > +#ifndef __ASSEMBLY__
> > +
> > +/* Need to differentiate between pre- and post paging enabled. */
> > +#ifdef __EARLY_SLAUNCH__
> > +#include <xen/macros.h>
> > +#define _txt(x) _p(x)
> > +#else
> > +#include <xen/types.h>
> > +#include <asm/page.h> // __va()
> > +#define _txt(x) __va(x)
> > +#endif
>
> I have to admit that I'm rather suspicious of this, but I'm going to
> have to wait to see later patches before making a suggestion. (I highly
> suspect we'll want a fixmap entry).
Leaving it as is for now.
> > +/*
> > + * Always use private space as some of registers are either read-only or not
> > + * present in public space.
> > + */
> > +static inline uint64_t read_txt_reg(int reg_no)
>
> unsigned int reg
OK, for both read and write functions.
> I'd be tempted to name it simply txt_read(). I don't think the extra
> "_reg" is a helpful suffix, and it makes the APIs consistent with
> txt_reset(). But I expect others may have opinions here.
Will be renamed to txt_read() and txt_write(), seems sensible to me.
> > +static inline void write_txt_reg(int reg_no, uint64_t val)
> > +{
> > + volatile uint64_t *reg = _txt(TXT_PRIV_CONFIG_REGS_BASE + reg_no);
> > + *reg = val;
> > + /* This serves as TXT register barrier */
> > + (void)read_txt_reg(TXTCR_ESTS);
>
> What is this barrier for?
>
> It's not for anything in the CPU side of things, because UC accesses are
> strictly ordered.
>
> I presume it's for LPC bus ordering then, but making every write be a
> write/read pair seems very expensive.
The barrier will be moved to txt_reset() as suggested by Krystian in his
reply.
> > +static inline void txt_reset(uint32_t error)
> > +{
> > + write_txt_reg(TXTCR_ERRORCODE, error);
> > + write_txt_reg(TXTCR_CMD_NO_SECRETS, 1);
> > + write_txt_reg(TXTCR_CMD_UNLOCK_MEM_CONFIG, 1);
> > + write_txt_reg(TXTCR_CMD_RESET, 1);
> > + while (1);
>
> for ( ;; )
> cpu_relax();
>
> please.
>
> Will the write to CMD_RESET try to initiate a platform reset, or will we
> just hang here forever?
Will mark the function as `noreturn` and use `unreachable()` instead.
The write sends a platform into a warm reboot which retains the error
code for later examination.
> > +/*
> > + * Functions to extract data from the Intel TXT Heap Memory. The layout
> > + * of the heap is as follows:
>
> This is quite horrible, but I presume this is as specified by TXT?
>
> To confirm, it's a tightly packed data structure where each of the 4
> blobs is variable size? Are there any alignment requirements on the
> table sizes? I suspect not, given the __packed attributes.
Will improve the comment to state this explicitly, including that no
particular alignment is assumed and why (as in Krystian's reply).
> > diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c
> > index d5db60d335..8a573d8c79 100644
> > --- a/xen/arch/x86/tboot.c
> > +++ b/xen/arch/x86/tboot.c
> > @@ -15,6 +15,7 @@
> > #include <asm/tboot.h>
> > #include <asm/setup.h>
> > #include <asm/trampoline.h>
> > +#include <asm/intel-txt.h>
> >
> > #include <crypto/vmac.h>
>
> I'll send a prep patch to fix tboot.c, but we're trying to keep includes
> sorted (for sanity of backports).
>
> ~Andrew
I've seen commits sorting includes and trying to do the same, but files
which don't have includes sorted yet often don't have any good place for
a new entry and I refrain from sorting them at the same time as I add a
new line.
Regards
On May 18, 2025, at 2:36 PM, Sergii Dmytruk <sergii.dmytruk@3mdeb.com> wrote: > > On Wed, May 14, 2025 at 03:55:51PM +0100, Andrew Cooper wrote: >> Please have at least a one-liner introduction to what TXT is. Is there >> a stable URL for the TXT spec? (I can't spot an obvious one, googling >> around) > > In addition to a short definition I'll add: > * https://www.intel.com/content/www/us/en/support/articles/000025873/processors.html > * unversioned link to Software Development Guide > * link to that unofficial archive (marked as such) mentioned by Krystian If there's no stable URL for the TXT spec, can we mirror the relevant doc(s) somewhere in the Xen docs tree? A trusted archive of the spec for trusted execution. Rich
On Sun, May 18, 2025 at 07:31:49PM -0400, Rich Persaud wrote: > If there's no stable URL for the TXT spec, can we mirror the relevant > doc(s) somewhere in the Xen docs tree? A trusted archive of the spec > for trusted execution. > > Rich By "unversioned link to Software Development Guide" I meant https://www.intel.com/content/www/us/en/content-details/315168/ which always provides the latest version. Regards
On May 19, 2025, at 9:43 AM, Sergii Dmytruk <sergii.dmytruk@3mdeb.com> wrote: > > On Sun, May 18, 2025 at 07:31:49PM -0400, Rich Persaud wrote: >> If there's no stable URL for the TXT spec, can we mirror the relevant >> doc(s) somewhere in the Xen docs tree? A trusted archive of the spec >> for trusted execution. >> >> Rich > > By "unversioned link to Software Development Guide" I meant > https://www.intel.com/content/www/us/en/content-details/315168/ > which always provides the latest version. By "trusted archive of the spec" I meant a server under control of Intel or the Xen community, hosting the specific version(s) of the spec that have been implemented in the Xen tree. Unless Intel reference PDFs are digitally signed by an Intel certificate, we should not be linking to non-Intel mirrors of Intel PDFs, which could be targeted by attackers to relay malware onto the workstations of developers of trusted execution software. If Intel reference PDFs are signed, we should include instructions for verifying their authenticity, if we are linking to non-Intel sources. Rich
On 14.05.2025 16:55, 'Andrew Cooper' via trenchboot-devel wrote:
> On 13/05/2025 6:05 pm, Sergii Dmytruk wrote:
>> diff --git a/xen/arch/x86/include/asm/intel-txt.h b/xen/arch/x86/include/asm/intel-txt.h
>> new file mode 100644
>> index 0000000000..07be43f557
>> --- /dev/null
>> +++ b/xen/arch/x86/include/asm/intel-txt.h
>> @@ -0,0 +1,277 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +
> Please have at least a one-liner introduction to what TXT is. Is there
> a stable URL for the TXT spec? (I can't spot an obvious one, googling
> around)
Not an official source and I don't know if it would be OK to use it
here, but my go-to source is https://kib.kiev.ua/x86docs/Intel/TXT/,
it even has previous versions, which may be helpful in understanding
some of deprecated (e.g. related to TPM 1.2) intricacies of
implementation.
>> +{
>> + volatile uint64_t *reg = _txt(TXT_PRIV_CONFIG_REGS_BASE + reg_no);
>> + return *reg;
>> +}
>> +
>> +static inline void write_txt_reg(int reg_no, uint64_t val)
>> +{
>> + volatile uint64_t *reg = _txt(TXT_PRIV_CONFIG_REGS_BASE + reg_no);
>> + *reg = val;
>> + /* This serves as TXT register barrier */
>> + (void)read_txt_reg(TXTCR_ESTS);
> What is this barrier for?
>
> It's not for anything in the CPU side of things, because UC accesses are
> strictly ordered.
>
> I presume it's for LPC bus ordering then, but making every write be a
> write/read pair seems very expensive.
According to TXT spec, it is required only for TXT.CMD.CLOSE-PRIVATE
and TXT.CMD.UNLOCK-MEM-CONFIG, and it probably could be changed to any
other serializing instruction. IIRC, those registers are written to
only in txt_reset(), so maybe adding a barrier there would suffice.
>> +/*
>> + * Functions to extract data from the Intel TXT Heap Memory. The layout
>> + * of the heap is as follows:
>> + * +------------------------------------+
>> + * | Size of Bios Data table (uint64_t) |
>> + * +------------------------------------+
>> + * | Bios Data table |
>> + * +------------------------------------+
>> + * | Size of OS MLE table (uint64_t) |
>> + * +------------------------------------+
>> + * | OS MLE table |
>> + * +-------------------------------- +
>> + * | Size of OS SINIT table (uint64_t) |
>> + * +------------------------------------+
>> + * | OS SINIT table |
>> + * +------------------------------------+
>> + * | Size of SINIT MLE table (uint64_t) |
>> + * +------------------------------------+
>> + * | SINIT MLE table |
>> + * +------------------------------------+
>> + *
>> + * NOTE: the table size fields include the 8 byte size field itself.
>> + */
>> +static inline uint64_t txt_bios_data_size(void *heap)
>> +{
>> + return *((uint64_t *)heap) - sizeof(uint64_t);
>> +}
> This is quite horrible, but I presume this is as specified by TXT?
Yes.
> To confirm, it's a tightly packed data structure where each of the 4
> blobs is variable size? Are there any alignment requirements on the
> table sizes? I suspect not, given the __packed attributes.
Yes, those are all of variable sizes. TXT specification notes that all
of the sizes must be a multiple of 8 bytes, but we've seen platforms
where BiosData (filled by BIOS ACM, so beyond our control) isn't
aligned, which makes following fields also unaligned. Because of that,
we treated it as if there was no such requirement.
--
Krystian Hebel
Firmware Engineer
https://3mdeb.com | @3mdeb_com
© 2016 - 2025 Red Hat, Inc.