| Message ID | 20190827160554.30995-3-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | KVM/ARM: Fix >256 vcpus | expand |
Hi Eric, On 2019/8/28 0:05, Eric Auger wrote: > Host kernels that expose the KVM_CAP_ARM_IRQ_LINE_LAYOUT_2 capability > allow injection of PPIs along with vcpu ids larger than 255. Let's > encode the vpcu id on 12 bits according to the upgraded KVM_IRQ_LINE > ABI when needed. > > Without that patch qemu exits with "kvm_set_irq: Invalid argument" > message. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > Reported-by: Zenghui Yu <yuzenghui@huawei.com> > --- > hw/intc/arm_gic_kvm.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c > index b56fda144f..889293e97f 100644 > --- a/hw/intc/arm_gic_kvm.c > +++ b/hw/intc/arm_gic_kvm.c > @@ -56,6 +56,7 @@ void kvm_arm_gic_set_irq(uint32_t num_irq, int irq, int level) > * CPU number and interrupt number. > */ > int kvm_irq, irqtype, cpu; > + int cpu_idx1 = 0, cpu_idx2 = 0; > > if (irq < (num_irq - GIC_INTERNAL)) { > /* External interrupt. The kernel numbers these like the GIC > @@ -63,17 +64,20 @@ void kvm_arm_gic_set_irq(uint32_t num_irq, int irq, int level) > * internal ones. > */ > irqtype = KVM_ARM_IRQ_TYPE_SPI; > - cpu = 0; > irq += GIC_INTERNAL; > } else { > /* Internal interrupt: decode into (cpu, interrupt id) */ > irqtype = KVM_ARM_IRQ_TYPE_PPI; > irq -= (num_irq - GIC_INTERNAL); > cpu = irq / GIC_INTERNAL; > + cpu_idx2 = cpu / 256; > + cpu_idx1 = cpu % 256; > irq %= GIC_INTERNAL; > } > - kvm_irq = (irqtype << KVM_ARM_IRQ_TYPE_SHIFT) > - | (cpu << KVM_ARM_IRQ_VCPU_SHIFT) | irq; > + kvm_irq = (irqtype << KVM_ARM_IRQ_TYPE_SHIFT) | > + (cpu_idx1 << KVM_ARM_IRQ_VCPU_SHIFT) | > + ((cpu_idx2 & KVM_ARM_IRQ_VCPU2_MASK) << KVM_ARM_IRQ_VCPU2_SHIFT) | > + irq; > > kvm_set_irq(kvm_state, kvm_irq, !!level); > } > For confirmation, should we also adjust the vcpu_index in arm_cpu_kvm_set_irq(), just like above? Thanks, zenghui
Hi Zenghui, On 8/29/19 4:53 AM, Zenghui Yu wrote: > Hi Eric, > > On 2019/8/28 0:05, Eric Auger wrote: >> Host kernels that expose the KVM_CAP_ARM_IRQ_LINE_LAYOUT_2 capability >> allow injection of PPIs along with vcpu ids larger than 255. Let's >> encode the vpcu id on 12 bits according to the upgraded KVM_IRQ_LINE >> ABI when needed. >> >> Without that patch qemu exits with "kvm_set_irq: Invalid argument" >> message. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> Reported-by: Zenghui Yu <yuzenghui@huawei.com> >> --- >> hw/intc/arm_gic_kvm.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c >> index b56fda144f..889293e97f 100644 >> --- a/hw/intc/arm_gic_kvm.c >> +++ b/hw/intc/arm_gic_kvm.c >> @@ -56,6 +56,7 @@ void kvm_arm_gic_set_irq(uint32_t num_irq, int irq, >> int level) >> * CPU number and interrupt number. >> */ >> int kvm_irq, irqtype, cpu; >> + int cpu_idx1 = 0, cpu_idx2 = 0; >> if (irq < (num_irq - GIC_INTERNAL)) { >> /* External interrupt. The kernel numbers these like the GIC >> @@ -63,17 +64,20 @@ void kvm_arm_gic_set_irq(uint32_t num_irq, int >> irq, int level) >> * internal ones. >> */ >> irqtype = KVM_ARM_IRQ_TYPE_SPI; >> - cpu = 0; >> irq += GIC_INTERNAL; >> } else { >> /* Internal interrupt: decode into (cpu, interrupt id) */ >> irqtype = KVM_ARM_IRQ_TYPE_PPI; >> irq -= (num_irq - GIC_INTERNAL); >> cpu = irq / GIC_INTERNAL; >> + cpu_idx2 = cpu / 256; >> + cpu_idx1 = cpu % 256; >> irq %= GIC_INTERNAL; >> } >> - kvm_irq = (irqtype << KVM_ARM_IRQ_TYPE_SHIFT) >> - | (cpu << KVM_ARM_IRQ_VCPU_SHIFT) | irq; >> + kvm_irq = (irqtype << KVM_ARM_IRQ_TYPE_SHIFT) | >> + (cpu_idx1 << KVM_ARM_IRQ_VCPU_SHIFT) | >> + ((cpu_idx2 & KVM_ARM_IRQ_VCPU2_MASK) << >> KVM_ARM_IRQ_VCPU2_SHIFT) | >> + irq; >> kvm_set_irq(kvm_state, kvm_irq, !!level); >> } >> > > For confirmation, should we also adjust the vcpu_index in > arm_cpu_kvm_set_irq(), just like above? I am not familiar with this path. in arm_cpu_initfn(), there is a comment saying "VIRQ and VFIQ are unused with KVM but we add them to maintain the same interface as non-KVM CPUs." So I don't know when that code gets executed. But maybe it would be more cautious to implement your suggestion here as well. Maybe Peter can provide more info here? Thanks Eric > > > Thanks, > zenghui > >
Hi, On 8/29/19 9:58 AM, Auger Eric wrote: > Hi Zenghui, > > On 8/29/19 4:53 AM, Zenghui Yu wrote: >> Hi Eric, >> >> On 2019/8/28 0:05, Eric Auger wrote: >>> Host kernels that expose the KVM_CAP_ARM_IRQ_LINE_LAYOUT_2 capability >>> allow injection of PPIs along with vcpu ids larger than 255. Let's >>> encode the vpcu id on 12 bits according to the upgraded KVM_IRQ_LINE >>> ABI when needed. >>> >>> Without that patch qemu exits with "kvm_set_irq: Invalid argument" >>> message. >>> >>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>> Reported-by: Zenghui Yu <yuzenghui@huawei.com> >>> --- >>> hw/intc/arm_gic_kvm.c | 10 +++++++--- >>> 1 file changed, 7 insertions(+), 3 deletions(-) >>> >>> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c >>> index b56fda144f..889293e97f 100644 >>> --- a/hw/intc/arm_gic_kvm.c >>> +++ b/hw/intc/arm_gic_kvm.c >>> @@ -56,6 +56,7 @@ void kvm_arm_gic_set_irq(uint32_t num_irq, int irq, >>> int level) >>> * CPU number and interrupt number. >>> */ >>> int kvm_irq, irqtype, cpu; >>> + int cpu_idx1 = 0, cpu_idx2 = 0; >>> if (irq < (num_irq - GIC_INTERNAL)) { >>> /* External interrupt. The kernel numbers these like the GIC >>> @@ -63,17 +64,20 @@ void kvm_arm_gic_set_irq(uint32_t num_irq, int >>> irq, int level) >>> * internal ones. >>> */ >>> irqtype = KVM_ARM_IRQ_TYPE_SPI; >>> - cpu = 0; >>> irq += GIC_INTERNAL; >>> } else { >>> /* Internal interrupt: decode into (cpu, interrupt id) */ >>> irqtype = KVM_ARM_IRQ_TYPE_PPI; >>> irq -= (num_irq - GIC_INTERNAL); >>> cpu = irq / GIC_INTERNAL; >>> + cpu_idx2 = cpu / 256; >>> + cpu_idx1 = cpu % 256; >>> irq %= GIC_INTERNAL; >>> } >>> - kvm_irq = (irqtype << KVM_ARM_IRQ_TYPE_SHIFT) >>> - | (cpu << KVM_ARM_IRQ_VCPU_SHIFT) | irq; >>> + kvm_irq = (irqtype << KVM_ARM_IRQ_TYPE_SHIFT) | >>> + (cpu_idx1 << KVM_ARM_IRQ_VCPU_SHIFT) | >>> + ((cpu_idx2 & KVM_ARM_IRQ_VCPU2_MASK) << >>> KVM_ARM_IRQ_VCPU2_SHIFT) | >>> + irq; >>> kvm_set_irq(kvm_state, kvm_irq, !!level); >>> } >>> >> >> For confirmation, should we also adjust the vcpu_index in >> arm_cpu_kvm_set_irq(), just like above? > > I am not familiar with this path. in arm_cpu_initfn(), there is a > comment saying "VIRQ and VFIQ are unused with KVM but we add them to > maintain the same interface as non-KVM CPUs." So I don't know when that > code gets executed. > > But maybe it would be more cautious to implement your suggestion here as > well. > > Maybe Peter can provide more info here? If this is supposed to get used along with kernel_irqchip=off, it seems this latter is not supported with GICv3 anyway. So max number of vcpus with GICv2 is 8. Thanks Eric > > Thanks > > Eric > > >> >> >> Thanks, >> zenghui >> >> >
On Thu, 29 Aug 2019 at 08:58, Auger Eric <eric.auger@redhat.com> wrote: > > Hi Zenghui, > > On 8/29/19 4:53 AM, Zenghui Yu wrote: > > For confirmation, should we also adjust the vcpu_index in > > arm_cpu_kvm_set_irq(), just like above? > > I am not familiar with this path. in arm_cpu_initfn(), there is a > comment saying "VIRQ and VFIQ are unused with KVM but we add them to > maintain the same interface as non-KVM CPUs." So I don't know when that > code gets executed. That comment is saying that all KVM guest CPUs are EL1-only (since we don't handle nested virt), and therefore they logically don't have an inbound VIRQ or VFIQ line. But we provide the qemu_irqs for them anyway, so that board code doesn't have to have tedious conditionals saying "if this CPU has EL2 then wire up VIRQ and VFIQ to the GIC". If you ever try to actually assert the VIRQ or VFIQ lines you will hit the g_assert_not_reached() in arm_cpu_kvm_set_irq(). thanks -- PMM
Hi Peter, On 9/3/19 10:29 AM, Peter Maydell wrote: > On Thu, 29 Aug 2019 at 08:58, Auger Eric <eric.auger@redhat.com> wrote: >> >> Hi Zenghui, >> >> On 8/29/19 4:53 AM, Zenghui Yu wrote: >>> For confirmation, should we also adjust the vcpu_index in >>> arm_cpu_kvm_set_irq(), just like above? >> >> I am not familiar with this path. in arm_cpu_initfn(), there is a >> comment saying "VIRQ and VFIQ are unused with KVM but we add them to >> maintain the same interface as non-KVM CPUs." So I don't know when that >> code gets executed. > > That comment is saying that all KVM guest CPUs are > EL1-only (since we don't handle nested virt), and therefore > they logically don't have an inbound VIRQ or VFIQ line. > But we provide the qemu_irqs for them anyway, so that > board code doesn't have to have tedious conditionals > saying "if this CPU has EL2 then wire up VIRQ and VFIQ > to the GIC". If you ever try to actually assert the VIRQ > or VFIQ lines you will hit the g_assert_not_reached() in > arm_cpu_kvm_set_irq(). OK thanks for the clarification. I mixed things up. I guess arm_cpu_kvm_set_irq attempting to inject IRQ/FIQ into KVM is used with userspace GIC emulation, which is not supported along with GICv3. But anyway, I guess it does not hurt to set vcpu_index2 in arm_cpu_kvm_set_irq? Thanks Eric > > thanks > -- PMM >
On Tue, 3 Sep 2019 at 09:40, Auger Eric <eric.auger@redhat.com> wrote: > I guess arm_cpu_kvm_set_irq attempting to inject IRQ/FIQ into KVM is > used with userspace GIC emulation, which is not supported along with > GICv3. But anyway, I guess it does not hurt to set vcpu_index2 in > arm_cpu_kvm_set_irq? Having now got up to speed with the kernel patchset that goes with this one: yes, we should set the vcpu_index2 in the arm_cpu_kvm_set_irq function as well. Given that we have two callsites that now need to assemble the value for kvm_set_irq() and the cpu_index field is in two pieces, maybe we should define a utility function that takes cpu-index, irq-type and irq-id as separate arguments and assembles the fields into the right places and calls kvm_set_irq() ? thanks -- PMM
diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c index b56fda144f..889293e97f 100644 --- a/hw/intc/arm_gic_kvm.c +++ b/hw/intc/arm_gic_kvm.c @@ -56,6 +56,7 @@ void kvm_arm_gic_set_irq(uint32_t num_irq, int irq, int level) * CPU number and interrupt number. */ int kvm_irq, irqtype, cpu; + int cpu_idx1 = 0, cpu_idx2 = 0; if (irq < (num_irq - GIC_INTERNAL)) { /* External interrupt. The kernel numbers these like the GIC @@ -63,17 +64,20 @@ void kvm_arm_gic_set_irq(uint32_t num_irq, int irq, int level) * internal ones. */ irqtype = KVM_ARM_IRQ_TYPE_SPI; - cpu = 0; irq += GIC_INTERNAL; } else { /* Internal interrupt: decode into (cpu, interrupt id) */ irqtype = KVM_ARM_IRQ_TYPE_PPI; irq -= (num_irq - GIC_INTERNAL); cpu = irq / GIC_INTERNAL; + cpu_idx2 = cpu / 256; + cpu_idx1 = cpu % 256; irq %= GIC_INTERNAL; } - kvm_irq = (irqtype << KVM_ARM_IRQ_TYPE_SHIFT) - | (cpu << KVM_ARM_IRQ_VCPU_SHIFT) | irq; + kvm_irq = (irqtype << KVM_ARM_IRQ_TYPE_SHIFT) | + (cpu_idx1 << KVM_ARM_IRQ_VCPU_SHIFT) | + ((cpu_idx2 & KVM_ARM_IRQ_VCPU2_MASK) << KVM_ARM_IRQ_VCPU2_SHIFT) | + irq; kvm_set_irq(kvm_state, kvm_irq, !!level); }
Host kernels that expose the KVM_CAP_ARM_IRQ_LINE_LAYOUT_2 capability allow injection of PPIs along with vcpu ids larger than 255. Let's encode the vpcu id on 12 bits according to the upgraded KVM_IRQ_LINE ABI when needed. Without that patch qemu exits with "kvm_set_irq: Invalid argument" message. Signed-off-by: Eric Auger <eric.auger@redhat.com> Reported-by: Zenghui Yu <yuzenghui@huawei.com> --- hw/intc/arm_gic_kvm.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)