On 18 May 2015 at 18:56, Julien Grall <julien.gr...@citrix.com> wrote:
> Hi Parth,
>
> On 17/05/15 21:03, Parth Dixit wrote:
> > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> > index 935999e..096e9ef 100644
> > --- a/xen/arch/arm/Makefile
> > +++ b/xen/arch/arm/Makefile
> > @@ -2,6 +2,7 @@ subdir-$(arm32) += arm32
> > subdir-$(arm64) += arm64
> > subdir-y += platforms
> > subdir-$(arm64) += efi
> > +subdir-$(HAS_ACPI) += acpi
> >
> > obj-$(EARLY_PRINTK) += early_printk.o
> > obj-y += cpu.o
> > diff --git a/xen/arch/arm/acpi/Makefile b/xen/arch/arm/acpi/Makefile
> > new file mode 100644
> > index 0000000..b5be22d
> > --- /dev/null
> > +++ b/xen/arch/arm/acpi/Makefile
> > @@ -0,0 +1 @@
> > +obj-y += lib.o
> > diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c
> > new file mode 100644
> > index 0000000..650beed
> > --- /dev/null
> > +++ b/xen/arch/arm/acpi/lib.c
> > @@ -0,0 +1,8 @@
> > +#include <xen/acpi.h>
> > +#include <asm/mm.h>
> > +
> > +void __iomem *
> > +acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
> > +{
> > + return __va(phys);
> > +}
>
> I would have prefer two distinct patch: one for the refactoring of
> acpi_os_map_memory and the other for implementing the ARM part
> explaining why only using __va.
>
> __va should only be used when the memory is direct-mapped to Xen (i.e
> accessible directly). On ARM64, this only the case for the RAM. Can you
> confirm that ACPI will always reside to the RAM?
>
> I already asked the same question on the previous version but got no
> answer from you...
>
> I did not found any document which says it will always reside in RAM or
otherwise..
> > /*
> > * Important Safety Note: The fixed ACPI page numbers are *subtracted*
> > * from the fixed base. That's why we start at FIX_ACPI_END and
> > diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
> > index 93c983c..958caae 100644
> > --- a/xen/drivers/acpi/osl.c
> > +++ b/xen/drivers/acpi/osl.c
> > @@ -87,16 +87,7 @@ acpi_physical_address __init
> acpi_os_get_root_pointer(void)
> > void __iomem *
> > acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
> > {
> > - if (system_state >= SYS_STATE_active) {
> > - unsigned long pfn = PFN_DOWN(phys);
> > - unsigned int offs = phys & (PAGE_SIZE - 1);
> > -
> > - /* The low first Mb is always mapped. */
> > - if ( !((phys + size - 1) >> 20) )
> > - return __va(phys);
> > - return __vmap(&pfn, PFN_UP(offs + size), 1, 1,
> PAGE_HYPERVISOR_NOCACHE) + offs;
> > - }
> > - return __acpi_map_table(phys, size);
> > + return acpi_os_map_iomem(phys, size);
>
> The naming is wrong. It's really hard to differentiate
> acpi_os_map_memory from acpi_os_map_iomem. I would rename to something
> more meaningful such as arch_acpi_os_map_memory.
>
> Although, given that acpi_os_map_memory only call acpi_os_map_iomem. I
> would move acpi_os_map_memory per-architecture. FWIW, it's what Linux does.
>
> --
> Julien Grall
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
