Re: [PATCH v2 1/2] x86/ioremap: introduce helper to implement xxx_is_setup_data()

[Tom Lendacky <thomas.lendacky@amd.com>]

On 11/17/24 19:08, Baoquan He wrote:
> Functions memremap_is_setup_data() and early_memremap_is_setup_data()
> share completely the same process and handling, except of the
> different memremap/unmap invocations.
> 
> So add helper __memremap_is_setup_data() to extract the common part,
> parameter 'early' is used to decide what kind of memremap/unmap
> APIs are called. This simplifies codes a lot by removing the duplicated
> codes, and also removes the similar code comment above them.
> 
> And '__ref' is added to __memremap_is_setup_data() to suppress below
> section mismatch warning:
> 
> ARNING: modpost: vmlinux: section mismatch in reference: __memremap_is_setup_data+0x5f (section: .text) ->
> early_memunmap (section: .init.text)
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  arch/x86/mm/ioremap.c | 108 +++++++++++++++---------------------------
>  1 file changed, 38 insertions(+), 70 deletions(-)
> 
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index 8d29163568a7..68d78e2b1203 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -628,12 +628,13 @@ static bool memremap_is_efi_data(resource_size_t phys_addr,
>  	return false;
>  }
>  
> +#define SD_SIZE sizeof(struct setup_data)

Nit, I still think you should use "sizeof(*data)" in the code instead of
creating a #define.

>  /*
>   * Examine the physical address to determine if it is boot data by checking
>   * it against the boot params setup_data chain.
>   */
> -static bool memremap_is_setup_data(resource_size_t phys_addr,
> -				   unsigned long size)
> +static bool __ref __memremap_is_setup_data(resource_size_t phys_addr,

Oh, I see why the __ref is needed now, because this calls an __init
function based on the early bool.

While this nicely consolidates the checking, I'll let the x86
maintainers decide whether they like that an __init function is calling
a non __init function.

> +						bool early)
>  {
>  	struct setup_indirect *indirect;
>  	struct setup_data *data;
> @@ -641,31 +642,45 @@ static bool memremap_is_setup_data(resource_size_t phys_addr,
>  
>  	paddr = boot_params.hdr.setup_data;
>  	while (paddr) {
> -		unsigned int len;
> +		unsigned int len, size;
>  
>  		if (phys_addr == paddr)
>  			return true;
>  
> -		data = memremap(paddr, sizeof(*data),
> -				MEMREMAP_WB | MEMREMAP_DEC);
> +		if (early)
> +			data = early_memremap_decrypted(paddr, SD_SIZE);
> +		else
> +			data = memremap(paddr, SD_SIZE,
> +					MEMREMAP_WB | MEMREMAP_DEC);
>  		if (!data) {
>  			pr_warn("failed to memremap setup_data entry\n");
>  			return false;
>  		}
>  
> +		size = SD_SIZE;
> +
>  		paddr_next = data->next;
>  		len = data->len;
>  
>  		if ((phys_addr > paddr) &&
> -		    (phys_addr < (paddr + sizeof(struct setup_data) + len))) {
> -			memunmap(data);
> +		    (phys_addr < (paddr + SD_SIZE + len))) {
> +			if (early)
> +				early_memunmap(data, SD_SIZE);
> +			else
> +				memunmap(data);
>  			return true;
>  		}
>  
>  		if (data->type == SETUP_INDIRECT) {
> -			memunmap(data);
> -			data = memremap(paddr, sizeof(*data) + len,
> -					MEMREMAP_WB | MEMREMAP_DEC);
> +			size += len;
> +			if (early) {
> +				early_memunmap(data, SD_SIZE);
> +				data = early_memremap_decrypted(paddr, size);
> +			} else {
> +				memunmap(data);
> +				data = memremap(paddr, size,
> +						MEMREMAP_WB | MEMREMAP_DEC);
> +			}
>  			if (!data) {
>  				pr_warn("failed to memremap indirect setup_data\n");
>  				return false;
> @@ -679,7 +694,10 @@ static bool memremap_is_setup_data(resource_size_t phys_addr,
>  			}
>  		}
>  
> -		memunmap(data);
> +		if (early)
> +			early_memunmap(data, size);
> +		else
> +			memunmap(data);
>  
>  		if ((phys_addr > paddr) && (phys_addr < (paddr + len)))
>  			return true;
> @@ -689,68 +707,18 @@ static bool memremap_is_setup_data(resource_size_t phys_addr,
>  
>  	return false;
>  }
> +#undef SD_SIZE
>  
> -/*
> - * Examine the physical address to determine if it is boot data by checking
> - * it against the boot params setup_data chain (early boot version).
> - */
> -static bool __init early_memremap_is_setup_data(resource_size_t phys_addr,
> -						unsigned long size)
> +static bool memremap_is_setup_data(resource_size_t phys_addr,
> +				   unsigned long size)
>  {
> -	struct setup_indirect *indirect;
> -	struct setup_data *data;
> -	u64 paddr, paddr_next;
> -
> -	paddr = boot_params.hdr.setup_data;
> -	while (paddr) {
> -		unsigned int len, size;
> -
> -		if (phys_addr == paddr)
> -			return true;
> -
> -		data = early_memremap_decrypted(paddr, sizeof(*data));
> -		if (!data) {
> -			pr_warn("failed to early memremap setup_data entry\n");
> -			return false;
> -		}
> -
> -		size = sizeof(*data);
> -
> -		paddr_next = data->next;
> -		len = data->len;
> -
> -		if ((phys_addr > paddr) &&
> -		    (phys_addr < (paddr + sizeof(struct setup_data) + len))) {
> -			early_memunmap(data, sizeof(*data));
> -			return true;
> -		}
> -
> -		if (data->type == SETUP_INDIRECT) {
> -			size += len;
> -			early_memunmap(data, sizeof(*data));
> -			data = early_memremap_decrypted(paddr, size);
> -			if (!data) {
> -				pr_warn("failed to early memremap indirect setup_data\n");
> -				return false;
> -			}
> -
> -			indirect = (struct setup_indirect *)data->data;
> -
> -			if (indirect->type != SETUP_INDIRECT) {
> -				paddr = indirect->addr;
> -				len = indirect->len;
> -			}
> -		}
> -
> -		early_memunmap(data, size);
> -
> -		if ((phys_addr > paddr) && (phys_addr < (paddr + len)))
> -			return true;
> -
> -		paddr = paddr_next;
> -	}
> +	return __memremap_is_setup_data(phys_addr, false);
> +}
>  
> -	return false;
> +static bool early_memremap_is_setup_data(resource_size_t phys_addr,

This should retain the original __init reference.

Thanks,
Tom

> +						unsigned long size)
> +{
> +	return __memremap_is_setup_data(phys_addr, true);
>  }
>  
>  /*