Re: [PATCH 1/2] memattrs: Get rid of bit fields

[CLEMENT MATHIEU--DRIF]

Hi,

On 20/01/2025 17:38, Zhao Liu wrote:
> Caution: External email. Do not open attachments or click links, unless this 
> email comes from a known sender and you know the content is safe.
>
>
> Hi Peter,
>
>>>       /*
>>>        * PID (PCI PASID) support: Limited to 8 bits process identifier.
>>>        */
>>> -    unsigned int pid:8;
>>> -} MemTxAttrs;
>>> +    uint8_t pid;
>>> +
>>> +    /* Requester ID (for MSI for example) */
>>> +    uint16_t requester_id;
>>> +} QEMU_PACKED MemTxAttrs;
>>
>> If we pull the requester_id up to the top of the struct
>> we don't need the QEMU_PACKED, I think? (You get worse codegen
>> on some platforms if you use 'packed' when you don't need to.)
>
> Yes! I agree.
>
>> It would be good to note in the commit message:
>> (1) that this doesn't change the size of MemTxAttrs,
>> which is important because we pass it around directly,
>> not via a pointer (or does it raise it from 4 to 8 bytes?)
>
> MemTxAttrs is raised to 8 bytes (yes, I should mention this).
>
>> (2) that it does mean we have no spare space in the
>> struct for new fields without moving beyond 8 bytes.
>
> Thanks for the reminder, yes it is currently full. I found I missed
> a commnet from Paolo [*], that he suggested only convert `unspecified`
> to a bool. My bad :-(
>
> It still raises the size to 8 bytes but saves spare space, like:
>
> typedef struct MemTxAttrs {
>      unsigned int secure:1;
>      unsigned int space:2;
>      unsigned int user:1;
>      unsigned int memory:1;
>      unsigned int requester_id:16;
>      unsigned int pid:8;
>      bool unspecified;
>      uint8_t _reserved1;
>      uint16_t _reserved2;
> } MemTxAttrs;

Don't you think this will be an issue as some devices will need to
support more than 256 PID/PASID? The PCIe spec allows using up to 20 bits.

>
> Similar to your comment above, to get pakced structure, I think I need
> push `unspecified` field down to other bit fields.
>
> The rust side would require extra work to ZERO the other bit fields
> though. I'll go back to that mail thread and discuss the details again.
>
> [*]: 
> https://lore.kernel.org/qemu-devel/20241205060714.256270-1-zhao1.liu@intel.com/T/#m8b05874d630e3ec8834617babb97b32ec3b39fce
>
>> In particular we're talking about maybe adding a
>> "debug" attribute; so this is an unfortunate refactoring
>> from that point of view.
>
> Thank you for your comment. In v2, I will try converting only
> `unspecified` to a bool. Will that meet your expectations?
>
> Regards,
> Zhao
>
>

Thanks
cmd