Re: [PATCH early-RFC 3/5] xen/arm: mm: Introduce helpers to prepare/enable/disable the identity mapping

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien

> On 25 Mar 2022, at 14:48, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> 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.

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