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

[Julien Grall <julien@xxxxxxx>]

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