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

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

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 😉

--
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