| Message ID | bd8d65e6-e887-4afe-8ff0-df86416fa869@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | AMD/IOMMU: assorted corrections | expand |
On 03/02/2025 4:26 pm, Jan Beulich wrote: > ... now that static initialization is possible. Use RADIX_TREE() for > pci_segments and ivrs_maps. > > Requested-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> I'd not considered having RADIX_TREE() but it's nicer than my attempt. However, > --- a/xen/include/xen/radix-tree.h > +++ b/xen/include/xen/radix-tree.h > @@ -72,6 +72,9 @@ struct radix_tree_root { > *** radix-tree API starts here ** > */ > > +#define RADIX_TREE_INIT() {} ... this ought to be (struct radix_tree_root){} so it can't be used with other types, and radix_tree_init() wants to become: void radix_tree_init(struct radix_tree_root *root) { *root = RADIX_TREE_INIT(); } instead of the raw memset(), so the connection is retained. Assuming you're happy with these adjustments, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> ~Andrew
On 03.02.2025 17:48, Andrew Cooper wrote: > On 03/02/2025 4:26 pm, Jan Beulich wrote: >> ... now that static initialization is possible. Use RADIX_TREE() for >> pci_segments and ivrs_maps. >> >> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > I'd not considered having RADIX_TREE() but it's nicer than my attempt. > > However, > >> --- a/xen/include/xen/radix-tree.h >> +++ b/xen/include/xen/radix-tree.h >> @@ -72,6 +72,9 @@ struct radix_tree_root { >> *** radix-tree API starts here ** >> */ >> >> +#define RADIX_TREE_INIT() {} > > ... this ought to be (struct radix_tree_root){} so it can't be used with > other types, and radix_tree_init() wants to become: > > void radix_tree_init(struct radix_tree_root *root) > { > *root = RADIX_TREE_INIT(); > } > > instead of the raw memset(), so the connection is retained. Can do; in fact I did consider both, but omitted them for simplicity. > Assuming you're happy with these adjustments, Reviewed-by: Andrew Cooper > <andrew.cooper3@citrix.com> Thanks. Jan
On 03.02.2025 17:58, Jan Beulich wrote: > On 03.02.2025 17:48, Andrew Cooper wrote: >> On 03/02/2025 4:26 pm, Jan Beulich wrote: >>> ... now that static initialization is possible. Use RADIX_TREE() for >>> pci_segments and ivrs_maps. >>> >>> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> I'd not considered having RADIX_TREE() but it's nicer than my attempt. >> >> However, >> >>> --- a/xen/include/xen/radix-tree.h >>> +++ b/xen/include/xen/radix-tree.h >>> @@ -72,6 +72,9 @@ struct radix_tree_root { >>> *** radix-tree API starts here ** >>> */ >>> >>> +#define RADIX_TREE_INIT() {} >> >> ... this ought to be (struct radix_tree_root){} so it can't be used with >> other types, and radix_tree_init() wants to become: >> >> void radix_tree_init(struct radix_tree_root *root) >> { >> *root = RADIX_TREE_INIT(); >> } >> >> instead of the raw memset(), so the connection is retained. > > Can do; in fact I did consider both, but omitted them for simplicity. Sadly I've now learned the hard way that the former can't be done. Gcc 4.3 complains "initializer element is not constant", which - aiui - is correct when considering plain C99 (and apparently the GNU99 extension to this was introduced only later). What I can do is make this compiler version dependent, adding type- safety on at least all more modern compilers. Thoughts? Jan
On 04.02.2025 09:36, Jan Beulich wrote: > On 03.02.2025 17:58, Jan Beulich wrote: >> On 03.02.2025 17:48, Andrew Cooper wrote: >>> On 03/02/2025 4:26 pm, Jan Beulich wrote: >>>> ... now that static initialization is possible. Use RADIX_TREE() for >>>> pci_segments and ivrs_maps. >>>> >>>> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> >>> I'd not considered having RADIX_TREE() but it's nicer than my attempt. >>> >>> However, >>> >>>> --- a/xen/include/xen/radix-tree.h >>>> +++ b/xen/include/xen/radix-tree.h >>>> @@ -72,6 +72,9 @@ struct radix_tree_root { >>>> *** radix-tree API starts here ** >>>> */ >>>> >>>> +#define RADIX_TREE_INIT() {} >>> >>> ... this ought to be (struct radix_tree_root){} so it can't be used with >>> other types, and radix_tree_init() wants to become: >>> >>> void radix_tree_init(struct radix_tree_root *root) >>> { >>> *root = RADIX_TREE_INIT(); >>> } >>> >>> instead of the raw memset(), so the connection is retained. >> >> Can do; in fact I did consider both, but omitted them for simplicity. > > Sadly I've now learned the hard way that the former can't be done. Gcc > 4.3 complains "initializer element is not constant", which - aiui - is > correct when considering plain C99 (and apparently the GNU99 extension > to this was introduced only later). And then I can't use RADIX_TREE_INIT() in radix_tree_init() anymore. All quite unhelpful. Jan > What I can do is make this compiler version dependent, adding type- > safety on at least all more modern compilers. Thoughts? > > Jan
On 04/02/2025 8:36 am, Jan Beulich wrote: > On 03.02.2025 17:58, Jan Beulich wrote: >> On 03.02.2025 17:48, Andrew Cooper wrote: >>> On 03/02/2025 4:26 pm, Jan Beulich wrote: >>>> ... now that static initialization is possible. Use RADIX_TREE() for >>>> pci_segments and ivrs_maps. >>>> >>>> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> I'd not considered having RADIX_TREE() but it's nicer than my attempt. >>> >>> However, >>> >>>> --- a/xen/include/xen/radix-tree.h >>>> +++ b/xen/include/xen/radix-tree.h >>>> @@ -72,6 +72,9 @@ struct radix_tree_root { >>>> *** radix-tree API starts here ** >>>> */ >>>> >>>> +#define RADIX_TREE_INIT() {} >>> ... this ought to be (struct radix_tree_root){} so it can't be used with >>> other types, and radix_tree_init() wants to become: >>> >>> void radix_tree_init(struct radix_tree_root *root) >>> { >>> *root = RADIX_TREE_INIT(); >>> } >>> >>> instead of the raw memset(), so the connection is retained. >> Can do; in fact I did consider both, but omitted them for simplicity. > Sadly I've now learned the hard way that the former can't be done. Gcc > 4.3 complains "initializer element is not constant", which - aiui - is > correct when considering plain C99 (and apparently the GNU99 extension > to this was introduced only later). > > What I can do is make this compiler version dependent, adding type- > safety on at least all more modern compilers. Thoughts? I think you can guess what my view is about us still supporting GCC 4.3. As this needs backporting, lets just go with {} for now. ~Andrew
--- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -31,7 +31,7 @@ static struct tasklet amd_iommu_irq_task unsigned int __read_mostly amd_iommu_acpi_info; unsigned int __read_mostly ivrs_bdf_entries; u8 __read_mostly ivhd_type; -static struct radix_tree_root ivrs_maps; +static RADIX_TREE(ivrs_maps); LIST_HEAD_RO_AFTER_INIT(amd_iommu_head); bool iommuv2_enabled; @@ -1408,7 +1408,6 @@ int __init amd_iommu_prepare(bool xt) goto error_out; ivrs_bdf_entries = rc; - radix_tree_init(&ivrs_maps); for_each_amd_iommu ( iommu ) { rc = amd_iommu_prepare_one(iommu); --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -68,7 +68,7 @@ bool pcidevs_locked(void) return rspin_is_locked(&_pcidevs_lock); } -static struct radix_tree_root pci_segments; +static RADIX_TREE(pci_segments); static inline struct pci_seg *get_pseg(u16 seg) { @@ -124,7 +124,6 @@ static int pci_segments_iterate( void __init pci_segments_init(void) { - radix_tree_init(&pci_segments); if ( !alloc_pseg(0) ) panic("Could not initialize PCI segment 0\n"); } --- a/xen/include/xen/radix-tree.h +++ b/xen/include/xen/radix-tree.h @@ -72,6 +72,9 @@ struct radix_tree_root { *** radix-tree API starts here ** */ +#define RADIX_TREE_INIT() {} +#define RADIX_TREE(name) struct radix_tree_root name = RADIX_TREE_INIT() + void radix_tree_init(struct radix_tree_root *root); void radix_tree_destroy(
... now that static initialization is possible. Use RADIX_TREE() for pci_segments and ivrs_maps. Requested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: New.