[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Minios-devel] [UNIKRAFT PATCHv7 2/8] drivers/ofw: Enable build support for internal fdt interfaces



Hi Sharan

> -----Original Message-----
> From: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
> Sent: 2019年7月3日 23:42
> To: Justin He (Arm Technology China) <Justin.He@xxxxxxx>; minios-
> devel@xxxxxxxxxxxxxxxxxxxx; Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> Cc: Florian Schmidt <florian.schmidt@xxxxxxxxx>; Felipe Huici
> <felipe.huici@xxxxxxxxx>; Julien Grall <Julien.Grall@xxxxxxx>;
> yuri.volchkov@xxxxxxxxx; Kaly Xin (Arm Technology China)
> <Kaly.Xin@xxxxxxx>
> Subject: Re: [UNIKRAFT PATCHv7 2/8] drivers/ofw: Enable build support for
> internal fdt interfaces
>
> Hello Jia He,
>
> On 7/3/19 5:09 PM, Justin He (Arm Technology China) wrote:
> > Hi Sharan
> >
> >> -----Original Message-----
> >> From: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
> >> Sent: 2019年7月3日 17:25
> >> To: Justin He (Arm Technology China) <Justin.He@xxxxxxx>; minios-
> >> devel@xxxxxxxxxxxxxxxxxxxx; Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> >> Cc: Florian Schmidt <florian.schmidt@xxxxxxxxx>; Felipe Huici
> >> <felipe.huici@xxxxxxxxx>; Julien Grall <Julien.Grall@xxxxxxx>;
> >> yuri.volchkov@xxxxxxxxx; Kaly Xin (Arm Technology China)
> >> <Kaly.Xin@xxxxxxx>
> >> Subject: Re: [UNIKRAFT PATCHv7 2/8] drivers/ofw: Enable build support
> for
> >> internal fdt interfaces
> >>
> >> Hello Justin,
> >>
> >> One more suggestion, I would prefer if we split the device tree driver
> >> as a separate library from the kvm platform library. This is similar to
> >> what we do virtio or pci drivers in
> >> 'plat/kvm/Makefile.uk'.
> > Ok with me to split the libofw (is this lib name ok with you?)
> > Since the libofw is not dedicated to kvm plat, do you think it would be
> better move
> > below directories to ./lib/ofw and ./lib/ofw/include ?
> > ./plat/drivers/include/ofw
> > ./plat/drivers/ofw
> >
> I would still keep it within the plat/drivers and follow the naming
> convention we have adopted so far which in this case is "libkvmofw". The
> prefixing the "kvm" is to make sure these libraries platform specific
> and each platform enable the driver it needs.

Ok.
>
> Of course as a next step we can make it independent, but then we might
> have to extend the build system further to process the driver Makefiles
> separately. So I avoid it for this patch set.

Ok, I tried it just now, not so much changes as follows, I can make it in the 
future
(not his patch series if it is ok with you):

diff --git a/plat/kvm/Config.uk b/plat/kvm/Config.uk
index 5a6dfc1..096beb1 100644
--- a/plat/kvm/Config.uk
+++ b/plat/kvm/Config.uk
@@ -7,6 +7,7 @@ menuconfig PLAT_KVM
        select LIBUKTIMECONV
        select LIBNOLIBC if !HAVE_LIBC
        select LIBFDT if ARCH_ARM_64
+       select LIBOFW if ARCH_ARM_64
        help
                 Create a Unikraft image that runs as a KVM guest

@@ -80,4 +81,9 @@ config VIRTIO_NET
        help
               Virtual network driver.
 endmenu
+
+config LIBOFW
+       bool "Open Firmware library support"
+       default n
+       select LIBFDT
 endif
diff --git a/plat/kvm/Makefile.uk b/plat/kvm/Makefile.uk
index 3c3c006..3aeab2c 100644
--- a/plat/kvm/Makefile.uk
+++ b/plat/kvm/Makefile.uk
@@ -10,6 +10,7 @@ $(eval $(call addplatlib,kvm,libkvmplat))
 $(eval $(call addplatlib_s,kvm,libkvmpci,$(CONFIG_KVM_PCI)))
 $(eval $(call addplatlib_s,kvm,libkvmvirtio,$(CONFIG_VIRTIO_BUS)))
 $(eval $(call addplatlib_s,kvm,libkvmvirtionet,$(CONFIG_VIRTIO_NET)))
+$(eval $(call addplatlib_s,kvm,libkvmofw,$(CONFIG_LIBOFW)))

 ##
 ## Platform library definitions
@@ -66,7 +67,6 @@ LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) += 
$(UK_PLAT_COMMON_BASE)/arm/cache64.S|co
 LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) += 
$(UK_PLAT_COMMON_BASE)/arm/psci_arm64.S|common
 LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) += 
$(UK_PLAT_COMMON_BASE)/arm/time.c|common
 LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) += 
$(UK_PLAT_COMMON_BASE)/arm/traps.c|common
-LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) += 
$(UK_PLAT_DRIVERS_BASE)/ofw/fdt.c|common
 LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) += $(LIBKVMPLAT_BASE)/arm/entry64.S
 LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) += $(LIBKVMPLAT_BASE)/arm/exceptions.S
 LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) += $(LIBKVMPLAT_BASE)/arm/pagetable64.S
