Re: [PATCH v3 6/7] xen: allow mapping ACPI data using a different physical address

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

On 10.09.24 14:34, Jan Beulich wrote:
> On 10.09.2024 12:39, Juergen Gross wrote:
> > When running as a Xen PV dom0 the system needs to map ACPI data of the
> > host using host physical addresses, while those addresses can conflict
> > with the guest physical addresses of the loaded linux kernel. The same
> > problem might apply in case a PV guest is configured to use the host
> > memory map.
> >
> > This conflict can be solved by mapping the ACPI data to a different
> > guest physical address, but mapping the data via acpi_os_ioremap()
> > must still be possible using the host physical address, as this
> > address might be generated by AML when referencing some of the ACPI
> > data.
> >
> > When configured to support running as a Xen PV domain, have an
> > implementation of acpi_os_ioremap() being aware of the possibility to
> > need above mentioned translation of a host physical address to the
> > guest physical address.
> >
> > This modification requires to fix some #include of asm/acpi.h in x86
> > code to use linux/acpi.h instead.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> with a request to comment a tiny bit more:
>
> > @@ -836,6 +837,33 @@ void __init xen_do_remap_nonram(void)
> >         pr_info("Remapped %u non-RAM page(s)\n", remapped);
> >   }
> >   
> > +#ifdef CONFIG_ACPI
> > +/*
> > + * Xen variant of acpi_os_ioremap() taking potentially remapped non-RAM
> > + * regions into acount.
> (Nit: account)
Indeed.

> > + * Any attempt to map an area crossing a remap boundary will produce a
> > + * WARN() splat.
> > + */
> > +static void __iomem *xen_acpi_os_ioremap(acpi_physical_address phys,
> > +                                        acpi_size size)
> > +{
> > +       unsigned int i;
> > +       const struct nonram_remap *remap = xen_nonram_remap;
> > +
> > +       for (i = 0; i < nr_nonram_remap; i++) {
> > +               if (phys + size > remap->maddr &&
> > +                   phys < remap->maddr + remap->size) {
> > +                       WARN_ON(phys < remap->maddr ||
> > +                               phys + size > remap->maddr + remap->size);
> > +                       phys = remap->paddr + phys - remap->maddr;
> This might be slightly easier / more logical to read as
>
>                         phys += remap->paddr - remap->maddr;
>
> Also because of "phys" not consistently expressing a physical address
> (when you need convert it, the incoming value is a machine address) a
> comment may help here. In fact at the first glance (and despite having
> seen the code before) I thought the translation was done the wrong way
> round, simply because of the name of the variable.
Will add a comment and change the line as you suggest.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature