[Xen-devel] [PATCH for-4.13 v2 0/3] fixes for make_[memory/cpu]_node

[Xen-devel] [PATCH for-4.13 v2 0/3] fixes for make_[memory/cpu]_node

[Stefano Stabellini]

Hi all,

This is a small collection of fixes for make_memory_node and
make_cpus_node for 4.13.

Cheers,

Stefano


Stefano Stabellini (3):
      xen/arm: fix buf size in make_cpus_node
      xen/arm: make_memory_node return error on nr_banks == 0
      xen/arm: fix duplicate memory node in DT

 xen/arch/arm/domain_build.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 1/3] xen/arm: fix buf size in make_cpus_node

[Stefano Stabellini]

The size of buf is calculated wrongly: the number is 64bit, not 32bit.
Also the number is printed as a hexadecimal number, so we need 8 bytes
for 32bit, not 10 bytes.

As a result, it should be sizeof("cpu@") + 16 bytes for a 64-bit number
+ 1 byte for \0. Total = 21.

Fixes: fafd682c3e (xen/arm: Create a fake cpus node in dom0 device tree)
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
Changes in v2:
- patch added
---
 xen/arch/arm/domain_build.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 921b054520..60923a7051 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -788,8 +788,8 @@ static int __init make_cpus_node(const struct domain *d, void *fdt)
     unsigned int cpu;
     const void *compatible = NULL;
     u32 len;
-    /* Placeholder for cpu@ + a 32-bit number + \0 */
-    char buf[15];
+    /* Placeholder for cpu@ + a 64-bit number + \0 */
+    char buf[21];
     u32 clock_frequency;
     bool clock_valid;
     uint64_t mpidr_aff;
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 2/3] xen/arm: make_memory_node return error on nr_banks == 0

[Stefano Stabellini]

Call make_memory_node for reserved_memory only if we actually have any
reserved_memory regions to handle.

Add a check in make_memory_node to return an error if it has been called
with no memory banks as argument.

Fixes: 248faa637d2 (xen/arm: add reserved-memory regions to the dom0 memory node)
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
Changes in v2:
- patch added
---
 xen/arch/arm/domain_build.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 60923a7051..ea01aada0b 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -650,6 +650,8 @@ static int __init make_memory_node(const struct domain *d,
     __be32 *cells;
 
     BUG_ON(nr_cells >= ARRAY_SIZE(reg));
+    if ( mem->nr_banks == 0 )
+        return -ENOENT;
 
     dt_dprintk("Create memory node (reg size %d, nr cells %d)\n",
                reg_size, nr_cells);
@@ -1540,10 +1542,13 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
          * Create a second memory node to store the ranges covering
          * reserved-memory regions.
          */
-        res = make_memory_node(d, kinfo->fdt, addrcells, sizecells,
-                               &bootinfo.reserved_mem);
-        if ( res )
-            return res;
+        if ( bootinfo.reserved_mem.nr_banks > 0 )
+        {
+            res = make_memory_node(d, kinfo->fdt, addrcells, sizecells,
+                                   &bootinfo.reserved_mem);
+            if ( res )
+                return res;
+        }
     }
 
     res = fdt_end_node(kinfo->fdt);
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 3/3] xen/arm: fix duplicate memory node in DT

[Stefano Stabellini]

When reserved-memory regions are present in the host device tree, dom0
is started with multiple memory nodes. Each memory node should have a
unique name, but today they are all called "memory" leading to Linux
printing the following warning at boot:

  OF: Duplicate name in base, renamed to "memory#1"

This patch fixes the problem by appending a "@<unit-address>" to the
name, as per the Device Tree specification, where <unit-address> matches
the base of address of the first region.

Fixes: 248faa637d2 (xen/arm: add reserved-memory regions to the dom0 memory node)
Reported-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
Changes in v2:
- fix buf size calculation: the number is 64bit and printed as
  hexadecimal
- move check on nr_banks to a separate patch
---
 xen/arch/arm/domain_build.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index ea01aada0b..3de4dafaed 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -646,6 +646,8 @@ static int __init make_memory_node(const struct domain *d,
     int res, i;
     int reg_size = addrcells + sizecells;
     int nr_cells = reg_size * mem->nr_banks;
+    /* Placeholder for memory@ + a 64-bit number + \0 */
+    char buf[24];
     __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */];
     __be32 *cells;
 
@@ -657,7 +659,8 @@ static int __init make_memory_node(const struct domain *d,
                reg_size, nr_cells);
 
     /* ePAPR 3.4 */
-    res = fdt_begin_node(fdt, "memory");
+    snprintf(buf, sizeof(buf), "memory@%"PRIx64, mem->bank[0].start);
+    res = fdt_begin_node(fdt, buf);
     if ( res )
         return res;
 
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.13 v2 0/3] fixes for make_[memory/cpu]_node

[Jürgen Groß]

On 08.10.19 03:14, Stefano Stabellini wrote:
> Hi all,
> 
> This is a small collection of fixes for make_memory_node and
> make_cpus_node for 4.13.
> 
> Cheers,
> 
> Stefano
> 
> 
> Stefano Stabellini (3):
>        xen/arm: fix buf size in make_cpus_node
>        xen/arm: make_memory_node return error on nr_banks == 0
>        xen/arm: fix duplicate memory node in DT
> 
>   xen/arch/arm/domain_build.c | 22 +++++++++++++++-------
>   1 file changed, 15 insertions(+), 7 deletions(-)
> 

For the series:

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/3] xen/arm: fix buf size in make_cpus_node

[Julien Grall]

Hi Stefano,

On 10/8/19 2:14 AM, Stefano Stabellini wrote:
> The size of buf is calculated wrongly: the number is 64bit, not 32bit.

While the variable mpdir_aff is 64-bit, we only write the first 32-bit 
in the property reg (#address-cells == 1 and fdt_property_cell()). So 
what needs to be modified is the format here.

Also, looking the CPU bindings (see 
linux/Documentation/devicetree/bindings/arm/cpus.yaml), technically only 
the bits [23:0] of the mpidr should be used. The rest is zeroed.

This is ok because vcpuid_to_vaffinity() is always returning a value 
following the requirements above. However, for correctness, this may 
want to be fixed.

> Also the number is printed as a hexadecimal number, so we need 8 bytes
> for 32bit, not 10 bytes.
> 
> As a result, it should be sizeof("cpu@") + 16 bytes for a 64-bit number
> + 1 byte for \0. Total = 21.
> 
> Fixes: fafd682c3e (xen/arm: Create a fake cpus node in dom0 device tree)

I am afraid this is not fixing this patch:

snprintf(buf, sizeof(buf), "cpu@%u", cpu);

So the 10 bytes were actually correct back then.

The problem was introduced by commit c81a791d34 "xen/arm: Set 'reg' of 
cpu node for dom0 to match MPIDR's affinity".

> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> ---
> Changes in v2:
> - patch added
> ---
>   xen/arch/arm/domain_build.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 921b054520..60923a7051 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -788,8 +788,8 @@ static int __init make_cpus_node(const struct domain *d, void *fdt)
>       unsigned int cpu;
>       const void *compatible = NULL;
>       u32 len;
> -    /* Placeholder for cpu@ + a 32-bit number + \0 */
> -    char buf[15];
> +    /* Placeholder for cpu@ + a 64-bit number + \0 */
> +    char buf[21];
>       u32 clock_frequency;
>       bool clock_valid;
>       uint64_t mpidr_aff;
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] xen/arm: make_memory_node return error on nr_banks == 0

[Julien Grall]

On 10/8/19 2:15 AM, Stefano Stabellini wrote:
> Call make_memory_node for reserved_memory only if we actually have any
> reserved_memory regions to handle.
> 
> Add a check in make_memory_node to return an error if it has been called
> with no memory banks as argument.
> 
> Fixes: 248faa637d2 (xen/arm: add reserved-memory regions to the dom0 memory node)
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 3/3] xen/arm: fix duplicate memory node in DT

[Julien Grall]

Hi,

On 10/8/19 2:15 AM, Stefano Stabellini wrote:
> When reserved-memory regions are present in the host device tree, dom0
> is started with multiple memory nodes. Each memory node should have a
> unique name, but today they are all called "memory" leading to Linux
> printing the following warning at boot:
> 
>    OF: Duplicate name in base, renamed to "memory#1"
> 
> This patch fixes the problem by appending a "@<unit-address>" to the
> name, as per the Device Tree specification, where <unit-address> matches
> the base of address of the first region.
> 
> Fixes: 248faa637d2 (xen/arm: add reserved-memory regions to the dom0 memory node)
> Reported-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

> ---
> Changes in v2:
> - fix buf size calculation: the number is 64bit and printed as
>    hexadecimal
> - move check on nr_banks to a separate patch
> ---
>   xen/arch/arm/domain_build.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index ea01aada0b..3de4dafaed 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -646,6 +646,8 @@ static int __init make_memory_node(const struct domain *d,
>       int res, i;
>       int reg_size = addrcells + sizecells;
>       int nr_cells = reg_size * mem->nr_banks;
> +    /* Placeholder for memory@ + a 64-bit number + \0 */
> +    char buf[24];
>       __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */];
>       __be32 *cells;
>   
> @@ -657,7 +659,8 @@ static int __init make_memory_node(const struct domain *d,
>                  reg_size, nr_cells);
>   
>       /* ePAPR 3.4 */
> -    res = fdt_begin_node(fdt, "memory");
> +    snprintf(buf, sizeof(buf), "memory@%"PRIx64, mem->bank[0].start);
> +    res = fdt_begin_node(fdt, buf);
>       if ( res )
>           return res;
>   
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/3] xen/arm: fix buf size in make_cpus_node

