Re: [PATCH for-5.1 3/7] hw/mips: Add CPU IRQ3 delivery for KVM - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
To: Huacai Chen <chenhuacai@gmail.com>,
	Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
Cc: chen huacai <zltjiangshi@gmail.com>,
	qemu-level <qemu-devel@nongnu.org>,
	Aurelien Jarno <aurelien@aurel32.net>
Subject: Re: [PATCH for-5.1 3/7] hw/mips: Add CPU IRQ3 delivery for KVM
Date: Wed, 29 Apr 2020 11:17:59 +0200	[thread overview]
Message-ID: <9d7b79c9-8d95-93ef-e0c3-ddccf5b46a84@amsat.org> (raw)
In-Reply-To: <CAAhV-H6tTU0jLfFFotrfCE_4sQKB0UEhKbxoGQrOhqzVBvo35g@mail.gmail.com>

On 4/29/20 3:52 AM, Huacai Chen wrote:
> Hi, Philippe and Aleksandar,
> 
> I'm not refusing to change my patch, but I have two questions:
> 1, Why we should identify Loongson-3 to deliver IP3? It seems that
> deliver all IPs (IP2~IP7) unconditionally is harmless as well.
> 2, How to identify Loongson-3 by Config6/Config7? Loongson-3 is not
> the only processor which has Config6/Config7.
Please don't top-post on technical lists, it makes the conversation
harder to follow.

This code is modelling the device, not KVM.

Commit b1bd8b28cca is not very verbose. I wonder why not delivering all 
IRQs to kvm_mips_set_interrupt, that would make this patch simpler.

I think the problem in QEMU MIPS IRQ delivery is one implementation is 
in cpu_mips_irq_request() while another one (vectored IRQ) in 
cpu_mips_hw_interrupts_pending (see 138afb024bb) and KVM is also in the 
middle.

And I see you selected CP0C3_VInt in the R4 definition... so what is 
delivered here?

> 
> Huacai
> 
> On Wed, Apr 29, 2020 at 2:58 AM Aleksandar Markovic
> <aleksandar.qemu.devel@gmail.com> wrote:
>>
>> уто, 28. апр 2020. у 10:21 chen huacai <zltjiangshi@gmail.com> је написао/ла:
>>>
>>> Hi, Philippe,
>>>
>>> On Mon, Apr 27, 2020 at 5:57 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>>
>>>> On 4/27/20 11:33 AM, Huacai Chen wrote:
>>>>> Currently, KVM/MIPS only deliver I/O interrupt via IP2, this patch add
>>>>> IP2 delivery as well, because Loongson-3 based machine use both IRQ2

"IP3 delivery as well"?

>>>>> (CPU's IP2) and IRQ3 (CPU's IP3).
>>>>>
>>>>> Signed-off-by: Huacai Chen <chenhc@lemote.com>
>>>>> Co-developed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>>>>> ---
>>>>>   hw/mips/mips_int.c | 6 ++----
>>>>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
>>>>> index 796730b..5526219 100644
>>>>> --- a/hw/mips/mips_int.c
>>>>> +++ b/hw/mips/mips_int.c
>>>>> @@ -48,16 +48,14 @@ static void cpu_mips_irq_request(void *opaque, int irq, int level)
>>>>>       if (level) {
>>>>>           env->CP0_Cause |= 1 << (irq + CP0Ca_IP);
>>>>>
>>>>> -        if (kvm_enabled() && irq == 2) {
>>>>> +        if (kvm_enabled() && (irq == 2 || irq == 3))
>>>>
>>>> Shouldn't we check env->CP0_Config6 (or Config7) has the required
>>>> feature first?
>>> I'm sorry that I can't understand IRQ delivery has something to do
>>> with Config6/Config7, to identify Loongson-3?
>>>
>>
>> Obviously, yes.
>>
>> Thanks,
>> Aleksandar
>>
>>
>>>>
>>>>>               kvm_mips_set_interrupt(cpu, irq, level);
>>>>> -        }
>>>>>
>>>>>       } else {
>>>>>           env->CP0_Cause &= ~(1 << (irq + CP0Ca_IP));
>>>>>
>>>>> -        if (kvm_enabled() && irq == 2) {
>>>>> +        if (kvm_enabled() && (irq == 2 || irq == 3))
>>>>>               kvm_mips_set_interrupt(cpu, irq, level);
>>>>> -        }
>>>>>       }
>>>>>
>>>>>       if (env->CP0_Cause & CP0Ca_IP_mask) {
>>>>>
>>>
>>>
>>>
>>> --
>>> Huacai Chen
> 