[PATCH v2 0/2] x86/ioremap: clean up the mess in xxx_is_setup_data

[PATCH v2 0/2] x86/ioremap: clean up the mess in xxx_is_setup_data

[Baoquan He]

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. The code can be extracted and put
into a helper function __memremap_is_setup_data().

And parameter 'size' is unused in implementation of memremap_is_efi_data(),
memremap_is_setup_data and early_memremap_is_setup_data().

This patchset is made to clean them up. It sits on top of tip/x86/urgent
commit 8d9ffb2fe65a ("x86/mm: Fix a kdump kernel failure on SME system
when CONFIG_IMA_KEXEC=y")

v1->v2:
- Remove __init from helper __memremap_is_setup_data(), add __ref to
  helper suppress mismatch section warning.
- Merge the old patch 1 and 2 into one patch. 
  - Both are suggested by Tom during reviewing. Thanks to him.

Baoquan He (2):
  x86/ioremap: introduce helper to implement xxx_is_setup_data()
  x86/mm: clean up unused parameters of functions

 arch/x86/mm/ioremap.c | 117 +++++++++++++++---------------------------
 1 file changed, 41 insertions(+), 76 deletions(-)

-- 
2.41.0

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

[Baoquan He]

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)
 /*
  * 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,
+						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,
+						unsigned long size)
+{
+	return __memremap_is_setup_data(phys_addr, true);
 }
 
 /*
-- 
2.41.0

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

[Tom Lendacky]

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);
>  }
>  
>  /*

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

[Baoquan He]

On 11/18/24 at 09:19am, Tom Lendacky wrote:
> 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.

Thanks for reviewing, Tom.

Boris suggested this. Both is fine to me. If there is indeed a tiny
preference, I would choose SD_SIZE. It's going a bit far, but not too
far.

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

Exactly, I explained in another thread replying to you, it could be
ignored.

> 
> 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.
...... snip.......
> > -	return false;
> > +static bool early_memremap_is_setup_data(resource_size_t phys_addr,
> 
> This should retain the original __init reference.

OK, so you suggest they are like below, right?

static bool __ref __memremap_is_setup_data(resource_size_t phys_addr,
                                                bool early)
{
	......
}

static bool memremap_is_setup_data(resource_size_t phys_addr)
{
        return __memremap_is_setup_data(phys_addr, false);
}
 
static bool __init early_memremap_is_setup_data(resource_size_t phys_addr)
{
        return __memremap_is_setup_data(phys_addr, true);
}

I can make v3 if we all agree on this.

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

[Ingo Molnar]

* Baoquan He <bhe@redhat.com> wrote:

> On 11/18/24 at 09:19am, Tom Lendacky wrote:
> > 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.
> 
> Thanks for reviewing, Tom.
> 
> Boris suggested this. Both is fine to me. If there is indeed a tiny
> preference, I would choose SD_SIZE. It's going a bit far, but not too
> far.

Yeah, I'd prefer Boris's SD_SIZE suggestion too: while *normally* we'd 
use the 'sizeof(*data)' pattern, this particular size repeats a number 
of times and not all contexts are obvious - so abstracting it out into 
a trivial define looks like the proper cleanup.

Maybe such material changes should be done in a separate patch though:

   x86/ioremap: Introduce helper to implement xxx_is_setup_data()
   x86/ioremap: Clean up size calculations in xxx_is_setup_data()

... or so, where the first patch is a trivial refactoring that keeps 
the existing patterns - which would make the series easier to review.

Thanks,

	Ingo

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

[Baoquan He]

On 11/19/24 at 11:55am, Ingo Molnar wrote:
> 
> * Baoquan He <bhe@redhat.com> wrote:
> 
> > On 11/18/24 at 09:19am, Tom Lendacky wrote:
> > > 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.
> > 
> > Thanks for reviewing, Tom.
> > 
> > Boris suggested this. Both is fine to me. If there is indeed a tiny
> > preference, I would choose SD_SIZE. It's going a bit far, but not too
> > far.
> 
> Yeah, I'd prefer Boris's SD_SIZE suggestion too: while *normally* we'd 
> use the 'sizeof(*data)' pattern, this particular size repeats a number 
> of times and not all contexts are obvious - so abstracting it out into 
> a trivial define looks like the proper cleanup.

Totally agree.

> 
> Maybe such material changes should be done in a separate patch though:
> 
>    x86/ioremap: Introduce helper to implement xxx_is_setup_data()
>    x86/ioremap: Clean up size calculations in xxx_is_setup_data()
> 
> ... or so, where the first patch is a trivial refactoring that keeps 
> the existing patterns - which would make the series easier to review.

OK, will do like this if Tom doesn't oppose it.

During v1, Tom suggested squashing the helper introducing patch and
helper using patch into one patch. I personally prefer splitting code
changes into multiple independent units so that reviewing is focused on
the controversial change, but not the whole big patch including many
controversial points is posted again and again. 

Thanks a lot for careful reviewing and great suggestions.

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

[Baoquan He]

On 11/19/24 at 11:07am, Baoquan He wrote:
> On 11/18/24 at 09:19am, Tom Lendacky wrote:
> > On 11/17/24 19:08, Baoquan He wrote:
......snip.......
> > >  /*
> > >   * 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.
> 
> Exactly, I explained in another thread replying to you, it could be
> ignored.
> 
> > 
> > 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.
> ...... snip.......
> > > -	return false;
> > > +static bool early_memremap_is_setup_data(resource_size_t phys_addr,

Hi Tom, Ingo,

What's your suggestion about the __init and __ref adding/removing on
below functions as Tom pointed out?

> > 
> > This should retain the original __init reference.
> 
> OK, so you suggest they are like below, right?
> 
> static bool __ref __memremap_is_setup_data(resource_size_t phys_addr,
>                                                 bool early)
> {
> 	......
> }
> 
> static bool memremap_is_setup_data(resource_size_t phys_addr)
> {
>         return __memremap_is_setup_data(phys_addr, false);
> }
>  
> static bool __init early_memremap_is_setup_data(resource_size_t phys_addr)
> {
>         return __memremap_is_setup_data(phys_addr, true);
> }
> 
> I can make v3 if we all agree on this.
> 

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

[Ingo Molnar]

* Tom Lendacky <thomas.lendacky@amd.com> wrote:

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

So why would it be a problem? Only non-__init calling __init is a bug, 
because __init functions cease to exist after early bootup. Also, 
calling certain kernel subsystems too early, before they are 
initialized, is a bug as well.

But calling non-__init functions that have initialized already is like 
totally normal: printk() for example, but also all locking facilities, 
etc.

Am I missing anything here?

Thanks,

	Ingo

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

[Tom Lendacky]

On 11/20/24 02:25, Ingo Molnar wrote:
> 
> * Tom Lendacky <thomas.lendacky@amd.com> wrote:
> 
>>>  /*
>>>   * 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.
> 
> So why would it be a problem? Only non-__init calling __init is a bug, 
> because __init functions cease to exist after early bootup. Also, 
> calling certain kernel subsystems too early, before they are 
> initialized, is a bug as well.

I brought it up because that is what could happen if the wrong boolean
value is supplied to the helper function. The helper function is marked
non-__init but calls a __init function if the boolean value is true, hence
the need for the __ref tagging.

But, I don't anticipate that this helper will be called by anything else
than what is currently calling it and the proper boolean values are set on
those calls.

I just wanted to raise awareness. I'm ok with using __ref, just wanted to
make sure everyone else is, too.

Thanks,
Tom

> 
> But calling non-__init functions that have initialized already is like 
> totally normal: printk() for example, but also all locking facilities, 
> etc.
> 
> Am I missing anything here?
> 
> Thanks,
> 
> 	Ingo

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

[Ingo Molnar]

* Tom Lendacky <thomas.lendacky@amd.com> wrote:

> On 11/20/24 02:25, Ingo Molnar wrote:
> > 
> > * Tom Lendacky <thomas.lendacky@amd.com> wrote:
> > 
> >>>  /*
> >>>   * 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.
> > 
> > So why would it be a problem? Only non-__init calling __init is a bug, 
> > because __init functions cease to exist after early bootup. Also, 
> > calling certain kernel subsystems too early, before they are 
> > initialized, is a bug as well.
> 
> I brought it up because that is what could happen if the wrong boolean
> value is supplied to the helper function. The helper function is marked
> non-__init but calls a __init function if the boolean value is true, hence
> the need for the __ref tagging.

Oh, so I misunderstood your point, because you typoed the direction:

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

The problem is the inverse: that a non-__init generic facility may be 
calling an __init function if the wrong flag is supplied. As you wrote 
a sentence earlier, but I only responded to this paragraph :-/

So yeah, that's a fragility indeed - which happens sometimes when 
generic MM facilities share code (I think 
mm/sparse.c::section_deactivate() is similar), but I tend to agree that 
this pattern could perhaps be improved:

+                       if (early)
+                               early_memunmap(data, SD_SIZE);
+                       else
+                               memunmap(data);

Could we perhaps un-__init early_memunmap(), and call memunmap() if 
it's in a late context? (Also early_memremap_decrypted().)

That way this code could just use early_memunmap() and 
early_memremap_decrypted() and skip the boolean complication?

> But, I don't anticipate that this helper will be called by anything 
> else than what is currently calling it and the proper boolean values 
> are set on those calls.
> 
> I just wanted to raise awareness. I'm ok with using __ref, just 
> wanted to make sure everyone else is, too.

It's a fair argument I misunderstood :-)

Thanks,

	Ingo

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

[Baoquan He]

On 11/25/24 at 10:07am, Ingo Molnar wrote:
> 
> * Tom Lendacky <thomas.lendacky@amd.com> wrote:
> 
> > On 11/20/24 02:25, Ingo Molnar wrote:
> > > 
> > > * Tom Lendacky <thomas.lendacky@amd.com> wrote:
> > > 
> > >>>  /*
> > >>>   * 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.
> > > 
> > > So why would it be a problem? Only non-__init calling __init is a bug, 
> > > because __init functions cease to exist after early bootup. Also, 
> > > calling certain kernel subsystems too early, before they are 
> > > initialized, is a bug as well.
> > 
> > I brought it up because that is what could happen if the wrong boolean
> > value is supplied to the helper function. The helper function is marked
> > non-__init but calls a __init function if the boolean value is true, hence
> > the need for the __ref tagging.
> 
> Oh, so I misunderstood your point, because you typoed the direction:
> 
>   > >> 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.
> 
> The problem is the inverse: that a non-__init generic facility may be 
> calling an __init function if the wrong flag is supplied. As you wrote 
> a sentence earlier, but I only responded to this paragraph :-/
> 
> So yeah, that's a fragility indeed - which happens sometimes when 
> generic MM facilities share code (I think 
> mm/sparse.c::section_deactivate() is similar), but I tend to agree that 
> this pattern could perhaps be improved:
> 
> +                       if (early)
> +                               early_memunmap(data, SD_SIZE);
> +                       else
> +                               memunmap(data);
> 
> Could we perhaps un-__init early_memunmap(), and call memunmap() if 
> it's in a late context? (Also early_memremap_decrypted().)
> 
> That way this code could just use early_memunmap() and 
> early_memremap_decrypted() and skip the boolean complication?

If trying to skip the bool 'early' checking, we can possibly do it as
below:

                       if (system_state < SYSTEM_FREEING_INITMEM)
                               early_memunmap(data, SD_SIZE);
                       else
                               memunmap(data);

As for un-__init early_memunmap(), do you mean we just remove the __init
from early_memunmap()/early_memremap_decrypted() which are located in
kernel/early_ioremap.c and arch/x86/mm/ioremap.c? This doesn't cost much
memory, while it may make people complain if we only want to optimize
code when doing reducing code duplication, my humble opinion. Not sure
if I got your suggestion correctly.

[PATCH v2 2/2] x86/mm: clean up unused parameters of functions

[Baoquan He]

For functions memremap_is_efi_data(), memremap_is_setup_data and
early_memremap_is_setup_data(), their parameter 'size' is not used
and sometime cause confusion. Remove it now.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 arch/x86/mm/ioremap.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 68d78e2b1203..569a62b047a0 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -593,8 +593,7 @@ static bool memremap_should_map_decrypted(resource_size_t phys_addr,
  * Examine the physical address to determine if it is EFI data. Check
  * it against the boot params structure and EFI tables and memory types.
  */
