Re: [Xen-devel] [PATCH v2 05/41] acpi : add helper function for mapping memory

[Parth Dixit <parth.dixit@xxxxxxxxxx>]

On 18 May 2015 at 18:56, Julien Grall <julien.grall@xxxxxxxxxx> 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel