Re: [Minios-devel] [UNIKRAFT RFC PATCH] Implement PL031 RTC library for Arm

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

Hi,

> -----Original Message-----
> From: Julien Grall <julien.grall@xxxxxxx>
> Sent: Monday, November 12, 2018 6:37 PM
> To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@xxxxxxx>; minios-
> devel@xxxxxxxxxxxxxxxxxxxx; simon.kuenzer@xxxxxxxxx
> Cc: Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>; nd
> <nd@xxxxxxx>; Wei Chen (Arm Technology China) <Wei.Chen@xxxxxxx>
> Subject: Re: [Minios-devel] [UNIKRAFT RFC PATCH] Implement PL031 RTC
> library for Arm
> 
> 
> 
> On 11/11/18 1:59 AM, Jianyong Wu (Arm Technology China) wrote:
> > Hi,
> 
> Hi,
> 
> >>> +void rtc_clear_intr(void)
> >>> +{
> >>> + ioreg_write32(RTC_REG(RTC_REG_ICR), 1); }
> >>> +
> >>> +int _dtb_init_rtc(void *dtb)
> >>> +{
> >>> + uint32_t idx;
> >>> + int fdt_rtc, naddr, nsize, size, prop_len, prop_min_len;
> >>> + const uint64_t *regs;
> >>> +
> >>> + uk_printd(DLVL_INFO, "Probing RTC...\n");
> >>> + fdt_rtc = uk_dtb_find_device(dtb, rtc_device_list,
> >>> +         sizeof(rtc_device_list));
> >>
> >> I would not assume the RTC is always present.
> >
> > The check is inside uk_dtb_read_region.
> 
> Your check is an ASSERT(...). This means that if an RTC is not present then 
> you
> will get Unikraft crashed in debug build.
> 
> On non-debug build, it will just continue silently.
> 
> In both case, I don't think this is the correct behavior. If the RTC is a
> mandatory device, then it should be an error/crash in appropriate place
> (from uk_dtb_read_region it is not very obvious what failed).
> 
> If the RTC is not mandatory, then you should return here and not configured
> the RTC.
> 

Ok, I will check fdt_rtc and give a crash if there is no that device.

> >>
> >>> + naddr = uk_dtb_read_region(dtb, fdt_rtc, &nsize, &regs);
> >>> + rtc_base_addr = uk_dtb_read_term(regs, 0, naddr, nsize, &size);
> >>
> >> Similarly here, I would not assume uk_dtb_read_term() will always
> succeed.
> >> It would make the code more safe if the DT passed is wrong.
> >
> >
> > Check has been done in this function.
> 
> I am afraid this is not the case. If you look at your function there are no 
> check
> whether the region N you are trying to access is in the device-tree.
> 
> There are an ASSERT(...) in uk_dtb_read_region but that's only make sure
> "regs" describes at least one region.
> 
Ok, I will check rtc_base_addr.
 
> >>
> >>> +
> >>> + /* Record the boot seconds */
> >>> + rtc_boot_seconds = rtc_read();
> >>> +
> >>> + uk_printd(DLVL_INFO, "Found RTC on: %p\n", rtc_base_addr);
> >>
> >> I am slightly surprised this compile in Unikraft. This function is
> >> meant to return an integer but I don't see any return here.
> >
> > Which function? Rtc_read()? Check it again.
> 
> Please tone down your writing if you want people to continue reviewing your
> code.
> 
> In that particular case, my comment was at the end of _dtb_init_rtc. The
> prototype of the function is:
> 
> int _dtb_init_rtc(void *dtb)
> 
> That function return an int but I can't see a 'return X' in the code. It seems
> Unikraft does not use -Werror flags, which means this kind of error would be
> only a warning. Yet the return value will be unknown.
> 
> Regarding -Werror, I think Unikraft community should consider use it as a lot
> of warning can be harmful in long run.

Oh, I'm sorry. It's not my intention to offend you. Words does not convey the 
exact emotion of the talker.
Anyway I will watch out of my words. I am really happy to have my patch 
reviewed by someone like you. Thanks.

 Yeah, there is really my fault to miss that return statement. I will add it.

Bests
Jianyong wu
> 
> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel