On 20/01/17 20:07, Christoffer Dall wrote
Hi Christoffer,
> On Fri, Jan 20, 2017 at 06:07:26PM +0000, Andre Przywara wrote:
>> Hej Christoffer,
>>
>> On 20/01/17 10:33, Christoffer Dall wrote:
>>> Add a file to debugfs to read the in-kernel state of the vgic. We don't
>>> do any locking of the entire VGIC state while traversing all the IRQs,
>>> so if the VM is running the user/developer may not see a quiesced state,
>>> but should take care to pause the VM using facilities in user space for
>>> that purpose.
>>>
>>> We also don't support LPIs yet, but they can be added easily if needed.
>>>
>>> Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
>>> ---
>>> arch/arm/kvm/Makefile | 1 +
>>> arch/arm64/kvm/Makefile | 1 +
>>> include/kvm/arm_vgic.h | 5 +
>>> virt/kvm/arm/vgic/vgic-debug.c | 278 +++++++++++++++++++++++++++++++++++++++++
>>> virt/kvm/arm/vgic/vgic-init.c | 4 +
>>> virt/kvm/arm/vgic/vgic.h | 3 +
>>> 6 files changed, 292 insertions(+)
>>> create mode 100644 virt/kvm/arm/vgic/vgic-debug.c
>>>
>>> diff --git a/arch/arm/kvm/Makefile b/arch/arm/kvm/Makefile
>>> index d571243..12b6281 100644
>>> --- a/arch/arm/kvm/Makefile
>>> +++ b/arch/arm/kvm/Makefile
>>> @@ -33,5 +33,6 @@ obj-y += $(KVM)/arm/vgic/vgic-mmio-v2.o
>>> obj-y += $(KVM)/arm/vgic/vgic-mmio-v3.o
>>> obj-y += $(KVM)/arm/vgic/vgic-kvm-device.o
>>> obj-y += $(KVM)/arm/vgic/vgic-its.o
>>> +obj-y += $(KVM)/arm/vgic/vgic-debug.o
>>> obj-y += $(KVM)/irqchip.o
>>> obj-y += $(KVM)/arm/arch_timer.o
>>> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>>> index d50a82a..e025bec 100644
>>> --- a/arch/arm64/kvm/Makefile
>>> +++ b/arch/arm64/kvm/Makefile
>>> @@ -31,6 +31,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio-v2.o
>>> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio-v3.o
>>> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-kvm-device.o
>>> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-its.o
>>> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-debug.o
>>> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/irqchip.o
>>> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arch_timer.o
>>> kvm-$(CONFIG_KVM_ARM_PMU) += $(KVM)/arm/pmu.o
>>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>>> index 002f092..1087aee 100644
>>> --- a/include/kvm/arm_vgic.h
>>> +++ b/include/kvm/arm_vgic.h
>>> @@ -165,6 +165,8 @@ struct vgic_its {
>>> struct list_head collection_list;
>>> };
>>>
>>> +struct vgic_state_iter;
>>> +
>>> struct vgic_dist {
>>> bool in_kernel;
>>> bool ready;
>>> @@ -212,6 +214,9 @@ struct vgic_dist {
>>> spinlock_t lpi_list_lock;
>>> struct list_head lpi_list_head;
>>> int lpi_list_count;
>>> +
>>> + /* used by vgic-debug */
>>> + struct vgic_state_iter *iter;
>>> };
>>>
>>> struct vgic_v2_cpu_if {
>>> diff --git a/virt/kvm/arm/vgic/vgic-debug.c b/virt/kvm/arm/vgic/vgic-debug.c
>>> new file mode 100644
>>> index 0000000..76e8b11
>>> --- /dev/null
>>> +++ b/virt/kvm/arm/vgic/vgic-debug.c
>>> @@ -0,0 +1,278 @@
>>> +/*
>>> + * Copyright (C) 2016 Linaro
>>> + * Author: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#include <linux/cpu.h>
>>> +#include <linux/debugfs.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/kvm_host.h>
>>> +#include <linux/seq_file.h>
>>> +#include <linux/uaccess.h>
>>> +#include <kvm/arm_vgic.h>
>>> +#include <asm/kvm_mmu.h>
>>> +#include "vgic.h"
>>> +
>>> +/*
>>> + * Structure to control looping through the entire vgic state. We start at
>>> + * zero for each field and move upwards. So, if dist_id is 0 we print the
>>> + * distributor info. When dist_id is 1, we have already printed it and move
>>> + * on.
>>> + *
>>> + * When vcpu_id < nr_cpus we print the vcpu info until vcpu_id == nr_cpus and
>>> + * so on.
>>> + */
>>> +struct vgic_state_iter {
>>> + int nr_cpus;
>>> + int nr_spis;
>>> + int dist_id;
>>> + int vcpu_id;
>>> + int intid;
>>> +};
>>> +
>>> +static void iter_next(struct vgic_state_iter *iter)
>>> +{
>>> + if (iter->dist_id == 0) {
>>> + iter->dist_id++;
>>> + return;
>>> + }
>>> +
>>> + iter->intid++;
>>> + if (iter->intid == VGIC_NR_PRIVATE_IRQS &&
>>> + ++iter->vcpu_id < iter->nr_cpus)
>>> + iter->intid = 0;
>>> +}
>>> +
>>> +static void iter_init(struct kvm *kvm, struct vgic_state_iter *iter,
>>> + loff_t pos)
>>> +{
>>> + int nr_cpus = atomic_read(&kvm->online_vcpus);
>>> +
>>> + memset(iter, 0, sizeof(*iter));
>>> +
>>> + iter->nr_cpus = nr_cpus;
>>> + iter->nr_spis = kvm->arch.vgic.nr_spis;
>>> +
>>> + /* Fast forward to the right position if needed */
>>> + while (pos--)
>>> + iter_next(iter);
>>> +}
>>> +
>>> +static bool the_end(struct vgic_state_iter *iter)
>>
>> "Of everything that stands, the end"? ;-)
>
> "It's the end of the world as we know it". How appropriate on this day.
And I was reviewing the patch to distract me from that ;-)
Any chance you can talk your son into running for president?
>> I wonder if we really need this. AFAICT the_end never returns true after
>> init has been called, so the only caller is after the iter_next(). So
>> can't we just close the Door(s) and fold the_end() into
>> iter_next()?
>>
>
> see below.
>
>>> +{
>>> + return iter->dist_id > 0 &&
>>> + iter->vcpu_id == iter->nr_cpus &&
>>> + (iter->intid - VGIC_NR_PRIVATE_IRQS) == iter->nr_spis;
>>> +}
>>> +
>>> +static void *debug_start(struct seq_file *s, loff_t *pos)
>>> +{
>>> + struct kvm *kvm = (struct kvm *)s->private;
>>> + struct vgic_state_iter *iter;
>>> +
>>> + mutex_lock(&kvm->lock);
>>> + iter = kvm->arch.vgic.iter;
>>> + if (iter) {
>>> + iter = ERR_PTR(-EBUSY);
>>> + goto out;
>>> + }
>>> +
>>> + iter = kmalloc(sizeof(*iter), GFP_KERNEL);
>>> + if (!iter) {
>>> + iter = ERR_PTR(-ENOMEM);
>>> + goto out;
>>> + }
>>> +
>>> + iter_init(kvm, iter, *pos);
>>> + kvm->arch.vgic.iter = iter;
>>> +
>>> + if (the_end(iter))
>>> + iter = NULL;
>>
>> I don't understand the need for that. If you have just initialized iter,
>> the_end() will never return true, as at least dist_id is always 0.
>> Or is there some magic (maybe future?) feature that I miss here?
>>
>
> This has file position semantics, so it's possible to call debug_start
> with a non-zero offset, and in fact possible to jump all the way ahead
> past the end of the file. In this case, this function must return NULL.
Arrg, sorry, I missed that pos. Shouldn't review "just this one patch
before leaving for the weekend" ...
> I base this on a number of examples out there (there's a recent one on
> lwn.net) and on trying to understand the seq file logic.
Which I admittedly still have to do.
>>> +out:
>>> + mutex_unlock(&kvm->lock);
>>> + return iter;
>>> +}
>>> +
>>> +static void *debug_next(struct seq_file *s, void *v, loff_t *pos)
>>> +{
>>> + struct kvm *kvm = (struct kvm *)s->private;
>>> + struct vgic_state_iter *iter = kvm->arch.vgic.iter;
>>> +
>>> + ++*pos;
>>> + iter_next(iter);
>>> + if (the_end(iter))
>>> + iter = NULL;
>>> + return iter;
>>> +}
>>> +
>>> +static void debug_stop(struct seq_file *s, void *v)
>>> +{
>>> + struct kvm *kvm = (struct kvm *)s->private;
>>> + struct vgic_state_iter *iter;
>>> +
>>> + mutex_lock(&kvm->lock);
>>> + iter = kvm->arch.vgic.iter;
>>> + kfree(iter);
>>> + kvm->arch.vgic.iter = NULL;
>>
>> Would it be better to set the kvm->iter pointer to NULL before we free
>> it? Or doesn't this matter as we are under the lock and the only other
>> code who cares is debug_start()?
>
> Yeah, that would be the definition of a critical section wouldn't it?
>
> I don't mind moving it around though, if it makes some
No, please leave it, I guess I was just confused why you had this iter
variable in the first place.
>> So far, need to wrap my mind around this sequential file operations
>> scheme first ...
>>
>
> Thanks for having a look. I don't think we need to overdo this one,
> it's a debug feature, it seems pretty stable, and you need root access
> to look at it, but of course it shouldn't be entirely shitty either.
No, I insist on some serious bike shedding here ;-)
I will test it quickly on Monday and the give you at least a Tested-by
(since I just disqualified myself for anything else).
Cheers,
Andre.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
