Re: [PATCH for-4.15] xen/vgic: Implement write to ISPENDR in vGICv{2, 3}

[Julien Grall <julien@xen.org>]

On 22/02/2021 13:45, Bertrand Marquis wrote:
> Hi Julien,
> 
>> On 20 Feb 2021, at 14:04, Julien Grall <julien@xen.org> wrote:
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Currently, Xen will send a data abort to a guest trying to write to the
>> ISPENDR.
>>
>> Unfortunately, recent version of Linux (at least 5.9+) will start
>> writing to the register if the interrupt needs to be re-triggered
>> (see the callback irq_retrigger). This can happen when a driver (such as
>> the xgbe network driver on AMD Seattle) re-enable an interrupt:
>>
>> (XEN) d0v0: vGICD: unhandled word write 0x00000004000000 to ISPENDR44
>> [...]
>> [   25.635837] Unhandled fault at 0xffff80001000522c
>> [...]
>> [   25.818716]  gic_retrigger+0x2c/0x38
>> [   25.822361]  irq_startup+0x78/0x138
>> [   25.825920]  __enable_irq+0x70/0x80
>> [   25.829478]  enable_irq+0x50/0xa0
>> [   25.832864]  xgbe_one_poll+0xc8/0xd8
>> [   25.836509]  net_rx_action+0x110/0x3a8
>> [   25.840328]  __do_softirq+0x124/0x288
>> [   25.844061]  irq_exit+0xe0/0xf0
>> [   25.847272]  __handle_domain_irq+0x68/0xc0
>> [   25.851442]  gic_handle_irq+0xa8/0xe0
>> [   25.855171]  el1_irq+0xb0/0x180
>> [   25.858383]  arch_cpu_idle+0x18/0x28
>> [   25.862028]  default_idle_call+0x24/0x5c
>> [   25.866021]  do_idle+0x204/0x278
>> [   25.869319]  cpu_startup_entry+0x24/0x68
>> [   25.873313]  rest_init+0xd4/0xe4
>> [   25.876611]  arch_call_rest_init+0x10/0x1c
>> [   25.880777]  start_kernel+0x5b8/0x5ec
>>
>> As a consequence, the OS may become unusable.
>>
>> Implementing the write part of ISPENDR is somewhat easy. For
>> virtual interrupt, we only need to inject the interrupt again.
>>
>> For physical interrupt, we need to be more careful as the de-activation
>> of the virtual interrupt will be propagated to the physical distributor.
>> For simplicity, the physical interrupt will be set pending so the
>> workflow will not differ from a "real" interrupt.
>>
>> Longer term, we could possible directly activate the physical interrupt
>> and avoid taking an exception to inject the interrupt to the domain.
>> (This is the approach taken by the new vGIC based on KVM).
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> This is something which will not be done by a guest very often so I think your
> implementation actually makes it simpler and reduce possibilities of race conditions
> so I am not even sure that the XXX comment is needed.

I think the XXX is useful as if someone notice an issue with the code, 
then they know what they could try.

I am open to suggestion how we could keep track of potential improvement.

> But i am ok with it being in or not so:
> 
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Thanks!

Cheers,
-- 
Julien Grall