Re: [PATCH v2 5/7] xen: add capability to remap non-RAM pages to different PFNs

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.08.2024 10:20, Juergen Gross wrote:
> When running as a Xen PV dom0 it can happen that the kernel is being
> loaded to a guest physical address conflicting with the host memory
> map.
> 
> In order to be able to resolve this conflict, add the capability to
> remap non-RAM areas to different guest PFNs. A function to use this
> remapping information for other purposes than doing the remap will be
> added when needed.
> 
> As the number of conflicts should be rather low (currently only
> machines with max. 1 conflict are known), save the remap data in a
> small statically allocated array.
> 
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
> V2:
> - split off from patch 5 of V1 of the series
> - moved to p2m.c
> ---
>  arch/x86/xen/p2m.c     | 65 ++++++++++++++++++++++++++++++++++++++++++
>  arch/x86/xen/xen-ops.h |  3 ++
>  2 files changed, 68 insertions(+)
> 
> diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
> index 7c735b730acd..bb55e0fe1a04 100644
> --- a/arch/x86/xen/p2m.c
> +++ b/arch/x86/xen/p2m.c
> @@ -80,6 +80,7 @@
>  #include <asm/xen/hypervisor.h>
>  #include <xen/balloon.h>
>  #include <xen/grant_table.h>
> +#include <xen/hvc-console.h>
>  
>  #include "xen-ops.h"
>  
> @@ -792,6 +793,70 @@ int clear_foreign_p2m_mapping(struct 
> gnttab_unmap_grant_ref *unmap_ops,
>       return ret;
>  }
>  
> +/* Remapped non-RAM areas */
> +#define NR_NONRAM_REMAP 4
> +static struct nonram_remap {
> +     phys_addr_t maddr;
> +     phys_addr_t paddr;
> +     unsigned long size;

size_t?

> +} xen_nonram_remap[NR_NONRAM_REMAP];
> +static unsigned int nr_nonram_remap;

Both __initdata? Or, considering patch 6, at least __ro_after_init?

> +/*
> + * Do the real remapping of non-RAM regions as specified in the
> + * xen_nonram_remap[] array.
> + * In case of an error just crash the system.
> + */
> +void __init xen_do_remap_nonram(void)
> +{
> +     unsigned int i;
> +     unsigned int remapped = 0;
> +     struct nonram_remap *remap = xen_nonram_remap;
> +     unsigned long pfn, mfn, len;
> +
> +     if (!nr_nonram_remap)
> +             return;
> +
> +     for (i = 0; i < nr_nonram_remap; i++) {
> +             pfn = PFN_DOWN(remap->paddr);
> +             mfn = PFN_DOWN(remap->maddr);
> +             for (len = 0; len < remap->size; len += PAGE_SIZE) {
> +                     if (!set_phys_to_machine(pfn, mfn)) {
> +                             pr_err("Failed to set p2m mapping for pfn=%ld 
> mfn=%ld\n",

I'm not convinced that frame numbers logged in decimal are overly useful.

> +                                    pfn, mfn);
> +                             BUG();
> +                     }
> +
> +                     pfn++;
> +                     mfn++;
> +                     remapped++;
> +             }
> +
> +             remap++;
> +     }
> +
> +     pr_info("Remapped %u non-RAM page(s)\n", remapped);

This message may be useful in a log also when nothing was remapped - maybe
drop the initial if()?

> +}
> +
> +/*
> + * Add a new non-RAM remap entry.
> + * In case of no free entry found, just crash the system.
> + */
> +void __init xen_add_remap_nonram(phys_addr_t maddr, phys_addr_t paddr,
> +                              unsigned long size)
> +{
> +     if (nr_nonram_remap == NR_NONRAM_REMAP) {
> +             xen_raw_console_write("Number of required E820 entry remapping 
> actions exceed maximum value\n");
> +             BUG();
> +     }
> +
> +     xen_nonram_remap[nr_nonram_remap].maddr = maddr;
> +     xen_nonram_remap[nr_nonram_remap].paddr = paddr;
> +     xen_nonram_remap[nr_nonram_remap].size = size;
> +
> +     nr_nonram_remap++;
> +}

You don't enforce any constraints on the addresses / size here. With
this the loop in xen_do_remap_nonram() may terminate too early if non-
page-aligned values were passed into here. Both addresses not having
the same offset-into-page may also end up anomalous. Might be worth
switching to frame numbers / number-of-pages for the tracking struct.

Jan