[Stefano Stabellini]

On Tue, 8 Oct 2019, Julien Grall wrote:
> On 10/8/19 2:14 AM, Stefano Stabellini wrote:
> > The size of buf is calculated wrongly: the number is 64bit, not 32bit.
> 
> While the variable mpdir_aff is 64-bit, we only write the first 32-bit in the
> property reg (#address-cells == 1 and fdt_property_cell()). So what needs to
> be modified is the format here.
> 
> Also, looking the CPU bindings (see
> linux/Documentation/devicetree/bindings/arm/cpus.yaml), technically only the
> bits [23:0] of the mpidr should be used. The rest is zeroed.
> 
> This is ok because vcpuid_to_vaffinity() is always returning a value following
> the requirements above. However, for correctness, this may want to be fixed.

It looks like it would be best to change mpdir_aff to uint32_t and
change vcpuid_to_vaffinity to return a uint32_t.

Then of course the buf allocation would be buf[13].

Is that what you have in mind?


> > Also the number is printed as a hexadecimal number, so we need 8 bytes
> > for 32bit, not 10 bytes.
> > 
> > As a result, it should be sizeof("cpu@") + 16 bytes for a 64-bit number
> > + 1 byte for \0. Total = 21.
> > 
> > Fixes: fafd682c3e (xen/arm: Create a fake cpus node in dom0 device tree)
> 
> I am afraid this is not fixing this patch:
> 
> snprintf(buf, sizeof(buf), "cpu@%u", cpu);
> 
> So the 10 bytes were actually correct back then.
> 
> The problem was introduced by commit c81a791d34 "xen/arm: Set 'reg' of cpu
> node for dom0 to match MPIDR's affinity".

Yes, I'll change it


> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > ---
> > Changes in v2:
> > - patch added
> > ---
> >   xen/arch/arm/domain_build.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 921b054520..60923a7051 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -788,8 +788,8 @@ static int __init make_cpus_node(const struct domain *d,
> > void *fdt)
> >       unsigned int cpu;
> >       const void *compatible = NULL;
> >       u32 len;
> > -    /* Placeholder for cpu@ + a 32-bit number + \0 */
> > -    char buf[15];
> > +    /* Placeholder for cpu@ + a 64-bit number + \0 */
> > +    char buf[21];
> >       u32 clock_frequency;
> >       bool clock_valid;
> >       uint64_t mpidr_aff;

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/3] xen/arm: fix buf size in make_cpus_node

[Julien Grall]

Hi Stefano,

On 08/10/2019 22:18, Stefano Stabellini wrote:
> On Tue, 8 Oct 2019, Julien Grall wrote:
>> On 10/8/19 2:14 AM, Stefano Stabellini wrote:
>>> The size of buf is calculated wrongly: the number is 64bit, not 32bit.
>>
>> While the variable mpdir_aff is 64-bit, we only write the first 32-bit in the
>> property reg (#address-cells == 1 and fdt_property_cell()). So what needs to
>> be modified is the format here.
>>
>> Also, looking the CPU bindings (see
>> linux/Documentation/devicetree/bindings/arm/cpus.yaml), technically only the
>> bits [23:0] of the mpidr should be used. The rest is zeroed.
>>
>> This is ok because vcpuid_to_vaffinity() is always returning a value following
>> the requirements above. However, for correctness, this may want to be fixed.
> 
> It looks like it would be best to change mpdir_aff to uint32_t and
> change vcpuid_to_vaffinity to return a uint32_t.

vcpuid_to_vaffinity is meant to return the AFFx bits of the MIDR (so 
32-bit on Arm32 and 64-bit on Arm64). We are only using AFF0 and AFF1, 
so the rest is zeroed. But this does not mean we should switch to 32-bit.

If we want to change the interface then, it should be register_t and not 
32-bit.

However, I didn't suggest to switch to 32-bit but to transfer the bits 
[23:0] to a variable and possibly check that the rest is 0.

Maybe something like:

uint32_t reg;

reg = mpidr_aff & GENMASK(23, 0);
/* We only are able to deal with AFF{0, 1, 2} stored in bits [23:0] at 
the moment */
if ( reg != mpidr_aff )
{
   printk(XENLOG_ERR "Unable to handle MPIDR AFFINITY 0x%"PRIx64"\n", 
mpidr_aff);
   return -EINVAL;
}

Cheers,

-- 
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel