[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d32735a1-79a9-43c1-b4a6-15ddbc203646@suse.com>
Date: Tue, 20 Aug 2024 11:38:08 +0200
From: Jan Beulich <jbeulich@...e.com>
To: Juergen Gross <jgross@...e.com>
Cc: Boris Ostrovsky <boris.ostrovsky@...cle.com>,
Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>,
"H. Peter Anvin" <hpa@...or.com>, xen-devel@...ts.xenproject.org,
linux-kernel@...r.kernel.org, x86@...nel.org
Subject: Re: [PATCH v2 5/7] xen: add capability to remap non-RAM pages to
different PFNs
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@...e.com>
> ---
> 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
Powered by blists - more mailing lists