... 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()
+
 void radix_tree_init(struct radix_tree_root *root);
 
 void radix_tree_destroy(

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