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....@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
