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

Re: [Minios-devel] [PATCH 00/40] MINI-OS: enable the arm64 support



Hi Shijie,

First of all, I think this should be tagged with RFC as the code is not in shape to get merged (more details below).

Before speaking about this series itself, I think it is important to mention the state of the arm32 port. Putting aside the fact it does not compile, it also has quite a few shortcoming. To name a few:
        1) Use of short-descriptor page tables
        2) No support for SMP
        3) No coherence in the coding style
        4) Functions left unimplemented (e.g arch_pfn_add, allocat_ondemand)
5) Devices mapped using memory attribute. It relies on the hypervisor to tighten attributes. 6) Assumption of the VM layout which is not part of the ABI. Such as event IRQ and timer IRQ number and their configuration (i.e level-sensitive/edge-triggered).
        7) Assumption on the way DT has been created
        8) GICv2 driver no compliant with the spec

I could probably list more... but I think the list is enough to make my point. The current code for Arm in Mini-OS is currently not in shape.

5-8 are the most critical ones as it implies that Mini-OS does not follow the ABI and the architecture. This may get broken for any change in the hypervisor and potentially in the hardware.

Now, this series is adding arm64 port in the same directory, despite the fact the design chosen is fairly different. This is noticeable by the amount of #ifdef arm64 within the code.

Furthermore, if you re-use the same directory. I would have expected some preparatory patches to move arm32 specific code under in sub-directory. I am quite surprised this is not present in the diff below.

Overall, it feels there are a lack of cohesion between the Arm32 and Arm64 ports. This is surprising given that AFAIK the difference between arm32 and arm64 should be fairly limited in the context of Mini-OS. It is basically done to:
        - boot code
        - exception entry/exit
        - registers naming
        - system registers access (arm64) vs co-processors (arm32) access

Regarding page-tables, ARMv7 support both short-descriptor and LPAE. The latter is not optional when the processor provides virtual extensions. Arm64 only support LPAE format, if the Arm32 port is switch to LPAE code handling page-table could be shared here.

I can see two solutions going forward:
        1) The arm directory is first reshaped to welcome arm64. This means:
                * moving out arm32 specific code
                * switch to LPAE page-table
                * introducing helpers for common code to call arch-specific code
           On the code is reshaped, the arm64 series is added on top.

        2) Start the arm64 port from a clean slate and then port arm32 over.

Knowing the state of the arm32 port, I would lean towards 2). This would allow more flexibility and make easier to review. At the moment, I have to hunt down the code to see what is missing.

I would be interested to hear the opinion of the maintainers here.

Cheers,

On 03/11/17 03:11, Huang Shijie wrote:
The work is based on Chen Baozi and Volodymyr's patches.
I tested this patch set with my Softiron board.

This patch set is based on the latest mini-os code:
  (git tree: https://github.com/lorc/mini-os.git
   The top is " 0b4b789 Update Xen header files again")

After this patch set:
        1.) The scheduler for arm64 works fine.
        2.) The gic/timer for arm64 works fine.

I tested this patch set on Softiron(arm64) and x86_64 platform.

Please see the log after the patch set:
   ---------------------------------------------------------
    (d1) - Mini-OS booting -
    (d1) - Setup CPU -
    (d1) - Setup booting pagetable -
    (d1) - MMU on -
    (d1) - Setup stack -
    (d1) - Jumping to C entry -
    (d1) Virtual -> physical offset = ffffffbfc0000000
    (d1) Checking DTB at 0xffffffc008000000...
    (d1) MM: Init
    (d1)     _text:       0xffffffc000000000(VA)
    (d1)     _etext:      0xffffffc000019a0c(VA)
    (d1)     _erodata:    0xffffffc000020000(VA)
    (d1)     _edata:      0xffffffc00002a38c(VA)
    (d1)     stack start: 0xffffffc000026000(VA)
    (d1)     _end:        0xffffffc0000337a0(VA)
    (d1) Found memory at 0x40000000 (len 0x20000000)
    (d1) Using pages 262196 to 393216 as free space for heap.
    (d1) MM: Initialise page allocator for 
ffffffc000034000(40034000)-ffffffc01ffff000(5ffff000)
    (d1)     Adding memory range 40040000-5ffff000
    (d1) MM: done
    (d1) Found GIC: gicd_base = 0xffffffbfc3001000, gicc_base = 
0xffffffbfc3002000
    (d1) Initialising timer interface
    (d1) Virtual Count register is 2abcb3e, freq = 250000000 Hz
    (d1) Initialising console ... done.
    (d1) FDT suggests grant table base 38000000
    (d1) gnttab_table mapped at 0xffffffbff8000000.
    (d1) Initialising scheduler
    (d1) Thread "Idle": pointer: 0x0xffffffc01fff8078, stack: 
