|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 10/16] xen/riscv: imsic_init() implementation
On 5/15/25 10:42 AM, Jan Beulich wrote:
On 06.05.2025 18:51, Oleksii Kurochko wrote: Oops, missed to drop it in asm/imsic.h. Thanks. + if ( rc ) + return rc; + + nr_mmios = imsic_cfg.nr_mmios; + + /* Allocate MMIO resource array */ + imsic_cfg.mmios = xzalloc_array(struct imsic_mmios, nr_mmios); + if ( !imsic_cfg.mmios ) + return -ENOMEM; + + imsic_cfg.msi = xzalloc_array(struct imsic_msi, nr_parent_irqs); + if ( !imsic_cfg.msi ) + return -ENOMEM;Leaking the earlier successful allocation? In this cases should be:
{
rc = -ENOMEM;
goto imsic_init_err;
Agree, it should translation of cpuid to Xen CPU numbering space as cpuid could be
sparsed according to the RISC-V spec, which guarantees only that cpuid 0 will be present,
all other could be any number.
I thought about to update that with:
xen_cpuid = hartid_to_cpuid(hartid);
if ( xen_cpuid >= num_possible_cpus() )
{
printk(XENLOG_WARNING "%s: unsupported cpu ID=%lu for parent irq%u\n",
node->name, hartid, i);
continue;
}
...
/* defined in asm/smp.h */
static inline unsigned long hartid_to_cpuid(unsigned long hartid)
{
for ( unsigned int cpuid = 0; cpuid < ARRAY_SIZE(pcpu_info); cpuid++ )
{
if ( hartid == cpuid_to_hartid(cpuid) )
return cpuid;
}
return NR_CPUS;
}
(the proper name of variable 'cpuid' I think we will agree in the discussion of one of the
earlier patches.)
But then it will be an issue that if hart_id (from DT) hasn't been assigned to Xen's cpuid,
then IMSIC's msi[] and mmio[] won't be configured properly.
Probably this is not an issue and this assignment of cpuid to hartid could be done before
imsic_init() in case of CONFIG_SMP=y.
+ continue; + } + + /* Find MMIO location of MSI page */ + index = nr_mmios; + reloff = i * BIT(imsic_cfg.guest_index_bits, UL) * IMSIC_MMIO_PAGE_SZ; + for ( unsigned int j = 0; nr_mmios; j++ )DYM "j < nr_mmios"? Yes, the condition should be corrected. Interesting why I am not faced an issue with such condition, nr_mmios shouldn't be zero (I'll double check) and Linux kernel has the same condition: https://github.com/torvalds/linux/blob/master/drivers/irqchip/irq-riscv-imsic-state.c#L907C31-L908C1 It seems like LK wants to have a fix too.
There is no need. I will drop 'j'. + node->name, imsic_cfg.msi[cpuid].base_addr + reloff); + imsic_cfg.msi[cpuid].offset = 0; + imsic_cfg.msi[cpuid].base_addr = 0; + continue; + } + + bitmap_set(imsic_cfg.mmios[index].cpus, cpuid, 1);Depending on clarification on the number space used, this may want to be cpumask_set_cpu() instead. Else I think this is simply __set_bit(). Considering that cpuid is taken from DT and the value in device tree is what we are using as hartid and hartid could be according any number from 0 to sizeof(unsigned long) then we can't use cpuid for msi[] as overflow could happen, it seems like we should use an id from Xen space. (so basically xen_cpuid which I mentioned above) + imsic_cfg.msi[cpuid].base_addr = imsic_cfg.mmios[index].base_addr; + imsic_cfg.msi[cpuid].offset = reloff;How come it's cpuid that's used as array index here? Shouldn't this be i, seeing that the array allocation is using nr_parent_irqs? Agree, something wrong is here. As I mentioned above, I think, Xen cpu space should be used here. + XFREE(imsic_cfg.msi); + + return rc; +} --- /dev/null +++ b/xen/arch/riscv/include/asm/imsic.h @@ -0,0 +1,65 @@ +/* SPDX-License-Identifier: MIT */ + +/* + * xen/arch/riscv/imsic.h + * + * RISC-V Incoming MSI Controller support + * + * (c) 2023 Microchip Technology Inc. + */ + +#ifndef ASM__RISCV__IMSIC_H +#define ASM__RISCV__IMSIC_H + +#include <xen/types.h> + +#define IMSIC_MMIO_PAGE_SHIFT 12 +#define IMSIC_MMIO_PAGE_SZ (1UL << IMSIC_MMIO_PAGE_SHIFT) + +#define IMSIC_MIN_ID 63This isn't the "minimum ID", but the "minimum permissible number of IDs" as per its first use in imsic_parse_node(). Further uses there consider it a mask, which makes me wonder whether the imsic_cfg.nr_ids field name is actually correct: Isn't this the maximum valid ID rather than their count/number? imsic_cfg.nr_ids it is a value of interrupt identities supported by IMSIC interrupt file according to
DT binding:
riscv,num-ids:
$ref: /schemas/types.yaml#/definitions/uint32
minimum: 63
maximum: 2047
description:
Number of interrupt identities supported by IMSIC interrupt file.
>From some point of view it could be called as maximum valid ID specifically for a platform, but the number
of ids looks better to me.
Thanks.
~ Oleksii
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |