Re: [PATCH 1/4] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()

[Jan Beulich <jbeulich@suse.com>]

On 26.09.2020 22:55, Julien Grall wrote:
> @@ -49,6 +53,12 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>      return ((char *) base + offset);
>  }
>  
> +bool __acpi_unmap_table(void *ptr, unsigned long size)
> +{
> +    return ( vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) &&
> +             vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE) );

Nit: If this wasn't Arm code, I'd ask for the parentheses to be
dropped altogether, but at least the stray blanks should go away
imo.

> --- a/xen/arch/x86/acpi/lib.c
> +++ b/xen/arch/x86/acpi/lib.c
> @@ -46,6 +46,10 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>  	if ((phys + size) <= (1 * 1024 * 1024))
>  		return __va(phys);
>  
> +	/* No arch specific implementation after early boot */
> +	if (system_state >= SYS_STATE_boot)
> +		return NULL;

Considering the code in context above, the comment isn't entirely
correct.

> @@ -66,6 +70,20 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>  	return ((char *) base + offset);
>  }
>  
> +bool __acpi_unmap_table(void *ptr, unsigned long size)

Is there anything standing in the way of making ptr "const void *"?

> +{
> +	unsigned long vaddr = (unsigned long)ptr;
> +
> +	if (vaddr >= DIRECTMAP_VIRT_START &&
> +	    vaddr < DIRECTMAP_VIRT_END) {
> +		ASSERT(!((__pa(ptr) + size - 1) >> 20));
> +		return true;
> +	}
> +
> +	return (vaddr >= __fix_to_virt(FIX_ACPI_END)) &&
> +		(vaddr < (__fix_to_virt(FIX_ACPI_BEGIN) + PAGE_SIZE));

Indentation is slightly wrong here. Also please consider reducing
the number of parentheses here; at the very least the return and
the earlier if() want to be consistent in when ones are(n't) used.

> --- a/xen/drivers/acpi/osl.c
> +++ b/xen/drivers/acpi/osl.c
> @@ -92,27 +92,29 @@ 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_boot) {
> -		mfn_t mfn = _mfn(PFN_DOWN(phys));
> -		unsigned int offs = phys & (PAGE_SIZE - 1);
> -
> -		/* The low first Mb is always mapped on x86. */
> -		if (IS_ENABLED(CONFIG_X86) && !((phys + size - 1) >> 20))
> -			return __va(phys);
> -		return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
> -			      ACPI_MAP_MEM_ATTR, VMAP_DEFAULT) + offs;
> -	}
> -	return __acpi_map_table(phys, size);
> +	void *ptr;
> +	mfn_t mfn = _mfn(PFN_DOWN(phys));
> +	unsigned int offs = phys & (PAGE_SIZE - 1);

Open-coding PAGE_OFFSET()?

> +	/* Try the arch specific implementation first */
> +	ptr = __acpi_map_table(phys, size);
> +	if (ptr)
> +		return ptr;
> +
> +	/* No common implementation for early boot map */
> +	if (unlikely(system_state < SYS_STATE_boot))
> +	     return NULL;

Consistently hard tabs for indentation here, please.

> +	ptr = __vmap(&mfn, PFN_UP(offs + size), 1, 1,
> +		     ACPI_MAP_MEM_ATTR, VMAP_DEFAULT);
> +
> +	return !ptr ? NULL : (ptr + offs);

Slightly odd that you don't let the success case go first, the
more that it's minimally shorter:

	return ptr ? ptr + offs : NULL;

Jan