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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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