Re: [Xen-devel] [PATCH v6 6/8] AMD/IOMMU: tidy struct ivrs_mappings

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 24 September 2019 10:02
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Suravee Suthikulpanit 
> <suravee.suthikulpanit@xxxxxxx>; Andrew
> Cooper <Andrew.Cooper3@xxxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v6 6/8] AMD/IOMMU: tidy struct ivrs_mappings
> 
> On 23.09.2019 18:25, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Jan 
> >> Beulich
> >> Sent: 19 September 2019 14:24
> >> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> >> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Suravee Suthikulpanit
> <suravee.suthikulpanit@xxxxxxx>
> >> Subject: [Xen-devel] [PATCH v6 6/8] AMD/IOMMU: tidy struct ivrs_mappings
> >>
> >> Move the device flags field up into an unused hole, thus shrinking
> >> overall structure size by 8 bytes. Use bool and uint<N>_t as
> >> appropriate. Drop pointless (redundant) initializations.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >
> > Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> 
> Thanks.
> 
> > ...although I wonder...
> >
> >> --- a/xen/include/asm-x86/amd-iommu.h
> >> +++ b/xen/include/asm-x86/amd-iommu.h
> >> @@ -106,12 +106,16 @@ struct amd_iommu {
> >>  };
> >>
> >>  struct ivrs_mappings {
> >> -    u16 dte_requestor_id;
> >> -    u8 dte_allow_exclusion;
> >> -    u8 unity_map_enable;
> >> -    u8 write_permission;
> >> -    u8 read_permission;
> >> +    uint16_t dte_requestor_id;
> >>      bool valid;
> >> +    bool dte_allow_exclusion;
> >> +    bool unity_map_enable;
> >> +    bool write_permission;
> >> +    bool read_permission;
> >
> > Could you shrink this even more by using a bit-field instead of this 
> > sequence of bools?
> 
> Indeed I had been considering this. Besides the fact that making
> such a move simply didn't look to fit other things here very well
> when introducing the "valid" flag in an earlier path, and then
> also not here, do you realize though that this wouldn't shrink
> the structure's size right now (i.e. it would only be potentially
> reducing future growth)?

Yes, I'd failed to note the 'unsigned long' afterwards, but...

> This was my main argument against going
> this further step; let me know what you think.
> 

I still think a pre-emptive squash into a uint8_t bit-field followed by the 
device_flags field would give the struct a nice 32-bit hole for potential 
future use.

  Paul

> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel