| Message ID | 20191008011501.21038-1-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fixes for make_[memory/cpu]_node | expand |
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,
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;
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,
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;
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(-)