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

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

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

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