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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 17 Nov 2022, at 20:18, Julien Grall <julien@xxxxxxx> wrote:
> 
> 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?

Hi Julien,

Yes as you pointed out, the assert was not right in that context and it can be 
removed without issues, I’ve had a look on the serie and the changes looks ok 
to me, I’ve also tested
that it works on arm64 and arm32 using FVP.

Cheers,
Luca

> 
> Cheers,
> 
> -- 
> Julien Grall