Re: [PATCH v3 02/22] include/xen/slr-table.h: Secure Launch Resource Table definitions

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.07.2025 18:55, Sergii Dmytruk wrote:
> On Wed, Jul 02, 2025 at 04:36:27PM +0200, Jan Beulich wrote:
>> On 30.05.2025 15:17, Sergii Dmytruk wrote:
>>> The file provides constants, structures and several helper functions for
>>> parsing SLRT.
>>>
>>> The data described by the structures is passed to Xen by a bootloader
>>> which initiated DRTM.
>>>
>>> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>>> Signed-off-by: Ross Philipson <ross.philipson@xxxxxxxxxx>
>>> Signed-off-by: Sergii Dmytruk <sergii.dmytruk@xxxxxxxxx>
>>> ---
>>>  xen/include/xen/slr-table.h | 276 ++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 276 insertions(+)
>>>  create mode 100644 xen/include/xen/slr-table.h
>>
>> Btw, please don't forget to Cc maintainers of code you're changing / adding.
> 
> What do you mean?  I'm running add_maintainers.pl on the patches.

The Cc: list had none of the REST maintainers. (Whether there's a bug in the
script I can't tell.)

>>> +/*
>>> + * Prototype of a function pointed to by slr_entry_dl_info::dl_handler.
>>> + */
>>> +typedef void (*dl_handler_func)(struct slr_bl_context *bl_context);
>>
>> I keep wondering why this ...
>>
>>> +/*
>>> + * DRTM Dynamic Launch Configuration
>>> + */
>>> +struct slr_entry_dl_info
>>> +{
>>> +    struct slr_entry_hdr hdr;
>>> +    uint64_t dce_size;
>>> +    uint64_t dce_base;
>>> +    uint64_t dlme_size;
>>> +    uint64_t dlme_base;
>>> +    uint64_t dlme_entry;
>>> +    struct slr_bl_context bl_context;
>>> +    uint64_t dl_handler;
>>
>> ... then isn't used right here, instead requiring a cast somewhere 
>> (presumably,
>> as code using this isn't visible in this patch).
> 
> As was mentioned earlier: because size of a pointer between Xen and a
> bootloader doesn't necessarily match.  What you're proposing can break
> under certain conditions.

But the header isn't shared with a bootloader's code base. It's private to
Xen.

>>> +} __packed;
>>
>> I similarly keep wondering why there are all these packed attributes here, 
>> when
>> (afaics) all of the structures are defined in a way that any padding is 
>> explicit
>> anyway.
> 
> As before: it won't hurt, it's future-proof, just in case and to let
> reader of the code know the structure must have no padding.  If you want
> them gone, I can do that, but I think it will make the code harder to
> maintain.

Well, if there's a maintenance concern, then I would of course like to learn
about that. I could see such being the case if the file was imported from
somewhere. But with neither description nor any code comment not saying so,
that doesn't look to be the case.

Jan