Re: [PATCH v1 33/43] hw/intc: Add LoongArch ls7a interrupt controller support(PCH-PIC)

[yangxiaojuan <yangxiaojuan@loongson.cn>]

[-- Attachment #1: Type: text/plain, Size: 1320 bytes --]

Hi, Richard

On 2022/4/18 上午11:15, Richard Henderson wrote:
> On 4/15/22 02:40, Xiaojuan Yang wrote:
>> +static void pch_pic_update_irq(LoongArchPCHPIC *s, uint32_t mask,
>> +                               int level, int hi)
>> +{
>> +    uint32_t val, irq;
>> +
>> +    if (level == 1) {
>> +        if (hi) {
>> +            val = mask & s->intirr_hi & (~s->int_mask_hi);
>> +            irq = find_first_bit((unsigned long *)&val, 32);
>
> This does not work -- you're accessing beyond the end of the uint32_t 
> for all LP64 hosts.  I think you just want ctz32()...
>
>
>> +            if (irq != 32) {
>> +                s->intisr_hi |= 1ULL << irq;
>> + qemu_set_irq(s->parent_irq[s->htmsi_vector[irq + 32]], 1);
>> +            }
>
> ... which should in fact only be tested if val != 0, which is to what 
> this IF equates.
>
> Is there a good reason that this function is treating hi and lo 
> separately, as opposed to simply doing all of the computation on 
> uint64_t?
>

In the part of linux kernel, pch pic driver use 32 bits for reading and 
writing.
e.g in the drivers/irqchip/irq-loongson-pch-pic.c， pch_pic_mask_irq() 
function use writel() to write pch_pic mask reg.

Thanks.
Xiaojuan
>
> r~

[-- Attachment #2: Type: text/html, Size: 5341 bytes --]