@@ -82,6 +82,18 @@ LIBKVMPLAT_SRCS-y              += 
$(UK_PLAT_COMMON_BASE)/lcpu.c|common
 LIBKVMPLAT_SRCS-y              += $(UK_PLAT_COMMON_BASE)/memory.c|common
 LIBKVMPLAT_SRCS-y              += $(KVM_LDSCRIPT_SRC-y)

+##
+## OFW library definitions
+##
+LIBKVMOFW_ASINCLUDES-y        += -I$(LIBKVMPLAT_BASE)/include
+LIBKVMOFW_ASINCLUDES-y        += -I$(UK_PLAT_COMMON_BASE)/include
+LIBKVMOFW_CINCLUDES-y         += -I$(LIBKVMPLAT_BASE)/include
+LIBKVMOFW_CINCLUDES-y         += -I$(UK_PLAT_COMMON_BASE)/include
+LIBKVMOFW_CINCLUDES-y         += -I$(UK_PLAT_DRIVERS_BASE)/include
+
+LIBKVMOFW_SRCS-$(CONFIG_ARCH_ARM_64) += \
+                                 $(UK_PLAT_DRIVERS_BASE)/ofw/fdt.c
+
 ##
 ## PCI library definitions
 ##
>
> Thanks & Regards
> Sharan
>
> > Thanks for the clarification
> >
> > --
> > Cheers,
> > Justin (Jia He)
> >
> >
> >>
> >> We can do it as a part of this series or take it up as a subsequent
> >> patch. This would help us with maintaining the modularity of the library.
> >>
> >> Please find the other comment inline.
> >>
> >> Thanks & Regards
> >> Sharan Santhanam
> >>
> >> On 7/2/19 3:35 AM, Justin He (Arm Technology China) wrote:
> >>> Hi Sharan
> >>>
> >>>> -----Original Message-----
> >>>> From: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
> >>>> Sent: 2019年7月1日 22:54
> >>>> To: Justin He (Arm Technology China) <Justin.He@xxxxxxx>; minios-
> >>>> devel@xxxxxxxxxxxxxxxxxxxx; Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> >>>> Cc: Florian Schmidt <florian.schmidt@xxxxxxxxx>; Felipe Huici
> >>>> <felipe.huici@xxxxxxxxx>; Julien Grall <Julien.Grall@xxxxxxx>;
> >>>> yuri.volchkov@xxxxxxxxx; Kaly Xin (Arm Technology China)
> >> <Kaly.Xin@xxxxxxx>
> >>>> Subject: Re: [UNIKRAFT PATCHv7 2/8] drivers/ofw: Enable build support
> >> for
> >>>> internal fdt interfaces
> >>>>
> >>>> Hello Jia He,
> >>>>
> >>>> This patch seems fine.
> >>>>
> >>>> My suggestion would be in the next version of the patch series we can
> >>>> combine this patch with the previous one where we include the
> function.
> >>>>
> >>>
> >>> Thanks, ok I will merge it into previous one
> >>>
> >>>> Thanks & Regards
> >>>> Sharan Santhanam
> >>>>
> >>>> On 6/27/19 10:55 AM, Jia He wrote:
> >>>>> This enable the build support for unikraft internal fdt interfaces
> >>>>>
> >>>>> Signed-off-by: Jia He <justin.he@xxxxxxx>
> >>>>> ---
> >>>>>     plat/kvm/Makefile.uk | 1 +
> >>>>>     1 file changed, 1 insertion(+)
> >>>>>
> >>>>> diff --git a/plat/kvm/Makefile.uk b/plat/kvm/Makefile.uk
> >>>>> index 71c4c41..18eaca4 100644
> >>>>> --- a/plat/kvm/Makefile.uk
> >>>>> +++ b/plat/kvm/Makefile.uk
> >>>>> @@ -65,6 +65,7 @@ LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) +=
> >>>> $(UK_PLAT_COMMON_BASE)/arm/cache64.S|co
> >>>>>     LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) +=
> >>>> $(UK_PLAT_COMMON_BASE)/arm/psci_arm64.S|common
> >>>>>     LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) +=
> >>>> $(UK_PLAT_COMMON_BASE)/arm/time.c|common
> >>>>>     LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) +=
> >>>> $(UK_PLAT_COMMON_BASE)/arm/traps.c|common
> >>>>> +LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) +=
> >>>> $(UK_PLAT_DRIVERS_BASE)/ofw/fdt.c|common
> >>>> Unfortunately, adding this to files causes compilation error. This was
> >>>> related to the previous patch where we include print.h instead of
> >>>> assert.h .
> >>>>
> >>> I  will check it
> >>
> >> We are also missing the include path for the ofw/fdt.h header/

I have fixed it in patch v8 [1]
[1] 
https://lists.xenproject.org/archives/html/minios-devel/2019-07/msg00035.html

--
Cheers,
Justin (Jia He)


IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.