Re: [PATCH 13/22] xen/x86: Add support for the PMAP

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.01.2023 18:50, Julien Grall wrote:
> On 05/01/2023 16:46, Jan Beulich wrote:
>> On 16.12.2022 12:48, Julien Grall wrote:
>>> --- a/xen/arch/x86/include/asm/fixmap.h
>>> +++ b/xen/arch/x86/include/asm/fixmap.h
>>> @@ -21,6 +21,8 @@
>>>   
>>>   #include <xen/acpi.h>
>>>   #include <xen/pfn.h>
>>> +#include <xen/pmap.h>
>>> +
>>>   #include <asm/apicdef.h>
>>>   #include <asm/msi.h>
>>>   #include <acpi/apei.h>
>>> @@ -54,6 +56,8 @@ enum fixed_addresses {
>>>       FIX_XEN_SHARED_INFO,
>>>   #endif /* CONFIG_XEN_GUEST */
>>>       /* Everything else should go further down. */
>>> +    FIX_PMAP_BEGIN,
>>> +    FIX_PMAP_END = FIX_PMAP_BEGIN + NUM_FIX_PMAP,
>>
>> ... you've inserted the new entries after the respective comment? Is
>> there a reason you don't insert farther towards the end of this
>> enumeration?
> 
> I will answer this below.
> 
>>
>>> --- /dev/null
>>> +++ b/xen/arch/x86/include/asm/pmap.h
>>> @@ -0,0 +1,25 @@
>>> +#ifndef __ASM_PMAP_H__
>>> +#define __ASM_PMAP_H__
>>> +
>>> +#include <asm/fixmap.h>
>>> +
>>> +static inline void arch_pmap_map(unsigned int slot, mfn_t mfn)
>>> +{
>>> +    unsigned long linear = (unsigned long)fix_to_virt(slot);
>>> +    l1_pgentry_t *pl1e = &l1_fixmap[l1_table_offset(linear)];
>>> +
>>> +    ASSERT(!(l1e_get_flags(*pl1e) & _PAGE_PRESENT));
>>> +
>>> +    l1e_write_atomic(pl1e, l1e_from_mfn(mfn, PAGE_HYPERVISOR));
>>> +}
>>> +
>>> +static inline void arch_pmap_unmap(unsigned int slot)
>>> +{
>>> +    unsigned long linear = (unsigned long)fix_to_virt(slot);
>>> +    l1_pgentry_t *pl1e = &l1_fixmap[l1_table_offset(linear)];
>>> +
>>> +    l1e_write_atomic(pl1e, l1e_empty());
>>> +    flush_tlb_one_local(linear);
>>> +}
>>
>> You're effectively open-coding {set,clear}_fixmap(), just without
>> the L1 table allocation (should such be necessary). If you depend
>> on using the build-time L1 table, then you need to move your
>> entries ahead of said comment.
> 
> So the problem is less about the allocation be more the fact that we 
> can't use map_pages_to_xen() because it would call pmap_map().
> 
> So we need to break the loop. Hence why set_fixmap()/clear_fixmap() are 
> open-coded.
> 
> And indeed, we would need to rely on the build-time L1 table in this 
> case. So I will move the entries earlier.

Additionally we will now need to (finally) gain a build-time check that
all "early" entries actually fit in the static L1 table. XHCI has pushed
us quite a bit up here, and I could see us considering to alter (bump)
the number of PMAP entries.

>> But independent of that you want
>> to either use the existing macros / functions, or explain why you
>> can't.
> 
> This is explained in the caller of arch_pmap*():
> 
>      /*
>       * We cannot use set_fixmap() here. We use PMAP when the domain map
>       * page infrastructure is not yet initialized, so 
> map_pages_to_xen() called
>       * by set_fixmap() needs to map pages on demand, which then calls 
> pmap()
>       * again, resulting in a loop. Modify the PTEs directly instead. 
> The same
>       * is true for pmap_unmap().
>       */
> 
> The comment is valid for Arm, x86 and (I would expect in the future) 
> RISC-V because the page-tables may be allocated in domheap (so not 
> always mapped).
> 
> So I don't feel this comment should be duplicated in the header. But I 
> can certainly explain it in the commit message.

Right, that's what I was after; I'm sorry for not having worded this
precisely enough.

Jan