Re: [PATCH v4 0/9] KVM: arm64: Initial host support for the Apple M1

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi Marc,

On 6/22/21 4:51 PM, Marc Zyngier wrote:
> Hi Alex,
>
> On Tue, 22 Jun 2021 16:39:11 +0100,
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>> Hi Marc,
>>
>> On 6/1/21 11:39 AM, Marc Zyngier wrote:
>>> This is a new version of the series previously posted at [3], reworking
>>> the vGIC and timer code to cope with the M1 braindead^Wamusing nature.
>>>
>>> Hardly any change this time around, mostly rebased on top of upstream
>>> now that the dependencies have made it in.
>>>
>>> Tested with multiple concurrent VMs running from an initramfs.
>>>
>>> Until someone shouts loudly now, I'll take this into 5.14 (and in
>>> -next from tomorrow).
>> I am not familiar with irqdomains or with the irqchip
>> infrastructure, so I can't really comment on patch #8.
>>
>> I tried testing this with a GICv3 by modifying the driver to set
>> no_hw_deactivation and no_maint_irq_mask:
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
>> index 340c51d87677..d0c6f808d7f4 100644
>> --- a/arch/arm64/kvm/vgic/vgic-init.c
>> +++ b/arch/arm64/kvm/vgic/vgic-init.c
>> @@ -565,8 +565,10 @@ int kvm_vgic_hyp_init(void)
>>         if (ret)
>>                 return ret;
>>  
>> +       /*
>>         if (!has_mask)
>>                 return 0;
>> +               */
>>  
>>         ret = request_percpu_irq(kvm_vgic_global_state.maint_irq,
>>                                  vgic_maintenance_handler,
>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>> index 453fc425eede..9ce4dee20655 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -1850,6 +1850,12 @@ static void __init gic_of_setup_kvm_info(struct device_node
>> *node)
>>         if (!ret)
>>                 gic_v3_kvm_info.vcpu = r;
>>  
>> +       gic_v3_kvm_info.no_hw_deactivation = true;
> Blink...
>
>> +       gic_v3_kvm_info.no_maint_irq_mask = true;
>> +
>> +       vgic_set_kvm_info(&gic_v3_kvm_info);
>> +       return;
>> +
>>         gic_v3_kvm_info.has_v4 = gic_data.rdists.has_vlpis;
>>         gic_v3_kvm_info.has_v4_1 = gic_data.rdists.has_rvpeid;
>>         vgic_set_kvm_info(&gic_v3_kvm_info);
>>
>> Kept the maintenance irq ID so the IRQ gets enabled at the
>> Redistributor level. I don't know if I managed to break something
>> with those changes, but when testing on the model and on a rockpro64
>> (with the patches cherry-picked on top of v5.13-rc7) I kept seeing
>> rcu stalls. I assume I did something wrong.
> If you do that, the interrupts that are forwarded to the guest
> (timers) will never be deactivated, and will be left dangling after
> the first injection. This is bound to create havoc, as we will then
> use mask/unmask to control the timer delivery (remember that the
> Active state is just another form of auto-masking on top of the
> standard enable bit)
>
> On the contrary, the AIC only has a single bit to control the timer
> (used as a mask), which is what the irqdomain stuff implements to
> mimic the active state.

So these patches work **only** with the AIC, not with a standard GICv3 without the
HW bit in the LR registers and with an unmaskable maintenance IRQ? Because from
the commit message from #8 I got the impression that the purpose of the change is
to make timers work on a standard GICv3, sans those required architectural features.

Thanks,

Alex

>
> Thanks,
>
> 	M.
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel