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

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello,

On 3/22/19 7:23 PM, Julien Grall wrote:
> 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.
Misunderstood the idea. I agree with your approach.

> Assuming you don't enable the split EOI, the EOI will drop the priority 
> and de-activate it.
> Cheers,
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel