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

[Jürgen Groß <jgross@xxxxxxxx>]

On 10.09.24 14:26, Jan Beulich wrote:
> On 10.09.2024 12:39, 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>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> with two cosmetic remarks:
>
> > @@ -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;
> > +       size_t size;
> > +} xen_nonram_remap[NR_NONRAM_REMAP] __ro_after_init;
> > +static unsigned int nr_nonram_remap __ro_after_init;
> I take it this is the canonical placement of section attributes in the
> kernel? (In Xen I'd ask for the attributes to be moved ahead of the
> identifiers being declared.)
I didn't find an explicit mentioning in the coding style, but most
examples I've found place the section attribute after the name of the
variable.

> > +/*
> > + * 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;
> > +       const struct nonram_remap *remap = xen_nonram_remap;
> > +       unsigned long pfn, mfn, end_pfn;
> > +
> > +       for (i = 0; i < nr_nonram_remap; i++) {
> > +               end_pfn = PFN_UP(remap->paddr + remap->size);
> > +               pfn = PFN_DOWN(remap->paddr);
> > +               mfn = PFN_DOWN(remap->maddr);
> > +               while (pfn < end_pfn) {
> > +                       if (!set_phys_to_machine(pfn, mfn)) {
> > +                               pr_err("Failed to set p2m mapping for pfn=%lx 
> > mfn=%lx\n",
> > +                                      pfn, mfn);
> > +                               BUG();
> Wouldn't panic() get you both in one operation? Or do you expect the call
> trace / register state to be of immediate relevance for analysis?
You are right, in this case panic() is a better option.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature