[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 08/14] xen/riscv: imsic_init() implementation
On 04.06.2025 15:42, Oleksii Kurochko wrote: > > On 6/2/25 12:22 PM, Jan Beulich wrote: >> On 27.05.2025 13:30, Oleksii Kurochko wrote: >>> On 5/26/25 8:44 PM, Oleksii Kurochko wrote: >>>>>> + if ( !dt_property_read_u32(node, "riscv,guest-index-bits", >>>>>> + &imsic_cfg.guest_index_bits) ) >>>>>> + imsic_cfg.guest_index_bits = 0; >>>>>> + tmp = BITS_PER_LONG - IMSIC_MMIO_PAGE_SHIFT; >>>>>> + if ( tmp < imsic_cfg.guest_index_bits ) >>>>>> + { >>>>>> + printk(XENLOG_ERR "%s: guest index bits too big\n", >>>>>> + dt_node_name(node)); >>>>>> + rc = -ENOENT; >>>>>> + goto cleanup; >>>>>> + } >>>>>> + >>>>>> + /* Find number of HART index bits */ >>>>>> + if ( !dt_property_read_u32(node, "riscv,hart-index-bits", >>>>>> + &imsic_cfg.hart_index_bits) ) >>>>>> + { >>>>>> + /* Assume default value */ >>>>>> + imsic_cfg.hart_index_bits = fls(*nr_parent_irqs); >>>>>> + if ( BIT(imsic_cfg.hart_index_bits, UL) < *nr_parent_irqs ) >>>>>> + imsic_cfg.hart_index_bits++; >>>>> Since fls() returns a 1-based bit number, isn't it rather that in the >>>>> exact-power-of-2 case you'd need to subtract 1? >>>> Agree, in this case, -1 should be taken into account. >>> Hmm, it seems like in case of fls() returns a 1-based bit number there >>> is not need for the check: >>> (2) if ( BIT(imsic_cfg.hart_index_bits, UL) < *nr_parent_irqs ) >>> >>> We could do imsic_cfg.hart_index_bits = fls(*nr_parent_irqs - 1) (1) without >>> checking *nr_parent_irqs is power-of-two or not, and then just leave the >>> check (2). >>> And with (1), the check (2) is only needed for the case *nr_parent_irqs=1, >>> if >>> I amn't mistaken something. And if I'm not mistaken, then probably it make >>> sense to change (2) to if ( *nr_parent_irqs == 1 ) + some comment why this >>> case is so special. >>> >>> Does it make sense? >> Can't easily tell; I'd like to see the resulting code instead of the textual >> description. > > Here is the code: > /* Find number of HART index bits */ > if ( !dt_property_read_u32(node, "riscv,hart-index-bits", > &imsic_cfg.hart_index_bits) ) > { > /* Assume default value */ > imsic_cfg.hart_index_bits = fls(*nr_parent_irqs - 1) + > (*nr_parent_irqs == 1); > } > > It seems like it covers all the cases. *nr_parent_irqs imsic_cfg.hart_index_bits 1 0 2 2 (1 + 1) 3 2 4 2 5 3 6 3 IOW why the special casing of *nr_parent_irqs == 1? Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |