On 06.07.2025 17:57, Sergii Dmytruk wrote:
> 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.
Well, no, doing this per patch is possible and relevant. For cascading
effects their mentioning in a revlog can be pretty brief.
>>> +#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:
Well, the comment was meant a little indirectly: The correctness in
particular wrt the __EARLY_SLAUNCH__ distinction.
>>> +/*
>>> + * 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.
In the context of Misra there has been discussion about doing so. But in our
present code base you will find such as the exception, not the rule.
>>> + 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__).
Yeah, untangling that may be a little involved. Open-coding halt() is an
option, as long as you clearly indicate it as such (for e.g. grep to still
find that instance).
Jan
