From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v2 05/41] acpi : add helper function for mapping memory Date: Mon, 18 May 2015 14:26:43 +0100 Message-ID: <5559E893.4000303@citrix.com> References: <1431893048-5214-1-git-send-email-parth.dixit@linaro.org> <1431893048-5214-6-git-send-email-parth.dixit@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1431893048-5214-6-git-send-email-parth.dixit@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Parth Dixit , xen-devel@lists.xen.org Cc: keir@xen.org, ian.campbell@citrix.com, andrew.cooper3@citrix.com, tim@xen.org, julien.grall@citrix.com, stefano.stabellini@citrix.com, jbeulich@suse.com, christoffer.dall@linaro.org List-Id: xen-devel@lists.xenproject.org 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 > +#include > + > +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... > /* > * 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