[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


  • To: Sergii Dmytruk <sergii.dmytruk@xxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 7 Jul 2025 10:24:37 +0200
  • Autocrypt: addr=jbeulich@xxxxxxxx; keydata= xsDiBFk3nEQRBADAEaSw6zC/EJkiwGPXbWtPxl2xCdSoeepS07jW8UgcHNurfHvUzogEq5xk hu507c3BarVjyWCJOylMNR98Yd8VqD9UfmX0Hb8/BrA+Hl6/DB/eqGptrf4BSRwcZQM32aZK 7Pj2XbGWIUrZrd70x1eAP9QE3P79Y2oLrsCgbZJfEwCgvz9JjGmQqQkRiTVzlZVCJYcyGGsD /0tbFCzD2h20ahe8rC1gbb3K3qk+LpBtvjBu1RY9drYk0NymiGbJWZgab6t1jM7sk2vuf0Py O9Hf9XBmK0uE9IgMaiCpc32XV9oASz6UJebwkX+zF2jG5I1BfnO9g7KlotcA/v5ClMjgo6Gl MDY4HxoSRu3i1cqqSDtVlt+AOVBJBACrZcnHAUSuCXBPy0jOlBhxPqRWv6ND4c9PH1xjQ3NP nxJuMBS8rnNg22uyfAgmBKNLpLgAGVRMZGaGoJObGf72s6TeIqKJo/LtggAS9qAUiuKVnygo 3wjfkS9A3DRO+SpU7JqWdsveeIQyeyEJ/8PTowmSQLakF+3fote9ybzd880fSmFuIEJldWxp Y2ggPGpiZXVsaWNoQHN1c2UuY29tPsJgBBMRAgAgBQJZN5xEAhsDBgsJCAcDAgQVAggDBBYC AwECHgECF4AACgkQoDSui/t3IH4J+wCfQ5jHdEjCRHj23O/5ttg9r9OIruwAn3103WUITZee e7Sbg12UgcQ5lv7SzsFNBFk3nEQQCACCuTjCjFOUdi5Nm244F+78kLghRcin/awv+IrTcIWF hUpSs1Y91iQQ7KItirz5uwCPlwejSJDQJLIS+QtJHaXDXeV6NI0Uef1hP20+y8qydDiVkv6l IreXjTb7DvksRgJNvCkWtYnlS3mYvQ9NzS9PhyALWbXnH6sIJd2O9lKS1Mrfq+y0IXCP10eS FFGg+Av3IQeFatkJAyju0PPthyTqxSI4lZYuJVPknzgaeuJv/2NccrPvmeDg6Coe7ZIeQ8Yj t0ARxu2xytAkkLCel1Lz1WLmwLstV30g80nkgZf/wr+/BXJW/oIvRlonUkxv+IbBM3dX2OV8 AmRv1ySWPTP7AAMFB/9PQK/VtlNUJvg8GXj9ootzrteGfVZVVT4XBJkfwBcpC/XcPzldjv+3 HYudvpdNK3lLujXeA5fLOH+Z/G9WBc5pFVSMocI71I8bT8lIAzreg0WvkWg5V2WZsUMlnDL9 mpwIGFhlbM3gfDMs7MPMu8YQRFVdUvtSpaAs8OFfGQ0ia3LGZcjA6Ik2+xcqscEJzNH+qh8V m5jjp28yZgaqTaRbg3M/+MTbMpicpZuqF4rnB0AQD12/3BNWDR6bmh+EkYSMcEIpQmBM51qM EKYTQGybRCjpnKHGOxG0rfFY1085mBDZCH5Kx0cl0HVJuQKC+dV2ZY5AqjcKwAxpE75MLFkr wkkEGBECAAkFAlk3nEQCGwwACgkQoDSui/t3IH7nnwCfcJWUDUFKdCsBH/E5d+0ZnMQi+G0A nAuWpQkjM1ASeQwSHEeAWPgskBQL
  • Cc: "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>, Ross Philipson <ross.philipson@xxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Lukasz Hawrylko <lukasz@xxxxxxxxxxx>, Mateusz Mówka <mateusz.mowka@xxxxxxxxx>, trenchboot-devel@xxxxxxxxxxxxxxxx, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Mon, 07 Jul 2025 08:24:51 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.