Re: [RFC v2 04/12] xen/arm32: head: Remove restriction where to load Xen

[Julien Grall <julien@xxxxxxx>]

Hi Luca,

On 25/10/2022 12:56, Luca Fancellu wrote:
> > On 22 Oct 2022, at 16:04, Julien Grall <julien@xxxxxxx> wrote:
> >
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > At the moment, bootloaders can load Xen anywhere in memory but the
> > region 2MB - 4MB. While I am not aware of any issue, we have no way
> > to tell the bootloader to avoid that region.
> >
> > In addition to that, in the future, Xen may grow over 2MB if we
> > enable feature like UBSAN or GCOV. To avoid widening the restriction
> > on the load address, it would be better to get rid of it.
> >
> > When the identity mapping is clashing with the Xen runtime mapping,
> > we need an extra indirection to be able to replace the identity
> > mapping with the Xen runtime mapping.
> >
> > Reserve a new memory region that will be used to temporarily map Xen.
> > For convenience, the new area is re-using the same first slot as the
> > domheap which is used for per-cpu temporary mapping after a CPU has
> > booted.
> >
> > Furthermore, directly map boot_second (which cover Xen and more)
> > to the temporary area. This will avoid to allocate an extra page-table
> > for the second-level and will helpful for follow-up patches (we will
> > want to use the fixmap whilst in the temporary mapping).
> >
> > Lastly, some part of the code now needs to know whether the temporary
> > mapping was created. So reserve r12 to store this information.
> >
> > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> > ----
> >
> >     Changes in v2:
> >         - Patch added
> > ---
> Hi Julien,
>
> I’m hitting an assert with this one, tested on qemu and fvp:
Thanks for testing the series!

> Xen 4.17-rc
> (XEN) Xen version 4.17-rc (user@hostname) (arm-poky-linux-gnueabi-gcc (GCC) 
> 11.3.0) debug=y Tue Oct 25 10:51:06 UTC 2022
> (XEN) Latest ChangeSet:
> (XEN) build-id: ab143b13f4394ced5331d6ff1cedebdb2ffadc07
> (XEN) Processor: 412fc0f1: "ARM Limited", variant: 0x2, part 0xc0f,rev 0x1
> (XEN) 32-bit Execution:
> (XEN)   Processor Features: 00001131:00011001
> (XEN)     Instruction Sets: AArch32 A32 Thumb Thumb-2 ThumbEE Jazelle
> (XEN)     Extensions: GenericTimer
> (XEN)   Debug Features: 02010555
> (XEN)   Auxiliary Features: 00000000
> (XEN)   Memory Model Features: 10201105 20000000
> (XEN)                          01240000 02102211
> (XEN)   ISA Features: 02101110 13112111 21232041
> (XEN)                 11112131 10011142 00000000
> (XEN) Using SMC Calling Convention v1.0
> (XEN) Using PSCI v0.2
> (XEN) SMP: Allowing 4 CPUs
> (XEN) Generic Timer IRQ: phys=30 hyp=26 virt=27 Freq: 62500 KHz
> (XEN) GICv2m extension register frame:
> (XEN)         gic_v2m_addr=0000000008020000
> (XEN)         gic_v2m_size=0000000000001000
> (XEN)         gic_v2m_spi_base=80
> (XEN)         gic_v2m_num_spis=64
> (XEN) GICv2 initialization:
> (XEN)         gic_dist_addr=0000000008000000
> (XEN)         gic_cpu_addr=0000000008010000
> (XEN)         gic_hyp_addr=0000000008030000
> (XEN)         gic_vcpu_addr=0000000008040000
> (XEN)         gic_maintenance_irq=25
> (XEN) GICv2: 288 lines, 4 cpus (IID 00000000).
> (XEN) XSM Framework v1.0.1 initialized
> (XEN) Initialising XSM SILO mode
> (XEN) Using scheduler: SMP Credit Scheduler rev2 (credit2)
> (XEN) Initializing Credit2 scheduler
> (XEN)  load_precision_shift: 18
> (XEN)  load_window_shift: 30
> (XEN)  underload_balance_tolerance: 0
> (XEN)  overload_balance_tolerance: -3
> (XEN)  runqueues arrangement: socket
> (XEN)  cap enforcement granularity: 10ms
> (XEN) load tracking window length 1073741824 ns
> (XEN) Allocated console ring of 32 KiB.
> (XEN) VFP implementer 0x41 architecture 4 part 0x30 variant 0xf rev 0x0
> (XEN) CPU0: Guest atomics will try 1 times before pausing the domain
> (XEN) Bringing up CPU1
> (XEN) Assertion '!lpae_is_valid(*entry)' failed at arch/arm/domain_page.c:69
So this is asserting because, so far, for secondary CPUs, we are copying 
the content of the CPU0 root table to the secondary CPU root table and 
then update the entry.
So the entry would logical be valid. This is fine to be valid because 
the root able is not yet live.
I have follow-up patches (not yet sent) where the root table for 
secondary CPUs would also be live. I probably mistakenly tested with 
those patches.
Anyway, the ASSERT() here doesn't make sense in the context of this 
patch because we are still switching the CPU0 root table. So I will drop 
the ASSERT() for now.
I will re-introduce it in a follow-up series.

Before I send a new version, do you have any comments for the rest of 
the patches?
Cheers,

--
Julien Grall