* Re:Re: [PATCH] KVM: arm/arm64 : add lpi info in vgic-debug
@ 2018-03-23 13:33 peng.hao2
2018-03-23 14:45 ` Marc Zyngier
0 siblings, 1 reply; 4+ messages in thread
From: peng.hao2 @ 2018-03-23 13:33 UTC (permalink / raw)
To: marc.zyngier; +Cc: linux-arm-kernel, linux-kernel, christoffer.dall, kvmarm
[-- Attachment #1.1: Type: text/plain, Size: 6834 bytes --]
>On 23/03/18 10:36, Peng Hao wrote:
>> Add lpi debug info to vgic-stat.
>> the printed info like this:
>> SPI 287 0 000001 0 0 0 160 -1
>> LPI 8192 2 000100 0 0 0 160 -1
>>
>> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
>> ---
>> virt/kvm/arm/vgic/vgic-debug.c | 61 ++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 56 insertions(+), 5 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-debug.c b/virt/kvm/arm/vgic/vgic-debug.c
>> index 10b3817..444115e 100644
>> --- a/virt/kvm/arm/vgic/vgic-debug.c
>> +++ b/virt/kvm/arm/vgic/vgic-debug.c
>> @@ -36,9 +36,12 @@
>> struct vgic_state_iter {
>> int nr_cpus;
>> int nr_spis;
>> + int nr_lpis;
>> int dist_id;
>> int vcpu_id;
>> int intid;
>> + int lpi_print_count;
>> + struct vgic_irq **lpi_irqs;
>> };
>>
>> static void iter_next(struct vgic_state_iter *iter)
>> @@ -52,6 +55,40 @@ static void iter_next(struct vgic_state_iter *iter)
>> if (iter->intid == VGIC_NR_PRIVATE_IRQS &&
>> ++iter->vcpu_id < iter->nr_cpus)
>> iter->intid = 0;
>> +
>> + if (iter->intid >= VGIC_NR_PRIVATE_IRQS + iter->nr_spis) {
>> + if (iter->lpi_print_count < iter->nr_lpis)
>> + iter->intid = iter->lpi_irqs[iter->lpi_print_count]->intid;
>> + iter->lpi_print_count++;
>> + }
>> +}
>> +
>> +static void vgic_debug_get_lpis(struct kvm *kvm, struct vgic_state_iter *iter)
>> +{
>> + struct vgic_dist *dist = &kvm->arch.vgic;
>> + int i = 0;
>> + struct vgic_irq *irq = NULL, **lpi_irqs;
>> +
>> +again:
>> + iter->nr_lpis = dist->lpi_list_count;
>> + lpi_irqs = kmalloc_array(iter->nr_lpis, sizeof(irq), GFP_KERNEL);
>> + if (!lpi_irqs) {
>> + iter->nr_lpis = 0;
>> + return;
>> + }
>> + spin_lock(&dist->lpi_list_lock);
>> + if (iter->nr_lpis != dist->lpi_list_count) {
>> + kfree(lpi_irqs);
>> + spin_unlock(&dist->lpi_list_lock);
>> + goto again;
>> + }
>Why do we need an exact count? It is fine to have a transient count, and
>the debug code should be able to come with that without performing this
>terrible loop.
yeah, it is enough to have a rough count for debug code .
>We also already have some code that snapshot the the LPIs (see
>vgic_copy_lpi_list), so please consider reusing that instead.
I can't reuse vgic_copy_lpi_list. It snapshots based on LPI's target vcpu.
>> +
>> + list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) {
>> + vgic_get_irq_kref(irq);
>> + lpi_irqs[i++] = irq;
>> + }
>> + spin_unlock(&dist->lpi_list_lock);
>> + iter->lpi_irqs = lpi_irqs;
>Messing with the internals of the refcounts is really a bad idea. Please
>use vgic_get_irq() in conjunction with the above, and allow it to fail
>gracefully.
vgic_get_irq require intid as input and vgic_get_lpi that vgic_get_irq calling will traverse the lpi_list with holding lpi_list_lock again,
but here I has held lpi_list_lock. So I think calling vgic_get_irq_kref is safe with holding the
lpi_list_lock.
> > }
> >
>> static void iter_init(struct kvm *kvm, struct vgic_state_iter *iter,
>> @@ -64,6 +101,8 @@ static void iter_init(struct kvm *kvm, struct vgic_state_iter *iter,
>> iter->nr_cpus = nr_cpus;
>> iter->nr_spis = kvm->arch.vgic.nr_spis;
>>
>> + if (vgic_supports_direct_msis(kvm) && !pos)
>> + vgic_debug_get_lpis(kvm, iter);
>> /* Fast forward to the right position if needed */
>> while (pos--)
>> iter_next(iter);
>> @@ -73,7 +112,9 @@ static bool end_of_vgic(struct vgic_state_iter *iter)
>> {
>> return iter->dist_id > 0 &&
>> iter->vcpu_id == iter->nr_cpus &&
>> - (iter->intid - VGIC_NR_PRIVATE_IRQS) == iter->nr_spis;
>> + (iter->intid - VGIC_NR_PRIVATE_IRQS) >= iter->nr_spis &&
>> + ((iter->nr_lpis == 0) ||
>> + (iter->lpi_print_count == iter->nr_lpis + 1));
>> }
>>
>> static void *vgic_debug_start(struct seq_file *s, loff_t *pos)
>> @@ -130,6 +171,7 @@ static void vgic_debug_stop(struct seq_file *s, void *v)
>>
>> mutex_lock(&kvm->lock);
>> iter = kvm->arch.vgic.iter;
>> + kfree(iter->lpi_irqs);
>> kfree(iter);
>> kvm->arch.vgic.iter = NULL;
>> mutex_unlock(&kvm->lock);
>> @@ -154,7 +196,7 @@ static void print_header(struct seq_file *s, struct vgic_irq *irq,
>> struct kvm_vcpu *vcpu)
>> {
>> int id = 0;
>> - char *hdr = "SPI ";
>> + char *hdr = "S/LPI ";
>>
>> if (vcpu) {
>> hdr = "VCPU";
>> @@ -162,7 +204,10 @@ static void print_header(struct seq_file *s, struct vgic_irq *irq,
>> }
>>
>> seq_printf(s, "\n");
>> - seq_printf(s, "%s%2d TYP ID TGT_ID PLAEHC HWID TARGET SRC PRI VCPU_ID\n", hdr, id);
>> + if (vcpu)
>> + seq_printf(s, "%s%2d TYP ID TGT_ID PLAEHC HWID TARGET SRC PRI VCPU_ID\n", hdr, id);
>> + else
>> + seq_printf(s, "%s TYP ID TGT_ID PLAEHC HWID TARGET SRC PRI VCPU_ID\n", hdr);
>This feels like an unnecessary change. But if you really want that kind
>of detail, change your "S/LPI" to say something more generic, such as
>"Global".
I modify this just for aligned print output. "Global" is great.
>> seq_printf(s, "---------------------------------------------------------------\n");> }
>>
>> @@ -174,8 +219,10 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
>> type = "SGI";
>> else if (irq->intid < VGIC_NR_PRIVATE_IRQS)
>> type = "PPI";
>> - else
>> + else if (irq->intid < VGIC_MAX_SPI)
>> type = "SPI";
>> + else if (irq->intid >= VGIC_MIN_LPI)
>> + type = "LPI";
>>
>> if (irq->intid ==0 || irq->intid == VGIC_NR_PRIVATE_IRQS)
>> print_header(s, irq, vcpu);
>> @@ -220,7 +267,9 @@ static int vgic_debug_show(struct seq_file *s, void *v)
>> if (!kvm->arch.vgic.initialized)
>> return 0;
>>
>> - if (iter->vcpu_id < iter->nr_cpus) {
>> + if (iter->intid >= VGIC_MIN_LPI)
>> + irq = iter->lpi_irqs[iter->lpi_print_count - 1];
>> + else if (iter->vcpu_id < iter->nr_cpus) {
>> vcpu = kvm_get_vcpu(kvm, iter->vcpu_id);
>> irq = &vcpu->arch.vgic_cpu.private_irqs[iter->intid];
>> } else {
>> @@ -230,6 +279,8 @@ static int vgic_debug_show(struct seq_file *s, void *v)
>> spin_lock(&irq->irq_lock);
>> print_irq_state(s, irq, vcpu);
>> spin_unlock(&irq->irq_lock);
>> + if (iter->intid >= VGIC_MIN_LPI)
>> + vgic_put_irq(kvm, irq);
>If you adopt the scheme I outlined above, you can have a balanced
>get/put behaviour, irrespective of the interrupt type, and a much nicer
>result.
yeah, "if (iter->intid >= VGIC_MIN_LPI)" is unnecessary.
>> return 0;
>> }
>>
>Thanks,
> M.
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] KVM: arm/arm64 : add lpi info in vgic-debug
2018-03-23 13:33 Re:Re: [PATCH] KVM: arm/arm64 : add lpi info in vgic-debug peng.hao2
@ 2018-03-23 14:45 ` Marc Zyngier
0 siblings, 0 replies; 4+ messages in thread
From: Marc Zyngier @ 2018-03-23 14:45 UTC (permalink / raw)
To: peng.hao2; +Cc: Christoffer Dall, linux-kernel, kvmarm, linux-arm-kernel
[fixing Christoffer's email address]
On 23/03/18 13:33, peng.hao2@zte.com.cn wrote:
>> On 23/03/18 10:36, Peng Hao wrote:
>>> Add lpi debug info to vgic-stat.
>>> the printed info like this:
>>> SPI 287 0 000001 0 0 0 160 -1
>>> LPI 8192 2 000100 0 0 0 160 -1
>>>
>>> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
>>> ---
>>> virt/kvm/arm/vgic/vgic-debug.c | 61 ++++++++++++++++++++++++++++++++++++++----
>>> 1 file changed, 56 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/virt/kvm/arm/vgic/vgic-debug.c b/virt/kvm/arm/vgic/vgic-debug.c
>>> index 10b3817..444115e 100644
>>> --- a/virt/kvm/arm/vgic/vgic-debug.c
>>> +++ b/virt/kvm/arm/vgic/vgic-debug.c
>>> @@ -36,9 +36,12 @@
>>> struct vgic_state_iter {
>>> int nr_cpus;
>>> int nr_spis;
>>> + int nr_lpis;
>>> int dist_id;
>>> int vcpu_id;
>>> int intid;
>>> + int lpi_print_count;
>>> + struct vgic_irq **lpi_irqs;
>>> };
>>>
>>> static void iter_next(struct vgic_state_iter *iter)
>>> @@ -52,6 +55,40 @@ static void iter_next(struct vgic_state_iter *iter)
>>> if (iter->intid == VGIC_NR_PRIVATE_IRQS &&
>>> ++iter->vcpu_id < iter->nr_cpus)
>>> iter->intid = 0;
>>> +
>>> + if (iter->intid >= VGIC_NR_PRIVATE_IRQS + iter->nr_spis) {
>>> + if (iter->lpi_print_count < iter->nr_lpis)
>>> + iter->intid = iter->lpi_irqs[iter->lpi_print_count]->intid;
>>> + iter->lpi_print_count++;
>>> + }
>>> +}
>>> +
>>> +static void vgic_debug_get_lpis(struct kvm *kvm, struct vgic_state_iter *iter)
>>> +{
>>> + struct vgic_dist *dist = &kvm->arch.vgic;
>>> + int i = 0;
>>> + struct vgic_irq *irq = NULL, **lpi_irqs;
>>> +
>>> +again:
>>> + iter->nr_lpis = dist->lpi_list_count;
>>> + lpi_irqs = kmalloc_array(iter->nr_lpis, sizeof(irq), GFP_KERNEL);
>>> + if (!lpi_irqs) {
>>> + iter->nr_lpis = 0;
>>> + return;
>>> + }
>>> + spin_lock(&dist->lpi_list_lock);
>>> + if (iter->nr_lpis != dist->lpi_list_count) {
>>> + kfree(lpi_irqs);
>>> + spin_unlock(&dist->lpi_list_lock);
>>> + goto again;
>>> + }
>
>> Why do we need an exact count? It is fine to have a transient count, and
>> the debug code should be able to come with that without performing this
>> terrible loop.
> yeah, it is enough to have a rough count for debug code .
>> We also already have some code that snapshot the the LPIs (see
>> vgic_copy_lpi_list), so please consider reusing that instead.
> I can't reuse vgic_copy_lpi_list. It snapshots based on LPI's target vcpu.
Guess what? You can also change it!
>>> +
>>> + list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) {
>>> + vgic_get_irq_kref(irq);
>>> + lpi_irqs[i++] = irq;
>>> + }
>>> + spin_unlock(&dist->lpi_list_lock);
>>> + iter->lpi_irqs = lpi_irqs;
>
>> Messing with the internals of the refcounts is really a bad idea. Please
>> use vgic_get_irq() in conjunction with the above, and allow it to fail
>> gracefully.
> vgic_get_irq require intid as input and vgic_get_lpi that vgic_get_irq calling will traverse the lpi_list with holding lpi_list_lock again,
> but here I has held lpi_list_lock. So I think calling vgic_get_irq_kref is safe with holding the
> lpi_list_lock.
It is safe but terribly ugly. Traversing the LPI list is completely
fine, and we have this API for a reason (not reinventing the wheel). I
don't think the debug code should sidestep it. If the list proves to be
too slow to traverse, then we should adopt a better data structure.
>>> }
>>>
>>> static void iter_init(struct kvm *kvm, struct vgic_state_iter *iter,
>>> @@ -64,6 +101,8 @@ static void iter_init(struct kvm *kvm, struct vgic_state_iter *iter,
>>> iter->nr_cpus = nr_cpus;
>>> iter->nr_spis = kvm->arch.vgic.nr_spis;
>>>
>>> + if (vgic_supports_direct_msis(kvm) && !pos)
>>> + vgic_debug_get_lpis(kvm, iter);
BTW, what's the point of this?
>>> /* Fast forward to the right position if needed */
>>> while (pos--)
>>> iter_next(iter);
>>> @@ -73,7 +112,9 @@ static bool end_of_vgic(struct vgic_state_iter *iter)
>>> {
>>> return iter->dist_id > 0 &&
>>> iter->vcpu_id == iter->nr_cpus &&
>>> - (iter->intid - VGIC_NR_PRIVATE_IRQS) == iter->nr_spis;
>>> + (iter->intid - VGIC_NR_PRIVATE_IRQS) >= iter->nr_spis &&
>>> + ((iter->nr_lpis == 0) ||
>>> + (iter->lpi_print_count == iter->nr_lpis + 1));
>>> }
>>>
>>> static void *vgic_debug_start(struct seq_file *s, loff_t *pos)
>>> @@ -130,6 +171,7 @@ static void vgic_debug_stop(struct seq_file *s, void *v)
>>>
>>> mutex_lock(&kvm->lock);
>>> iter = kvm->arch.vgic.iter;
>>> + kfree(iter->lpi_irqs);
>>> kfree(iter);
>>> kvm->arch.vgic.iter = NULL;
>>> mutex_unlock(&kvm->lock);
>>> @@ -154,7 +196,7 @@ static void print_header(struct seq_file *s, struct vgic_irq *irq,
>>> struct kvm_vcpu *vcpu)
>>> {
>>> int id = 0;
>>> - char *hdr = "SPI ";
>>> + char *hdr = "S/LPI ";
>>>
>>> if (vcpu) {
>>> hdr = "VCPU";
>>> @@ -162,7 +204,10 @@ static void print_header(struct seq_file *s, struct vgic_irq *irq,
>>> }
>>>
>>> seq_printf(s, "\n");
>>> - seq_printf(s, "%s%2d TYP ID TGT_ID PLAEHC HWID TARGET SRC PRI VCPU_ID\n", hdr, id);
>>> + if (vcpu)
>>> + seq_printf(s, "%s%2d TYP ID TGT_ID PLAEHC HWID TARGET SRC PRI VCPU_ID\n", hdr, id);
>>> + else
>>> + seq_printf(s, "%s TYP ID TGT_ID PLAEHC HWID TARGET SRC PRI VCPU_ID\n", hdr);
>
>> This feels like an unnecessary change. But if you really want that kind
>> of detail, change your "S/LPI" to say something more generic, such as
>> "Global".
> I modify this just for aligned print output. "Global" is great.
>>> seq_printf(s, "---------------------------------------------------------------\n");> }
>>>
>>> @@ -174,8 +219,10 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
>>> type = "SGI";
>>> else if (irq->intid < VGIC_NR_PRIVATE_IRQS)
>>> type = "PPI";
>>> - else
>>> + else if (irq->intid < VGIC_MAX_SPI)
>>> type = "SPI";
>>> + else if (irq->intid >= VGIC_MIN_LPI)
>>> + type = "LPI";
>>>
>>> if (irq->intid ==0 || irq->intid == VGIC_NR_PRIVATE_IRQS)
>>> print_header(s, irq, vcpu);
>>> @@ -220,7 +267,9 @@ static int vgic_debug_show(struct seq_file *s, void *v)
>>> if (!kvm->arch.vgic.initialized)
>>> return 0;
>>>
>>> - if (iter->vcpu_id < iter->nr_cpus) {
>>> + if (iter->intid >= VGIC_MIN_LPI)
>>> + irq = iter->lpi_irqs[iter->lpi_print_count - 1];
>>> + else if (iter->vcpu_id < iter->nr_cpus) {
>>> vcpu = kvm_get_vcpu(kvm, iter->vcpu_id);
>>> irq = &vcpu->arch.vgic_cpu.private_irqs[iter->intid];
>>> } else {
>>> @@ -230,6 +279,8 @@ static int vgic_debug_show(struct seq_file *s, void *v)
>>> spin_lock(&irq->irq_lock);
>>> print_irq_state(s, irq, vcpu);
>>> spin_unlock(&irq->irq_lock);
>>> + if (iter->intid >= VGIC_MIN_LPI)
>>> + vgic_put_irq(kvm, irq);
>
>> If you adopt the scheme I outlined above, you can have a balanced
>> get/put behaviour, irrespective of the interrupt type, and a much nicer
>> result.
> yeah, "if (iter->intid >= VGIC_MIN_LPI)" is unnecessary.
I'm still adamant that you should have get/put for every interrupt. It
won't cost anything, and the code will look better.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] KVM: arm/arm64 : add lpi info in vgic-debug
@ 2018-03-23 10:36 Peng Hao
2018-03-23 11:20 ` Marc Zyngier
0 siblings, 1 reply; 4+ messages in thread
From: Peng Hao @ 2018-03-23 10:36 UTC (permalink / raw)
To: christoffer.dall, marc.zyngier
Cc: linux-kernel, kvmarm, linux-arm-kernel, Peng Hao
Add lpi debug info to vgic-stat.
the printed info like this:
SPI 287 0 000001 0 0 0 160 -1
LPI 8192 2 000100 0 0 0 160 -1
Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
virt/kvm/arm/vgic/vgic-debug.c | 61 ++++++++++++++++++++++++++++++++++++++----
1 file changed, 56 insertions(+), 5 deletions(-)
diff --git a/virt/kvm/arm/vgic/vgic-debug.c b/virt/kvm/arm/vgic/vgic-debug.c
index 10b3817..444115e 100644
--- a/virt/kvm/arm/vgic/vgic-debug.c
+++ b/virt/kvm/arm/vgic/vgic-debug.c
@@ -36,9 +36,12 @@
struct vgic_state_iter {
int nr_cpus;
int nr_spis;
+ int nr_lpis;
int dist_id;
int vcpu_id;
int intid;
+ int lpi_print_count;
+ struct vgic_irq **lpi_irqs;
};
static void iter_next(struct vgic_state_iter *iter)
@@ -52,6 +55,40 @@ static void iter_next(struct vgic_state_iter *iter)
if (iter->intid == VGIC_NR_PRIVATE_IRQS &&
++iter->vcpu_id < iter->nr_cpus)
iter->intid = 0;
+
+ if (iter->intid >= VGIC_NR_PRIVATE_IRQS + iter->nr_spis) {
+ if (iter->lpi_print_count < iter->nr_lpis)
+ iter->intid = iter->lpi_irqs[iter->lpi_print_count]->intid;
+ iter->lpi_print_count++;
+ }
+}
+
+static void vgic_debug_get_lpis(struct kvm *kvm, struct vgic_state_iter *iter)
+{
+ struct vgic_dist *dist = &kvm->arch.vgic;
+ int i = 0;
+ struct vgic_irq *irq = NULL, **lpi_irqs;
+
+again:
+ iter->nr_lpis = dist->lpi_list_count;
+ lpi_irqs = kmalloc_array(iter->nr_lpis, sizeof(irq), GFP_KERNEL);
+ if (!lpi_irqs) {
+ iter->nr_lpis = 0;
+ return;
+ }
+ spin_lock(&dist->lpi_list_lock);
+ if (iter->nr_lpis != dist->lpi_list_count) {
+ kfree(lpi_irqs);
+ spin_unlock(&dist->lpi_list_lock);
+ goto again;
+ }
+
+ list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) {
+ vgic_get_irq_kref(irq);
+ lpi_irqs[i++] = irq;
+ }
+ spin_unlock(&dist->lpi_list_lock);
+ iter->lpi_irqs = lpi_irqs;
}
static void iter_init(struct kvm *kvm, struct vgic_state_iter *iter,
@@ -64,6 +101,8 @@ static void iter_init(struct kvm *kvm, struct vgic_state_iter *iter,
iter->nr_cpus = nr_cpus;
iter->nr_spis = kvm->arch.vgic.nr_spis;
+ if (vgic_supports_direct_msis(kvm) && !pos)
+ vgic_debug_get_lpis(kvm, iter);
/* Fast forward to the right position if needed */
while (pos--)
iter_next(iter);
@@ -73,7 +112,9 @@ static bool end_of_vgic(struct vgic_state_iter *iter)
{
return iter->dist_id > 0 &&
iter->vcpu_id == iter->nr_cpus &&
- (iter->intid - VGIC_NR_PRIVATE_IRQS) == iter->nr_spis;
+ (iter->intid - VGIC_NR_PRIVATE_IRQS) >= iter->nr_spis &&
+ ((iter->nr_lpis == 0) ||
+ (iter->lpi_print_count == iter->nr_lpis + 1));
}
static void *vgic_debug_start(struct seq_file *s, loff_t *pos)
@@ -130,6 +171,7 @@ static void vgic_debug_stop(struct seq_file *s, void *v)
mutex_lock(&kvm->lock);
iter = kvm->arch.vgic.iter;
+ kfree(iter->lpi_irqs);
kfree(iter);
kvm->arch.vgic.iter = NULL;
mutex_unlock(&kvm->lock);
@@ -154,7 +196,7 @@ static void print_header(struct seq_file *s, struct vgic_irq *irq,
struct kvm_vcpu *vcpu)
{
int id = 0;
- char *hdr = "SPI ";
+ char *hdr = "S/LPI ";
if (vcpu) {
hdr = "VCPU";
@@ -162,7 +204,10 @@ static void print_header(struct seq_file *s, struct vgic_irq *irq,
}
seq_printf(s, "\n");
- seq_printf(s, "%s%2d TYP ID TGT_ID PLAEHC HWID TARGET SRC PRI VCPU_ID\n", hdr, id);
+ if (vcpu)
+ seq_printf(s, "%s%2d TYP ID TGT_ID PLAEHC HWID TARGET SRC PRI VCPU_ID\n", hdr, id);
+ else
+ seq_printf(s, "%s TYP ID TGT_ID PLAEHC HWID TARGET SRC PRI VCPU_ID\n", hdr);
seq_printf(s, "---------------------------------------------------------------\n");
}
@@ -174,8 +219,10 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
type = "SGI";
else if (irq->intid < VGIC_NR_PRIVATE_IRQS)
type = "PPI";
- else
+ else if (irq->intid < VGIC_MAX_SPI)
type = "SPI";
+ else if (irq->intid >= VGIC_MIN_LPI)
+ type = "LPI";
if (irq->intid ==0 || irq->intid == VGIC_NR_PRIVATE_IRQS)
print_header(s, irq, vcpu);
@@ -220,7 +267,9 @@ static int vgic_debug_show(struct seq_file *s, void *v)
if (!kvm->arch.vgic.initialized)
return 0;
- if (iter->vcpu_id < iter->nr_cpus) {
+ if (iter->intid >= VGIC_MIN_LPI)
+ irq = iter->lpi_irqs[iter->lpi_print_count - 1];
+ else if (iter->vcpu_id < iter->nr_cpus) {
vcpu = kvm_get_vcpu(kvm, iter->vcpu_id);
irq = &vcpu->arch.vgic_cpu.private_irqs[iter->intid];
} else {
@@ -230,6 +279,8 @@ static int vgic_debug_show(struct seq_file *s, void *v)
spin_lock(&irq->irq_lock);
print_irq_state(s, irq, vcpu);
spin_unlock(&irq->irq_lock);
+ if (iter->intid >= VGIC_MIN_LPI)
+ vgic_put_irq(kvm, irq);
return 0;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] KVM: arm/arm64 : add lpi info in vgic-debug
2018-03-23 10:36 Peng Hao
@ 2018-03-23 11:20 ` Marc Zyngier
0 siblings, 0 replies; 4+ messages in thread
From: Marc Zyngier @ 2018-03-23 11:20 UTC (permalink / raw)
To: Peng Hao, christoffer.dall; +Cc: linux-kernel, kvmarm, linux-arm-kernel
On 23/03/18 10:36, Peng Hao wrote:
> Add lpi debug info to vgic-stat.
> the printed info like this:
> SPI 287 0 000001 0 0 0 160 -1
> LPI 8192 2 000100 0 0 0 160 -1
>
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> ---
> virt/kvm/arm/vgic/vgic-debug.c | 61 ++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 56 insertions(+), 5 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-debug.c b/virt/kvm/arm/vgic/vgic-debug.c
> index 10b3817..444115e 100644
> --- a/virt/kvm/arm/vgic/vgic-debug.c
> +++ b/virt/kvm/arm/vgic/vgic-debug.c
> @@ -36,9 +36,12 @@
> struct vgic_state_iter {
> int nr_cpus;
> int nr_spis;
> + int nr_lpis;
> int dist_id;
> int vcpu_id;
> int intid;
> + int lpi_print_count;
> + struct vgic_irq **lpi_irqs;
> };
>
> static void iter_next(struct vgic_state_iter *iter)
> @@ -52,6 +55,40 @@ static void iter_next(struct vgic_state_iter *iter)
> if (iter->intid == VGIC_NR_PRIVATE_IRQS &&
> ++iter->vcpu_id < iter->nr_cpus)
> iter->intid = 0;
> +
> + if (iter->intid >= VGIC_NR_PRIVATE_IRQS + iter->nr_spis) {
> + if (iter->lpi_print_count < iter->nr_lpis)
> + iter->intid = iter->lpi_irqs[iter->lpi_print_count]->intid;
> + iter->lpi_print_count++;
> + }
> +}
> +
> +static void vgic_debug_get_lpis(struct kvm *kvm, struct vgic_state_iter *iter)
> +{
> + struct vgic_dist *dist = &kvm->arch.vgic;
> + int i = 0;
> + struct vgic_irq *irq = NULL, **lpi_irqs;
> +
> +again:
> + iter->nr_lpis = dist->lpi_list_count;
> + lpi_irqs = kmalloc_array(iter->nr_lpis, sizeof(irq), GFP_KERNEL);
> + if (!lpi_irqs) {
> + iter->nr_lpis = 0;
> + return;
> + }
> + spin_lock(&dist->lpi_list_lock);
> + if (iter->nr_lpis != dist->lpi_list_count) {
> + kfree(lpi_irqs);
> + spin_unlock(&dist->lpi_list_lock);
> + goto again;
> + }
Why do we need an exact count? It is fine to have a transient count, and
the debug code should be able to come with that without performing this
terrible loop.
We also already have some code that snapshot the the LPIs (see
vgic_copy_lpi_list), so please consider reusing that instead.
> +
> + list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) {
> + vgic_get_irq_kref(irq);
> + lpi_irqs[i++] = irq;
> + }
> + spin_unlock(&dist->lpi_list_lock);
> + iter->lpi_irqs = lpi_irqs;
Messing with the internals of the refcounts is really a bad idea. Please
use vgic_get_irq() in conjunction with the above, and allow it to fail
gracefully.
> }
>
> static void iter_init(struct kvm *kvm, struct vgic_state_iter *iter,
> @@ -64,6 +101,8 @@ static void iter_init(struct kvm *kvm, struct vgic_state_iter *iter,
> iter->nr_cpus = nr_cpus;
> iter->nr_spis = kvm->arch.vgic.nr_spis;
>
> + if (vgic_supports_direct_msis(kvm) && !pos)
> + vgic_debug_get_lpis(kvm, iter);
> /* Fast forward to the right position if needed */
> while (pos--)
> iter_next(iter);
> @@ -73,7 +112,9 @@ static bool end_of_vgic(struct vgic_state_iter *iter)
> {
> return iter->dist_id > 0 &&
> iter->vcpu_id == iter->nr_cpus &&
> - (iter->intid - VGIC_NR_PRIVATE_IRQS) == iter->nr_spis;
> + (iter->intid - VGIC_NR_PRIVATE_IRQS) >= iter->nr_spis &&
> + ((iter->nr_lpis == 0) ||
> + (iter->lpi_print_count == iter->nr_lpis + 1));
> }
>
> static void *vgic_debug_start(struct seq_file *s, loff_t *pos)
> @@ -130,6 +171,7 @@ static void vgic_debug_stop(struct seq_file *s, void *v)
>
> mutex_lock(&kvm->lock);
> iter = kvm->arch.vgic.iter;
> + kfree(iter->lpi_irqs);
> kfree(iter);
> kvm->arch.vgic.iter = NULL;
> mutex_unlock(&kvm->lock);
> @@ -154,7 +196,7 @@ static void print_header(struct seq_file *s, struct vgic_irq *irq,
> struct kvm_vcpu *vcpu)
> {
> int id = 0;
> - char *hdr = "SPI ";
> + char *hdr = "S/LPI ";
>
> if (vcpu) {
> hdr = "VCPU";
> @@ -162,7 +204,10 @@ static void print_header(struct seq_file *s, struct vgic_irq *irq,
> }
>
> seq_printf(s, "\n");
> - seq_printf(s, "%s%2d TYP ID TGT_ID PLAEHC HWID TARGET SRC PRI VCPU_ID\n", hdr, id);
> + if (vcpu)
> + seq_printf(s, "%s%2d TYP ID TGT_ID PLAEHC HWID TARGET SRC PRI VCPU_ID\n", hdr, id);
> + else
> + seq_printf(s, "%s TYP ID TGT_ID PLAEHC HWID TARGET SRC PRI VCPU_ID\n", hdr);
This feels like an unnecessary change. But if you really want that kind
of detail, change your "S/LPI" to say something more generic, such as
"Global".
> seq_printf(s, "---------------------------------------------------------------\n");
> }
>
> @@ -174,8 +219,10 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq,
> type = "SGI";
> else if (irq->intid < VGIC_NR_PRIVATE_IRQS)
> type = "PPI";
> - else
> + else if (irq->intid < VGIC_MAX_SPI)
> type = "SPI";
> + else if (irq->intid >= VGIC_MIN_LPI)
> + type = "LPI";
>
> if (irq->intid ==0 || irq->intid == VGIC_NR_PRIVATE_IRQS)
> print_header(s, irq, vcpu);
> @@ -220,7 +267,9 @@ static int vgic_debug_show(struct seq_file *s, void *v)
> if (!kvm->arch.vgic.initialized)
> return 0;
>
> - if (iter->vcpu_id < iter->nr_cpus) {
> + if (iter->intid >= VGIC_MIN_LPI)
> + irq = iter->lpi_irqs[iter->lpi_print_count - 1];
> + else if (iter->vcpu_id < iter->nr_cpus) {
> vcpu = kvm_get_vcpu(kvm, iter->vcpu_id);
> irq = &vcpu->arch.vgic_cpu.private_irqs[iter->intid];
> } else {
> @@ -230,6 +279,8 @@ static int vgic_debug_show(struct seq_file *s, void *v)
> spin_lock(&irq->irq_lock);
> print_irq_state(s, irq, vcpu);
> spin_unlock(&irq->irq_lock);
> + if (iter->intid >= VGIC_MIN_LPI)
> + vgic_put_irq(kvm, irq);
If you adopt the scheme I outlined above, you can have a balanced
get/put behaviour, irrespective of the interrupt type, and a much nicer
result.
>
> return 0;
> }
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-03-23 14:45 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-03-23 13:33 Re:Re: [PATCH] KVM: arm/arm64 : add lpi info in vgic-debug peng.hao2
2018-03-23 14:45 ` Marc Zyngier
-- strict thread matches above, loose matches on Subject: below --
2018-03-23 10:36 Peng Hao
2018-03-23 11:20 ` Marc Zyngier
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox