Re: [PATCH v2 01/22] x86/include/asm/intel-txt.h: constants and accessors for TXT registers and heap

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.05.2025 19:05, Sergii Dmytruk wrote:
> From: Krystian Hebel <krystian.hebel@xxxxxxxxx>
> 
> 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@xxxxxxxxx>
> Signed-off-by: Sergii Dmytruk <sergii.dmytruk@xxxxxxxxx>
> ---
>  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