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
