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

["Jianyong Wu (Arm Technology China)" <Jianyong.Wu@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien.grall@xxxxxxx>
> Sent: Tuesday, April 23, 2019 5:23 PM
> To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@xxxxxxx>; Justin
> He (Arm Technology China) <Justin.He@xxxxxxx>; minios-
> devel@xxxxxxxxxxxxxxxxxxxx; Simon Kuenzer <simon.kuenzer@xxxxxxxxx>;
> Sharan.Santhanam@xxxxxxxxx
> Cc: Florian Schmidt <florian.schmidt@xxxxxxxxx>; Felipe Huici
> <felipe.huici@xxxxxxxxx>; yuri.volchkov@xxxxxxxxx; Kaly Xin (Arm
> Technology China) <Kaly.Xin@xxxxxxx>; Wei Chen (Arm Technology China)
> <Wei.Chen@xxxxxxx>
> Subject: Re: [UNIKRAFT PATCHv2 4/7] plat/common: Implement gic-v2 library
> for Arm
>
> 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.
>

Yeah, great, I will introduce enum instead of uint8.

> >>> + * 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.
>

It is really the case, very appreciate it.

Thanks
Jianyong Wu

> Cheers,
>
> --
> Julien Grall
IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel