Re: [Minios-devel] [UNIKRAFT PATCHv2 4/7] plat/common: Implement gic-v2 library for Arm

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 4/19/19 6:24 AM, Jianyong Wu (Arm Technology China) wrote:
> > > +/*
> > > + * @sgintid denotes the sgi ID;
> > > + * @targetfilter : this term is TargetListFilter
> > > + * 0 denotes forwarding interrupt to cpu specified in the
> > > + * target list; 1 denotes forwarding interrupt to cpu execpt the
> > > + * processor that request the interrupt; 2 denotes forwarding the
> > > + * interrupt only to the cpu that requtest the interrupt.
> > This is quite hard to read. How about introduce an enum so the caller don't
> > need to know the exact number?
> Em, This function will not be called by user directly as we have offered 3 APIs 
> below to handle
> these tough things. Also we offer macro to denotes the value of targetfilter.
While the user will not call that directly, a developer may need to 
modify this function and therefore understand what it does.
As you introduced define, you want the developer to use those defines 
rather than hardcoded value. Using the latter will only bring more 
confusion.
If you make targetfilter an enum rather than uint8_t, then your code 
becomes self-explanatory.
> > > + * targetlist. Targetlist is a 8-bit bitmap for 0~7 CPU.
> > > + * TODO: this will not work until SMP is supported
> > Most of the code in this file will not work for SMP :). For instance, you are
> > missing lock when dealing with the distributor. Also the Interface ID may not
> > match the CPUID.
> >
> > I pointed out in the past that it would be best to at least add the locking now
> > (even if they do nothing). This will be much easier than trying to retrofit it 
> > in a
> > couple of months.
> >
> > Anyway, I am not going to push for it.
> I am so sorry to let you down
> lock is really needed here. I will add it.
>   Please not give up on us.
It is not my plan ;). Usually when I write "I am not going to push for 
it" it means that this is not over-critical for this patch but the 
sooner it is fixed the better.
In this particular case, you don't have SMP support yet. So the 
spin_lock is not strictly necessary. However, it will be required as 
soon as you introduce SMP.
Cheers,

--
Julien Grall

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