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

[Julien Grall <julien@xxxxxxx>]

On 22/02/2021 13:45, Bertrand Marquis wrote:
> Hi Julien,
>
> > On 20 Feb 2021, at 14:04, Julien Grall <julien@xxxxxxx> wrote:
> >
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> 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@xxxxxxx>
Thanks!

Cheers,
--
Julien Grall