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

Re: [PATCH v3] xen/arm: vgic to ignore GICD ICPENDRn registers access



Hi Ian,

On 20/10/2021 14:30, Ian Jackson wrote:
~Bertrand Marquis writes ("Re: [PATCH v3] xen/arm: vgic to ignore GICD ICPENDRn 
registers access"):
[+Ian]
On 20 Oct 2021, at 11:10, Hongda Deng <Hongda.Deng@xxxxxxx> wrote:

Currently, Xen will return IO unhandled when guests access GICD ICPENRn
registers. This will raise a data abort inside guest. For Linux Guest,
these virtual registers will not be accessed. But for Zephyr, in its
GIC initialization code, these virtual registers will be accessed. And
zephyr guest will get an IO data abort in initilization stage and enter
fatal error. Emulating ICPENDR is not easy with the existing vGIC, so
we currently ignore these virtual registers access and print a message
about whether they are already pending instead of returning unhandled.
More details can be found at [1].

[1] https://github.com/zephyrproject-rtos/zephyr/blob/eaf6cf745df3807e6e
cc941c3a30de6c179ae359/drivers/interrupt_controller/intc_gicv3.c#L274

Signed-off-by: Hongda Deng <hongda.deng@xxxxxxx>
Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>

Ian this is fixing a bug in the vgic implementation which is preventing to run
Zephyr as a guest on top of Xen. Xen support has now been merged in Zephyr
so this is required to use it.

Could we consider adding this patch into the 4.16 release ?

Hi.  I'm definitely open to the idea, especially if it goes in soon.

I think this needs an ARM maintainer review, still ?

Yes. I am planning to review it later today.


It doesn't seem entirely straightforward.  I'd like to hear from the
maintainer, to confirm that they agree it's a bugfix, and to get an
idea of the risks vs benefits of this patch.

TL;DR: This is a bug fix and I agree that this should be included in 4.16.

ICPENDRn are a mandatory registers of the GIC implementation. But it is not trivial to emulate properly with our existing vGIC. So for the past years, we chose the lazy approach and inject a data abort when the vCPU access it.

IOW, this is not a new bug fix. We haven't seen any problem before because most of our users were using Linux based guests. Now this is starting to change and therefore we are exercising paths that Linux never used it. In this case, we would not be able to boot Zephyr on Xen.

During boot, Zephyr will write to ICPENDR to clear all the pending interrupts. I am not entirely convinced that from Zephyr PoV this is a useful things to do because, unless you quiesced the devices, interrupts can become pending again right away after clearing.

I would suggest to chat with the Zephyr folks to understand why they need to write to ICPENDR during boot.

In any case, I am assuming there are already Zephyr OS out there. So we need to solve the issue in Xen.

This patch doesn't fully emulate ICPENDR. The new appropach will ignore access and print a message when the OS is trying to clear a pending interrupt.

The code itself is only walking the internal structure. So as long as the correct locks are held, there is no change in the emulated state.

The only difference will happen at the domain level. Now, the domain will be able to continue booting. We will not clear pending interrupts but I think this is an acceptable approach as the worst that can happen is the guest may receive a "spurious" interrupt.

In both cases, I think the risks are limited and I would support the inclusion of this patch (pending appropriate acks) in 4.16.

Cheers,

--
Julien Grall



 


Rackspace

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