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

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

[Zhao Liu]

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;

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

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

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

[Zhao Liu]

Hi CLEMENT,

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

Maybe we can have the layout of MeymTxAttrs like this:

typedef struct MemTxAttrs {
    unsigned int pid:20;
    uint16_t requester_id:16;
    uint8_t secure:1;
    uint8_t space:2;
    uint8_t user:1;
    uint8_t memory:1;
    bool unspecified;
} MemTxAttrs;

I think a finer granularity division could solve this issue. And
there're still some spare spaces :-).

Thanks,
Zhao

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

[Paolo Bonzini]

On 1/20/25 17:38, Zhao Liu wrote:
> 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;
> 
> Similar to your comment above, to get pakced structure, I think I need
> push `unspecified` field down to other bit fields.

Right, this would allow for a 16-bit PASID, 19 free bits and no QEMU_PACKED:

typedef struct MemTxAttrs {
     bool unspecified;
     uint8_t int secure:1;
     uint8_t int space:2;
     uint8_t int user:1;
     uint8_t memory:1;
     uint16_t requester_id;
     uint16_t pid;
     uint16_t _reserved;
} MemTxAttrs;
QEMU_BUILD_BUG_ON(sizeof(MemTxAttrs) > 8);

Together with const_zero!() that would be fine for both C and Rust, and 
a bit more efficient too.

Paolo