[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 08/14] xen/riscv: imsic_init() implementation
On 6/5/25 8:50 AM, Jan Beulich wrote:
On 04.06.2025 17:36, Oleksii Kurochko wrote:On 6/2/25 12:21 PM, Jan Beulich wrote:On 26.05.2025 20:44, Oleksii Kurochko wrote:On 5/22/25 4:46 PM, Jan Beulich wrote:On 21.05.2025 18:03, Oleksii Kurochko wrote:+ /* Check MMIO register sets */ + for ( unsigned int i = 0; i < nr_mmios; i++ ) + { + if ( !alloc_cpumask_var(&imsic_cfg.mmios[i].cpus) ) + { + rc = -ENOMEM; + goto imsic_init_err; + } + + rc = dt_device_get_address(node, i, &imsic_cfg.mmios[i].base_addr, + &imsic_cfg.mmios[i].size); + if ( rc ) + { + printk(XENLOG_ERR "%s: unable to parse MMIO regset %u\n", + node->name, i); + goto imsic_init_err; + } + + base_addr = imsic_cfg.mmios[i].base_addr; + base_addr &= ~(BIT(imsic_cfg.guest_index_bits + + imsic_cfg.hart_index_bits + + IMSIC_MMIO_PAGE_SHIFT, UL) - 1); + base_addr &= ~((BIT(imsic_cfg.group_index_bits, UL) - 1) << + imsic_cfg.group_index_shift); + if ( base_addr != imsic_cfg.base_addr ) + { + rc = -EINVAL; + printk(XENLOG_ERR "%s: address mismatch for regset %u\n", + node->name, i); + goto imsic_init_err; + }Maybe just for my own understanding: There's no similar check for the sizes to match / be consistent wanted / needed?If you are speaking about imsic_cfg.mmios[i].size then it depends fully on h/w will provide, IMO. So I don't what is possible range for imsic_cfg.mmios[i].size.Well, all I can say is that's it feels odd that you sanity check base_addr but permit effectively any size.Okay, I think I have two ideas how to check the size: 1. Based on guest bits from IMSIC's DT node. QEMU calculates a size as: for (socket = 0; socket < socket_count; socket++) { imsic_addr = base_addr + socket * VIRT_IMSIC_GROUP_MAX_SIZE; imsic_size = IMSIC_HART_SIZE(imsic_guest_bits) * s->soc[socket].num_harts; ... where: #define IMSIC_MMIO_PAGE_SHIFT 12 #define IMSIC_MMIO_PAGE_SZ (1UL << IMSIC_MMIO_PAGE_SHIFT) #define IMSIC_HART_NUM_GUESTS(__guest_bits) \ (1U << (__guest_bits)) #define IMSIC_HART_SIZE(__guest_bits) \ (IMSIC_HART_NUM_GUESTS(__guest_bits) * IMSIC_MMIO_PAGE_SZ) 2. Just take a theoretical maximum for S-mode IMSIC's node: 16,384 * 64 1(S-mode interrupt file) + 63(max guest interrupt files)) * 4 KiB Where, 16,384 - maximum possible amount of harts according to AIA spec 64 - a maximum amount of possible interrupt file for S-mode IMSIC node: 1 - S interupt file + 63 guest interrupt files. 4 Kib - a maximum size of one interrupt file. Which option is preferred?I would have said 2, if your outline used "actual" rather than "maximum" values. In option 2 maximum possible values are used. If we want to use "actual" values then the option 1 should be used. At the moment, I did in the following way that S-mode IMSIC node, at least, should contain 1 S-mode interrupt file + an amount of guest interrupts files (based on imsic_cfg.guest_index_bits): +#define IMSIC_HART_SIZE(guest_bits_) (BIT(guest_bits_, U) * IMSIC_MMIO_PAGE_SZ) + #define IMSIC_DISABLE_EIDELIVERY 0 #define IMSIC_ENABLE_EIDELIVERY 1 #define IMSIC_DISABLE_EITHRESHOLD 1 @@ -359,6 +356,10 @@ int __init imsic_init(const struct dt_device_node *node) /* Check MMIO register sets */ for ( unsigned int i = 0; i < nr_mmios; i++ ) { + unsigned int guest_bits = imsic_cfg.guest_index_bits; + unsigned long expected_size = + IMSIC_HART_SIZE(guest_bits) * nr_parent_irqs; + rc = dt_device_get_address(node, i, &mmios[i].base_addr, &mmios[i].size); if ( rc ) @@ -369,7 +370,7 @@ int __init imsic_init(const struct dt_device_node *node) } base_addr = mmios[i].base_addr; - base_addr &= ~(BIT(imsic_cfg.guest_index_bits + + base_addr &= ~(BIT(guest_bits + imsic_cfg.hart_index_bits + IMSIC_MMIO_PAGE_SHIFT, UL) - 1); base_addr &= ~((BIT(imsic_cfg.group_index_bits, UL) - 1) << @@ -381,6 +382,14 @@ int __init imsic_init(const struct dt_device_node *node) node->name, i); goto imsic_init_err; } + + if ( mmios[i].size != expected_size ) + { + rc = -EINVAL; + printk(XENLOG_ERR "%s: IMSIC MMIO size is incorrect %ld, " + "max:%ld\n", node->name, mmios[i].size, max_size); + goto imsic_init_err; + } } The specification doesn’t seem to mention (or I couldn’t find) that all platforms must calculate the MMIO size in the same way QEMU does. Therefore, it’s probably better to use the approach described in option 2. On the other hand, I don't think a platform should be considered correct if it provides slightly more than needed but still less than the theoretical maximum.@@ -18,6 +19,18 @@ static inline unsigned long cpuid_to_hartid(unsigned long cpuid) return pcpu_info[cpuid].hart_id; } +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; + } + + /* hartid isn't valid for some reason */ + return NR_CPUS; +}Considering the values being returned, why's the function's return type "unsigned long"?mhartid register has MXLEN length, so theoretically we could have from 0 to MXLEN-1 Harts and so we could have the same amount of Xen's CPUIDs. and MXLEN is 32 for RV32 and MXLEN is 64 for RV64.Yet the return value here is always constrained by NR_CPUS, isn't it?I am not 100% sure that I get your point,NR_CPUS is guaranteed to fit in a unsigned int. Furthermore variables / parameters holding Xen-internal CPU numbers also are unsigned int everywhere else. Okay, then agree, hartid_to_cpuid() should return unsigned int. I'll update correspondingly. Thanks. ~ Oleksii but I put NR_CPUS here because of: /* * tp points to one of these per cpu. * * hart_id would be valid (no matter which value) if its * processor_id field is valid (less than NR_CPUS). */ struct pcpu_info pcpu_info[NR_CPUS] = { [0 ... NR_CPUS - 1] = { .processor_id = NR_CPUS, }}; As an option we could use ULONG_MAX. Would it be better?No. NR_CPUS is the appropriate value to use here, imo. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |