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

[Zhao Liu <zhao1.liu@intel.com>]

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