-static bool memremap_is_efi_data(resource_size_t phys_addr,
-				 unsigned long size)
+static bool memremap_is_efi_data(resource_size_t phys_addr)
 {
 	u64 paddr;
 
@@ -709,14 +708,12 @@ static bool __ref __memremap_is_setup_data(resource_size_t phys_addr,
 }
 #undef SD_SIZE
 
-static bool memremap_is_setup_data(resource_size_t phys_addr,
-				   unsigned long size)
+static bool memremap_is_setup_data(resource_size_t phys_addr)
 {
 	return __memremap_is_setup_data(phys_addr, false);
 }
 
-static bool early_memremap_is_setup_data(resource_size_t phys_addr,
-						unsigned long size)
+static bool early_memremap_is_setup_data(resource_size_t phys_addr)
 {
 	return __memremap_is_setup_data(phys_addr, true);
 }
@@ -739,8 +736,8 @@ bool arch_memremap_can_ram_remap(resource_size_t phys_addr, unsigned long size,
 		return false;
 
 	if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT)) {
-		if (memremap_is_setup_data(phys_addr, size) ||
-		    memremap_is_efi_data(phys_addr, size))
+		if (memremap_is_setup_data(phys_addr) ||
+		    memremap_is_efi_data(phys_addr))
 			return false;
 	}
 
@@ -765,8 +762,8 @@ pgprot_t __init early_memremap_pgprot_adjust(resource_size_t phys_addr,
 	encrypted_prot = true;
 
 	if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT)) {
-		if (early_memremap_is_setup_data(phys_addr, size) ||
-		    memremap_is_efi_data(phys_addr, size))
+		if (early_memremap_is_setup_data(phys_addr) ||
+		    memremap_is_efi_data(phys_addr))
 			encrypted_prot = false;
 	}
 
-- 
2.41.0