Re: [PATCH v5 23/23] KVM: arm64: GICv4.1: Expose HW-based SGIs in debugfs

From: Marc Zyngier <maz@kernel.org>
To: Auger Eric <eric.auger@redhat.com>
Cc: linux-arm-kernel@lists.infradead.org,
	kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Jason Cooper <jason@lakedaemon.net>,
	Robert Richter <rrichter@marvell.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Zenghui Yu <yuzenghui@huawei.com>,
	James Morse <james.morse@arm.com>,
	Julien Thierry <julien.thierry.kdev@gmail.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>
Subject: Re: [PATCH v5 23/23] KVM: arm64: GICv4.1: Expose HW-based SGIs in debugfs
Date: Thu, 19 Mar 2020 15:21:42 +0000	[thread overview]
Message-ID: <45c282bddd43420024633943c1befac3@kernel.org> (raw)
In-Reply-To: <4cb4c3d4-7b02-bb77-cd7a-c185346b6a2f@redhat.com>

Hi Eric,

On 2020-03-19 15:05, Auger Eric wrote:
> Hi Marc,
> 
> On 3/4/20 9:33 PM, Marc Zyngier wrote:
>> The vgic-state debugfs file could do with showing the pending state
>> of the HW-backed SGIs. Plug it into the low-level code.
>> 
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  virt/kvm/arm/vgic/vgic-debug.c | 14 +++++++++++++-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
>> 
>> diff --git a/virt/kvm/arm/vgic/vgic-debug.c 
>> b/virt/kvm/arm/vgic/vgic-debug.c
>> index cc12fe9b2df3..b13a9e3f99dd 100644
>> --- a/virt/kvm/arm/vgic/vgic-debug.c
>> +++ b/virt/kvm/arm/vgic/vgic-debug.c
>> @@ -178,6 +178,8 @@ static void print_irq_state(struct seq_file *s, 
>> struct vgic_irq *irq,
>>  			    struct kvm_vcpu *vcpu)
>>  {
>>  	char *type;
>> +	bool pending;
> nit: can be directly initialized to irq->pending_latch
>> +
>>  	if (irq->intid < VGIC_NR_SGIS)
>>  		type = "SGI";
>>  	else if (irq->intid < VGIC_NR_PRIVATE_IRQS)
>> @@ -190,6 +192,16 @@ static void print_irq_state(struct seq_file *s, 
>> struct vgic_irq *irq,
>>  	if (irq->intid ==0 || irq->intid == VGIC_NR_PRIVATE_IRQS)
>>  		print_header(s, irq, vcpu);
>> 
>> +	pending = irq->pending_latch;
>> +	if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
>> +		int err;
>> +
>> +		err = irq_get_irqchip_state(irq->host_irq,
>> +					    IRQCHIP_STATE_PENDING,
>> +					    &pending);
>> +		WARN_ON_ONCE(err);
>> +	}
>> +
>>  	seq_printf(s, "       %s %4d "
>>  		      "    %2d "
>>  		      "%d%d%d%d%d%d%d "
>> @@ -201,7 +213,7 @@ static void print_irq_state(struct seq_file *s, 
>> struct vgic_irq *irq,
>>  		      "\n",
>>  			type, irq->intid,
>>  			(irq->target_vcpu) ? irq->target_vcpu->vcpu_id : -1,
>> -			irq->pending_latch,
>> +			pending,
>>  			irq->line_level,
>>  			irq->active,
>>  			irq->enabled,
>> 
> The patch looks good to me but I am now lost about how we retrieve the
> pending stat of other hw mapped interrupts. Looks we use
> irq->pending_latch always. Is that correct?

Correct. GICv4.0 doesn't give us an architectural way to look at the
vLPI pending state (there isn't even a guarantee about when the GIC
will stop writing to memory, if it ever does).

With GICv4.1, you can introspect the HW state for SGIs. You can also
look at the vLPI state by peeking at the virtual pending table, but
you'd need to unmap the VPE first, which I obviously don't want to do
for this debug interface, specially as it can be used whilst the guest
is up and running.

In the future, we'll have to implement that in order to support guest
save/restore from a GICv4.1 system. I haven't given much thought to it
though.

> For the patch:
> Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...