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

["Justin He (Arm Technology China)" <Justin.He@xxxxxxx>]

> -----Original Message-----
> From: Minios-devel <minios-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf
> Of Justin He (Arm Technology China)
> Sent: Tuesday, October 22, 2019 10:29 AM
> To: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>; Costin Lupu
> <costin.lupu@xxxxxxxxx>; minios-devel@xxxxxxxxxxxxxxxxxxxx
> 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 Simon
> 
> > -----Original Message-----
> > From: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> > Sent: Monday, October 14, 2019 8:22 PM
> > To: Costin Lupu <costin.lupu@xxxxxxxxx>; Justin He (Arm Technology
> China)
> > <Justin.He@xxxxxxx>; minios-devel@xxxxxxxxxxxxxxxxxxxx
> > 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
> >
> > 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>
> 
> I prefer this because it has little impact on current layout. And we just need
> to
> change a few lines.
> 
> > 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/.
> 
> Maybe someone else can document it since I can describe it more clearly

s/can/can't/   

> 😉
> 
> --
> Cheers,
> Justin (Jia He)
> 
> 
> > 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
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel