[PATCH v1 0/2] [libbsd] Install correct machine include headers

Wed Jun 2 14:36:06 UTC 2021

Thanks, Chris & Christian.
I think I got a better understanding of the issues now and found a simpler solution which is less intrusive.
I will post a new patchset.

Best regards,

   Jan

> -----Original Message-----
> From: Chris Johns <chrisj at rtems.org>
> Sent: Tuesday, June 1, 2021 11:28 PM
> To: Christian Mauderer <oss at c-mauderer.de>; Gedare Bloom
> <gedare at rtems.org>; Sommer, Jan <Jan.Sommer at dlr.de>
> Cc: devel at rtems.org
> Subject: Re: [PATCH v1 0/2] [libbsd] Install correct machine include headers
> 
> On 2/6/21 4:20 am, Christian Mauderer wrote:
> > On 01/06/2021 19:24, Gedare Bloom wrote:
> >> On Mon, May 10, 2021 at 11:26 AM Jan Sommer <jan.sommer at dlr.de>
> wrote:
> >>>
> >>> Hello,
> >>>
> >>> This is a follow-up on this discussion regarding the installed
> >>> header files in libbsd:
> >>> https://lists.rtems.org/pipermail/devel/2021-April/066211.html
> >>>
> >>> The current situation is, that for example for all machines the
> >>> bus.h is installed from within the rtemsbsd directory
> >>> (https://git.rtems.org/rtems-
> libbsd/tree/rtemsbsd/include/machine/bus.h).
> >>>
> >>> According to the file docu it originates from the amd64 version of this
> file.
> >>> It also has the following section in it which we ran into when
> >>> compiling Chris' ptpd2 archive:
> >>>
> >>> #ifdef __i386__
> >>>    #error "your include paths are wrong"
> >>> #endif
> >>>
> >>> This patchset does the following:
> >>> - Add the target dependent machine include directory to
> >>> 'header-paths' in libbsd.py
> >>> - Import (mostly) '_bus.h', 'bus.h' and 'cpufunc.h' for the targets
> >>> from freebsd-org
> >>> - Remove those header files from rtemsbsd directory
> >>>
> >>> As a result the matching versions for machine dependent header files
> >>> are now installed for each BSP.
> >>> Would this be an acceptable solution?
> >>>
> >>> So far I compiled some BSPs for i386, arm, aarch64, powerpc, riscv,
> >>> sparc and
> >>> sparc64 to check that they still compile after the changes.
> >>> Are there any other architectures which should be included?
> >
> > I think the best starting point to find out the minimum supported
> > platforms is the nexus-devices.h. At least the officially supported
> > BSPs should have an entry there. That is:
> >
> > #if defined(LIBBSP_ARM_REALVIEW_PBX_A9_BSP_H)
> > #elif defined(LIBBSP_ARM_BEAGLE_BSP_H) #elif
> > defined(LIBBSP_ARM_LPC32XX_BSP_H) #elif
> > defined(LIBBSP_M68K_GENMCF548X_BSP_H)
> > #elif defined(LIBBSP_ARM_XILINX_ZYNQ_BSP_H)
> > #elif defined(LIBBSP_AARCH64_XILINX_ZYNQMP_BSP_H)
> > #elif defined(LIBBSP_ARM_ATSAM_BSP_H)
> > #elif defined(LIBBSP_ARM_ALTERA_CYCLONE_V_BSP_H)
> > #elif defined(LIBBSP_ARM_IMX_BSP_H)
> > #elif defined(LIBBSP_ARM_IMXRT_BSP_H)
> > #elif defined(LIBBSP_ARM_LPC24XX_BSP_H) #elif
> > defined(LIBBSP_ARM_STM32H7_BSP_H) #elif
> > defined(LIBBSP_I386_PC386_BSP_H) #elif
> > defined(LIBBSP_POWERPC_QORIQ_BSP_H)
> > #elif defined(LIBBSP_POWERPC_TQM8XX_BSP_H)
> > #elif defined(LIBBSP_POWERPC_MOTOROLA_POWERPC_BSP_H)
> >
> > So I think you should have a look at m68k too.
> >
> > Did you try to run it on any of the platforms?
> >
> >>>
> >>> I ran into one problem regarding the compilation of
> >>> rtemsbsd/sys/dev/dw_mmc/dw_mmc.c:105
> >>>
> >>>> return (bus_space_read_4(0, sc->bushandle, off));
> >>>
> >>> The first parameter creates an error for riscv (I think because
> >>> dereferencing a NULL pointer).
> >>> Are there any suggestion how to solve it (I am not familiar with the
> >>> bus space and there is a lot of macro magic going on)?
> >>>
> >> It looks like this will be a problem for any architecture. so should
> >> the function that calls bus_space_write_4(0 ...)
> >>   The macro trail goes...
> >>
> >> #define        __bs_rs(sz, t, h, o)
> >> \
> >>         (*(t)->__bs_opname(r,sz))((t)->bs_cookie, h, o)
> >>
> >> #define        bus_space_read_4(t, h, o)       __bs_rs(4,(t),(h),(o))
> >>
> >> so t is dereferenced. It appears to be an error in the API usage by
> >> the dw_mmc.c code to not specify a valid bus space.
> >
> > dw_mmc is only relevant for very few platforms. So in theory it could
> > be limited to these. But most likely that won't really solve the problem.
> >
> > The right answer is that dw_mmc doesn't use the API like intended
> > (like Gedare said). That could be a problem for more drivers that use
> > stuff from bus.h in rtemsbsd. They never had to use that API
> > correctly. Even worse: Some of the __rtems__ hacks in freebsd could have
> that problem too.
> >
> > I think for this patch set we either need to have a thorough look at
> > these locations or run (not only build) it on a number of platforms,
> > especially the ones with special drivers in rtemsbsd or with heavily adapted
> drivers.
> 
> The only arch that supports a tag we current have is the x86 and that is for IO
> vs mem space. I posted some patches earlier this year I need to revisit for
> the powerpc (mvme2700). If possible we prefer no tags and a single flat
> address space that is cache coherent. In the x86 tags cannot be avoided and
> supported drivers handle the tags correctly.
> 
> Be-careful reviewing FreeBSD's bus space implementations. For example the
> PowerPC support uses tables of calls and we do not need that overhead.
> 
> The issue here is using this driver on the x86 platform. Until that happens I
> am fine with the tag being 0. The API requires something even if it is not
> used.
> 

Thanks, Chris. No I understand the reason for the include guard in bus.h for i386.
Maybe an overall simpler solutions is possible.
I could add the bus.h for i386 to the existing machine directory in rtemsbsd/i386/include/machine
And then change https://git.rtems.org/rtems-libbsd/tree/rtemsbsd/include/machine/bus.h#n126
To something like this:

> Chris