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

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello,

please find the view comment inline:

Thanks & Regards
Sharan

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.
> > Cheers,
> >
> > --
> > Julien Grall
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel