[PATCH for-4.20 0/6] AMD/IOMMU: assorted corrections

[PATCH for-4.20 0/6] AMD/IOMMU: assorted corrections

[Jan Beulich]

The first two patches are functionally independent, and they're presented
here merely in the order I came to notice the respective issues. At least
patch 2 wants seriously considering for 4.20.

While alternatives were considered for patch 2, it's left as it was in v1
for now. The disposition there depends on (a) the four new patches, in
particular what the last patch does and (b) backporting considerations
(we probably don't want to backport any of the radix tree tidying).

1: AMD/IOMMU: drop stray MSI enabling
2: x86/PCI: init segments earlier
3: radix-tree: purge node allocation override hooks
4: radix-tree: drop "root" parameters from radix_tree_node_{alloc,free}()
5: radix-tree: introduce RADIX_TREE{,_INIT}()
6: PCI: drop pci_segments_init()

Jan

[PATCH v2 for-4.20? 1/6] AMD/IOMMU: drop stray MSI enabling

[Jan Beulich]

While the 2nd of the commits referenced below should have moved the call
to amd_iommu_msi_enable() instead of adding another one, the situation
wasn't quite right even before: It can't have done any good to enable
MSI when no IRQ was allocated for it, yet.

The other call to amd_iommu_msi_enable(), just out of patch context,
needs to stay there until S3 resume is re-worked. For the boot path that
call should be unnecessary, as iommu{,_maskable}_msi_startup() will have
done it already (by way of invoking iommu_msi_unmask()).

Fixes: 5f569f1ac50e ("AMD/IOMMU: allow enabling with IRQ not yet set up")
Fixes: d9e49d1afe2e ("AMD/IOMMU: adjust setup of internal interrupt for x2APIC mode")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
Tested-by: Jason Andryuk <jason.andryuk@amd.com>
---
v2: Extend description.

--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -902,8 +902,6 @@ static void enable_iommu(struct amd_iomm
         }
     }
 
-    amd_iommu_msi_enable(iommu, IOMMU_CONTROL_ENABLED);
-
     set_iommu_ht_flags(iommu);
     set_iommu_command_buffer_control(iommu, IOMMU_CONTROL_ENABLED);
 

[PATCH v2 for-4.20 2/6] x86/PCI: init segments earlier

[Jan Beulich]

In order for amd_iommu_detect_one_acpi()'s call to pci_ro_device() to
have permanent effect, pci_segments_init() needs to be called ahead of
making it there. Without this we're losing segment 0's r/o map, and thus
we're losing write-protection of the PCI devices representing IOMMUs.
Which in turn means that half-way recent Linux Dom0 will, as it boots,
turn off MSI on these devices, thus preventing any IOMMU events (faults
in particular) from being reported on pre-x2APIC hardware.

As the acpi_iommu_init() invocation was moved ahead of
acpi_mmcfg_init()'s by the offending commit, move the call to
pci_segments_init() accordingly.

Fixes: 3950f2485bbc ("x86/x2APIC: defer probe until after IOMMU ACPI table parsing")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
Tested-by: Jason Andryuk <jason.andryuk@amd.com>
---
Of course it would have been quite a bit easier to notice this issue if
radix_tree_insert() wouldn't work fine ahead of radix_tree_init() being
invoked for a given radix tree, when the index inserted at is 0.

While hunting down various other dead paths to actually find the root
cause, it occurred to me that it's probably not a good idea to fully
disallow config space writes for r/o devices: Dom0 won't be able to size
their BARs (luckily the IOMMU "devices" don't have any, but e.g. serial
ones generally will have at least one), for example. Without being able
to size BARs it also will likely be unable to correctly account for the
address space taken by these BARs. However, outside of vPCI it's not
really clear to me how we could reasonably emulate such BAR sizing
writes - we can't, after all, allow Dom0 to actually write to the
underlying physical registers, yet we don't intercept reads (i.e. we
can't mimic expected behavior then).

