[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH early-RFC 3/5] xen/arm: mm: Introduce helpers to prepare/enable/disable the identity mapping
On 25/03/2022 13:32, Bertrand Marquis wrote:
Hi Julien,
Hi,
On 9 Mar 2022, at 12:20, Julien Grall <julien@xxxxxxx> wrote:
From: Julien GralL <jgrall@xxxxxxxxxx>
In follow-up patches we will need to have part of Xen identity mapped in
order to safely switch the TTBR.
On some platform, the identity mapping may have to start at 0. If we always
keep the identity region mapped, NULL pointer ference would lead to access
to valid mapping.
It would be possible to relocate Xen to avoid clashing with address 0.
However the identity mapping is only meant to be used in very limited
places. Therefore it would be better to keep the identity region invalid
for most of the time.
Two new helpers are introduced:
- prepare_identity_mapping() will setup the page-tables so it is
easy to create the mapping afterwards.
- update_identity_mapping() will create/remove the identity mapping
Nit: Would be better to first say what the patch is doing and then explaining
the NULL pointer possible issue.
The NULL pointer is part of the problem statement. IOW, I would not have
introduced update_identity_mapping() if we were not concerned that the
mapping start 0.
So I don't think the commit message would read the same.
+/*
+ * The identity mapping may start at physical address 0. So don't want
+ * to keep it mapped longer than necessary.
+ *
+ * When this is called, we are still using the boot_pgtable.
+ *
+ * XXX: Handle Arm32 properly.
+ */
+static void prepare_identity_mapping(void)
+{
+ paddr_t id_addr = virt_to_maddr(_start);
+ lpae_t pte;
+ DECLARE_OFFSETS(id_offsets, id_addr);
+
+ printk("id_addr 0x%lx\n", id_addr);
Debug print that should be removed.
Will do. Note the "early-RFC" in the comment. I am not looking for a
detailed review (I didn't spend too much time cleaning up) but a
feedback on the approach.
+#ifdef CONFIG_ARM_64
+ if ( id_offsets[0] != 0 )
+ panic("Cannot handled ID mapping above 512GB\n");
The error message here might not be really helpful for the user.
How about saying that Xen cannot be loaded in memory with
a physical address above 512GB ?
Sure.
+#endif
+
+ /* Link first ID table */
+ pte = pte_of_xenaddr((vaddr_t)xen_first_id);
+ pte.pt.table = 1;
+ pte.pt.xn = 0;
+
+ write_pte(&boot_pgtable[id_offsets[0]], pte);
+
+ /* Link second ID table */
+ pte = pte_of_xenaddr((vaddr_t)xen_second_id);
+ pte.pt.table = 1;
+ pte.pt.xn = 0;
+
+ write_pte(&xen_first_id[id_offsets[1]], pte);
+
+ /* Link third ID table */
+ pte = pte_of_xenaddr((vaddr_t)xen_third_id);
+ pte.pt.table = 1;
+ pte.pt.xn = 0;
+
+ write_pte(&xen_second_id[id_offsets[2]], pte);
+
+ /* The mapping in the third table will be created at a later stage */
+
+ /*
+ * Link the identity mapping in the runtime Xen page tables. No need to
+ * use write_pte here as they are not live yet.
+ */
+ xen_pgtable[id_offsets[0]] = boot_pgtable[id_offsets[0]];
+}
+
+void update_identity_mapping(bool enable)
You probably want an __init attribute here.
I expect this helper to be necessary after boot (e.g. CPU bring-up,
suspend/resume). So I decided to keep it without _init.
+{
+ paddr_t id_addr = virt_to_maddr(_start);
+ int rc;
+
+ if ( enable )
+ rc = map_pages_to_xen(id_addr, maddr_to_mfn(id_addr), 1,
+ PAGE_HYPERVISOR_RX);
+ else
+ rc = destroy_xen_mappings(id_addr, id_addr + PAGE_SIZE);
+
+ BUG_ON(rc);
+}
+
/*
* After boot, Xen page-tables should not contain mapping that are both
* Writable and eXecutables.
@@ -609,6 +679,9 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
phys_offset = boot_phys_offset;
+ /* XXX: Find a better place to call it */
Why do you think this place is not right ?
Because the use in setup_pagetables() will soon disappear (my plan it to
completely remove setup_pagetables) and this will used in other
subsystem (CPU bring-up, suspend/resume).
Cheers,
--
Julien Grall
|
