[RFC,2/3] intc/arm_gic: Support PPI injection for more than 256 vpus
diff mbox series

Message ID 20190827160554.30995-3-eric.auger@redhat.com
State New
Headers show
Series
  • KVM/ARM: Fix >256 vcpus
Related show

Commit Message

Eric Auger Aug. 27, 2019, 4:05 p.m. UTC
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(-)

Comments

Zenghui Yu Aug. 29, 2019, 2:53 a.m. UTC | #1
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
Eric Auger Aug. 29, 2019, 7:58 a.m. UTC | #2
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
> 
>
Eric Auger Aug. 29, 2019, 9:21 a.m. UTC | #3
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
>>
>>
>
Peter Maydell Sept. 3, 2019, 8:29 a.m. UTC | #4
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
Eric Auger Sept. 3, 2019, 8:40 a.m. UTC | #5
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
>
Peter Maydell Sept. 6, 2019, 10:14 a.m. UTC | #6
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

Patch
diff mbox series

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);
 }