Re: [PATCH V3 (resend) 07/19] xen/x86: Add support for the PMAP

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, May 13, 2024 at 01:40:34PM +0000, Elias El Yandouzi wrote:
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> PMAP will be used in a follow-up patch to bootstrap map domain
> page infrastructure -- we need some way to map pages to setup the
> mapcache without a direct map.
> 
> The functions pmap_{map, unmap} open code {set, clear}_fixmap to break
> the loop.
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> Signed-off-by: Elias El Yandouzi <eliasely@xxxxxxxxxx>
> 
> ----
> 
>     The PMAP infrastructure was upstream separately for Arm since
>     Hongyan sent the secret-free hypervisor series. So this is a new
>     patch to plumb the feature on x86.
> 
>     Changes in v2:
>         * Declare PMAP entries earlier in fixed_addresses
>         * Reword the commit message
> 
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index b4ec0e582e..56feb0c564 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -27,6 +27,7 @@ config X86
>       select HAS_PCI
>       select HAS_PCI_MSI
>       select HAS_PIRQ
> +     select HAS_PMAP

Shouldn't it be selected by HAS_SECRET_HIDING rather than being
unconditionally selected?

>       select HAS_SCHED_GRANULARITY
>       select HAS_SECRET_HIDING
>       select HAS_UBSAN
> diff --git a/xen/arch/x86/include/asm/fixmap.h 
> b/xen/arch/x86/include/asm/fixmap.h
> index 516ec3fa6c..a7ac365fc6 100644
> --- 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>
> @@ -53,6 +55,8 @@ enum fixed_addresses {
>      FIX_PV_CONSOLE,
>      FIX_XEN_SHARED_INFO,
>  #endif /* CONFIG_XEN_GUEST */
> +    FIX_PMAP_BEGIN,
> +    FIX_PMAP_END = FIX_PMAP_BEGIN + NUM_FIX_PMAP,

This would better have

#ifdef CONFIG_HAS_PMAP

guards?

>      /* Everything else should go further down. */
>      FIX_APIC_BASE,
>      FIX_IO_APIC_BASE_0,
> diff --git a/xen/arch/x86/include/asm/pmap.h b/xen/arch/x86/include/asm/pmap.h
> new file mode 100644
> index 0000000000..62746e191d
> --- /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);
> +}
> +
> +#endif /* __ASM_PMAP_H__ */

We usually add a:

/*
 * Local variables:
 * mode: C
 * c-file-style: "BSD"
 * c-basic-offset: 4
 * indent-tabs-mode: nil
 * End:
 */

Footer.  No strict requirement, but it's nice so that your editor
already picks the correct defaults for tabs &c.

Thanks, Roger.