Re: [Minios-devel] [UNIKRAFT PATCHv2 1/3] build: Split gic-v2 from kvm plat to independent library

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

On 8/20/19 5:28 PM, Sharan Santhanam wrote:
> Hello Justin,
>
> please find the review comment inline.
>
> Thanks & Regards
>
> Sharan
>
> On 8/15/19 9:16 AM, Jia He wrote:
> > This patch splits the arm gic-v2 interrupt controller as a separate 
> > library
> > from the kvm platform library. This is similar to what we do for 
> > virtio or
> > pci drivers.
> >
> > Signed-off-by: Jia He <justin.he@xxxxxxx>
> > Suggested-by: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
> > ---
> >   plat/kvm/Config.uk   |  9 ++++++++-
> >   plat/kvm/Makefile.uk | 12 +++++++++++-
> >   2 files changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/plat/kvm/Config.uk b/plat/kvm/Config.uk
> > index 5a6dfc1..39da143 100644
> > --- a/plat/kvm/Config.uk
> > +++ b/plat/kvm/Config.uk
> > @@ -6,7 +6,7 @@ menuconfig PLAT_KVM
> >          select LIBUKALLOC
> >          select LIBUKTIMECONV
> >          select LIBNOLIBC if !HAVE_LIBC
> > -       select LIBFDT if ARCH_ARM_64
> Unfortunately this is still necessary as pl011 needs it.
>
> /plat/common/arm/pl011.c:21:20: fatal error: libfdt.h: No such file or 
> directory
> We can split the pl011 similar to gic as another patch series and 
> remove it from the platform. What do you think?
> > +       select LIBGICV2 if ARCH_ARM_64
Instead of  forcing the library to be enabled, I would add

default y if ARCH_ARM_64

within the config LIBGICv2. The reason being, as user of this plat kvm 
library I can still build a image without gic whereas selecting it
forces us to keep gic selected always.

> >          help
> >                   Create a Unikraft image that runs as a KVM guest
> >   @@ -80,4 +80,11 @@ config VIRTIO_NET
> >          help
> >                 Virtual network driver.
> >   endmenu
> > +
> > +config LIBGICV2
> > +       bool "Arm GIC (generic interrupt controller) v2 library support"
> > +       default n
> > +       select LIBOFW
> LIBOFW is still not available. We should add it once we introduce the 
> library.
> > +       depends on ARCH_ARM_64
> > +
> >   endif
> > diff --git a/plat/kvm/Makefile.uk b/plat/kvm/Makefile.uk
> > index 7d0c696..b1fd50b 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,libkvmgicv2,$(CONFIG_LIBGICV2)))
> >     ##
> >   ## Platform library definitions
> > @@ -67,7 +68,6 @@ LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) += 
> > $(UK_PLAT_COMMON_BASE)/arm/psci_arm64.S
> >   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) += 
> > $(UK_PLAT_DRIVERS_BASE)/gic/gic-v2.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
> > @@ -118,3 +118,13 @@ LIBKVMVIRTIONET_ASINCLUDES-y   += 
> > -I$(UK_PLAT_DRIVERS_BASE)/include
> >   LIBKVMVIRTIONET_CINCLUDES-y    += -I$(UK_PLAT_DRIVERS_BASE)/include
> >   LIBKVMVIRTIONET_SRCS-y +=\
> >               $(UK_PLAT_DRIVERS_BASE)/virtio/virtio_net.c
> > +
> > +##
> > +## GICv2 library definitions
> > +##
> > +LIBKVMGICV2_CINCLUDES-y         += -I$(LIBKVMPLAT_BASE)/include
> > +LIBKVMGICV2_CINCLUDES-y         += -I$(UK_PLAT_COMMON_BASE)/include
> > +LIBKVMGICV2_CINCLUDES-y         += -I$(UK_PLAT_DRIVERS_BASE)/include
> > +
> > +LIBKVMGICV2_SRCS-$(CONFIG_LIBGICV2) += \
> Is having CONFIG_LIBGICV2 necessary as we are adding for this library?
> > + $(UK_PLAT_DRIVERS_BASE)/gic/gic-v2.c
> _______________________________________________
> 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