| Message ID | 1587979995-17717-3-git-send-email-chenhc@lemote.com |
|---|---|
| State | New |
| Headers | show |
| Series | [for-5.1,1/7] configure: Add KVM target support for MIPS64 | expand |
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) { >
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) { > >
уто, 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
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
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 >
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 --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) {
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(-)