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
