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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

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.
> 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.
Cheers,

--
Julien Grall