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

[Julien Grall <julien.grall@xxxxxxx>]

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.
> > > +       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.
> > > +
> > > +       /* 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.
Cheers,

--
Julien Grall

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