On Thu, 25 Feb 2021, Julien Grall wrote: > On 22/02/2021 13:45, Bertrand Marquis wrote: > > Hi Julien, > > > > > On 20 Feb 2021, at 14:04, Julien Grall <jul...@xen.org> wrote: > > > > > > From: Julien Grall <jgr...@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 <jgr...@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.
It is worth capturing it somewhere. Maybe the commit message is better than as an in-code comment? Either way it is fine by me and feel free to make that kind of change on commit.
