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
