[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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 <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.

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.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.