--- a/xen/arch/x86/x86_64/mmconfig-shared.c
+++ b/xen/arch/x86/x86_64/mmconfig-shared.c
@@ -402,8 +402,6 @@ void __init acpi_mmcfg_init(void)
 {
     bool valid = true;
 
-    pci_segments_init();
-
     /* MMCONFIG disabled */
     if ((pci_probe & PCI_PROBE_MMCONF) == 0)
         return;
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -55,6 +55,8 @@ void __init acpi_iommu_init(void)
 {
     int ret = -ENODEV;
 
+    pci_segments_init();
+
     if ( !iommu_enable && !iommu_intremap )
         return;
 

[PATCH v2 for-4.20 3/6] radix-tree: purge node allocation override hooks

[Jan Beulich]

These were needed by TMEM only, which is long gone. The Linux original
doesn't have such either. This effectively reverts one of the "Other
changes" from 8dc6738dbb3c ("Update radix-tree.[ch] from upstream Linux
to gain RCU awareness").

Positive side effect: Two cf_check go away.

While there also convert xmalloc()+memset() to xzalloc(). (Don't convert
to xvzalloc(), as that would require touching the freeing side, too.)

Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/common/radix-tree.c
+++ b/xen/common/radix-tree.c
@@ -53,12 +53,6 @@ struct rcu_node {
 	struct rcu_head rcu_head;
 };
 
-static struct radix_tree_node *cf_check rcu_node_alloc(void *arg)
-{
-	struct rcu_node *rcu_node = xmalloc(struct rcu_node);
-	return rcu_node ? &rcu_node->node : NULL;
-}
-
 static void cf_check _rcu_node_free(struct rcu_head *head)
 {
 	struct rcu_node *rcu_node =
@@ -66,26 +60,19 @@ static void cf_check _rcu_node_free(stru
 	xfree(rcu_node);
 }
 
-static void cf_check rcu_node_free(struct radix_tree_node *node, void *arg)
-{
-	struct rcu_node *rcu_node = container_of(node, struct rcu_node, node);
-	call_rcu(&rcu_node->rcu_head, _rcu_node_free);
-}
-
 static struct radix_tree_node *radix_tree_node_alloc(
 	struct radix_tree_root *root)
 {
-	struct radix_tree_node *ret;
-	ret = root->node_alloc(root->node_alloc_free_arg);
-	if (ret)
-		memset(ret, 0, sizeof(*ret));
-	return ret;
+	struct rcu_node *rcu_node = xzalloc(struct rcu_node);
+
+	return rcu_node ? &rcu_node->node : NULL;
 }
 
 static void radix_tree_node_free(
 	struct radix_tree_root *root, struct radix_tree_node *node)
 {
-	root->node_free(node, root->node_alloc_free_arg);
+	struct rcu_node *rcu_node = container_of(node, struct rcu_node, node);
+	call_rcu(&rcu_node->rcu_head, _rcu_node_free);
 }
 
 /*
@@ -718,19 +705,6 @@ void radix_tree_destroy(
 void radix_tree_init(struct radix_tree_root *root)
 {
 	memset(root, 0, sizeof(*root));
-	root->node_alloc = rcu_node_alloc;
-	root->node_free = rcu_node_free;
-}
-
-void radix_tree_set_alloc_callbacks(
-	struct radix_tree_root *root,
-	radix_tree_alloc_fn_t *node_alloc,
-	radix_tree_free_fn_t *node_free,
-	void *node_alloc_free_arg)
-{
-	root->node_alloc = node_alloc;
-	root->node_free = node_free;
-	root->node_alloc_free_arg = node_alloc_free_arg;
 }
 
 static __init unsigned long __maxindex(unsigned int height)
--- a/xen/include/xen/radix-tree.h
+++ b/xen/include/xen/radix-tree.h
@@ -66,11 +66,6 @@ typedef void radix_tree_free_fn_t(struct
 struct radix_tree_root {
 	unsigned int		height;
 	struct radix_tree_node	__rcu *rnode;
-
-	/* Allow to specify custom node alloc/dealloc routines. */
-	radix_tree_alloc_fn_t *node_alloc;
-	radix_tree_free_fn_t *node_free;
-	void *node_alloc_free_arg;
 };
 
 /*
@@ -78,11 +73,6 @@ struct radix_tree_root {
  */
 
 void radix_tree_init(struct radix_tree_root *root);
-void radix_tree_set_alloc_callbacks(
-	struct radix_tree_root *root,
-	radix_tree_alloc_fn_t *node_alloc,
-	radix_tree_free_fn_t *node_free,
-	void *node_alloc_free_arg);
 
 void radix_tree_destroy(
 	struct radix_tree_root *root,

[PATCH v2 for-4.21 4/6] radix-tree: drop "root" parameters from radix_tree_node_{alloc,free}()

[Jan Beulich]

They aren't used anymore.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/common/radix-tree.c
+++ b/xen/common/radix-tree.c
@@ -60,16 +60,14 @@ static void cf_check _rcu_node_free(stru
 	xfree(rcu_node);
 }
 
-static struct radix_tree_node *radix_tree_node_alloc(
-	struct radix_tree_root *root)
+static struct radix_tree_node *radix_tree_node_alloc(void)
 {
 	struct rcu_node *rcu_node = xzalloc(struct rcu_node);
 
 	return rcu_node ? &rcu_node->node : NULL;
 }
 
-static void radix_tree_node_free(
-	struct radix_tree_root *root, struct radix_tree_node *node)
+static void radix_tree_node_free(struct radix_tree_node *node)
 {
 	struct rcu_node *rcu_node = container_of(node, struct rcu_node, node);
 	call_rcu(&rcu_node->rcu_head, _rcu_node_free);
@@ -104,7 +102,7 @@ static int radix_tree_extend(struct radi
 
 	do {
 		unsigned int newheight;
-		if (!(node = radix_tree_node_alloc(root)))
+		if (!(node = radix_tree_node_alloc()))
 			return -ENOMEM;
 
 		/* Increase the height.  */
@@ -155,7 +153,7 @@ int radix_tree_insert(struct radix_tree_
 	while (height > 0) {
 		if (slot == NULL) {
 			/* Have to add a child node.  */
-			if (!(slot = radix_tree_node_alloc(root)))
+			if (!(slot = radix_tree_node_alloc()))
 				return -ENOMEM;
 			slot->height = height;
 			if (node) {
@@ -574,7 +572,7 @@ static inline void radix_tree_shrink(str
 			*((unsigned long *)&to_free->slots[0]) |=
 						RADIX_TREE_INDIRECT_PTR;
 
-		radix_tree_node_free(root, to_free);
+		radix_tree_node_free(to_free);
 	}
 }
 
@@ -639,7 +637,7 @@ void *radix_tree_delete(struct radix_tre
 		 * last reference to it disappears (set NULL, above).
 		 */
 		if (to_free)
-			radix_tree_node_free(root, to_free);
+			radix_tree_node_free(to_free);
 
 		if (pathp->node->count) {
 			if (pathp->node == indirect_to_ptr(root->rnode))
@@ -655,7 +653,7 @@ void *radix_tree_delete(struct radix_tre
 	root->height = 0;
 	root->rnode = NULL;
 	if (to_free)
-		radix_tree_node_free(root, to_free);
+		radix_tree_node_free(to_free);
 
 out:
 	return slot;
@@ -682,7 +680,7 @@ radix_tree_node_destroy(
 		}
 	}
 
-	radix_tree_node_free(root, node);
+	radix_tree_node_free(node);
 }
 
 void radix_tree_destroy(

[PATCH v2 for-4.20? 5/6] radix-tree: introduce RADIX_TREE{,_INIT}()

[Jan Beulich]

... 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.

--- 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(

[PATCH v2 for-4.20? 6/6] PCI: drop pci_segments_init()

[Jan Beulich]

Have callers invoke pci_add_segment() directly instead. On x86 move the
invocation back to acpi_mmcfg_init(), where it was prior to ????????????
("x86/PCI: init segments earlier").
---
v2: New.

--- a/xen/arch/arm/pci/pci.c
+++ b/xen/arch/arm/pci/pci.c
@@ -88,7 +88,8 @@ static int __init pci_init(void)
     if ( !pci_passthrough_enabled )
         return 0;
 
-    pci_segments_init();
+    if ( pci_add_segment(0) )
+        panic("Could not initialize PCI segment 0\n");
 
     if ( acpi_disabled )
         return dt_pci_init();
--- a/xen/arch/x86/x86_64/mmconfig-shared.c
+++ b/xen/arch/x86/x86_64/mmconfig-shared.c
@@ -402,6 +402,9 @@ void __init acpi_mmcfg_init(void)
 {
     bool valid = true;
 
+    if ( pci_add_segment(0) )
+        panic("Could not initialize PCI segment 0\n");
+
     /* MMCONFIG disabled */
     if ((pci_probe & PCI_PROBE_MMCONF) == 0)
         return;
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -122,12 +122,6 @@ static int pci_segments_iterate(
     return rc;
 }
 
-void __init pci_segments_init(void)
-{
-    if ( !alloc_pseg(0) )
-        panic("Could not initialize PCI segment 0\n");
-}
-
 int __init pci_add_segment(u16 seg)
 {
     return alloc_pseg(seg) ? 0 : -ENOMEM;
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -55,8 +55,6 @@ void __init acpi_iommu_init(void)
 {
     int ret = -ENODEV;
 
-    pci_segments_init();
-
     if ( !iommu_enable && !iommu_intremap )
         return;
 
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -214,7 +214,6 @@ void setup_hwdom_pci_devices(struct doma
                              int (*handler)(uint8_t devfn,
                                             struct pci_dev *pdev));
 int pci_release_devices(struct domain *d);
-void pci_segments_init(void);
 int pci_add_segment(u16 seg);
 const unsigned long *pci_get_ro_map(u16 seg);
 int pci_add_device(u16 seg, u8 bus, u8 devfn,

Re: [PATCH v2 for-4.20 3/6] radix-tree: purge node allocation override hooks

[Jan Beulich]

On 03.02.2025 17:25, Jan Beulich wrote:
> These were needed by TMEM only, which is long gone. The Linux original
> doesn't have such either. This effectively reverts one of the "Other
> changes" from 8dc6738dbb3c ("Update radix-tree.[ch] from upstream Linux
> to gain RCU awareness").
> 
> Positive side effect: Two cf_check go away.
> 
> While there also convert xmalloc()+memset() to xzalloc(). (Don't convert
> to xvzalloc(), as that would require touching the freeing side, too.)
> 
> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I'm sorry, the question mark after for-4.20 was somehow lost in the
submission.

Jan

Re: [PATCH v2 for-4.20 3/6] radix-tree: purge node allocation override hooks

[Andrew Cooper]

On 03/02/2025 4:25 pm, Jan Beulich wrote:
> These were needed by TMEM only, which is long gone. The Linux original
> doesn't have such either. This effectively reverts one of the "Other
> changes" from 8dc6738dbb3c ("Update radix-tree.[ch] from upstream Linux
> to gain RCU awareness").
>
> Positive side effect: Two cf_check go away.

Not only that, they can now be inlined, although you've merged them
directly.

>
> While there also convert xmalloc()+memset() to xzalloc(). (Don't convert
> to xvzalloc(), as that would require touching the freeing side, too.)
>
> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

One formatting point.

> --- a/xen/common/radix-tree.c
> +++ b/xen/common/radix-tree.c
> @@ -66,26 +60,19 @@ static void cf_check _rcu_node_free(stru
>  	xfree(rcu_node);
>  }
>  
> -static void cf_check rcu_node_free(struct radix_tree_node *node, void *arg)
> -{
> -	struct rcu_node *rcu_node = container_of(node, struct rcu_node, node);
> -	call_rcu(&rcu_node->rcu_head, _rcu_node_free);
> -}
> -
>  static struct radix_tree_node *radix_tree_node_alloc(
>  	struct radix_tree_root *root)
>  {
> -	struct radix_tree_node *ret;
> -	ret = root->node_alloc(root->node_alloc_free_arg);
> -	if (ret)
> -		memset(ret, 0, sizeof(*ret));
> -	return ret;
> +	struct rcu_node *rcu_node = xzalloc(struct rcu_node);
> +
> +	return rcu_node ? &rcu_node->node : NULL;
>  }
>  
>  static void radix_tree_node_free(
>  	struct radix_tree_root *root, struct radix_tree_node *node)
>  {
> -	root->node_free(node, root->node_alloc_free_arg);
> +	struct rcu_node *rcu_node = container_of(node, struct rcu_node, node);

Newline here.

~Andrew

> +	call_rcu(&rcu_node->rcu_head, _rcu_node_free);
>  }
>  
>  /*
>

Re: [PATCH for-4.20 0/6] AMD/IOMMU: assorted corrections

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 1007 bytes --]


On 2/3/25 5:22 PM, Jan Beulich wrote:
> The first two patches are functionally independent, and they're presented
> here merely in the order I came to notice the respective issues. At least
> patch 2 wants seriously considering for 4.20.
>
> While alternatives were considered for patch 2, it's left as it was in v1
> for now. The disposition there depends on (a) the four new patches, in
> particular what the last patch does and (b) backporting considerations
> (we probably don't want to backport any of the radix tree tidying).
>
> 1: AMD/IOMMU: drop stray MSI enabling
> 2: x86/PCI: init segments earlier

R-Acked-by: Oleksii Kurochko<oleksii.kurochko@gmail.com> for first two patches.

For others it seems like nothing serious will happen if to merge them after 4.20.

~ Oleksii

> 3: radix-tree: purge node allocation override hooks
> 4: radix-tree: drop "root" parameters from radix_tree_node_{alloc,free}()
> 5: radix-tree: introduce RADIX_TREE{,_INIT}()
> 6: PCI: drop pci_segments_init()
>
> Jan

[-- Attachment #2: Type: text/html, Size: 1615 bytes --]

Re: [PATCH v2 for-4.21 4/6] radix-tree: drop "root" parameters from radix_tree_node_{alloc,free}()

[Andrew Cooper]

On 03/02/2025 4:25 pm, Jan Beulich wrote:
> They aren't used anymore.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH v2 for-4.20? 5/6] radix-tree: introduce RADIX_TREE{,_INIT}()

[Andrew Cooper]

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

Re: [PATCH v2 for-4.20? 1/6] AMD/IOMMU: drop stray MSI enabling

[Roger Pau Monné]

On Mon, Feb 03, 2025 at 05:24:10PM +0100, Jan Beulich wrote:
> While the 2nd of the commits referenced below should have moved the call
> to amd_iommu_msi_enable() instead of adding another one, the situation
> wasn't quite right even before: It can't have done any good to enable
> MSI when no IRQ was allocated for it, yet.
> 
> The other call to amd_iommu_msi_enable(), just out of patch context,
> needs to stay there until S3 resume is re-worked. For the boot path that
> call should be unnecessary, as iommu{,_maskable}_msi_startup() will have
> done it already (by way of invoking iommu_msi_unmask()).
> 
> Fixes: 5f569f1ac50e ("AMD/IOMMU: allow enabling with IRQ not yet set up")
> Fixes: d9e49d1afe2e ("AMD/IOMMU: adjust setup of internal interrupt for x2APIC mode")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
> Tested-by: Jason Andryuk <jason.andryuk@amd.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH for-4.20 0/6] AMD/IOMMU: assorted corrections

[Andrew Cooper]

On 03/02/2025 4:38 pm, Oleksii Kurochko wrote:
> On 2/3/25 5:22 PM, Jan Beulich wrote:
>> The first two patches are functionally independent, and they're presented
>> here merely in the order I came to notice the respective issues. At least
>> patch 2 wants seriously considering for 4.20.
>>
>> While alternatives were considered for patch 2, it's left as it was in v1
>> for now. The disposition there depends on (a) the four new patches, in
>> particular what the last patch does and (b) backporting considerations
>> (we probably don't want to backport any of the radix tree tidying).
>>
>> 1: AMD/IOMMU: drop stray MSI enabling
>> 2: x86/PCI: init segments earlier
> R-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> for first two patches.
> For others it seems like nothing serious will happen if to merge them after 4.20.

The reason I asked for them is because I think they're a far more robust
fix than just patch 2 in isolation.  That goes for older versions of Xen
too.

~Andrew

Re: [PATCH v2 for-4.20? 5/6] radix-tree: introduce RADIX_TREE{,_INIT}()

[Jan Beulich]

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

Re: [PATCH v2 for-4.20? 6/6] PCI: drop pci_segments_init()

[Andrew Cooper]

On 03/02/2025 4:27 pm, Jan Beulich wrote:
> Have callers invoke pci_add_segment() directly instead. On x86 move the
> invocation back to acpi_mmcfg_init(), where it was prior to ????????????
> ("x86/PCI: init segments earlier").
> ---

Need a SoB.

I definitely prefer this version of the fix, but I think it would be
better if patch 2 were merged into this.

I know it means backporting the cleanup, but it is more robust IMO.

~Andrew

Re: [PATCH v2 for-4.20? 6/6] PCI: drop pci_segments_init()

[Roger Pau Monné]

On Mon, Feb 03, 2025 at 05:27:24PM +0100, Jan Beulich wrote:
> Have callers invoke pci_add_segment() directly instead. On x86 move the
> invocation back to acpi_mmcfg_init(), where it was prior to ????????????
> ("x86/PCI: init segments earlier").
> ---
> v2: New.
> 
> --- a/xen/arch/arm/pci/pci.c
> +++ b/xen/arch/arm/pci/pci.c
> @@ -88,7 +88,8 @@ static int __init pci_init(void)
>      if ( !pci_passthrough_enabled )
>          return 0;
>  
> -    pci_segments_init();
> +    if ( pci_add_segment(0) )
> +        panic("Could not initialize PCI segment 0\n");
>  
>      if ( acpi_disabled )
>          return dt_pci_init();
> --- a/xen/arch/x86/x86_64/mmconfig-shared.c
> +++ b/xen/arch/x86/x86_64/mmconfig-shared.c
> @@ -402,6 +402,9 @@ void __init acpi_mmcfg_init(void)
>  {
>      bool valid = true;
>  
> +    if ( pci_add_segment(0) )
> +        panic("Could not initialize PCI segment 0\n");

Do you still need the pci_add_segment() call here?

pci_add_device() will already add the segments as required, and
acpi_parse_mcfg() does also add the segments described in the MCFG.

Thanks, Roger.

Re: [PATCH for-4.20 0/6] AMD/IOMMU: assorted corrections

[Jan Beulich]

On 03.02.2025 17:38, Oleksii Kurochko wrote:
> 
> On 2/3/25 5:22 PM, Jan Beulich wrote:
>> The first two patches are functionally independent, and they're presented
>> here merely in the order I came to notice the respective issues. At least
>> patch 2 wants seriously considering for 4.20.
>>
>> While alternatives were considered for patch 2, it's left as it was in v1
>> for now. The disposition there depends on (a) the four new patches, in
>> particular what the last patch does and (b) backporting considerations
>> (we probably don't want to backport any of the radix tree tidying).
>>
>> 1: AMD/IOMMU: drop stray MSI enabling
>> 2: x86/PCI: init segments earlier
> 
> R-Acked-by: Oleksii Kurochko<oleksii.kurochko@gmail.com> for first two patches.
> 
> For others it seems like nothing serious will happen if to merge them after 4.20.

It took me some time to actually take two and two together, but: With the
observation underlying patch 6, patch 2 can actually be dropped altogether,
with what is now patch 5 taking the role of the bug fix. That'll make what
is now patch 3 a strict prereq then, though. I'll cut a shrunk down v3.

Jan

Re: [PATCH v2 for-4.20? 6/6] PCI: drop pci_segments_init()

[Jan Beulich]

On 03.02.2025 18:04, Andrew Cooper wrote:
> On 03/02/2025 4:27 pm, Jan Beulich wrote:
>> Have callers invoke pci_add_segment() directly instead. On x86 move the
>> invocation back to acpi_mmcfg_init(), where it was prior to ????????????
>> ("x86/PCI: init segments earlier").
>> ---
> 
> Need a SoB.

Oops.

> I definitely prefer this version of the fix, but I think it would be
> better if patch 2 were merged into this.

See the reply on the cover letter sub-thread. I'll be dropping patch 2
altogether, with patch 5 (perhaps moved ahead of patch 4) then being
the actual bugfix.

Jan

Re: [PATCH v2 for-4.20? 6/6] PCI: drop pci_segments_init()

[Jan Beulich]

On 03.02.2025 18:18, Roger Pau Monné wrote:
> On Mon, Feb 03, 2025 at 05:27:24PM +0100, Jan Beulich wrote:
>> --- a/xen/arch/x86/x86_64/mmconfig-shared.c
>> +++ b/xen/arch/x86/x86_64/mmconfig-shared.c
>> @@ -402,6 +402,9 @@ void __init acpi_mmcfg_init(void)
>>  {
>>      bool valid = true;
>>  
>> +    if ( pci_add_segment(0) )
>> +        panic("Could not initialize PCI segment 0\n");
> 
> Do you still need the pci_add_segment() call here?
> 
> pci_add_device() will already add the segments as required, and
> acpi_parse_mcfg() does also add the segments described in the MCFG.

Well, in principle you're right. It's more the action in case of
failure that makes me want to keep it: We won't fare very well on
about every system if we can't register segment 0.

Jan

Re: [PATCH v2 for-4.20? 6/6] PCI: drop pci_segments_init()

[Jan Beulich]

On 04.02.2025 08:45, Jan Beulich wrote:
> On 03.02.2025 18:18, Roger Pau Monné wrote:
>> On Mon, Feb 03, 2025 at 05:27:24PM +0100, Jan Beulich wrote:
>>> --- a/xen/arch/x86/x86_64/mmconfig-shared.c
>>> +++ b/xen/arch/x86/x86_64/mmconfig-shared.c
>>> @@ -402,6 +402,9 @@ void __init acpi_mmcfg_init(void)
>>>  {
>>>      bool valid = true;
>>>  
>>> +    if ( pci_add_segment(0) )
>>> +        panic("Could not initialize PCI segment 0\n");
>>
>> Do you still need the pci_add_segment() call here?
>>
>> pci_add_device() will already add the segments as required, and
>> acpi_parse_mcfg() does also add the segments described in the MCFG.
> 
> Well, in principle you're right. It's more the action in case of
> failure that makes me want to keep it: We won't fare very well on
> about every system if we can't register segment 0.

Thinking about it: Your question may be more applicable on Arm, as I'm
entirely uncertain whether there segment 0 is always going to be needed.
I could well imagine system designers deliberately putting all the
devices elsewhere. Segment 0 always being in use on x86 is more a
heritage thing, after all.

Jan

Re: [PATCH v2 for-4.20? 5/6] radix-tree: introduce RADIX_TREE{,_INIT}()

[Jan Beulich]

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

Re: [PATCH v2 for-4.20? 5/6] radix-tree: introduce RADIX_TREE{,_INIT}()

[Jan Beulich]

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

Re: [PATCH v2 for-4.20? 6/6] PCI: drop pci_segments_init()

[Roger Pau Monné]

On Tue, Feb 04, 2025 at 08:51:10AM +0100, Jan Beulich wrote:
> On 04.02.2025 08:45, Jan Beulich wrote:
> > On 03.02.2025 18:18, Roger Pau Monné wrote:
> >> On Mon, Feb 03, 2025 at 05:27:24PM +0100, Jan Beulich wrote:
> >>> --- a/xen/arch/x86/x86_64/mmconfig-shared.c
> >>> +++ b/xen/arch/x86/x86_64/mmconfig-shared.c
> >>> @@ -402,6 +402,9 @@ void __init acpi_mmcfg_init(void)
> >>>  {
> >>>      bool valid = true;
> >>>  
> >>> +    if ( pci_add_segment(0) )
> >>> +        panic("Could not initialize PCI segment 0\n");
> >>
> >> Do you still need the pci_add_segment() call here?
> >>
> >> pci_add_device() will already add the segments as required, and
> >> acpi_parse_mcfg() does also add the segments described in the MCFG.
> > 
> > Well, in principle you're right. It's more the action in case of
> > failure that makes me want to keep it: We won't fare very well on
> > about every system if we can't register segment 0.

pci_add_segment() should only fail due to being out of memory, and I'm
quite sure if pci_add_segment() was to fail here due to out of memory
issues Xen won't be able to complete booting anyway.

Note the usage of "should only fail", as it's possible for
radix_tree_insert() to return -EEXIST, but that shouldn't be possible
given alloc_pseg() checks whether the segment is already added.

> Thinking about it: Your question may be more applicable on Arm, as I'm
> entirely uncertain whether there segment 0 is always going to be needed.
> I could well imagine system designers deliberately putting all the
> devices elsewhere. Segment 0 always being in use on x86 is more a
> heritage thing, after all.

I guess I would leave that one to the Arm maintainers, but from my
knowledge I got the impression is fairly common for Arm to have
multiple segments, not sure whether it's also common to start at
segment 0.

I'm not strongly opposed to leaving the pci_add_segment(0) call on
x86, but I would recommend prepending a small comment to note adding
the segment is not strictly required; it's just done for better error
reporting, as other callers that add PCI segments might silently
ignore the failure to add segment 0.

An unrelated question: looking at MCFG handling I've noticed that
calling PHYSDEVOP_pci_mmcfg_reserved doesn't seem to result in the
segment being added.  Is it on purpose that pci_mmcfg_reserved()
doesn't attempt to allocate the hardware domain discovered segment?

Thanks, Roger.

Re: [PATCH v2 for-4.20? 6/6] PCI: drop pci_segments_init()

[Jan Beulich]

On 04.02.2025 09:56, Roger Pau Monné wrote:
> On Tue, Feb 04, 2025 at 08:51:10AM +0100, Jan Beulich wrote:
>> On 04.02.2025 08:45, Jan Beulich wrote:
>>> On 03.02.2025 18:18, Roger Pau Monné wrote:
>>>> On Mon, Feb 03, 2025 at 05:27:24PM +0100, Jan Beulich wrote:
>>>>> --- a/xen/arch/x86/x86_64/mmconfig-shared.c
>>>>> +++ b/xen/arch/x86/x86_64/mmconfig-shared.c
>>>>> @@ -402,6 +402,9 @@ void __init acpi_mmcfg_init(void)
>>>>>  {
>>>>>      bool valid = true;
>>>>>  
>>>>> +    if ( pci_add_segment(0) )
>>>>> +        panic("Could not initialize PCI segment 0\n");
>>>>
>>>> Do you still need the pci_add_segment() call here?
>>>>
>>>> pci_add_device() will already add the segments as required, and
>>>> acpi_parse_mcfg() does also add the segments described in the MCFG.
>>>
>>> Well, in principle you're right. It's more the action in case of
>>> failure that makes me want to keep it: We won't fare very well on
>>> about every system if we can't register segment 0.
> 
> pci_add_segment() should only fail due to being out of memory, and I'm
> quite sure if pci_add_segment() was to fail here due to out of memory
> issues Xen won't be able to complete booting anyway.
> 
> Note the usage of "should only fail", as it's possible for
> radix_tree_insert() to return -EEXIST, but that shouldn't be possible
> given alloc_pseg() checks whether the segment is already added.

Let's continue this on v3, where I'm extending remarks on this change.
An option is to simply leave out this patch altogether. Then a follow-
on option would be to purge the call to pci_segments_init() with an
entirely different justification (e.g. yours).

> An unrelated question: looking at MCFG handling I've noticed that
> calling PHYSDEVOP_pci_mmcfg_reserved doesn't seem to result in the
> segment being added.  Is it on purpose that pci_mmcfg_reserved()
> doesn't attempt to allocate the hardware domain discovered segment?

That hypercall was added solely for the purpose of reporting resource
reservation status. While we could decide to re-purpose it to also
record the segment, that wasn't the goal so far.

Jan

Re: [PATCH v2 for-4.20? 5/6] radix-tree: introduce RADIX_TREE{,_INIT}()

[Andrew Cooper]

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