[PATCH v3 for-4.20 0/4] AMD/IOMMU: assorted corrections (leftover)

[PATCH v3 for-4.20 0/4] AMD/IOMMU: assorted corrections (leftover)

[Jan Beulich]

What previously was the main bug fix in this series was dropped for v3. It
turns out what is now patch 2 is sufficient to address the issue, while
doing the requested tidying. The latter two patches are for 4.21 only, with
the final one being up for debate altogether.

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

Jan

[PATCH v3 for-4.20 1/4] 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>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v3: Add a missing blank line.
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,20 @@ 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 +706,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 v3 for-4.20 2/4] radix-tree: introduce RADIX_TREE{,_INIT}()

[Jan Beulich]

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

[PATCH v3 for-4.21 3/4] 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>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.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);
 
@@ -105,7 +103,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.  */
@@ -156,7 +154,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) {
@@ -575,7 +573,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);
 	}
 }
 
@@ -640,7 +638,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))
@@ -656,7 +654,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;
@@ -683,7 +681,7 @@ radix_tree_node_destroy(
 		}
 	}
 
-	radix_tree_node_free(root, node);
+	radix_tree_node_free(node);
 }
 
 void radix_tree_destroy(

[PATCH v3 for-4.21 4/4] PCI: drop pci_segments_init()

[Jan Beulich]

Have callers invoke pci_add_segment() directly instead: With radix tree
initialization moved out of the function, its name isn't quite
describing anymore what it actually does.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
This is entirely optional and up for discussion. There certainly also is
an argument towards keeping the function.
---
v3: Adjust description to accont for and re-base over dropped earlier
    patch.
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,7 +402,8 @@ void __init acpi_mmcfg_init(void)
 {
     bool valid = true;
 
-    pci_segments_init();
+    if ( pci_add_segment(0) )
+        panic("Could not initialize PCI segment 0\n");
 
     /* MMCONFIG disabled */
     if ((pci_probe & PCI_PROBE_MMCONF) == 0)
--- 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/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 v3 for-4.20 2/4] radix-tree: introduce RADIX_TREE{,_INIT}()

[Andrew Cooper]

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

Re: [PATCH v3 for-4.20 2/4] radix-tree: introduce RADIX_TREE{,_INIT}()

[Jan Beulich]

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

Re: [PATCH v3 for-4.20 2/4] radix-tree: introduce RADIX_TREE{,_INIT}()

[Andrew Cooper]

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

Re: [PATCH v3 for-4.20 2/4] radix-tree: introduce RADIX_TREE{,_INIT}()

[Jan Beulich]

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

Re: [PATCH v3 for-4.20 0/4] AMD/IOMMU: assorted corrections (leftover)

[Jan Beulich]

Oleksii,

On 04.02.2025 14:00, Jan Beulich wrote:
> What previously was the main bug fix in this series was dropped for v3. It
> turns out what is now patch 2 is sufficient to address the issue, while
> doing the requested tidying. The latter two patches are for 4.21 only, with
> the final one being up for debate altogether.
> 
> 1: radix-tree: purge node allocation override hooks
> 2: radix-tree: introduce RADIX_TREE{,_INIT}()
> 3: radix-tree: drop "root" parameters from radix_tree_node_{alloc,free}()
> 4: PCI: drop pci_segments_init()

any verdict one way or the other for the first two patches?

Thanks, Jan

Re: [PATCH v3 for-4.20 0/4] AMD/IOMMU: assorted corrections (leftover)

[Oleksii Kurochko]

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


On 2/6/25 1:35 PM, Jan Beulich wrote:
> Oleksii,
>
> On 04.02.2025 14:00, Jan Beulich wrote:
>> What previously was the main bug fix in this series was dropped for v3. It
>> turns out what is now patch 2 is sufficient to address the issue, while
>> doing the requested tidying. The latter two patches are for 4.21 only, with
>> the final one being up for debate altogether.
>>
>> 1: radix-tree: purge node allocation override hooks
>> 2: radix-tree: introduce RADIX_TREE{,_INIT}()
>> 3: radix-tree: drop "root" parameters from radix_tree_node_{alloc,free}()
>> 4: PCI: drop pci_segments_init()
> any verdict one way or the other for the first two patches?

Sorry, I missed the fixes tag so I thought that they are just refactoring.

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

~ Oleksii

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

Re: [PATCH v3 for-4.21 4/4] PCI: drop pci_segments_init()

[Roger Pau Monné]

On Tue, Feb 04, 2025 at 02:04:35PM +0100, Jan Beulich wrote:
> Have callers invoke pci_add_segment() directly instead: With radix tree
> initialization moved out of the function, its name isn't quite
> describing anymore what it actually does.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

IMO I would rather not add the segment here, and just make sure that
all callers that add segments have proper error reporting when it
fails.  Maybe alloc_pseg() should gain a printk on failure?

Thanks, Roger.

Re: [PATCH v3 for-4.21 4/4] PCI: drop pci_segments_init()

[Jan Beulich]

On 06.02.2025 16:08, Roger Pau Monné wrote:
> On Tue, Feb 04, 2025 at 02:04:35PM +0100, Jan Beulich wrote:
>> Have callers invoke pci_add_segment() directly instead: With radix tree
>> initialization moved out of the function, its name isn't quite
>> describing anymore what it actually does.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> IMO I would rather not add the segment here, and just make sure that
> all callers that add segments have proper error reporting when it
> fails.

Maybe. Yet then things may (on x86) work fine with secondary segments not
properly working. A log from one of the few multi-segment systems that I
had seen data from suggested that none of the devices on the secondary
segment were actually used by anything. This was, if I'm not mistaken,
the underlying reason why (on x86) we demand segment 0 to have proper
representation, but do things best effort only for other segments. Which
isn't to say that we can't change things and do better.

>  Maybe alloc_pseg() should gain a printk on failure?

Not sure that would buy us much, especially if (on x86) it's seg 0 which
is affected.

For the patch at hand - do you then suggest to simply drop it? The plan
here wasn't to re-work what we do, just tidy slightly how we do things.

Jan

Re: [PATCH v3 for-4.21 4/4] PCI: drop pci_segments_init()

[Roger Pau Monné]

On Mon, Feb 10, 2025 at 11:01:04AM +0100, Jan Beulich wrote:
> On 06.02.2025 16:08, Roger Pau Monné wrote:
> > On Tue, Feb 04, 2025 at 02:04:35PM +0100, Jan Beulich wrote:
> >> Have callers invoke pci_add_segment() directly instead: With radix tree
> >> initialization moved out of the function, its name isn't quite
> >> describing anymore what it actually does.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > IMO I would rather not add the segment here, and just make sure that
> > all callers that add segments have proper error reporting when it
> > fails.
> 
> Maybe. Yet then things may (on x86) work fine with secondary segments not
> properly working. A log from one of the few multi-segment systems that I
> had seen data from suggested that none of the devices on the secondary
> segment were actually used by anything. This was, if I'm not mistaken,
> the underlying reason why (on x86) we demand segment 0 to have proper
> representation, but do things best effort only for other segments. Which
> isn't to say that we can't change things and do better.
> 
> >  Maybe alloc_pseg() should gain a printk on failure?
> 
> Not sure that would buy us much, especially if (on x86) it's seg 0 which
> is affected.
> 
> For the patch at hand - do you then suggest to simply drop it? The plan
> here wasn't to re-work what we do, just tidy slightly how we do things.

I feel like acpi_mmcfg_init() is an obscure place to do this.  It
won't be strange to shuffle that call around and forgot it's also
adding segment 0.

I would prefer if such allocation of segment 0 was done in
__start_xen(), as that makes it much easier to identify dependencies
and prevent such kind of re-ordering mistakes.

Thanks, Roger.