Re: [Minios-devel] [UNIKRAFT PATCH v2 1/2] include/plat: Remove plat/common/include/memory.h

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

Hey all,

On 13.10.19 12:05, Costin Lupu wrote:
> On 10/12/19 5:19 PM, Justin He (Arm Technology China) wrote:
> > Hi Costin
> >
> > > -----Original Message-----
> > > From: Minios-devel <minios-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf
> > > Of Costin Lupu
> > > Sent: Saturday, October 12, 2019 7:12 PM
> > > To: Justin He (Arm Technology China) <Justin.He@xxxxxxx>; minios-
> > > devel@xxxxxxxxxxxxxxxxxxxx; Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> > > Cc: Felipe Huici <felipe.huici@xxxxxxxxx>; Kaly Xin (Arm Technology China)
> > > <Kaly.Xin@xxxxxxx>; Julien Grall <Julien.Grall@xxxxxxx>;
> > > Sharan.Santhanam@xxxxxxxxx; Santiago.Pagani@xxxxxxxxx; nd
> > > <nd@xxxxxxx>
> > > Subject: Re: [Minios-devel] [UNIKRAFT PATCH v2 1/2] include/plat: Remove
> > > plat/common/include/memory.h
> > >
> > > Hi Jia He,
> > >
> > > I agree that this warning has to be fixed. However, we cannot fix it by
> > > removing `plat/common/include/memory.h`, we should find another
> > > solution
> > > and this is why.
> > >
> > > Headers under `include` directory, such as `include/uk/plat/memory.h`,
> > > declare functions that are part of the platform public API. Headers
> > > under `plat/common/include/` declare functions that are used by more
> > > platforms, but that are not public. Therefore, we cannot make
> > > `_ukplat_mem_mappings_init()` public because that wasn't the intention
> > > behind defining it.
> > >
> > > Maybe we should think again whether we really need to include newlib's
> > > memory.h header in platform code and, if not, then find an elegant way
> > > to skip including that one.
> > If we can't remove include/uk/plat/memory.h, how about rename it with another
> > name? Otherwise this might be conflicted with newlibc/.../memory.h.
> In deed, renaming the header would temporary fix it. However, the real
> problem would remain. For the platform source, we should not include the
> external libs headers at all because platform code doesn't depend on
> external libs - this is one of the main design principles in Unikraft.
Yes, this is true. At least the library dependency that a platform 
library has should be kept as minimum as possible. It is okay to 
increase dependency with configurable options - like what happens on 
Xen: Xenbus depends on scheduling. This keeps our most minimal baseline 
minimal. Of course, a dependency to a libc is almost unavoidable (think 
of memcpy(), sprintf(), etc.).
> Therefore we should discriminate between the include paths and skip
> using the ones coming from external libs at least when building platform
> sources.
>
> I think that Simon should shed some light on how to do it properly.
I had a longer chat with Sharan and we think the root cause of this 
problem could be that the header priority order is currently broken in 
Unikraft.
In theory, it should work like the following:

For each #include directive of a source file, ...
- global includes (e.g., CINCLUDES-y) have lowest priority
- library-local includes (e.g., LIBNAME_CINCLUDES-y) are
  preferred over global includes
- file-related includes have highest priority and can overwrite both

This means, a '<memory.h>' provided through file-related or 
libray-related includes should automatically be taken instead of one 
provided with global `CINCLUDES-y`.
newlibc is is providing its headers via the global space (since it is a 
libc): CINCLUDES-y. The common platform headers are added by each 
platform library through library-local includes: LIBKVMPLAT_CINCLUDES-y.
This way it should work fine as long as you never include a global 
header that is including another header where you -by chance - have a 
replacement in your library (imagine newlibc stdio.h is itself including 
memory.h).
For such a case, I think name spacing/renaming is the only viable 
solution for our code. A proper way for name spacing are 
sub-directories. I am convinced that if we only rename memory.h to 
something unique, the next thing that will hit us is something like 
`time.h`. So, I would suggest to move the internal platform headers into 
sub directories. A single patch could do this with the common platform 
headers.
The interesting question is on which scheme we should agree on?

        #include <uk/plat/common/memory.h>
or
        #include <ukplat/common/memory.h>
or
        #include <uk/internal/plat/common/memory.h>
or
        #include <ukint/plat/common/memory.h>
or
        #include <ukplatint/common/memory.h>

What do you guys think?

As conclusion: Can you first check if the current include priority is 
actual working as intended? If not, please provide a patch to all the 
build rules to reflect this behavior. We probably should document it 
also in the corresponding guide within doc/.
If we have the corner case that newlibc is including memory.h somewhere, 
we should think about a sub-directory structure for those headers.
Thanks,

Simon

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