Re: [PATCH 2/2] xen/arm: Move early mapping operations to new function

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Julien,

thanks for your review.

>>  +void __init setup_early_mappings(paddr_t fdt_paddr)
>> +{
>> +    const char *cmdline;
>> +    struct bootmodule *xen_bootmodule;
>> +
>> +    device_tree_flattened = early_fdt_map(fdt_paddr);
>> +    if ( !device_tree_flattened )
>> +        panic("Invalid device tree blob at physical address %#"PRIpaddr".\n"
>> +              "The DTB must be 8-byte aligned and must not exceed 2 MB in 
>> size.\n\n"
>> +              "Please check your bootloader.\n",
>> +              fdt_paddr);
>> +
>> +    /* Register Xen's load address as a boot module. */
>> +    xen_bootmodule = add_boot_module(BOOTMOD_XEN,
>> +                             virt_to_maddr(_start),
>> +                             (paddr_t)(uintptr_t)(_end - _start), false);
>> +    BUG_ON(!xen_bootmodule);
> 
> Don't you need the code above for the MPU?
> 
>> +
>> +    cmdline = boot_fdt_cmdline(device_tree_flattened);
>> +    printk("Command line: %s\n", cmdline);
>> +    cmdline_parse(cmdline);
> 
> I find confusing this is part of early mappings. Shouldn't this be part of 
> common code?
> 
>> +
>> +    llc_coloring_init();
>> +
>> +    /*
>> +     * Page tables must be setup after LLC coloring initialization because
>> +     * coloring info are required in order to create colored mappings
>> +     */
>> +    setup_pagetables();
>> +    /* Device-tree was mapped in boot page tables, remap it in the new 
>> tables */
>> +    device_tree_flattened = early_fdt_map(fdt_paddr);
>> +}
>> +
>>  void *__init arch_vmap_virt_end(void)
>>  {
>>      return (void *)(VMAP_VIRT_START + VMAP_VIRT_SIZE);
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index c1f2d1b89d43..b2f34ba2a873 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -12,7 +12,6 @@
>>  #include <xen/device_tree.h>
>>  #include <xen/domain_page.h>
>>  #include <xen/grant_table.h>
>> -#include <xen/llc-coloring.h>
>>  #include <xen/types.h>
>>  #include <xen/string.h>
>>  #include <xen/serial.h>
>> @@ -300,8 +299,6 @@ size_t __read_mostly dcache_line_bytes;
>>  void asmlinkage __init start_xen(unsigned long fdt_paddr)
>>  {
>>      size_t fdt_size;
>> -    const char *cmdline;
>> -    struct bootmodule *xen_bootmodule;
>>      struct domain *d;
>>      int rc, i;
>>  @@ -315,35 +312,10 @@ void asmlinkage __init start_xen(unsigned long 
>> fdt_paddr)
>>        smp_clear_cpu_maps();
>>  -    device_tree_flattened = early_fdt_map(fdt_paddr);
>> -    if ( !device_tree_flattened )
>> -        panic("Invalid device tree blob at physical address %#lx.\n"
>> -              "The DTB must be 8-byte aligned and must not exceed 2 MB in 
>> size.\n\n"
>> -              "Please check your bootloader.\n",
>> -              fdt_paddr);
> 
> I understand why you don't need to call early_fdt_map() twice. But I am a bit 
> surprised why the two calls are moved to the MMU code. I think it would be 
> better if one of the call is kept here and early_fdt_map() is implemented for 
> the MPU.
> 
>> -
>> -    /* Register Xen's load address as a boot module. */
>> -    xen_bootmodule = add_boot_module(BOOTMOD_XEN,
>> -                             virt_to_maddr(_start),
>> -                             (paddr_t)(uintptr_t)(_end - _start), false);
>> -    BUG_ON(!xen_bootmodule);
>> +    setup_early_mappings(fdt_paddr);
>>        fdt_size = boot_fdt_info(device_tree_flattened, fdt_paddr);
> 
> This function will parse the memory banks. This is required by ...
> 
>>  -    cmdline = boot_fdt_cmdline(device_tree_flattened);
>> -    printk("Command line: %s\n", cmdline);
>> -    cmdline_parse(cmdline);
>> -
>> -    llc_coloring_init();
> 
> ... llc_coloring_init(). Yet you move it early. So I think it means the cache 
> coloring code would stop working.

woops, I didn’t see boot_fdt_info was parsing the memory banks, sorry I 
overlooked that,
I saw no dependencies on variables and given that I don’t have a working cache 
coloring setup
I didn’t notice. I’ll be more careful next time.

> 
> 
> > -> -    /*
>> -     * Page tables must be setup after LLC coloring initialization because
>> -     * coloring info are required in order to create colored mappings
>> -     */
>> -    setup_pagetables();
>> -    /* Device-tree was mapped in boot page tables, remap it in the new 
>> tables */
>> -    device_tree_flattened = early_fdt_map(fdt_paddr);
>> -
>>      setup_mm();
>>        vm_init();

So yes I would have used some duplication for the MPU part doing this:

void __init setup_early_mappings(paddr_t fdt_paddr)
{
    const char *cmdline;
    struct bootmodule *xen_bootmodule;

    <setup_mpu>

    device_tree_flattened = early_fdt_map(fdt_paddr);
    if ( !device_tree_flattened )
        panic("Invalid device tree blob at physical address %#"PRIpaddr".\n"
              "The DTB must be 8-byte aligned and must not exceed 2 MB in 
size.\n\n"
              "Please check your bootloader.\n",
              fdt_paddr);

    /* Register Xen's load address as a boot module. */
    xen_bootmodule = add_boot_module(BOOTMOD_XEN,
                             virt_to_maddr(_start),
                             (paddr_t)(uintptr_t)(_end - _start), false);
    BUG_ON(!xen_bootmodule);

    cmdline = boot_fdt_cmdline(device_tree_flattened);
    printk("Command line: %s\n", cmdline);
    cmdline_parse(cmdline);
}

I discussed with Michal before and he suggested to setup the MPU from the early 
ASM code,
it sounded a good idea to do that in the mpu enable_boot_cpu_mm, but then I 
realised my
MPU region table sits in .bss.

So I had some choices:
1) place the MPU region table in .data? I would still like it to be zeroed but 
I could do that in a “setup_mpu()” function.
    There is still the DT additional early_fdt_map call, but maybe I could move 
that into setup_pagetables() ?
    Or I could have a return value from llc_coloring_init() and make the second 
early_fdt_map call depending on if
    cache coloring is setup or not?
    I would then provide an empty stub for setup_pagetables() on MPU systems.

2) Provide a common function like what I’ve tried to do in this patch, so that 
different memory management subsystem
     could provide their implementation. Key differences as we saw are:
      a) MMU don’t call anything before early_fdt_map because it has already 
some data structure in place at this stage
      b) MMU needs to call llc_coloring_init, setup_pagetables and an 
additional early_fdt_map.
     Would something like pre_fdt_map(), post_fdt_map() work? pre_fdt_map() 
would be empty for MMU.

Please let me know your view on this, maybe you have some better design.

Cheers,
Luca