| Message ID | 6de11867-b872-a2a1-7c26-af004164bfea@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | AMD IOMMU: further improvements | expand |
> -----Original Message----- > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich > Sent: 19 September 2019 14:24 > To: xen-devel@lists.xenproject.org > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> > 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@suse.com> Reviewed-by: Paul Durrant <paul.durrant@citrix.com> ...although I wonder... > --- > v6: New. > > --- > xen/drivers/passthrough/amd/iommu_acpi.c | 6 +++--- > xen/drivers/passthrough/amd/iommu_init.c | 6 ------ > xen/include/asm-x86/amd-iommu.h | 17 +++++++++-------- > 3 files changed, 12 insertions(+), 17 deletions(-) > > --- a/xen/drivers/passthrough/amd/iommu_acpi.c > +++ b/xen/drivers/passthrough/amd/iommu_acpi.c > @@ -165,7 +165,7 @@ static void __init reserve_unity_map_for > /* extend r/w permissioms and keep aggregate */ > ivrs_mappings[bdf].write_permission = iw; > ivrs_mappings[bdf].read_permission = ir; > - ivrs_mappings[bdf].unity_map_enable = IOMMU_CONTROL_ENABLED; > + ivrs_mappings[bdf].unity_map_enable = true; > ivrs_mappings[bdf].addr_range_start = base; > ivrs_mappings[bdf].addr_range_length = length; > } > @@ -242,8 +242,8 @@ static int __init register_exclusion_ran > if ( limit >= iommu_top ) > { > reserve_iommu_exclusion_range(iommu, base, limit); > - ivrs_mappings[bdf].dte_allow_exclusion = IOMMU_CONTROL_ENABLED; > - ivrs_mappings[req].dte_allow_exclusion = IOMMU_CONTROL_ENABLED; > + ivrs_mappings[bdf].dte_allow_exclusion = true; > + ivrs_mappings[req].dte_allow_exclusion = true; > } > > return 0; > --- a/xen/drivers/passthrough/amd/iommu_init.c > +++ b/xen/drivers/passthrough/amd/iommu_init.c > @@ -1222,12 +1222,6 @@ static int __init alloc_ivrs_mappings(u1 > for ( bdf = 0; bdf < ivrs_bdf_entries; bdf++ ) > { > ivrs_mappings[bdf].dte_requestor_id = bdf; > - ivrs_mappings[bdf].dte_allow_exclusion = IOMMU_CONTROL_DISABLED; > - ivrs_mappings[bdf].unity_map_enable = IOMMU_CONTROL_DISABLED; > - ivrs_mappings[bdf].iommu = NULL; > - > - ivrs_mappings[bdf].intremap_table = NULL; > - ivrs_mappings[bdf].device_flags = 0; > > if ( amd_iommu_perdev_intremap ) > spin_lock_init(&ivrs_mappings[bdf].intremap_lock); > --- 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? > + > + /* ivhd device data settings */ > + uint8_t device_flags; > + > unsigned long addr_range_start; > unsigned long addr_range_length; > struct amd_iommu *iommu; > @@ -120,9 +124,6 @@ struct ivrs_mappings { > void *intremap_table; > unsigned long *intremap_inuse; > spinlock_t intremap_lock; > - > - /* ivhd device data settings */ > - u8 device_flags; > }; > > extern unsigned int ivrs_bdf_entries; > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel
On 23.09.2019 18:25, Paul Durrant wrote: >> -----Original Message----- >> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich >> Sent: 19 September 2019 14:24 >> To: xen-devel@lists.xenproject.org >> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> >> 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@suse.com> > > Reviewed-by: Paul Durrant <paul.durrant@citrix.com> 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)? This was my main argument against going this further step; let me know what you think. Jan
> -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 24 September 2019 10:02 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: xen-devel@lists.xenproject.org; Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; Andrew > Cooper <Andrew.Cooper3@citrix.com> > 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@lists.xenproject.org> On Behalf Of Jan Beulich > >> Sent: 19 September 2019 14:24 > >> To: xen-devel@lists.xenproject.org > >> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Suravee Suthikulpanit > <suravee.suthikulpanit@amd.com> > >> 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@suse.com> > > > > Reviewed-by: Paul Durrant <paul.durrant@citrix.com> > > 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
On 24.09.2019 11:08, Paul Durrant wrote: >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 24 September 2019 10:02 >> >> On 23.09.2019 18:25, Paul Durrant wrote: >>>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich >>>> Sent: 19 September 2019 14:24 >>>> >>>> --- 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. Okay, will do then. I take it your R-b can remain in place with this extra change. Jan
> -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 24 September 2019 10:13 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: SuraveeSuthikulpanit <suravee.suthikulpanit@amd.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; > xen-devel@lists.xenproject.org > Subject: Re: [Xen-devel] [PATCH v6 6/8] AMD/IOMMU: tidy struct ivrs_mappings > > On 24.09.2019 11:08, Paul Durrant wrote: > >> From: Jan Beulich <jbeulich@suse.com> > >> Sent: 24 September 2019 10:02 > >> > >> On 23.09.2019 18:25, Paul Durrant wrote: > >>>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich > >>>> Sent: 19 September 2019 14:24 > >>>> > >>>> --- 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. > > Okay, will do then. I take it your R-b can remain in place with this > extra change. Absolutely. Thanks, Paul > > Jan
On 19/09/2019 14:24, Jan Beulich wrote: > 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@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/drivers/passthrough/amd/iommu_acpi.c +++ b/xen/drivers/passthrough/amd/iommu_acpi.c @@ -165,7 +165,7 @@ static void __init reserve_unity_map_for /* extend r/w permissioms and keep aggregate */ ivrs_mappings[bdf].write_permission = iw; ivrs_mappings[bdf].read_permission = ir; - ivrs_mappings[bdf].unity_map_enable = IOMMU_CONTROL_ENABLED; + ivrs_mappings[bdf].unity_map_enable = true; ivrs_mappings[bdf].addr_range_start = base; ivrs_mappings[bdf].addr_range_length = length; } @@ -242,8 +242,8 @@ static int __init register_exclusion_ran if ( limit >= iommu_top ) { reserve_iommu_exclusion_range(iommu, base, limit); - ivrs_mappings[bdf].dte_allow_exclusion = IOMMU_CONTROL_ENABLED; - ivrs_mappings[req].dte_allow_exclusion = IOMMU_CONTROL_ENABLED; + ivrs_mappings[bdf].dte_allow_exclusion = true; + ivrs_mappings[req].dte_allow_exclusion = true; } return 0; --- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -1222,12 +1222,6 @@ static int __init alloc_ivrs_mappings(u1 for ( bdf = 0; bdf < ivrs_bdf_entries; bdf++ ) { ivrs_mappings[bdf].dte_requestor_id = bdf; - ivrs_mappings[bdf].dte_allow_exclusion = IOMMU_CONTROL_DISABLED; - ivrs_mappings[bdf].unity_map_enable = IOMMU_CONTROL_DISABLED; - ivrs_mappings[bdf].iommu = NULL; - - ivrs_mappings[bdf].intremap_table = NULL; - ivrs_mappings[bdf].device_flags = 0; if ( amd_iommu_perdev_intremap ) spin_lock_init(&ivrs_mappings[bdf].intremap_lock); --- 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; + + /* ivhd device data settings */ + uint8_t device_flags; + unsigned long addr_range_start; unsigned long addr_range_length; struct amd_iommu *iommu; @@ -120,9 +124,6 @@ struct ivrs_mappings { void *intremap_table; unsigned long *intremap_inuse; spinlock_t intremap_lock; - - /* ivhd device data settings */ - u8 device_flags; }; extern unsigned int ivrs_bdf_entries;
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@suse.com> --- v6: New. --- xen/drivers/passthrough/amd/iommu_acpi.c | 6 +++--- xen/drivers/passthrough/amd/iommu_init.c | 6 ------ xen/include/asm-x86/amd-iommu.h | 17 +++++++++-------- 3 files changed, 12 insertions(+), 17 deletions(-)