Re: [Minios-devel] [UNIKRAFT PATCH 5/7] plat/kvm: Implement intctrl APIs for Arm64

[Julien Grall <julien.grall@xxxxxxx>]

On 22/03/2019 15:56, Sharan Santhanam wrote:
> Hello,
Hi,

> please find the view comment inline:
> On 12/18/18 5:51 AM, Wei Chen (Arm Technology China) wrote:
> > Hi Julien,
> >
> > > -----Original Message-----
> > > From: Julien Grall <julien.grall@xxxxxxxxxx>
> > > Sent: 2018年12月14日 18:51
> > > To: Wei Chen (Arm Technology China) <Wei.Chen@xxxxxxx>; minios-
> > > devel@xxxxxxxxxxxxxxxxxxxx; simon.kuenzer@xxxxxxxxx; florian.schmidt@xxxxxxxxx;
> > > yuri.volchkov@xxxxxxxxx; Sharan.Santhanam@xxxxxxxxx; Felipe.Huici@xxxxxxxxx
> > > Cc: Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>;
> > > Jianyong Wu (Arm Technology China) <Jianyong.Wu@xxxxxxx>; Justin He (Arm
> > > Technology China) <Justin.He@xxxxxxx>
> > > Subject: Re: [Minios-devel] [UNIKRAFT PATCH 5/7] plat/kvm: Implement intctrl
> > > APIs for Arm64
> > >
> > > Hi Wei,
> > >
> > > On 13/12/2018 09:18, Wei Chen wrote:
> > > > Before GICv2 become ready, we had marked the intctrl APIs as TODO.
> > > > Now, we have enabled the GICv2, we can implement intctrl APIs with
> > > > related GIC APIs.
> > > >
> > > > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > > > ---
> > > >    plat/kvm/arm/intctrl.c | 25 ++++++++++++++++++-------
> > > >    plat/kvm/arm/setup.c   |  4 ++++
> > > >    2 files changed, 22 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/plat/kvm/arm/intctrl.c b/plat/kvm/arm/intctrl.c
> > > > index ac604a7..0662159 100644
> > > > --- a/plat/kvm/arm/intctrl.c
> > > > +++ b/plat/kvm/arm/intctrl.c
> > > > @@ -31,24 +31,35 @@
> > > >     *
> > > >     * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> > > >     */
> > > > +#include <uk/assert.h>
> > > > +#include <kvm/kernel.h>
> > > >    #include <kvm/intctrl.h>
> > > > +#include <arm/cpu.h>
> > > > +#include <arm/irq.h>
> > > > +#include <arm/gic-v2.h>
> > > >
> > > >    void intctrl_init(void)
> > > >    {
> > > > -    // TO DO
> > > > +    int ret;
> > > > +
> > > > +    /* Initialize GIC from DTB */
> > > > +    ret = _dtb_init_gic(_libkvmplat_dtb);
> > > > +    if (ret)
> > > > +        UK_CRASH("Initialize GIC from DTB failed, ret=%d\n", ret);
> > > > +
> > > >    }
> > > >
> > > > -void intctrl_ack_irq(unsigned int irq)
> > > > +void intctrl_ack_irq(uint32_t irq)
> > > >    {
> > > > -    // TO DO
> > > > +    gic_eoi_irq(irq);
> > > I think you want this helper to be a NOP. Otherwise you may end up to EOI
> > > twice
> > > the same interrupts (see patch #7).
> > >
> > > You can't drop the one in patch #7 because this function may not be called
> > > resulting to block the interrupts forever on that processor.
> > I think your suggestion is better. I had been confused by call eoi twice.
> > I would make this API as NOP
> The handling of the interrupt has changed since the patch was submitted. the 
> handler calls intctrl_ack_irq at the end of the interrupt handler. With the 
> current implementation, I would rather remove the gic_eoi_irq in patch #7.
I don't think the function intctlr_ack_irq is suitable here (or the name is 
misleading). The acknowledge has already been done by the interrupt controller 
when reading the IAR.
Assuming you don't enable the split EOI, the EOI will drop the priority and 
de-activate it.
Cheers,

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel