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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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...

>  /*
>   * 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