Hi Julien
> On 25 Mar 2022, at 14:48, Julien Grall <jul...@xen.org> wrote:
>
>
>
> On 25/03/2022 13:32, Bertrand Marquis wrote:
>> Hi Julien,
>
> Hi,
>
>>> On 9 Mar 2022, at 12:20, Julien Grall <jul...@xen.org> wrote:
>>>
>>> From: Julien GralL <jgr...@amazon.com>
>>>
>>> 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.
Somehow reading your commit message, the link between the first 2 paragraph and
the helpers introduced is not quite clear.
The NULL pointer problem is a consequence of the usage of an identity mapping
and maybe this explanation should be more in documentation or in a code comment
around this functionality.
>
>>> +/*
>>> + * 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.
I did read the code and it is easy to forget to remove those, so I just pointed
it :-)
>
>>> +#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.
Ok
>
>>> +{
>>> + 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).
Ok
Cheers
Bertrand
>
> Cheers,
>
> --
> Julien Grall