0x0xffffffc01fff0000
    (d1) Thread "xenstore": pointer: 0x0xffffffc01fff80d8, stack: 
0x0xffffffc01fff4000
    (d1) xenbus initialised on irq 1
    (d1) Thread "shutdown": pointer: 0x0xffffffc01fff8138, stack: 
0x0xffffffc01ffe0000
    (d1) kernel.c: dummy main: par=0
   ---------------------------------------------------------

Huang Shijie (40):
   mini-os: fix the wrong parameter for map_free() in
     init_page_allocator()
   mini-os: replace the L1_PAGETABLE_SHIFT with PAGE_SHIFT
   arm64: add the boot code
   arm64: change physical_address_offset to paddr_t
   arm64: fix the wrong mask for to_virt/to_phys
   arm64: add the __PAGE_SIZE macro in header file
   arm64: add exception support
   arm64: dump the registers for do_bad_mode()/do_sync()
   arm64: add the basic helpers for arm64
   arm64: define the quad_t for arm64
   arm64: time.c: fix the wrong format for printk
   mini-os: define ULONG_MAX/LONG_MAX for arm64
   mini-os: remove the e820 from common code
   arm64: mm.c: fix the compiler error
   arm64: refine the arch_init_mm
   arm64: add shared_info support
   mini-os: implement the memmove/memchr
   mini-os: fix the compilor error in time
   arm64: define the CALLEE_SAVED_REGISTERS
   arm64: implement the __arch_switch_threads
   arm64: implement the arm_start_thread
   arm64: change sp to "unsigned long" type
   arm64: fix the wrong size of the register
   arm64: implement the run_idle_thread
   arm64: set the stack for the arm_start_thread
   mini-os: update the .gitignore
   arm64: add the header file arch_wordsize.h
   arm64: add the hypercall support
   arm64: init the memory system before console/xenbus/gic
   arm64: set the mapping for console and xenbus
   arm: add a new helper ioremap
   arm: parse out the address/size for gicd/gicc
   arm64: gic: implement the REG_WRITE32/REG_READ32
   arm64: implement the timer helpers for arm64
   arm64: time.c: read out the frequency for arm64
   arm64: add the link file
   arm64: add the makefile
   mini-os: Set TARGET_ARCH_FAM for arm64
   mini-os: create the image for arm
   mini-os: compile the dtc submodule for arm64

  .gitignore                        |   4 +
  .gitmodules                       |   3 +
  Config.mk                         |  11 +-
  Makefile                          |  23 ++
  arch/arm/arm64/Makefile           |  26 ++
  arch/arm/arm64/arch.mk            |   7 +
  arch/arm/arm64/arm64.S            | 534 ++++++++++++++++++++++++++++++++++++++
  arch/arm/arm64/asm.h              |  18 ++
  arch/arm/arm64/hypercalls64.S     |  81 ++++++
  arch/arm/arm64/minios-arm64.lds.S |  76 ++++++
  arch/arm/arm64/traps.c            |  44 ++++
  arch/arm/gic.c                    |  81 +++++-
  arch/arm/mm.c                     | 216 +++++++++++++--
  arch/arm/sched.c                  |  34 ++-
  arch/arm/setup.c                  |  10 +-
  arch/arm/time.c                   |  21 +-
  arch/x86/mm.c                     |  17 +-
  include/arm/arch_limits.h         |   2 +
  include/arm/arch_mm.h             |  26 +-
  include/arm/arm32/os.h            |  32 +++
  include/arm/arm64/arch_wordsize.h |   7 +
  include/arm/arm64/os.h            |  41 +++
  include/arm/arm64/pagetable.h     | 106 ++++++++
  include/arm/os.h                  |  65 +----
  include/arm/traps.h               |  14 +
  include/mm.h                      |   3 +
  include/posix/limits.h            |   2 +-
  include/sys/time.h                |   4 +
  include/types.h                   |   2 +-
  lib/memmove.c                     |  44 ++++
  lib/string.c                      |  12 +
  mm.c                              |  15 +-
  32 files changed, 1451 insertions(+), 130 deletions(-)
  create mode 100644 .gitmodules
  create mode 100644 arch/arm/arm64/Makefile
  create mode 100644 arch/arm/arm64/arch.mk
  create mode 100644 arch/arm/arm64/arm64.S
  create mode 100644 arch/arm/arm64/asm.h
  create mode 100644 arch/arm/arm64/hypercalls64.S
  create mode 100644 arch/arm/arm64/minios-arm64.lds.S
  create mode 100644 arch/arm/arm64/traps.c
  create mode 100644 include/arm/arm32/os.h
  create mode 100644 include/arm/arm64/arch_wordsize.h
  create mode 100644 include/arm/arm64/os.h
  create mode 100644 include/arm/arm64/pagetable.h
  create mode 100644 lib/memmove.c


--
Julien Grall

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

 


Rackspace

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