| Message ID | e1114d64-61f9-47b9-a1ed-33b526d40089@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | AMD/IOMMU: assorted corrections (leftover) | expand |
On 04/02/2025 1:03 pm, Jan Beulich wrote: > ... now that static initialization is possible. Use RADIX_TREE() for > pci_segments and ivrs_maps. > > This then fixes an ordering issue on x86: With the call to > radix_tree_init(), acpi_mmcfg_init()'s invocation of pci_segments_init() > will zap the possible earlier introduction of segment 0 by > amd_iommu_detect_one_acpi()'s call to pci_ro_device(), and thus the > write-protection of the PCI devices representing AMD IOMMUs. > > Fixes: 3950f2485bbc ("x86/x2APIC: defer probe until after IOMMU ACPI table parsing") > Requested-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > Sadly gcc below 5.x doesn't support compound literals in static > initializers (Clang 3.5 does). Hence the request in response to v2 has > to remain un-addressed. > --- > v3: Extend description and add Fixes: tag. > v2: New. > > --- 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() We can still use this form in radix_tree_init(), can't we? ~Andrew
On 04.02.2025 14:10, Andrew Cooper wrote: > On 04/02/2025 1:03 pm, Jan Beulich wrote: >> --- 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() > > We can still use this form in radix_tree_init(), can't we? Only indirectly, and that's imo ugly: void radix_tree_init(struct radix_tree_root *root) { RADIX_TREE(init); *root = init; } RADIX_TREE_INIT() cannot (directly) be used because {} isn't an rvalue. Jan
On 04/02/2025 1:19 pm, Jan Beulich wrote: > On 04.02.2025 14:10, Andrew Cooper wrote: >> On 04/02/2025 1:03 pm, Jan Beulich wrote: >>> --- 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() >> We can still use this form in radix_tree_init(), can't we? > Only indirectly, and that's imo ugly: > > void radix_tree_init(struct radix_tree_root *root) > { > RADIX_TREE(init); > > *root = init; > } > > RADIX_TREE_INIT() cannot (directly) be used because {} isn't an rvalue. Even if it's not ideal, *root = *(struct radix_tree_root)RADIX_TREE_INIT(); works. We use this pattern elsewhere in Xen, so it surely will be fine even on ancient compilers. ~Andrew
On 04.02.2025 14:56, Andrew Cooper wrote: > On 04/02/2025 1:19 pm, Jan Beulich wrote: >> On 04.02.2025 14:10, Andrew Cooper wrote: >>> On 04/02/2025 1:03 pm, Jan Beulich wrote: >>>> --- 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() >>> We can still use this form in radix_tree_init(), can't we? >> Only indirectly, and that's imo ugly: >> >> void radix_tree_init(struct radix_tree_root *root) >> { >> RADIX_TREE(init); >> >> *root = init; >> } >> >> RADIX_TREE_INIT() cannot (directly) be used because {} isn't an rvalue. > > Even if it's not ideal, > > *root = *(struct radix_tree_root)RADIX_TREE_INIT(); > > works. We use this pattern elsewhere in Xen, so it surely will be fine > even on ancient compilers. Hmm, yes, with the * on the rhs dropped this ought to work. I'll switch, yet at the same time I have to admit that I don't recall having seen this pattern anywhere in our tree. Jan
--- 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. This then fixes an ordering issue on x86: With the call to radix_tree_init(), acpi_mmcfg_init()'s invocation of pci_segments_init() will zap the possible earlier introduction of segment 0 by amd_iommu_detect_one_acpi()'s call to pci_ro_device(), and thus the write-protection of the PCI devices representing AMD IOMMUs. Fixes: 3950f2485bbc ("x86/x2APIC: defer probe until after IOMMU ACPI table parsing") Requested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> --- Sadly gcc below 5.x doesn't support compound literals in static initializers (Clang 3.5 does). Hence the request in response to v2 has to remain un-addressed. --- v3: Extend description and add Fixes: tag. v2: New.