[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 01/22] x86/include/asm/intel-txt.h: constants and accessors for TXT registers and heap
On Wed, Jul 02, 2025 at 04:29:18PM +0200, Jan Beulich wrote: > Btw, a brief rev log would be nice here. I saw you have something in the > cover letter, but having to look in two places isn't very helpful. I don't really know how to effectively maintain 23 logs at the same time given that changing one patch has cascading effects on the rest. I'd suggest using `git diff-range` instead, commands for which I can include in cover letters for convenience. > > +#include <asm/page.h> // __va() > > Nit: No C++ style comments, please. Sure. > > +#define _txt(x) __va(x) > > +#endif > > Without any uses the correctness of the above is hard to judge. The _txt() macro is used right below: > > +/* > > + * Always use private space as some of registers are either read-only or > > not > > + * present in public space. > > + */ > > +static inline uint64_t txt_read(unsigned int reg_no) > > +{ > > + volatile uint64_t *reg = _txt(TXT_PRIV_CONFIG_REGS_BASE + reg_no); > > + return *reg; > > +} > > + > > +static inline void txt_write(unsigned 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 after writing to > > + * TXTCR_CMD_UNLOCK_MEM_CONFIG. Must be done to ensure that any future > > + * chipset operations see the write. > > + */ > > + (void)txt_read(TXTCR_ESTS); > > I don't think the cast is needed. It's not needed, but I think that explicitly discarding unused return value is a generally good practice even when there is a comment. > > + txt_write(TXTCR_CMD_RESET, 1); > > + unreachable(); > > What guarantees the write to take immediate effect? That is, shouldn't there > be e.g. an infinite loop here, just in case? I'll return infinite loop from v2. Tried adding `halt()` as Ross suggests, but including <asm/system.h> doesn't work in the early code (something about compat headers and missing expansion of things like __DeFiNe__). > > +static inline uint64_t txt_bios_data_size(const void *heap) > > +{ > > + return *(const uint64_t *)heap - sizeof(uint64_t); > > Like you already do here, ... > > > +} > > + > > +static inline void *txt_bios_data_start(const void *heap) > > +{ > > + return (void *)heap + sizeof(uint64_t); > > ... please don't cast away const-ness. I'm pretty sure I said before that > Misra objects to us doing so. Mind that you had left some comments on v2 after I sent v3. v4 will have this section rewritten using loops and constants, which resolves issues with constness and readability. > > @@ -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, > > Was this perhaps meant to be TXT_CONFIG_SPACE_SIZE? > > Jan Right, thanks, building without tboot didn't catch this. Regards
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |