diff mbox series

[for-5.1,3/7] hw/mips: Add CPU IRQ3 delivery for KVM

Message ID 1587979995-17717-3-git-send-email-chenhc@lemote.com (mailing list archive)
State New, archived
Headers show
Series [for-5.1,1/7] configure: Add KVM target support for MIPS64 | expand

Commit Message

chen huacai April 27, 2020, 9:33 a.m. UTC 
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
(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(-)

Comments

Philippe Mathieu-Daudé April 27, 2020, 9:57 a.m. UTC | #1 
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
> (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?

>              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) {
>
chen huacai April 28, 2020, 8:28 a.m. UTC | #2 
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
> > (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?

>
> >              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) {
> >
Aleksandar Markovic April 28, 2020, 6:58 p.m. UTC | #3 
уто, 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
> > > (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
Huacai Chen April 29, 2020, 1:52 a.m. UTC | #4 
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.

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
> > > > (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
Philippe Mathieu-Daudé April 29, 2020, 9:17 a.m. UTC | #5 
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
>
Huacai Chen April 29, 2020, 10:13 a.m. UTC | #6 
Hi, Philippe,

On Wed, Apr 29, 2020 at 5:18 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> 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.
I think the previous code only deliver IP2 is because KVM/MIPS only
use IP2 for external interrupts, but now I have changed KVM/MIPS as
well, please see:
https://patchwork.kernel.org/patch/11507591/

>
> And I see you selected CP0C3_VInt in the R4 definition... so what is
> delivered here?
CP0C3_VInt just indicates the capability, kernel of Loongson-3 doesn't use VINT.

>
> >
> > 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"?
Sorry, this is my fault.

>
> >>>>> (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
> >
diff mbox series

Patch

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))
             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) {