Re: [Minios-devel] [UNIKRAFT early RFC PATCH 00/11] Enble early SMP support on arm64 kvm plat

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

Thank you for sending the series.

On 21/06/2019 07:57, Jia He wrote:
> This is the early SMP support on arm64 kvm plat which based on previous series 
> #1-#5
A reviewer may have no idea what are "series #1 - #5". So I would recommend to 
give pointer to each of them. Also, for easing review, it is nice to have a repo 
with everything applied.
But I think basing your work on 6 different series is not really wise. For a 
first, there was a lot of comments on those series. Does it mean this is before 
or after them addressed?
Secondly this is making more difficult to go through the code and potentially 
for you to address comment. May I recommend to focus on the getting series #1-#5 
to be merged?
> Most of the codes refer to the implementation in freebsd.
Some general comments here. I know this is an early RFC, but I will point out 
for the next series :).
	1) Please be more verbose in the commit message. This is helpful for reviewer 
and future developers to understand what the patch does
	2) You probably want to make the GIC code SMP safe. Also, the GIC CPU ID may be 
different from the MIDR (you don't want to route interrupt to the wrong CPU ;)).
	3) It looks like spinlock is pretty much a NOP today. Is there any plan to 
address this?
Cheers,

> Jia He (11):
>    lib/nolibc: Introduce ndelay and mdelay for busy wait
>    plat/common/ofw: Move fdt_reg_read_number to header file for static
>      inline
>    plat/kvm/smp: Add psci call definitions in psci.h
>    plat/kvm/smp: Add smp definitions in smp.h
>    plat/kvm/smp: Add psci call psci_cpu_on
>    plat/kvm: arm64: Add a parameter to start_mmu
>    plat/kvm: arm64: Add entry point for APs (secondary cpus)
>    plat/kvm/arm: Implement smp boot on arm64 kvm plat
>    plat/kvm/arm: Add simple percpu variable support
>    lib/ukboot: Enable CONFIG_SMP for arm64
>    plat/kvm/arm: Enable scheduler on secondary APs
>
>   lib/nolibc/exportsyms.uk       |   4 +
>   lib/nolibc/include/time.h      |   3 +
>   lib/nolibc/time.c              |  16 ++-
>   lib/ukboot/Config.uk           |   5 +
>   lib/ukboot/Makefile.uk         |   1 +
>   lib/ukboot/boot.c              |  15 +++
>   plat/common/include/arm/psci.h |  87 ++++++++++++
>   plat/common/include/smp.h      |  49 +++++++
>   plat/drivers/include/ofw/fdt.h |  17 ++-
>   plat/drivers/ofw/fdt.c         |  17 +--
>   plat/kvm/Makefile.uk           |   1 +
>   plat/kvm/arm/entry64.S         |  31 +++++
>   plat/kvm/arm/pagetable64.S     |   7 +-
>   plat/kvm/arm/setup.c           | 233 +++++++++++++++++++++++++++++++++
>   14 files changed, 466 insertions(+), 20 deletions(-)
>   create mode 100644 plat/common/include/arm/psci.h
>   create mode 100644 plat/common/include/smp.h
--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel