Re: [RESEND PATCH] KVM: arm: VGIC: properly initialise private IRQ affinity

From: Marc Zyngier <marc.zyngier@arm.com>
To: Andre Przywara <andre.przywara@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>
Cc: Dave Martin <dave.martin@arm.com>,
	Julien Grall <julien.grall@arm.com>,
	kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [RESEND PATCH] KVM: arm: VGIC: properly initialise private IRQ affinity
Date: Thu, 22 Aug 2019 14:48:03 +0100	[thread overview]
Message-ID: <b00e47b9-bbe0-f30f-a2e4-f6265ca9d7ad@arm.com> (raw)
In-Reply-To: <20190822144224.12d51380@donnerap.cambridge.arm.com>

On 22/08/2019 14:42, Andre Przywara wrote:
> On Thu, 22 Aug 2019 01:13:32 +0800
> Zenghui Yu <yuzenghui@huawei.com> wrote:
> 
> Hi,
> 
>> On 2019/8/22 1:00, Andre Przywara wrote:
>>> At the moment we initialise the target *mask* of a virtual IRQ to the
>>> VCPU it belongs to, even though this mask is only defined for GICv2 and
>>> quickly runs out of bits for many GICv3 guests.
>>> This behaviour triggers an UBSAN complaint for more than 32 VCPUs:
>>> ------
>>> [ 5659.462377] UBSAN: Undefined behaviour in virt/kvm/arm/vgic/vgic-init.c:223:21
>>> [ 5659.471689] shift exponent 32 is too large for 32-bit type 'unsigned int'
>>> ------
>>> Also for GICv3 guests the reporting of TARGET in the "vgic-state" debugfs
>>> dump is wrong, due to this very same problem.
>>>
>>> Fix both issues by only initialising vgic_irq->targets for a vGICv2 guest,
>>> and by initialising vgic_irq->mpdir for vGICv3 guests instead. We can't
>>> use the actual MPIDR for that, as the VCPU's system register is not
>>> initialised at this point yet. This is not really an issue, as ->mpidr
>>> is just used for the debugfs output and the IROUTER MMIO register, which
>>> does not exist in redistributors (dealing with SGIs and PPIs).
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> Reported-by: Dave Martin <dave.martin@arm.com>
>>> ---
>>> Hi,
>>>
>>> this came up here again, I think it fell through the cracks back in
>>> March:
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2019-March/637209.html
>>>
>>> Cheers,
>>> Andre.
>>>
>>>   virt/kvm/arm/vgic/vgic-init.c | 9 ++++++---
>>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
>>> index 80127ca9269f..8bce2f75e0c1 100644
>>> --- a/virt/kvm/arm/vgic/vgic-init.c
>>> +++ b/virt/kvm/arm/vgic/vgic-init.c
>>> @@ -210,7 +210,6 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>>>   		irq->intid = i;
>>>   		irq->vcpu = NULL;
>>>   		irq->target_vcpu = vcpu;
>>> -		irq->targets = 1U << vcpu->vcpu_id;
>>>   		kref_init(&irq->refcount);
>>>   		if (vgic_irq_is_sgi(i)) {
>>>   			/* SGIs */
>>> @@ -221,10 +220,14 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>>>   			irq->config = VGIC_CONFIG_LEVEL;
>>>   		}
>>>   
>>> -		if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
>>> +		if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {  
>>
>> I still think that if user-space create VCPUs before vGIC (like what
>> Qemu does), the actual vGIC model will be unknown here. The UBSAN
>> warning will still show up when booting a vGIC-v3 guest (with Qemu).
> 
> Yes, you are right. I vaguely remembered this issue, but couldn't
> find anything on the list about it. So thanks for the heads up!
> 
> So I think I have a solution, where we drop the initialisation part
> here altogether, instead initialise mpdir/targets together with the
> group in vgic_init(). Unless there is some code which needs
> irq->group before that point?

You may want to check save/restore of a GICv3 guest using a mix of
group-0/1 interrupts. I seem to remember some breakage in that area.

> Will send a patch shortly.

OK. In the meantime, I'll drop your initial patch.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm