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.0On 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);
> }
>
> /*
* 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
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
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.
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.
>
* 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
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.
© 2016 - 2024 Red Hat, Inc.