[PATCH 0/2 v8] add reserved e820 ranges to the kdump kernel e820 table

[PATCH 0/2 v8] add reserved e820 ranges to the kdump kernel e820 table

[Lianbo Jiang]

This patchset did two things:
a). add a new I/O resource descriptor 'IORES_DESC_RESERVED'

When doing kexec_file_load, the first kernel needs to pass the e820
reserved ranges to the second kernel. But kernel can not exactly
match the e820 reserved ranges when walking through the iomem resources
with the descriptor 'IORES_DESC_NONE', because several e820 types(
e.g. E820_TYPE_RESERVED_KERN/E820_TYPE_RAM/E820_TYPE_UNUSABLE/E820
_TYPE_RESERVED) are converted to the descriptor 'IORES_DESC_NONE'. It
may pass these four types to the kdump kernel, that is not desired result.

So, this patch adds a new I/O resource descriptor 'IORES_DESC_RESERVED'
for the iomem resources search interfaces. It is helpful to exactly
match the reserved resource ranges when walking through iomem resources.

In addition, since the new descriptor 'IORES_DESC_RESERVED' is introduced,
these code originally related to the descriptor 'IORES_DESC_NONE' need to
be updated. Otherwise, it will be easily confused and also cause some
errors. Because the 'E820_TYPE_RESERVED' type is converted to the new
descriptor 'IORES_DESC_RESERVED' instead of 'IORES_DESC_NONE', it has been
changed.

b). add the e820 reserved ranges to kdump kernel e820 table

At present, when use the kexec_file_load syscall to load the kernel image
and initramfs(for example: kexec -s -p xxx), kernel does not pass the e820
reserved ranges to the second kernel, which might cause two problems:

The first one is the MMCONFIG issue. The basic problem is that this device
is in PCI segment 1 and the kernel PCI probing can not find it without all
the e820 I/O reservations being present in the e820 table. And the kdump
kernel does not have those reservations because the kexec command does not
pass the I/O reservation via the "memmap=xxx" command line option. (This
problem does not show up for other vendors, as SGI is apparently the
actually fails for everyone, but devices in segment 0 are then found by
some legacy lookup method.) The workaround for this is to pass the I/O
reserved regions to the kdump kernel.

MMCONFIG(aka ECAM) space is described in the ACPI MCFG table. If you don't
have ECAM: (a) PCI devices won't work at all on non-x86 systems that use
only ECAM for config access, (b) you won't be albe to access devices on
non-0 segments, (c) you won't be able to access extended config space(
address 0x100-0xffff), which means none of the Extended Capabilities will
be available(AER, ACS, ATS, etc). [Bjorn's comment]

The second issue is that the SME kdump kernel doesn't work without the
e820 reserved ranges. When SME is active in kdump kernel, actually, those
reserved regions are still decrypted, but because those reserved ranges are
not present at all in kdump kernel e820 table, those reserved regions are
considered as encrypted, it goes wrong.

The e820 reserved range is useful in kdump kernel, so it is necessary to
pass the e820 reserved ranges to kdump kernel.

Changes since v1:
1. Modified the value of flags to "0", when walking through the whole
tree for e820 reserved ranges.

Changes since v2:
1. Modified the value of flags to "0", when walking through the whole
tree for e820 reserved ranges.
2. Modified the invalid SOB chain issue.

Changes since v3:
1. Dropped [PATCH 1/3 v3] resource: fix an error which walks through iomem
   resources. Please refer to this commit <010a93bf97c7> "resource: Fix
   find_next_iomem_res() iteration issue"

Changes since v4:
1. Improve the patch log, and add kernel log.

Changes since v5:
1. Rewrite these patches log.

Changes since v6:
1. Modify the [PATCH 1/2], and add the new I/O resource descriptor
   'IORES_DESC_RESERVED' for the iomem resources search interfaces,
   and also updates these codes relates to 'IORES_DESC_NONE'.
2. Modify the [PATCH 2/2], and walk through io resource based on the
   new descriptor 'IORES_DESC_RESERVED'.
3. Update patch log.

Changes since v7:
1. Improve patch log.
2. Improve this function __ioremap_check_desc_other().
3. Modify code comment in the __ioremap_check_desc_other()


Lianbo Jiang (2):
  resource: add the new I/O resource descriptor 'IORES_DESC_RESERVED'
  x86/kexec_file: add reserved e820 ranges to kdump kernel e820 table

 arch/ia64/kernel/efi.c  |  4 ++++
 arch/x86/kernel/crash.c |  6 ++++++
 arch/x86/kernel/e820.c  |  2 +-
 arch/x86/mm/ioremap.c   | 13 ++++++++++++-
 include/linux/ioport.h  |  1 +
 kernel/resource.c       |  6 +++---
 6 files changed, 27 insertions(+), 5 deletions(-)

-- 
2.17.1

[PATCH 1/2 v8] resource: add the new I/O resource descriptor 'IORES_DESC_RESERVED'

[Lianbo Jiang]

When doing kexec_file_load, the first kernel needs to pass the e820
reserved ranges to the second kernel. But kernel can not exactly
match the e820 reserved ranges when walking through the iomem resources
with the descriptor 'IORES_DESC_NONE', because several e820 types(
e.g. E820_TYPE_RESERVED_KERN/E820_TYPE_RAM/E820_TYPE_UNUSABLE/E820
_TYPE_RESERVED) are converted to the descriptor 'IORES_DESC_NONE'. It
may pass these four types to the kdump kernel, that is not desired result.

So, this patch adds a new I/O resource descriptor 'IORES_DESC_RESERVED'
for the iomem resources search interfaces. It is helpful to exactly
match the reserved resource ranges when walking through iomem resources.

In addition, since the new descriptor 'IORES_DESC_RESERVED' is introduced,
these code originally related to the descriptor 'IORES_DESC_NONE' need to
be updated. Otherwise, it will be easily confused and also cause some
errors. Because the 'E820_TYPE_RESERVED' type is converted to the new
descriptor 'IORES_DESC_RESERVED' instead of 'IORES_DESC_NONE', it has been
changed.

Suggested-by: Dave Young <dyoung@redhat.com>
Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
---
 arch/ia64/kernel/efi.c |  4 ++++
 arch/x86/kernel/e820.c |  2 +-
 arch/x86/mm/ioremap.c  | 13 ++++++++++++-
 include/linux/ioport.h |  1 +
 kernel/resource.c      |  6 +++---
 5 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
index 8f106638913c..1841e9b4db30 100644
--- a/arch/ia64/kernel/efi.c
+++ b/arch/ia64/kernel/efi.c
@@ -1231,6 +1231,10 @@ efi_initialize_iomem_resources(struct resource *code_resource,
 				break;
 
 			case EFI_RESERVED_TYPE:
+				name = "reserved";
+				desc = IORES_DESC_RESERVED;
+				break;
+
 			case EFI_RUNTIME_SERVICES_CODE:
 			case EFI_RUNTIME_SERVICES_DATA:
 			case EFI_ACPI_RECLAIM_MEMORY:
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 50895c2f937d..57fafdafb860 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -1048,10 +1048,10 @@ static unsigned long __init e820_type_to_iores_desc(struct e820_entry *entry)
 	case E820_TYPE_NVS:		return IORES_DESC_ACPI_NV_STORAGE;
 	case E820_TYPE_PMEM:		return IORES_DESC_PERSISTENT_MEMORY;
 	case E820_TYPE_PRAM:		return IORES_DESC_PERSISTENT_MEMORY_LEGACY;
+	case E820_TYPE_RESERVED:	return IORES_DESC_RESERVED;
 	case E820_TYPE_RESERVED_KERN:	/* Fall-through: */
 	case E820_TYPE_RAM:		/* Fall-through: */
 	case E820_TYPE_UNUSABLE:	/* Fall-through: */
-	case E820_TYPE_RESERVED:	/* Fall-through: */
 	default:			return IORES_DESC_NONE;
 	}
 }
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 5378d10f1d31..fea2ef99415d 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -83,7 +83,18 @@ static bool __ioremap_check_ram(struct resource *res)
 
 static int __ioremap_check_desc_other(struct resource *res)
 {
-	return (res->desc != IORES_DESC_NONE);
+	/*
+	 * But now, the 'E820_TYPE_RESERVED' type is converted to the new
+	 * descriptor 'IORES_DESC_RESERVED' instead of 'IORES_DESC_NONE',
+	 * it has been changed. And the value of 'mem_flags.desc_other'
+	 * is equal to 'true' if we don't strengthen the condition in this
+	 * function, that is wrong. Because originally it is equal to
+	 * 'false' for the same reserved type.
+	 *
+	 * So, that would be nice to keep it the same as before.
+	 */
+	return ((res->desc != IORES_DESC_NONE) &&
+		(res->desc != IORES_DESC_RESERVED));
 }
 
 static int __ioremap_res_check(struct resource *res, void *arg)
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index da0ebaec25f0..6ed59de48bd5 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -133,6 +133,7 @@ enum {
 	IORES_DESC_PERSISTENT_MEMORY_LEGACY	= 5,
 	IORES_DESC_DEVICE_PRIVATE_MEMORY	= 6,
 	IORES_DESC_DEVICE_PUBLIC_MEMORY		= 7,
+	IORES_DESC_RESERVED			= 8,
 };
 
 /* helpers to define resources */
diff --git a/kernel/resource.c b/kernel/resource.c
index b0fbf685c77a..f34a632c4169 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -994,7 +994,7 @@ __reserve_region_with_split(struct resource *root, resource_size_t start,
 	res->start = start;
 	res->end = end;
 	res->flags = type | IORESOURCE_BUSY;
-	res->desc = IORES_DESC_NONE;
+	res->desc = IORES_DESC_RESERVED;
 
 	while (1) {
 
@@ -1029,7 +1029,7 @@ __reserve_region_with_split(struct resource *root, resource_size_t start,
 				next_res->start = conflict->end + 1;
 				next_res->end = end;
 				next_res->flags = type | IORESOURCE_BUSY;
-				next_res->desc = IORES_DESC_NONE;
+				next_res->desc = IORES_DESC_RESERVED;
 			}
 		} else {
 			res->start = conflict->end + 1;
@@ -1477,7 +1477,7 @@ static int __init reserve_setup(char *str)
 			res->start = io_start;
 			res->end = io_start + io_num - 1;
 			res->flags |= IORESOURCE_BUSY;
-			res->desc = IORES_DESC_NONE;
+			res->desc = IORES_DESC_RESERVED;
 			res->child = NULL;
 			if (request_resource(parent, res) == 0)
 				reserved = x+1;
-- 
2.17.1

[PATCH 2/2 v8] x86/kexec_file: add reserved e820 ranges to kdump kernel e820 table

[Lianbo Jiang]

At present, when use the kexec_file_load syscall to load the kernel image
and initramfs(for example: kexec -s -p xxx), kernel does not pass the e820
reserved ranges to the second kernel, which might cause two problems:

The first one is the MMCONFIG issue. The basic problem is that this device
is in PCI segment 1 and the kernel PCI probing can not find it without all
the e820 I/O reservations being present in the e820 table. And the kdump
kernel does not have those reservations because the kexec command does not
pass the I/O reservation via the "memmap=xxx" command line option. (This
problem does not show up for other vendors, as SGI is apparently the
actually fails for everyone, but devices in segment 0 are then found by
some legacy lookup method.) The workaround for this is to pass the I/O
reserved regions to the kdump kernel.

MMCONFIG(aka ECAM) space is described in the ACPI MCFG table. If you don't
have ECAM: (a) PCI devices won't work at all on non-x86 systems that use
only ECAM for config access, (b) you won't be albe to access devices on
non-0 segments, (c) you won't be able to access extended config space(
address 0x100-0xffff), which means none of the Extended Capabilities will
be available(AER, ACS, ATS, etc). [Bjorn's comment]

The second issue is that the SME kdump kernel doesn't work without the
e820 reserved ranges. When SME is active in kdump kernel, actually, those
reserved regions are still decrypted, but because those reserved ranges are
not present at all in kdump kernel e820 table, those reserved regions are
considered as encrypted, it goes wrong.

The e820 reserved range is useful in kdump kernel, so it is necessary to
pass the e820 reserved ranges to kdump kernel.

Suggested-by: Dave Young <dyoung@redhat.com>
Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
---
 arch/x86/kernel/crash.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index f631a3f15587..5354a84f1684 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -380,6 +380,12 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
 	walk_iomem_res_desc(IORES_DESC_ACPI_NV_STORAGE, flags, 0, -1, &cmd,
 			memmap_entry_callback);
 
+	/* Add e820 reserved ranges */
+	cmd.type = E820_TYPE_RESERVED;
+	flags = IORESOURCE_MEM;
+	walk_iomem_res_desc(IORES_DESC_RESERVED, flags, 0, -1, &cmd,
+			   memmap_entry_callback);
+
 	/* Add crashk_low_res region */
 	if (crashk_low_res.end) {
 		ei.addr = crashk_low_res.start;
-- 
2.17.1

Re: [PATCH 1/2 v8] resource: add the new I/O resource descriptor 'IORES_DESC_RESERVED'

[Dave Young]

+ more people

On 11/29/18 at 04:09pm, Lianbo Jiang wrote:
> When doing kexec_file_load, the first kernel needs to pass the e820
> reserved ranges to the second kernel. But kernel can not exactly
> match the e820 reserved ranges when walking through the iomem resources
> with the descriptor 'IORES_DESC_NONE', because several e820 types(
> e.g. E820_TYPE_RESERVED_KERN/E820_TYPE_RAM/E820_TYPE_UNUSABLE/E820
> _TYPE_RESERVED) are converted to the descriptor 'IORES_DESC_NONE'. It
> may pass these four types to the kdump kernel, that is not desired result.
> 
> So, this patch adds a new I/O resource descriptor 'IORES_DESC_RESERVED'
> for the iomem resources search interfaces. It is helpful to exactly
> match the reserved resource ranges when walking through iomem resources.
> 
> In addition, since the new descriptor 'IORES_DESC_RESERVED' is introduced,
> these code originally related to the descriptor 'IORES_DESC_NONE' need to
> be updated. Otherwise, it will be easily confused and also cause some
> errors. Because the 'E820_TYPE_RESERVED' type is converted to the new
> descriptor 'IORES_DESC_RESERVED' instead of 'IORES_DESC_NONE', it has been
> changed.
> 
> Suggested-by: Dave Young <dyoung@redhat.com>
> Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
> ---
>  arch/ia64/kernel/efi.c |  4 ++++
>  arch/x86/kernel/e820.c |  2 +-
>  arch/x86/mm/ioremap.c  | 13 ++++++++++++-
>  include/linux/ioport.h |  1 +
>  kernel/resource.c      |  6 +++---
>  5 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
> index 8f106638913c..1841e9b4db30 100644
> --- a/arch/ia64/kernel/efi.c
> +++ b/arch/ia64/kernel/efi.c
> @@ -1231,6 +1231,10 @@ efi_initialize_iomem_resources(struct resource *code_resource,
>  				break;
>  
>  			case EFI_RESERVED_TYPE:
> +				name = "reserved";

Ingo updated X86 code to use "Reserved",  I think it would be good to do
same for this case as well

> +				desc = IORES_DESC_RESERVED;
> +				break;
> +
>  			case EFI_RUNTIME_SERVICES_CODE:
>  			case EFI_RUNTIME_SERVICES_DATA:
>  			case EFI_ACPI_RECLAIM_MEMORY:

Originally, above 3 are all "reserved", so probably they all should be
IORES_DESC_RESERVED.

Can any IA64 people to review this?

> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index 50895c2f937d..57fafdafb860 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -1048,10 +1048,10 @@ static unsigned long __init e820_type_to_iores_desc(struct e820_entry *entry)
>  	case E820_TYPE_NVS:		return IORES_DESC_ACPI_NV_STORAGE;
>  	case E820_TYPE_PMEM:		return IORES_DESC_PERSISTENT_MEMORY;
>  	case E820_TYPE_PRAM:		return IORES_DESC_PERSISTENT_MEMORY_LEGACY;
> +	case E820_TYPE_RESERVED:	return IORES_DESC_RESERVED;
>  	case E820_TYPE_RESERVED_KERN:	/* Fall-through: */
>  	case E820_TYPE_RAM:		/* Fall-through: */
>  	case E820_TYPE_UNUSABLE:	/* Fall-through: */
> -	case E820_TYPE_RESERVED:	/* Fall-through: */
>  	default:			return IORES_DESC_NONE;
>  	}
>  }
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index 5378d10f1d31..fea2ef99415d 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -83,7 +83,18 @@ static bool __ioremap_check_ram(struct resource *res)
>  
>  static int __ioremap_check_desc_other(struct resource *res)
>  {
> -	return (res->desc != IORES_DESC_NONE);
> +	/*
> +	 * But now, the 'E820_TYPE_RESERVED' type is converted to the new
> +	 * descriptor 'IORES_DESC_RESERVED' instead of 'IORES_DESC_NONE',
> +	 * it has been changed. And the value of 'mem_flags.desc_other'
> +	 * is equal to 'true' if we don't strengthen the condition in this
> +	 * function, that is wrong. Because originally it is equal to
> +	 * 'false' for the same reserved type.
> +	 *
> +	 * So, that would be nice to keep it the same as before.
> +	 */
> +	return ((res->desc != IORES_DESC_NONE) &&
> +		(res->desc != IORES_DESC_RESERVED));
>  }

Added Tom since he added the check function.  Is it possible to only
check explict valid desc types instead of exclude IORES_DESC_NONE?

>  
>  static int __ioremap_res_check(struct resource *res, void *arg)
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index da0ebaec25f0..6ed59de48bd5 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -133,6 +133,7 @@ enum {
>  	IORES_DESC_PERSISTENT_MEMORY_LEGACY	= 5,
>  	IORES_DESC_DEVICE_PRIVATE_MEMORY	= 6,
>  	IORES_DESC_DEVICE_PUBLIC_MEMORY		= 7,
> +	IORES_DESC_RESERVED			= 8,
>  };
>  
>  /* helpers to define resources */
> diff --git a/kernel/resource.c b/kernel/resource.c
> index b0fbf685c77a..f34a632c4169 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -994,7 +994,7 @@ __reserve_region_with_split(struct resource *root, resource_size_t start,
>  	res->start = start;
>  	res->end = end;
>  	res->flags = type | IORESOURCE_BUSY;
> -	res->desc = IORES_DESC_NONE;
> +	res->desc = IORES_DESC_RESERVED;
>  
>  	while (1) {
>  
> @@ -1029,7 +1029,7 @@ __reserve_region_with_split(struct resource *root, resource_size_t start,
>  				next_res->start = conflict->end + 1;
>  				next_res->end = end;
>  				next_res->flags = type | IORESOURCE_BUSY;
> -				next_res->desc = IORES_DESC_NONE;
> +				next_res->desc = IORES_DESC_RESERVED;
>  			}
>  		} else {
>  			res->start = conflict->end + 1;
> @@ -1477,7 +1477,7 @@ static int __init reserve_setup(char *str)
>  			res->start = io_start;
>  			res->end = io_start + io_num - 1;
>  			res->flags |= IORESOURCE_BUSY;
> -			res->desc = IORES_DESC_NONE;
> +			res->desc = IORES_DESC_RESERVED;
>  			res->child = NULL;
>  			if (request_resource(parent, res) == 0)
>  				reserved = x+1;
> -- 
> 2.17.1
> 


There are a lot of places call region_intersects which use DESC_NONE,
I'm not sure if needed changes accordingly.  Cced Dan and Toshi.


Thanks
Dave

Re: [PATCH 1/2 v8] resource: add the new I/O resource descriptor 'IORES_DESC_RESERVED'

[Dave Young]

On 11/29/18 at 04:09pm, Lianbo Jiang wrote:
> When doing kexec_file_load, the first kernel needs to pass the e820
> reserved ranges to the second kernel. But kernel can not exactly
> match the e820 reserved ranges when walking through the iomem resources
> with the descriptor 'IORES_DESC_NONE', because several e820 types(
> e.g. E820_TYPE_RESERVED_KERN/E820_TYPE_RAM/E820_TYPE_UNUSABLE/E820
> _TYPE_RESERVED) are converted to the descriptor 'IORES_DESC_NONE'. It
> may pass these four types to the kdump kernel, that is not desired result.
> 
> So, this patch adds a new I/O resource descriptor 'IORES_DESC_RESERVED'
> for the iomem resources search interfaces. It is helpful to exactly
> match the reserved resource ranges when walking through iomem resources.
> 
> In addition, since the new descriptor 'IORES_DESC_RESERVED' is introduced,
> these code originally related to the descriptor 'IORES_DESC_NONE' need to
> be updated. Otherwise, it will be easily confused and also cause some
> errors. Because the 'E820_TYPE_RESERVED' type is converted to the new
> descriptor 'IORES_DESC_RESERVED' instead of 'IORES_DESC_NONE', it has been
> changed.
> 
> Suggested-by: Dave Young <dyoung@redhat.com>

This was suggested by Boris instead :) 

> Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
> ---
>  arch/ia64/kernel/efi.c |  4 ++++
>  arch/x86/kernel/e820.c |  2 +-
>  arch/x86/mm/ioremap.c  | 13 ++++++++++++-
>  include/linux/ioport.h |  1 +
>  kernel/resource.c      |  6 +++---
>  5 files changed, 21 insertions(+), 5 deletions(-)
> 
> 
[snip]

Thanks
Dave

Re: [PATCH 1/2 v8] resource: add the new I/O resource descriptor 'IORES_DESC_RESERVED'

[Dave Young]

Correct Toshi's email addr
On 11/30/18 at 11:37am, Dave Young wrote:
> + more people
> 
> On 11/29/18 at 04:09pm, Lianbo Jiang wrote:
> > When doing kexec_file_load, the first kernel needs to pass the e820
> > reserved ranges to the second kernel. But kernel can not exactly
> > match the e820 reserved ranges when walking through the iomem resources
> > with the descriptor 'IORES_DESC_NONE', because several e820 types(
> > e.g. E820_TYPE_RESERVED_KERN/E820_TYPE_RAM/E820_TYPE_UNUSABLE/E820
> > _TYPE_RESERVED) are converted to the descriptor 'IORES_DESC_NONE'. It
> > may pass these four types to the kdump kernel, that is not desired result.
> > 
> > So, this patch adds a new I/O resource descriptor 'IORES_DESC_RESERVED'
> > for the iomem resources search interfaces. It is helpful to exactly
> > match the reserved resource ranges when walking through iomem resources.
> > 
> > In addition, since the new descriptor 'IORES_DESC_RESERVED' is introduced,
> > these code originally related to the descriptor 'IORES_DESC_NONE' need to
> > be updated. Otherwise, it will be easily confused and also cause some
> > errors. Because the 'E820_TYPE_RESERVED' type is converted to the new
> > descriptor 'IORES_DESC_RESERVED' instead of 'IORES_DESC_NONE', it has been
> > changed.
> > 
> > Suggested-by: Dave Young <dyoung@redhat.com>
> > Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
> > ---
> >  arch/ia64/kernel/efi.c |  4 ++++
> >  arch/x86/kernel/e820.c |  2 +-
> >  arch/x86/mm/ioremap.c  | 13 ++++++++++++-
> >  include/linux/ioport.h |  1 +
> >  kernel/resource.c      |  6 +++---
> >  5 files changed, 21 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
> > index 8f106638913c..1841e9b4db30 100644
> > --- a/arch/ia64/kernel/efi.c
> > +++ b/arch/ia64/kernel/efi.c
> > @@ -1231,6 +1231,10 @@ efi_initialize_iomem_resources(struct resource *code_resource,
> >  				break;
> >  
> >  			case EFI_RESERVED_TYPE:
> > +				name = "reserved";
> 
> Ingo updated X86 code to use "Reserved",  I think it would be good to do
> same for this case as well
> 
> > +				desc = IORES_DESC_RESERVED;
> > +				break;
> > +
> >  			case EFI_RUNTIME_SERVICES_CODE:
> >  			case EFI_RUNTIME_SERVICES_DATA:
> >  			case EFI_ACPI_RECLAIM_MEMORY:
> 
> Originally, above 3 are all "reserved", so probably they all should be
> IORES_DESC_RESERVED.
> 
> Can any IA64 people to review this?
> 
> > diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> > index 50895c2f937d..57fafdafb860 100644
> > --- a/arch/x86/kernel/e820.c
> > +++ b/arch/x86/kernel/e820.c
> > @@ -1048,10 +1048,10 @@ static unsigned long __init e820_type_to_iores_desc(struct e820_entry *entry)
> >  	case E820_TYPE_NVS:		return IORES_DESC_ACPI_NV_STORAGE;
> >  	case E820_TYPE_PMEM:		return IORES_DESC_PERSISTENT_MEMORY;
> >  	case E820_TYPE_PRAM:		return IORES_DESC_PERSISTENT_MEMORY_LEGACY;
> > +	case E820_TYPE_RESERVED:	return IORES_DESC_RESERVED;
> >  	case E820_TYPE_RESERVED_KERN:	/* Fall-through: */
> >  	case E820_TYPE_RAM:		/* Fall-through: */
> >  	case E820_TYPE_UNUSABLE:	/* Fall-through: */
> > -	case E820_TYPE_RESERVED:	/* Fall-through: */
> >  	default:			return IORES_DESC_NONE;
> >  	}
> >  }
> > diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> > index 5378d10f1d31..fea2ef99415d 100644
> > --- a/arch/x86/mm/ioremap.c
> > +++ b/arch/x86/mm/ioremap.c
> > @@ -83,7 +83,18 @@ static bool __ioremap_check_ram(struct resource *res)
> >  
> >  static int __ioremap_check_desc_other(struct resource *res)
> >  {
> > -	return (res->desc != IORES_DESC_NONE);
> > +	/*
> > +	 * But now, the 'E820_TYPE_RESERVED' type is converted to the new
> > +	 * descriptor 'IORES_DESC_RESERVED' instead of 'IORES_DESC_NONE',
> > +	 * it has been changed. And the value of 'mem_flags.desc_other'
> > +	 * is equal to 'true' if we don't strengthen the condition in this
> > +	 * function, that is wrong. Because originally it is equal to
> > +	 * 'false' for the same reserved type.
> > +	 *
> > +	 * So, that would be nice to keep it the same as before.
> > +	 */
> > +	return ((res->desc != IORES_DESC_NONE) &&
> > +		(res->desc != IORES_DESC_RESERVED));
> >  }
> 
> Added Tom since he added the check function.  Is it possible to only
> check explict valid desc types instead of exclude IORES_DESC_NONE?
> 
> >  
> >  static int __ioremap_res_check(struct resource *res, void *arg)
> > diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> > index da0ebaec25f0..6ed59de48bd5 100644
> > --- a/include/linux/ioport.h
> > +++ b/include/linux/ioport.h
> > @@ -133,6 +133,7 @@ enum {
> >  	IORES_DESC_PERSISTENT_MEMORY_LEGACY	= 5,
> >  	IORES_DESC_DEVICE_PRIVATE_MEMORY	= 6,
> >  	IORES_DESC_DEVICE_PUBLIC_MEMORY		= 7,
> > +	IORES_DESC_RESERVED			= 8,
> >  };
> >  
> >  /* helpers to define resources */
> > diff --git a/kernel/resource.c b/kernel/resource.c
> > index b0fbf685c77a..f34a632c4169 100644
> > --- a/kernel/resource.c
> > +++ b/kernel/resource.c
> > @@ -994,7 +994,7 @@ __reserve_region_with_split(struct resource *root, resource_size_t start,
> >  	res->start = start;
> >  	res->end = end;
> >  	res->flags = type | IORESOURCE_BUSY;
> > -	res->desc = IORES_DESC_NONE;
> > +	res->desc = IORES_DESC_RESERVED;
> >  
> >  	while (1) {
> >  
> > @@ -1029,7 +1029,7 @@ __reserve_region_with_split(struct resource *root, resource_size_t start,
> >  				next_res->start = conflict->end + 1;
> >  				next_res->end = end;
> >  				next_res->flags = type | IORESOURCE_BUSY;
> > -				next_res->desc = IORES_DESC_NONE;
> > +				next_res->desc = IORES_DESC_RESERVED;
> >  			}
> >  		} else {
> >  			res->start = conflict->end + 1;
> > @@ -1477,7 +1477,7 @@ static int __init reserve_setup(char *str)
> >  			res->start = io_start;
> >  			res->end = io_start + io_num - 1;
> >  			res->flags |= IORESOURCE_BUSY;
> > -			res->desc = IORES_DESC_NONE;
> > +			res->desc = IORES_DESC_RESERVED;
> >  			res->child = NULL;
> >  			if (request_resource(parent, res) == 0)
> >  				reserved = x+1;
> > -- 
> > 2.17.1
> > 
> 
> 
> There are a lot of places call region_intersects which use DESC_NONE,
> I'm not sure if needed changes accordingly.  Cced Dan and Toshi.
> 
> 
> Thanks
> Dave

Re: [PATCH 1/2 v8] resource: add the new I/O resource descriptor 'IORES_DESC_RESERVED'

[lijiang]

在 2018年11月30日 11:41, Dave Young 写道:
> On 11/29/18 at 04:09pm, Lianbo Jiang wrote:
>> When doing kexec_file_load, the first kernel needs to pass the e820
>> reserved ranges to the second kernel. But kernel can not exactly
>> match the e820 reserved ranges when walking through the iomem resources
>> with the descriptor 'IORES_DESC_NONE', because several e820 types(
>> e.g. E820_TYPE_RESERVED_KERN/E820_TYPE_RAM/E820_TYPE_UNUSABLE/E820
>> _TYPE_RESERVED) are converted to the descriptor 'IORES_DESC_NONE'. It
>> may pass these four types to the kdump kernel, that is not desired result.
>>
>> So, this patch adds a new I/O resource descriptor 'IORES_DESC_RESERVED'
>> for the iomem resources search interfaces. It is helpful to exactly
>> match the reserved resource ranges when walking through iomem resources.
>>
>> In addition, since the new descriptor 'IORES_DESC_RESERVED' is introduced,
>> these code originally related to the descriptor 'IORES_DESC_NONE' need to
>> be updated. Otherwise, it will be easily confused and also cause some
>> errors. Because the 'E820_TYPE_RESERVED' type is converted to the new
>> descriptor 'IORES_DESC_RESERVED' instead of 'IORES_DESC_NONE', it has been
>> changed.
>>
>> Suggested-by: Dave Young <dyoung@redhat.com>
> 
> This was suggested by Boris instead :) 
> 

Sorry for this, i forgot to change it.
And i will correct this in next version.

Thanks.

>> Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
>> ---
>>  arch/ia64/kernel/efi.c |  4 ++++
>>  arch/x86/kernel/e820.c |  2 +-
>>  arch/x86/mm/ioremap.c  | 13 ++++++++++++-
>>  include/linux/ioport.h |  1 +
>>  kernel/resource.c      |  6 +++---
>>  5 files changed, 21 insertions(+), 5 deletions(-)
>>
>>
> [snip]
> 
> Thanks
> Dave
> 

Re: [PATCH 1/2 v8] resource: add the new I/O resource descriptor 'IORES_DESC_RESERVED'

[lijiang]

在 2018年11月30日 11:37, Dave Young 写道:
> + more people
> 

Thank you, Dave.

That would be fine to let more people review this patch.

> On 11/29/18 at 04:09pm, Lianbo Jiang wrote:
>> When doing kexec_file_load, the first kernel needs to pass the e820
>> reserved ranges to the second kernel. But kernel can not exactly
>> match the e820 reserved ranges when walking through the iomem resources
>> with the descriptor 'IORES_DESC_NONE', because several e820 types(
>> e.g. E820_TYPE_RESERVED_KERN/E820_TYPE_RAM/E820_TYPE_UNUSABLE/E820
>> _TYPE_RESERVED) are converted to the descriptor 'IORES_DESC_NONE'. It
>> may pass these four types to the kdump kernel, that is not desired result.
>>
>> So, this patch adds a new I/O resource descriptor 'IORES_DESC_RESERVED'
>> for the iomem resources search interfaces. It is helpful to exactly
>> match the reserved resource ranges when walking through iomem resources.
>>
>> In addition, since the new descriptor 'IORES_DESC_RESERVED' is introduced,
>> these code originally related to the descriptor 'IORES_DESC_NONE' need to
>> be updated. Otherwise, it will be easily confused and also cause some
>> errors. Because the 'E820_TYPE_RESERVED' type is converted to the new
>> descriptor 'IORES_DESC_RESERVED' instead of 'IORES_DESC_NONE', it has been
>> changed.
>>
>> Suggested-by: Dave Young <dyoung@redhat.com>
>> Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
>> ---
>>  arch/ia64/kernel/efi.c |  4 ++++
>>  arch/x86/kernel/e820.c |  2 +-
>>  arch/x86/mm/ioremap.c  | 13 ++++++++++++-
>>  include/linux/ioport.h |  1 +
>>  kernel/resource.c      |  6 +++---
>>  5 files changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
>> index 8f106638913c..1841e9b4db30 100644
>> --- a/arch/ia64/kernel/efi.c
>> +++ b/arch/ia64/kernel/efi.c
>> @@ -1231,6 +1231,10 @@ efi_initialize_iomem_resources(struct resource *code_resource,
>>  				break;
>>  
>>  			case EFI_RESERVED_TYPE:
>> +				name = "reserved";
> 
> Ingo updated X86 code to use "Reserved",  I think it would be good to do
> same for this case as well
> 

I have noticed the changes on x86, but for IA64, i'm not sure whether it should do the same
thing, so keep it as before.

If IA64 people would like to give any comment, that will be appreciated.


>> +				desc = IORES_DESC_RESERVED;
>> +				break;
>> +
>>  			case EFI_RUNTIME_SERVICES_CODE:
>>  			case EFI_RUNTIME_SERVICES_DATA:
>>  			case EFI_ACPI_RECLAIM_MEMORY:
> 
> Originally, above 3 are all "reserved", so probably they all should be
> IORES_DESC_RESERVED.
> 

This is a good question.

If above 3 types are converted to the new descriptor 'IORES_DESC_RESERVED', how to handle
the all 'default' case? Because the all 'default' case is also converted to the descriptor
'IORES_DESC_NONE' and the name 'reserved'.

> Can any IA64 people to review this?
> 
>> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
>> index 50895c2f937d..57fafdafb860 100644
>> --- a/arch/x86/kernel/e820.c
>> +++ b/arch/x86/kernel/e820.c
>> @@ -1048,10 +1048,10 @@ static unsigned long __init e820_type_to_iores_desc(struct e820_entry *entry)
>>  	case E820_TYPE_NVS:		return IORES_DESC_ACPI_NV_STORAGE;
>>  	case E820_TYPE_PMEM:		return IORES_DESC_PERSISTENT_MEMORY;
>>  	case E820_TYPE_PRAM:		return IORES_DESC_PERSISTENT_MEMORY_LEGACY;
>> +	case E820_TYPE_RESERVED:	return IORES_DESC_RESERVED;
>>  	case E820_TYPE_RESERVED_KERN:	/* Fall-through: */
>>  	case E820_TYPE_RAM:		/* Fall-through: */
>>  	case E820_TYPE_UNUSABLE:	/* Fall-through: */
>> -	case E820_TYPE_RESERVED:	/* Fall-through: */
>>  	default:			return IORES_DESC_NONE;
>>  	}
>>  }
>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>> index 5378d10f1d31..fea2ef99415d 100644
>> --- a/arch/x86/mm/ioremap.c
>> +++ b/arch/x86/mm/ioremap.c
>> @@ -83,7 +83,18 @@ static bool __ioremap_check_ram(struct resource *res)
>>  
>>  static int __ioremap_check_desc_other(struct resource *res)
>>  {
>> -	return (res->desc != IORES_DESC_NONE);
>> +	/*
>> +	 * But now, the 'E820_TYPE_RESERVED' type is converted to the new
>> +	 * descriptor 'IORES_DESC_RESERVED' instead of 'IORES_DESC_NONE',
>> +	 * it has been changed. And the value of 'mem_flags.desc_other'
>> +	 * is equal to 'true' if we don't strengthen the condition in this
>> +	 * function, that is wrong. Because originally it is equal to
>> +	 * 'false' for the same reserved type.
>> +	 *
>> +	 * So, that would be nice to keep it the same as before.
>> +	 */
>> +	return ((res->desc != IORES_DESC_NONE) &&
>> +		(res->desc != IORES_DESC_RESERVED));
>>  }
> 
> Added Tom since he added the check function.  Is it possible to only
> check explict valid desc types instead of exclude IORES_DESC_NONE?
> 

So sorry that i forgot to add Tom.

I'm looking forward to Tom's reply. If i made a mistake, please help me
to point out. Thanks a lot.

Regards,
Lianbo

>>  
>>  static int __ioremap_res_check(struct resource *res, void *arg)
>> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
>> index da0ebaec25f0..6ed59de48bd5 100644
>> --- a/include/linux/ioport.h
>> +++ b/include/linux/ioport.h
>> @@ -133,6 +133,7 @@ enum {
>>  	IORES_DESC_PERSISTENT_MEMORY_LEGACY	= 5,
>>  	IORES_DESC_DEVICE_PRIVATE_MEMORY	= 6,
>>  	IORES_DESC_DEVICE_PUBLIC_MEMORY		= 7,
>> +	IORES_DESC_RESERVED			= 8,
>>  };
>>  
>>  /* helpers to define resources */
>> diff --git a/kernel/resource.c b/kernel/resource.c
>> index b0fbf685c77a..f34a632c4169 100644
>> --- a/kernel/resource.c
>> +++ b/kernel/resource.c
>> @@ -994,7 +994,7 @@ __reserve_region_with_split(struct resource *root, resource_size_t start,
>>  	res->start = start;
>>  	res->end = end;
>>  	res->flags = type | IORESOURCE_BUSY;
>> -	res->desc = IORES_DESC_NONE;
>> +	res->desc = IORES_DESC_RESERVED;
>>  
>>  	while (1) {
>>  
>> @@ -1029,7 +1029,7 @@ __reserve_region_with_split(struct resource *root, resource_size_t start,
>>  				next_res->start = conflict->end + 1;
>>  				next_res->end = end;
>>  				next_res->flags = type | IORESOURCE_BUSY;
>> -				next_res->desc = IORES_DESC_NONE;
>> +				next_res->desc = IORES_DESC_RESERVED;
>>  			}
>>  		} else {
>>  			res->start = conflict->end + 1;
>> @@ -1477,7 +1477,7 @@ static int __init reserve_setup(char *str)
>>  			res->start = io_start;
>>  			res->end = io_start + io_num - 1;
>>  			res->flags |= IORESOURCE_BUSY;
>> -			res->desc = IORES_DESC_NONE;
>> +			res->desc = IORES_DESC_RESERVED;
>>  			res->child = NULL;
>>  			if (request_resource(parent, res) == 0)
>>  				reserved = x+1;
>> -- 
>> 2.17.1
>>
> 
> 
> There are a lot of places call region_intersects which use DESC_NONE,
> I'm not sure if needed changes accordingly.  Cced Dan and Toshi.
> 
> 
> Thanks
> Dave
> 

Re: [PATCH 1/2 v8] resource: add the new I/O resource descriptor 'IORES_DESC_RESERVED'

[Lendacky, Thomas]

On 11/29/2018 09:37 PM, Dave Young wrote:
> + more people
> 
> On 11/29/18 at 04:09pm, Lianbo Jiang wrote:
>> When doing kexec_file_load, the first kernel needs to pass the e820
>> reserved ranges to the second kernel. But kernel can not exactly
>> match the e820 reserved ranges when walking through the iomem resources
>> with the descriptor 'IORES_DESC_NONE', because several e820 types(
>> e.g. E820_TYPE_RESERVED_KERN/E820_TYPE_RAM/E820_TYPE_UNUSABLE/E820
>> _TYPE_RESERVED) are converted to the descriptor 'IORES_DESC_NONE'. It
>> may pass these four types to the kdump kernel, that is not desired result.
>>
>> So, this patch adds a new I/O resource descriptor 'IORES_DESC_RESERVED'
>> for the iomem resources search interfaces. It is helpful to exactly
>> match the reserved resource ranges when walking through iomem resources.
>>
>> In addition, since the new descriptor 'IORES_DESC_RESERVED' is introduced,
>> these code originally related to the descriptor 'IORES_DESC_NONE' need to
>> be updated. Otherwise, it will be easily confused and also cause some
>> errors. Because the 'E820_TYPE_RESERVED' type is converted to the new
>> descriptor 'IORES_DESC_RESERVED' instead of 'IORES_DESC_NONE', it has been
>> changed.
>>
>> Suggested-by: Dave Young <dyoung@redhat.com>
>> Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
>> ---
>>  arch/ia64/kernel/efi.c |  4 ++++
>>  arch/x86/kernel/e820.c |  2 +-
>>  arch/x86/mm/ioremap.c  | 13 ++++++++++++-
>>  include/linux/ioport.h |  1 +
>>  kernel/resource.c      |  6 +++---
>>  5 files changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
>> index 8f106638913c..1841e9b4db30 100644
>> --- a/arch/ia64/kernel/efi.c
>> +++ b/arch/ia64/kernel/efi.c
>> @@ -1231,6 +1231,10 @@ efi_initialize_iomem_resources(struct resource *code_resource,
>>  				break;
>>  
>>  			case EFI_RESERVED_TYPE:
>> +				name = "reserved";
> 
> Ingo updated X86 code to use "Reserved",  I think it would be good to do
> same for this case as well
> 
>> +				desc = IORES_DESC_RESERVED;
>> +				break;
>> +
>>  			case EFI_RUNTIME_SERVICES_CODE:
>>  			case EFI_RUNTIME_SERVICES_DATA:
>>  			case EFI_ACPI_RECLAIM_MEMORY:
> 
> Originally, above 3 are all "reserved", so probably they all should be
> IORES_DESC_RESERVED.
> 
> Can any IA64 people to review this?
> 
>> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
>> index 50895c2f937d..57fafdafb860 100644
>> --- a/arch/x86/kernel/e820.c
>> +++ b/arch/x86/kernel/e820.c
>> @@ -1048,10 +1048,10 @@ static unsigned long __init e820_type_to_iores_desc(struct e820_entry *entry)
>>  	case E820_TYPE_NVS:		return IORES_DESC_ACPI_NV_STORAGE;
>>  	case E820_TYPE_PMEM:		return IORES_DESC_PERSISTENT_MEMORY;
>>  	case E820_TYPE_PRAM:		return IORES_DESC_PERSISTENT_MEMORY_LEGACY;
>> +	case E820_TYPE_RESERVED:	return IORES_DESC_RESERVED;
>>  	case E820_TYPE_RESERVED_KERN:	/* Fall-through: */
>>  	case E820_TYPE_RAM:		/* Fall-through: */
>>  	case E820_TYPE_UNUSABLE:	/* Fall-through: */
>> -	case E820_TYPE_RESERVED:	/* Fall-through: */
>>  	default:			return IORES_DESC_NONE;
>>  	}
>>  }
>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>> index 5378d10f1d31..fea2ef99415d 100644
>> --- a/arch/x86/mm/ioremap.c
>> +++ b/arch/x86/mm/ioremap.c
>> @@ -83,7 +83,18 @@ static bool __ioremap_check_ram(struct resource *res)
>>  
>>  static int __ioremap_check_desc_other(struct resource *res)
>>  {
>> -	return (res->desc != IORES_DESC_NONE);
>> +	/*
>> +	 * But now, the 'E820_TYPE_RESERVED' type is converted to the new
>> +	 * descriptor 'IORES_DESC_RESERVED' instead of 'IORES_DESC_NONE',
>> +	 * it has been changed. And the value of 'mem_flags.desc_other'
>> +	 * is equal to 'true' if we don't strengthen the condition in this
>> +	 * function, that is wrong. Because originally it is equal to
>> +	 * 'false' for the same reserved type.
>> +	 *
>> +	 * So, that would be nice to keep it the same as before.
>> +	 */
>> +	return ((res->desc != IORES_DESC_NONE) &&
>> +		(res->desc != IORES_DESC_RESERVED));
>>  }
> 
> Added Tom since he added the check function.  Is it possible to only
> check explict valid desc types instead of exclude IORES_DESC_NONE?

Sorry for the delay...

The original intent of the check was to map most memory as encrypted under
SEV if it was marked with a specific descriptor, since it was likely to
not be MMIO. I tried converting most things that mapped memory to memremap
vs ioremap, but ACPI was one area that I left alone and this check catches
the mapping of the ACPI tables. I suppose it's possible to change this to
check just for IORES_DESC_ACPI_* values, but I would have to do some
testing.

Thanks,
Tom

> 
>>  
>>  static int __ioremap_res_check(struct resource *res, void *arg)
>> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
>> index da0ebaec25f0..6ed59de48bd5 100644
>> --- a/include/linux/ioport.h
>> +++ b/include/linux/ioport.h
>> @@ -133,6 +133,7 @@ enum {
>>  	IORES_DESC_PERSISTENT_MEMORY_LEGACY	= 5,
>>  	IORES_DESC_DEVICE_PRIVATE_MEMORY	= 6,
>>  	IORES_DESC_DEVICE_PUBLIC_MEMORY		= 7,
>> +	IORES_DESC_RESERVED			= 8,
>>  };
>>  
>>  /* helpers to define resources */
>> diff --git a/kernel/resource.c b/kernel/resource.c
>> index b0fbf685c77a..f34a632c4169 100644
>> --- a/kernel/resource.c
>> +++ b/kernel/resource.c
>> @@ -994,7 +994,7 @@ __reserve_region_with_split(struct resource *root, resource_size_t start,
>>  	res->start = start;
>>  	res->end = end;
>>  	res->flags = type | IORESOURCE_BUSY;
>> -	res->desc = IORES_DESC_NONE;
>> +	res->desc = IORES_DESC_RESERVED;
>>  
>>  	while (1) {
>>  
>> @@ -1029,7 +1029,7 @@ __reserve_region_with_split(struct resource *root, resource_size_t start,
>>  				next_res->start = conflict->end + 1;
>>  				next_res->end = end;
>>  				next_res->flags = type | IORESOURCE_BUSY;
>> -				next_res->desc = IORES_DESC_NONE;
>> +				next_res->desc = IORES_DESC_RESERVED;
>>  			}
>>  		} else {
>>  			res->start = conflict->end + 1;
>> @@ -1477,7 +1477,7 @@ static int __init reserve_setup(char *str)
>>  			res->start = io_start;
>>  			res->end = io_start + io_num - 1;
>>  			res->flags |= IORESOURCE_BUSY;
>> -			res->desc = IORES_DESC_NONE;
>> +			res->desc = IORES_DESC_RESERVED;
>>  			res->child = NULL;
>>  			if (request_resource(parent, res) == 0)
>>  				reserved = x+1;
>> -- 
>> 2.17.1
>>
> 
> 
> There are a lot of places call region_intersects which use DESC_NONE,
> I'm not sure if needed changes accordingly.  Cced Dan and Toshi.
> 
> 
> Thanks
> Dave
> 

Re: [PATCH 1/2 v8] resource: add the new I/O resource descriptor 'IORES_DESC_RESERVED'

[Borislav Petkov]

On Fri, Nov 30, 2018 at 03:04:44PM +0800, lijiang wrote:
> I have noticed the changes on x86, but for IA64, i'm not sure whether it should do the same
> thing, so keep it as before.
> 
> If IA64 people would like to give any comment, that will be appreciated.

I think you should not touch ia64 and not make Tony unnecessarily power
up the old rust.

:-)

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 1/2 v8] resource: add the new I/O resource descriptor 'IORES_DESC_RESERVED'

[lijiang]

在 2018年12月07日 04:11, Borislav Petkov 写道:
> On Fri, Nov 30, 2018 at 03:04:44PM +0800, lijiang wrote:
>> I have noticed the changes on x86, but for IA64, i'm not sure whether it should do the same
>> thing, so keep it as before.
>>
>> If IA64 people would like to give any comment, that will be appreciated.
> 
> I think you should not touch ia64 and not make Tony unnecessarily power
> up the old rust.
> 
> :-)
> 
Ok, it's good to me. And i will get rid of these changes for ia64 in patch v9.

Thanks.
Lianbo

Re: [PATCH 1/2 v8] resource: add the new I/O resource descriptor 'IORES_DESC_RESERVED'

[lijiang]

在 2018年12月05日 05:33, Lendacky, Thomas 写道:
> On 11/29/2018 09:37 PM, Dave Young wrote:
>> + more people
>>
>> On 11/29/18 at 04:09pm, Lianbo Jiang wrote:
>>> When doing kexec_file_load, the first kernel needs to pass the e820
>>> reserved ranges to the second kernel. But kernel can not exactly
>>> match the e820 reserved ranges when walking through the iomem resources
>>> with the descriptor 'IORES_DESC_NONE', because several e820 types(
>>> e.g. E820_TYPE_RESERVED_KERN/E820_TYPE_RAM/E820_TYPE_UNUSABLE/E820
>>> _TYPE_RESERVED) are converted to the descriptor 'IORES_DESC_NONE'. It
>>> may pass these four types to the kdump kernel, that is not desired result.
>>>
>>> So, this patch adds a new I/O resource descriptor 'IORES_DESC_RESERVED'
>>> for the iomem resources search interfaces. It is helpful to exactly
>>> match the reserved resource ranges when walking through iomem resources.
>>>
>>> In addition, since the new descriptor 'IORES_DESC_RESERVED' is introduced,
>>> these code originally related to the descriptor 'IORES_DESC_NONE' need to
>>> be updated. Otherwise, it will be easily confused and also cause some
>>> errors. Because the 'E820_TYPE_RESERVED' type is converted to the new
>>> descriptor 'IORES_DESC_RESERVED' instead of 'IORES_DESC_NONE', it has been
>>> changed.
>>>
>>> Suggested-by: Dave Young <dyoung@redhat.com>
>>> Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
>>> ---
>>>  arch/ia64/kernel/efi.c |  4 ++++
>>>  arch/x86/kernel/e820.c |  2 +-
>>>  arch/x86/mm/ioremap.c  | 13 ++++++++++++-
>>>  include/linux/ioport.h |  1 +
>>>  kernel/resource.c      |  6 +++---
>>>  5 files changed, 21 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
>>> index 8f106638913c..1841e9b4db30 100644
>>> --- a/arch/ia64/kernel/efi.c
>>> +++ b/arch/ia64/kernel/efi.c
>>> @@ -1231,6 +1231,10 @@ efi_initialize_iomem_resources(struct resource *code_resource,
>>>  				break;
>>>  
>>>  			case EFI_RESERVED_TYPE:
>>> +				name = "reserved";
>>
>> Ingo updated X86 code to use "Reserved",  I think it would be good to do
>> same for this case as well
>>
>>> +				desc = IORES_DESC_RESERVED;
>>> +				break;
>>> +
>>>  			case EFI_RUNTIME_SERVICES_CODE:
>>>  			case EFI_RUNTIME_SERVICES_DATA:
>>>  			case EFI_ACPI_RECLAIM_MEMORY:
>>
>> Originally, above 3 are all "reserved", so probably they all should be
>> IORES_DESC_RESERVED.
>>
>> Can any IA64 people to review this?
>>
>>> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
>>> index 50895c2f937d..57fafdafb860 100644
>>> --- a/arch/x86/kernel/e820.c
>>> +++ b/arch/x86/kernel/e820.c
>>> @@ -1048,10 +1048,10 @@ static unsigned long __init e820_type_to_iores_desc(struct e820_entry *entry)
>>>  	case E820_TYPE_NVS:		return IORES_DESC_ACPI_NV_STORAGE;
>>>  	case E820_TYPE_PMEM:		return IORES_DESC_PERSISTENT_MEMORY;
>>>  	case E820_TYPE_PRAM:		return IORES_DESC_PERSISTENT_MEMORY_LEGACY;
>>> +	case E820_TYPE_RESERVED:	return IORES_DESC_RESERVED;
>>>  	case E820_TYPE_RESERVED_KERN:	/* Fall-through: */
>>>  	case E820_TYPE_RAM:		/* Fall-through: */
>>>  	case E820_TYPE_UNUSABLE:	/* Fall-through: */
>>> -	case E820_TYPE_RESERVED:	/* Fall-through: */
>>>  	default:			return IORES_DESC_NONE;
>>>  	}
>>>  }
>>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>>> index 5378d10f1d31..fea2ef99415d 100644
>>> --- a/arch/x86/mm/ioremap.c
>>> +++ b/arch/x86/mm/ioremap.c
>>> @@ -83,7 +83,18 @@ static bool __ioremap_check_ram(struct resource *res)
>>>  
>>>  static int __ioremap_check_desc_other(struct resource *res)
>>>  {
>>> -	return (res->desc != IORES_DESC_NONE);
>>> +	/*
>>> +	 * But now, the 'E820_TYPE_RESERVED' type is converted to the new
>>> +	 * descriptor 'IORES_DESC_RESERVED' instead of 'IORES_DESC_NONE',
>>> +	 * it has been changed. And the value of 'mem_flags.desc_other'
>>> +	 * is equal to 'true' if we don't strengthen the condition in this
>>> +	 * function, that is wrong. Because originally it is equal to
>>> +	 * 'false' for the same reserved type.
>>> +	 *
>>> +	 * So, that would be nice to keep it the same as before.
>>> +	 */
>>> +	return ((res->desc != IORES_DESC_NONE) &&
>>> +		(res->desc != IORES_DESC_RESERVED));
>>>  }
>>
>> Added Tom since he added the check function.  Is it possible to only
>> check explict valid desc types instead of exclude IORES_DESC_NONE?
> 
> Sorry for the delay...
> 
> The original intent of the check was to map most memory as encrypted under
> SEV if it was marked with a specific descriptor, since it was likely to
> not be MMIO. I tried converting most things that mapped memory to memremap
> vs ioremap, but ACPI was one area that I left alone and this check catches
> the mapping of the ACPI tables. I suppose it's possible to change this to
> check just for IORES_DESC_ACPI_* values, but I would have to do some
> testing.
> 
 
I'm looking forward to the result of your test about this change.
Once changed, this patch also needs to be changed accordingly.

Thank you, Tom.

Lianbo
> Thanks,
> Tom
> 
>>
>>>  
>>>  static int __ioremap_res_check(struct resource *res, void *arg)
>>> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
>>> index da0ebaec25f0..6ed59de48bd5 100644
>>> --- a/include/linux/ioport.h
>>> +++ b/include/linux/ioport.h
>>> @@ -133,6 +133,7 @@ enum {
>>>  	IORES_DESC_PERSISTENT_MEMORY_LEGACY	= 5,
>>>  	IORES_DESC_DEVICE_PRIVATE_MEMORY	= 6,
>>>  	IORES_DESC_DEVICE_PUBLIC_MEMORY		= 7,
>>> +	IORES_DESC_RESERVED			= 8,
>>>  };
>>>  
>>>  /* helpers to define resources */
>>> diff --git a/kernel/resource.c b/kernel/resource.c
>>> index b0fbf685c77a..f34a632c4169 100644
>>> --- a/kernel/resource.c
>>> +++ b/kernel/resource.c
>>> @@ -994,7 +994,7 @@ __reserve_region_with_split(struct resource *root, resource_size_t start,
>>>  	res->start = start;
>>>  	res->end = end;
>>>  	res->flags = type | IORESOURCE_BUSY;
>>> -	res->desc = IORES_DESC_NONE;
>>> +	res->desc = IORES_DESC_RESERVED;
>>>  
>>>  	while (1) {
>>>  
>>> @@ -1029,7 +1029,7 @@ __reserve_region_with_split(struct resource *root, resource_size_t start,
>>>  				next_res->start = conflict->end + 1;
>>>  				next_res->end = end;
>>>  				next_res->flags = type | IORESOURCE_BUSY;
>>> -				next_res->desc = IORES_DESC_NONE;
>>> +				next_res->desc = IORES_DESC_RESERVED;
>>>  			}
>>>  		} else {
>>>  			res->start = conflict->end + 1;
>>> @@ -1477,7 +1477,7 @@ static int __init reserve_setup(char *str)
>>>  			res->start = io_start;
>>>  			res->end = io_start + io_num - 1;
>>>  			res->flags |= IORESOURCE_BUSY;
>>> -			res->desc = IORES_DESC_NONE;
>>> +			res->desc = IORES_DESC_RESERVED;
>>>  			res->child = NULL;
>>>  			if (request_resource(parent, res) == 0)
>>>  				reserved = x+1;
>>> -- 
>>> 2.17.1
>>>
>>
>>
>> There are a lot of places call region_intersects which use DESC_NONE,
>> I'm not sure if needed changes accordingly.  Cced Dan and Toshi.
>>
>>
>> Thanks
>> Dave
>>

Re: [PATCH 1/2 v8] resource: add the new I/O resource descriptor 'IORES_DESC_RESERVED'

[lijiang]

在 2018年12月07日 04:11, Borislav Petkov 写道:
> On Fri, Nov 30, 2018 at 03:04:44PM +0800, lijiang wrote:
>> I have noticed the changes on x86, but for IA64, i'm not sure whether it should do the same
>> thing, so keep it as before.
>>
>> If IA64 people would like to give any comment, that will be appreciated.
> 
> I think you should not touch ia64 and not make Tony unnecessarily power
> up the old rust.
> 

Ok, i will get rid of previous changes to IA64 in next post.

Thanks.

> :-)
> 

Re: [PATCH 1/2 v8] resource: add the new I/O resource descriptor 'IORES_DESC_RESERVED'

[lijiang]

在 2018年12月05日 05:33, Lendacky, Thomas 写道:
> On 11/29/2018 09:37 PM, Dave Young wrote:
>> + more people
>>
>> On 11/29/18 at 04:09pm, Lianbo Jiang wrote:
>>> When doing kexec_file_load, the first kernel needs to pass the e820
>>> reserved ranges to the second kernel. But kernel can not exactly
>>> match the e820 reserved ranges when walking through the iomem resources
>>> with the descriptor 'IORES_DESC_NONE', because several e820 types(
>>> e.g. E820_TYPE_RESERVED_KERN/E820_TYPE_RAM/E820_TYPE_UNUSABLE/E820
>>> _TYPE_RESERVED) are converted to the descriptor 'IORES_DESC_NONE'. It
>>> may pass these four types to the kdump kernel, that is not desired result.
>>>
>>> So, this patch adds a new I/O resource descriptor 'IORES_DESC_RESERVED'
>>> for the iomem resources search interfaces. It is helpful to exactly
>>> match the reserved resource ranges when walking through iomem resources.
>>>
>>> In addition, since the new descriptor 'IORES_DESC_RESERVED' is introduced,
>>> these code originally related to the descriptor 'IORES_DESC_NONE' need to
>>> be updated. Otherwise, it will be easily confused and also cause some
>>> errors. Because the 'E820_TYPE_RESERVED' type is converted to the new
>>> descriptor 'IORES_DESC_RESERVED' instead of 'IORES_DESC_NONE', it has been
>>> changed.
>>>
>>> Suggested-by: Dave Young <dyoung@redhat.com>
>>> Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
>>> ---
>>>  arch/ia64/kernel/efi.c |  4 ++++
>>>  arch/x86/kernel/e820.c |  2 +-
>>>  arch/x86/mm/ioremap.c  | 13 ++++++++++++-
>>>  include/linux/ioport.h |  1 +
>>>  kernel/resource.c      |  6 +++---
>>>  5 files changed, 21 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
>>> index 8f106638913c..1841e9b4db30 100644
>>> --- a/arch/ia64/kernel/efi.c
>>> +++ b/arch/ia64/kernel/efi.c
>>> @@ -1231,6 +1231,10 @@ efi_initialize_iomem_resources(struct resource *code_resource,
>>>  				break;
>>>  
>>>  			case EFI_RESERVED_TYPE:
>>> +				name = "reserved";
>>
>> Ingo updated X86 code to use "Reserved",  I think it would be good to do
>> same for this case as well
>>
>>> +				desc = IORES_DESC_RESERVED;
>>> +				break;
>>> +
>>>  			case EFI_RUNTIME_SERVICES_CODE:
>>>  			case EFI_RUNTIME_SERVICES_DATA:
>>>  			case EFI_ACPI_RECLAIM_MEMORY:
>>
>> Originally, above 3 are all "reserved", so probably they all should be
>> IORES_DESC_RESERVED.
>>
>> Can any IA64 people to review this?
>>
>>> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
>>> index 50895c2f937d..57fafdafb860 100644
>>> --- a/arch/x86/kernel/e820.c
>>> +++ b/arch/x86/kernel/e820.c
>>> @@ -1048,10 +1048,10 @@ static unsigned long __init e820_type_to_iores_desc(struct e820_entry *entry)
>>>  	case E820_TYPE_NVS:		return IORES_DESC_ACPI_NV_STORAGE;
>>>  	case E820_TYPE_PMEM:		return IORES_DESC_PERSISTENT_MEMORY;
>>>  	case E820_TYPE_PRAM:		return IORES_DESC_PERSISTENT_MEMORY_LEGACY;
>>> +	case E820_TYPE_RESERVED:	return IORES_DESC_RESERVED;
>>>  	case E820_TYPE_RESERVED_KERN:	/* Fall-through: */
>>>  	case E820_TYPE_RAM:		/* Fall-through: */
>>>  	case E820_TYPE_UNUSABLE:	/* Fall-through: */
>>> -	case E820_TYPE_RESERVED:	/* Fall-through: */
>>>  	default:			return IORES_DESC_NONE;
>>>  	}
>>>  }
>>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>>> index 5378d10f1d31..fea2ef99415d 100644
>>> --- a/arch/x86/mm/ioremap.c
>>> +++ b/arch/x86/mm/ioremap.c
>>> @@ -83,7 +83,18 @@ static bool __ioremap_check_ram(struct resource *res)
>>>  
>>>  static int __ioremap_check_desc_other(struct resource *res)
>>>  {
>>> -	return (res->desc != IORES_DESC_NONE);
>>> +	/*
>>> +	 * But now, the 'E820_TYPE_RESERVED' type is converted to the new
>>> +	 * descriptor 'IORES_DESC_RESERVED' instead of 'IORES_DESC_NONE',
>>> +	 * it has been changed. And the value of 'mem_flags.desc_other'
>>> +	 * is equal to 'true' if we don't strengthen the condition in this
>>> +	 * function, that is wrong. Because originally it is equal to
>>> +	 * 'false' for the same reserved type.
>>> +	 *
>>> +	 * So, that would be nice to keep it the same as before.
>>> +	 */
>>> +	return ((res->desc != IORES_DESC_NONE) &&
>>> +		(res->desc != IORES_DESC_RESERVED));
>>>  }
>>
>> Added Tom since he added the check function.  Is it possible to only
>> check explict valid desc types instead of exclude IORES_DESC_NONE?
> 
> Sorry for the delay...
> 
> The original intent of the check was to map most memory as encrypted under
> SEV if it was marked with a specific descriptor, since it was likely to
> not be MMIO. I tried converting most things that mapped memory to memremap
> vs ioremap, but ACPI was one area that I left alone and this check catches
> the mapping of the ACPI tables. I suppose it's possible to change this to
> check just for IORES_DESC_ACPI_* values, but I would have to do some
> testing.
> 

If you have any updates, would you mind sharing the test results about this
issue?

Thank you, Tom.

Lianbo

> Thanks,
> Tom
> 
>>
>>>  
>>>  static int __ioremap_res_check(struct resource *res, void *arg)
>>> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
>>> index da0ebaec25f0..6ed59de48bd5 100644
>>> --- a/include/linux/ioport.h
>>> +++ b/include/linux/ioport.h
>>> @@ -133,6 +133,7 @@ enum {
>>>  	IORES_DESC_PERSISTENT_MEMORY_LEGACY	= 5,
>>>  	IORES_DESC_DEVICE_PRIVATE_MEMORY	= 6,
>>>  	IORES_DESC_DEVICE_PUBLIC_MEMORY		= 7,
>>> +	IORES_DESC_RESERVED			= 8,
>>>  };
>>>  
>>>  /* helpers to define resources */
>>> diff --git a/kernel/resource.c b/kernel/resource.c
>>> index b0fbf685c77a..f34a632c4169 100644
>>> --- a/kernel/resource.c
>>> +++ b/kernel/resource.c
>>> @@ -994,7 +994,7 @@ __reserve_region_with_split(struct resource *root, resource_size_t start,
>>>  	res->start = start;
>>>  	res->end = end;
>>>  	res->flags = type | IORESOURCE_BUSY;
>>> -	res->desc = IORES_DESC_NONE;
>>> +	res->desc = IORES_DESC_RESERVED;
>>>  
>>>  	while (1) {
>>>  
>>> @@ -1029,7 +1029,7 @@ __reserve_region_with_split(struct resource *root, resource_size_t start,
>>>  				next_res->start = conflict->end + 1;
>>>  				next_res->end = end;
>>>  				next_res->flags = type | IORESOURCE_BUSY;
>>> -				next_res->desc = IORES_DESC_NONE;
>>> +				next_res->desc = IORES_DESC_RESERVED;
>>>  			}
>>>  		} else {
>>>  			res->start = conflict->end + 1;
>>> @@ -1477,7 +1477,7 @@ static int __init reserve_setup(char *str)
>>>  			res->start = io_start;
>>>  			res->end = io_start + io_num - 1;
>>>  			res->flags |= IORESOURCE_BUSY;
>>> -			res->desc = IORES_DESC_NONE;
>>> +			res->desc = IORES_DESC_RESERVED;
>>>  			res->child = NULL;
>>>  			if (request_resource(parent, res) == 0)
>>>  				reserved = x+1;
>>> -- 
>>> 2.17.1
>>>
>>
>>
>> There are a lot of places call region_intersects which use DESC_NONE,
>> I'm not sure if needed changes accordingly.  Cced Dan and Toshi.
>>
>>
>> Thanks
>> Dave
>>

Re: [PATCH 1/2 v8] resource: add the new I/O resource descriptor 'IORES_DESC_RESERVED'

[lijiang]

在 2018年12月05日 05:33, Lendacky, Thomas 写道:
> On 11/29/2018 09:37 PM, Dave Young wrote:
>> + more people
>>
>> On 11/29/18 at 04:09pm, Lianbo Jiang wrote:
>>> When doing kexec_file_load, the first kernel needs to pass the e820
>>> reserved ranges to the second kernel. But kernel can not exactly
>>> match the e820 reserved ranges when walking through the iomem resources
>>> with the descriptor 'IORES_DESC_NONE', because several e820 types(
>>> e.g. E820_TYPE_RESERVED_KERN/E820_TYPE_RAM/E820_TYPE_UNUSABLE/E820
>>> _TYPE_RESERVED) are converted to the descriptor 'IORES_DESC_NONE'. It
>>> may pass these four types to the kdump kernel, that is not desired result.
>>>
>>> So, this patch adds a new I/O resource descriptor 'IORES_DESC_RESERVED'
>>> for the iomem resources search interfaces. It is helpful to exactly
>>> match the reserved resource ranges when walking through iomem resources.
>>>
>>> In addition, since the new descriptor 'IORES_DESC_RESERVED' is introduced,
>>> these code originally related to the descriptor 'IORES_DESC_NONE' need to
>>> be updated. Otherwise, it will be easily confused and also cause some
>>> errors. Because the 'E820_TYPE_RESERVED' type is converted to the new
>>> descriptor 'IORES_DESC_RESERVED' instead of 'IORES_DESC_NONE', it has been
>>> changed.
>>>
>>> Suggested-by: Dave Young <dyoung@redhat.com>
>>> Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
>>> ---
>>>  arch/ia64/kernel/efi.c |  4 ++++
>>>  arch/x86/kernel/e820.c |  2 +-
>>>  arch/x86/mm/ioremap.c  | 13 ++++++++++++-
>>>  include/linux/ioport.h |  1 +
>>>  kernel/resource.c      |  6 +++---
>>>  5 files changed, 21 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
>>> index 8f106638913c..1841e9b4db30 100644
>>> --- a/arch/ia64/kernel/efi.c
>>> +++ b/arch/ia64/kernel/efi.c
>>> @@ -1231,6 +1231,10 @@ efi_initialize_iomem_resources(struct resource *code_resource,
>>>  				break;
>>>  
>>>  			case EFI_RESERVED_TYPE:
>>> +				name = "reserved";
>>
>> Ingo updated X86 code to use "Reserved",  I think it would be good to do
>> same for this case as well
>>
>>> +				desc = IORES_DESC_RESERVED;
>>> +				break;
>>> +
>>>  			case EFI_RUNTIME_SERVICES_CODE:
>>>  			case EFI_RUNTIME_SERVICES_DATA:
>>>  			case EFI_ACPI_RECLAIM_MEMORY:
>>
>> Originally, above 3 are all "reserved", so probably they all should be
>> IORES_DESC_RESERVED.
>>
>> Can any IA64 people to review this?
>>
>>> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
>>> index 50895c2f937d..57fafdafb860 100644
>>> --- a/arch/x86/kernel/e820.c
>>> +++ b/arch/x86/kernel/e820.c
>>> @@ -1048,10 +1048,10 @@ static unsigned long __init e820_type_to_iores_desc(struct e820_entry *entry)
>>>  	case E820_TYPE_NVS:		return IORES_DESC_ACPI_NV_STORAGE;
>>>  	case E820_TYPE_PMEM:		return IORES_DESC_PERSISTENT_MEMORY;
>>>  	case E820_TYPE_PRAM:		return IORES_DESC_PERSISTENT_MEMORY_LEGACY;
>>> +	case E820_TYPE_RESERVED:	return IORES_DESC_RESERVED;
>>>  	case E820_TYPE_RESERVED_KERN:	/* Fall-through: */
>>>  	case E820_TYPE_RAM:		/* Fall-through: */
>>>  	case E820_TYPE_UNUSABLE:	/* Fall-through: */
>>> -	case E820_TYPE_RESERVED:	/* Fall-through: */
>>>  	default:			return IORES_DESC_NONE;
>>>  	}
>>>  }
>>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>>> index 5378d10f1d31..fea2ef99415d 100644
>>> --- a/arch/x86/mm/ioremap.c
>>> +++ b/arch/x86/mm/ioremap.c
>>> @@ -83,7 +83,18 @@ static bool __ioremap_check_ram(struct resource *res)
>>>  
>>>  static int __ioremap_check_desc_other(struct resource *res)
>>>  {
>>> -	return (res->desc != IORES_DESC_NONE);
>>> +	/*
>>> +	 * But now, the 'E820_TYPE_RESERVED' type is converted to the new
>>> +	 * descriptor 'IORES_DESC_RESERVED' instead of 'IORES_DESC_NONE',
>>> +	 * it has been changed. And the value of 'mem_flags.desc_other'
>>> +	 * is equal to 'true' if we don't strengthen the condition in this
>>> +	 * function, that is wrong. Because originally it is equal to
>>> +	 * 'false' for the same reserved type.
>>> +	 *
>>> +	 * So, that would be nice to keep it the same as before.
>>> +	 */
>>> +	return ((res->desc != IORES_DESC_NONE) &&
>>> +		(res->desc != IORES_DESC_RESERVED));
>>>  }
>>
>> Added Tom since he added the check function.  Is it possible to only
>> check explict valid desc types instead of exclude IORES_DESC_NONE?
> 
> Sorry for the delay...
> 
> The original intent of the check was to map most memory as encrypted under
> SEV if it was marked with a specific descriptor, since it was likely to
> not be MMIO. I tried converting most things that mapped memory to memremap
> vs ioremap, but ACPI was one area that I left alone and this check catches
> the mapping of the ACPI tables. I suppose it's possible to change this to
> check just for IORES_DESC_ACPI_* values, but I would have to do some
> testing.

Recently, i tested it according to your advice, here it is really checking for the
'IORES_DESC_ACPI_*' values.  If you agree to this change, i would add the following
patch into this patch set and post them again.

[root@localhost linux]# git diff arch/x86/mm/ioremap.c
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 0029604af8a4..0e3ba620612d 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -83,7 +83,8 @@ static bool __ioremap_check_ram(struct resource *res)
 
 static int __ioremap_check_desc_other(struct resource *res)
 {
-       return (res->desc != IORES_DESC_NONE);
+       return ((res->desc == IORES_DESC_ACPI_TABLES) ||
+               (res->desc == IORES_DESC_ACPI_NV_STORAGE));
 }


Thanks.
Lianbo

> 
> Thanks,
> Tom
> 
>>
>>>  
>>>  static int __ioremap_res_check(struct resource *res, void *arg)
>>> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
>>> index da0ebaec25f0..6ed59de48bd5 100644
>>> --- a/include/linux/ioport.h
>>> +++ b/include/linux/ioport.h
>>> @@ -133,6 +133,7 @@ enum {
>>>  	IORES_DESC_PERSISTENT_MEMORY_LEGACY	= 5,
>>>  	IORES_DESC_DEVICE_PRIVATE_MEMORY	= 6,
>>>  	IORES_DESC_DEVICE_PUBLIC_MEMORY		= 7,
>>> +	IORES_DESC_RESERVED			= 8,
>>>  };
>>>  
>>>  /* helpers to define resources */
>>> diff --git a/kernel/resource.c b/kernel/resource.c
>>> index b0fbf685c77a..f34a632c4169 100644
>>> --- a/kernel/resource.c
>>> +++ b/kernel/resource.c
>>> @@ -994,7 +994,7 @@ __reserve_region_with_split(struct resource *root, resource_size_t start,
>>>  	res->start = start;
>>>  	res->end = end;
>>>  	res->flags = type | IORESOURCE_BUSY;
>>> -	res->desc = IORES_DESC_NONE;
>>> +	res->desc = IORES_DESC_RESERVED;
>>>  
>>>  	while (1) {
>>>  
>>> @@ -1029,7 +1029,7 @@ __reserve_region_with_split(struct resource *root, resource_size_t start,
>>>  				next_res->start = conflict->end + 1;
>>>  				next_res->end = end;
>>>  				next_res->flags = type | IORESOURCE_BUSY;
>>> -				next_res->desc = IORES_DESC_NONE;
>>> +				next_res->desc = IORES_DESC_RESERVED;
>>>  			}
>>>  		} else {
>>>  			res->start = conflict->end + 1;
>>> @@ -1477,7 +1477,7 @@ static int __init reserve_setup(char *str)
>>>  			res->start = io_start;
>>>  			res->end = io_start + io_num - 1;
>>>  			res->flags |= IORESOURCE_BUSY;
>>> -			res->desc = IORES_DESC_NONE;
>>> +			res->desc = IORES_DESC_RESERVED;
>>>  			res->child = NULL;
>>>  			if (request_resource(parent, res) == 0)
>>>  				reserved = x+1;
>>> -- 
>>> 2.17.1
>>>
>>
>>
>> There are a lot of places call region_intersects which use DESC_NONE,
>> I'm not sure if needed changes accordingly.  Cced Dan and Toshi.
>>
>>
>> Thanks
>> Dave
>>

Re: [PATCH 1/2 v8] resource: add the new I/O resource descriptor 'IORES_DESC_RESERVED'

[Lendacky, Thomas]

On 3/16/19 2:31 AM, lijiang wrote:
> 
> 
> 在 2018年12月05日 05:33, Lendacky, Thomas 写道:
>> On 11/29/2018 09:37 PM, Dave Young wrote:
>>> + more people
>>>
>>> On 11/29/18 at 04:09pm, Lianbo Jiang wrote:
>>>> When doing kexec_file_load, the first kernel needs to pass the e820
>>>> reserved ranges to the second kernel. But kernel can not exactly
>>>> match the e820 reserved ranges when walking through the iomem resources
>>>> with the descriptor 'IORES_DESC_NONE', because several e820 types(
>>>> e.g. E820_TYPE_RESERVED_KERN/E820_TYPE_RAM/E820_TYPE_UNUSABLE/E820
>>>> _TYPE_RESERVED) are converted to the descriptor 'IORES_DESC_NONE'. It
>>>> may pass these four types to the kdump kernel, that is not desired result.
>>>>
>>>> So, this patch adds a new I/O resource descriptor 'IORES_DESC_RESERVED'
>>>> for the iomem resources search interfaces. It is helpful to exactly
>>>> match the reserved resource ranges when walking through iomem resources.
>>>>
>>>> In addition, since the new descriptor 'IORES_DESC_RESERVED' is introduced,
>>>> these code originally related to the descriptor 'IORES_DESC_NONE' need to
>>>> be updated. Otherwise, it will be easily confused and also cause some
>>>> errors. Because the 'E820_TYPE_RESERVED' type is converted to the new
>>>> descriptor 'IORES_DESC_RESERVED' instead of 'IORES_DESC_NONE', it has been
>>>> changed.
>>>>
>>>> Suggested-by: Dave Young <dyoung@redhat.com>
>>>> Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
>>>> ---
>>>>  arch/ia64/kernel/efi.c |  4 ++++
>>>>  arch/x86/kernel/e820.c |  2 +-
>>>>  arch/x86/mm/ioremap.c  | 13 ++++++++++++-
>>>>  include/linux/ioport.h |  1 +
>>>>  kernel/resource.c      |  6 +++---
>>>>  5 files changed, 21 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
>>>> index 8f106638913c..1841e9b4db30 100644
>>>> --- a/arch/ia64/kernel/efi.c
>>>> +++ b/arch/ia64/kernel/efi.c
>>>> @@ -1231,6 +1231,10 @@ efi_initialize_iomem_resources(struct resource *code_resource,
>>>>  				break;
>>>>  
>>>>  			case EFI_RESERVED_TYPE:
>>>> +				name = "reserved";
>>>
>>> Ingo updated X86 code to use "Reserved",  I think it would be good to do
>>> same for this case as well
>>>
>>>> +				desc = IORES_DESC_RESERVED;
>>>> +				break;
>>>> +
>>>>  			case EFI_RUNTIME_SERVICES_CODE:
>>>>  			case EFI_RUNTIME_SERVICES_DATA:
>>>>  			case EFI_ACPI_RECLAIM_MEMORY:
>>>
>>> Originally, above 3 are all "reserved", so probably they all should be
>>> IORES_DESC_RESERVED.
>>>
>>> Can any IA64 people to review this?
>>>
>>>> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
>>>> index 50895c2f937d..57fafdafb860 100644
>>>> --- a/arch/x86/kernel/e820.c
>>>> +++ b/arch/x86/kernel/e820.c
>>>> @@ -1048,10 +1048,10 @@ static unsigned long __init e820_type_to_iores_desc(struct e820_entry *entry)
>>>>  	case E820_TYPE_NVS:		return IORES_DESC_ACPI_NV_STORAGE;
>>>>  	case E820_TYPE_PMEM:		return IORES_DESC_PERSISTENT_MEMORY;
>>>>  	case E820_TYPE_PRAM:		return IORES_DESC_PERSISTENT_MEMORY_LEGACY;
>>>> +	case E820_TYPE_RESERVED:	return IORES_DESC_RESERVED;
>>>>  	case E820_TYPE_RESERVED_KERN:	/* Fall-through: */
>>>>  	case E820_TYPE_RAM:		/* Fall-through: */
>>>>  	case E820_TYPE_UNUSABLE:	/* Fall-through: */
>>>> -	case E820_TYPE_RESERVED:	/* Fall-through: */
>>>>  	default:			return IORES_DESC_NONE;
>>>>  	}
>>>>  }
>>>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>>>> index 5378d10f1d31..fea2ef99415d 100644
>>>> --- a/arch/x86/mm/ioremap.c
>>>> +++ b/arch/x86/mm/ioremap.c
>>>> @@ -83,7 +83,18 @@ static bool __ioremap_check_ram(struct resource *res)
>>>>  
>>>>  static int __ioremap_check_desc_other(struct resource *res)
>>>>  {
>>>> -	return (res->desc != IORES_DESC_NONE);
>>>> +	/*
>>>> +	 * But now, the 'E820_TYPE_RESERVED' type is converted to the new
>>>> +	 * descriptor 'IORES_DESC_RESERVED' instead of 'IORES_DESC_NONE',
>>>> +	 * it has been changed. And the value of 'mem_flags.desc_other'
>>>> +	 * is equal to 'true' if we don't strengthen the condition in this
>>>> +	 * function, that is wrong. Because originally it is equal to
>>>> +	 * 'false' for the same reserved type.
>>>> +	 *
>>>> +	 * So, that would be nice to keep it the same as before.
>>>> +	 */
>>>> +	return ((res->desc != IORES_DESC_NONE) &&
>>>> +		(res->desc != IORES_DESC_RESERVED));
>>>>  }
>>>
>>> Added Tom since he added the check function.  Is it possible to only
>>> check explict valid desc types instead of exclude IORES_DESC_NONE?
>>
>> Sorry for the delay...
>>
>> The original intent of the check was to map most memory as encrypted under
>> SEV if it was marked with a specific descriptor, since it was likely to
>> not be MMIO. I tried converting most things that mapped memory to memremap
>> vs ioremap, but ACPI was one area that I left alone and this check catches
>> the mapping of the ACPI tables. I suppose it's possible to change this to
>> check just for IORES_DESC_ACPI_* values, but I would have to do some
>> testing.
> 
> Recently, i tested it according to your advice, here it is really checking for the
> 'IORES_DESC_ACPI_*' values.  If you agree to this change, i would add the following
> patch into this patch set and post them again.
> 
> [root@localhost linux]# git diff arch/x86/mm/ioremap.c
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index 0029604af8a4..0e3ba620612d 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -83,7 +83,8 @@ static bool __ioremap_check_ram(struct resource *res)
>  
>  static int __ioremap_check_desc_other(struct resource *res)
>  {
> -       return (res->desc != IORES_DESC_NONE);
> +       return ((res->desc == IORES_DESC_ACPI_TABLES) ||
> +               (res->desc == IORES_DESC_ACPI_NV_STORAGE));

I'm not a big fan of this. I think you should leave it as the previous
check you had for IORES_DESC_NONE and IORES_DESC_RESERVED. There's no
telling what type of resources may be mapped in the future where this
will break.

Adding a nice comment here about how IORES_DESC_NONE originally was to
identify MMIO and reserved areas. Now IORES_DESC_RESERVED has been created
for the reserved areas so the check needs to be expanded so that these
areas aren't mapped encrypted when using ioremap.

Thanks,
Tom

>  }
> 
> 
> Thanks.
> Lianbo
> 
>>
>> Thanks,
>> Tom
>>
>>>
>>>>  
>>>>  static int __ioremap_res_check(struct resource *res, void *arg)
>>>> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
>>>> index da0ebaec25f0..6ed59de48bd5 100644
>>>> --- a/include/linux/ioport.h
>>>> +++ b/include/linux/ioport.h
>>>> @@ -133,6 +133,7 @@ enum {
>>>>  	IORES_DESC_PERSISTENT_MEMORY_LEGACY	= 5,
>>>>  	IORES_DESC_DEVICE_PRIVATE_MEMORY	= 6,
>>>>  	IORES_DESC_DEVICE_PUBLIC_MEMORY		= 7,
>>>> +	IORES_DESC_RESERVED			= 8,
>>>>  };
>>>>  
>>>>  /* helpers to define resources */
>>>> diff --git a/kernel/resource.c b/kernel/resource.c
>>>> index b0fbf685c77a..f34a632c4169 100644
>>>> --- a/kernel/resource.c
>>>> +++ b/kernel/resource.c
>>>> @@ -994,7 +994,7 @@ __reserve_region_with_split(struct resource *root, resource_size_t start,
>>>>  	res->start = start;
>>>>  	res->end = end;
>>>>  	res->flags = type | IORESOURCE_BUSY;
>>>> -	res->desc = IORES_DESC_NONE;
>>>> +	res->desc = IORES_DESC_RESERVED;
>>>>  
>>>>  	while (1) {
>>>>  
>>>> @@ -1029,7 +1029,7 @@ __reserve_region_with_split(struct resource *root, resource_size_t start,
>>>>  				next_res->start = conflict->end + 1;
>>>>  				next_res->end = end;
>>>>  				next_res->flags = type | IORESOURCE_BUSY;
>>>> -				next_res->desc = IORES_DESC_NONE;
>>>> +				next_res->desc = IORES_DESC_RESERVED;
>>>>  			}
>>>>  		} else {
>>>>  			res->start = conflict->end + 1;
>>>> @@ -1477,7 +1477,7 @@ static int __init reserve_setup(char *str)
>>>>  			res->start = io_start;
>>>>  			res->end = io_start + io_num - 1;
>>>>  			res->flags |= IORESOURCE_BUSY;
>>>> -			res->desc = IORES_DESC_NONE;
>>>> +			res->desc = IORES_DESC_RESERVED;
>>>>  			res->child = NULL;
>>>>  			if (request_resource(parent, res) == 0)
>>>>  				reserved = x+1;
>>>> -- 
>>>> 2.17.1
>>>>
>>>
>>>
>>> There are a lot of places call region_intersects which use DESC_NONE,
>>> I'm not sure if needed changes accordingly.  Cced Dan and Toshi.
>>>
>>>
>>> Thanks
>>> Dave
>>>

Re: [PATCH 1/2 v8] resource: add the new I/O resource descriptor 'IORES_DESC_RESERVED'

[lijiang]

在 2019年03月26日 03:34, Lendacky, Thomas 写道:
> On 3/16/19 2:31 AM, lijiang wrote:
>>
>>
>> 在 2018年12月05日 05:33, Lendacky, Thomas 写道:
>>> On 11/29/2018 09:37 PM, Dave Young wrote:
>>>> + more people
>>>>
>>>> On 11/29/18 at 04:09pm, Lianbo Jiang wrote:
>>>>> When doing kexec_file_load, the first kernel needs to pass the e820
>>>>> reserved ranges to the second kernel. But kernel can not exactly
>>>>> match the e820 reserved ranges when walking through the iomem resources
>>>>> with the descriptor 'IORES_DESC_NONE', because several e820 types(
>>>>> e.g. E820_TYPE_RESERVED_KERN/E820_TYPE_RAM/E820_TYPE_UNUSABLE/E820
>>>>> _TYPE_RESERVED) are converted to the descriptor 'IORES_DESC_NONE'. It
>>>>> may pass these four types to the kdump kernel, that is not desired result.
>>>>>
>>>>> So, this patch adds a new I/O resource descriptor 'IORES_DESC_RESERVED'
>>>>> for the iomem resources search interfaces. It is helpful to exactly
>>>>> match the reserved resource ranges when walking through iomem resources.
>>>>>
>>>>> In addition, since the new descriptor 'IORES_DESC_RESERVED' is introduced,
>>>>> these code originally related to the descriptor 'IORES_DESC_NONE' need to
>>>>> be updated. Otherwise, it will be easily confused and also cause some
>>>>> errors. Because the 'E820_TYPE_RESERVED' type is converted to the new
>>>>> descriptor 'IORES_DESC_RESERVED' instead of 'IORES_DESC_NONE', it has been
>>>>> changed.
>>>>>
>>>>> Suggested-by: Dave Young <dyoung@redhat.com>
>>>>> Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
>>>>> ---
>>>>>  arch/ia64/kernel/efi.c |  4 ++++
>>>>>  arch/x86/kernel/e820.c |  2 +-
>>>>>  arch/x86/mm/ioremap.c  | 13 ++++++++++++-
>>>>>  include/linux/ioport.h |  1 +
>>>>>  kernel/resource.c      |  6 +++---
>>>>>  5 files changed, 21 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
>>>>> index 8f106638913c..1841e9b4db30 100644
>>>>> --- a/arch/ia64/kernel/efi.c
>>>>> +++ b/arch/ia64/kernel/efi.c
>>>>> @@ -1231,6 +1231,10 @@ efi_initialize_iomem_resources(struct resource *code_resource,
>>>>>  				break;
>>>>>  
>>>>>  			case EFI_RESERVED_TYPE:
>>>>> +				name = "reserved";
>>>>
>>>> Ingo updated X86 code to use "Reserved",  I think it would be good to do
>>>> same for this case as well
>>>>
>>>>> +				desc = IORES_DESC_RESERVED;
>>>>> +				break;
>>>>> +
>>>>>  			case EFI_RUNTIME_SERVICES_CODE:
>>>>>  			case EFI_RUNTIME_SERVICES_DATA:
>>>>>  			case EFI_ACPI_RECLAIM_MEMORY:
>>>>
>>>> Originally, above 3 are all "reserved", so probably they all should be
>>>> IORES_DESC_RESERVED.
>>>>
>>>> Can any IA64 people to review this?
>>>>
>>>>> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
>>>>> index 50895c2f937d..57fafdafb860 100644
>>>>> --- a/arch/x86/kernel/e820.c
>>>>> +++ b/arch/x86/kernel/e820.c
>>>>> @@ -1048,10 +1048,10 @@ static unsigned long __init e820_type_to_iores_desc(struct e820_entry *entry)
>>>>>  	case E820_TYPE_NVS:		return IORES_DESC_ACPI_NV_STORAGE;
>>>>>  	case E820_TYPE_PMEM:		return IORES_DESC_PERSISTENT_MEMORY;
>>>>>  	case E820_TYPE_PRAM:		return IORES_DESC_PERSISTENT_MEMORY_LEGACY;
>>>>> +	case E820_TYPE_RESERVED:	return IORES_DESC_RESERVED;
>>>>>  	case E820_TYPE_RESERVED_KERN:	/* Fall-through: */
>>>>>  	case E820_TYPE_RAM:		/* Fall-through: */
>>>>>  	case E820_TYPE_UNUSABLE:	/* Fall-through: */
>>>>> -	case E820_TYPE_RESERVED:	/* Fall-through: */
>>>>>  	default:			return IORES_DESC_NONE;
>>>>>  	}
>>>>>  }
>>>>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>>>>> index 5378d10f1d31..fea2ef99415d 100644
>>>>> --- a/arch/x86/mm/ioremap.c
>>>>> +++ b/arch/x86/mm/ioremap.c
>>>>> @@ -83,7 +83,18 @@ static bool __ioremap_check_ram(struct resource *res)
>>>>>  
>>>>>  static int __ioremap_check_desc_other(struct resource *res)
>>>>>  {
>>>>> -	return (res->desc != IORES_DESC_NONE);
>>>>> +	/*
>>>>> +	 * But now, the 'E820_TYPE_RESERVED' type is converted to the new
>>>>> +	 * descriptor 'IORES_DESC_RESERVED' instead of 'IORES_DESC_NONE',
>>>>> +	 * it has been changed. And the value of 'mem_flags.desc_other'
>>>>> +	 * is equal to 'true' if we don't strengthen the condition in this
>>>>> +	 * function, that is wrong. Because originally it is equal to
>>>>> +	 * 'false' for the same reserved type.
>>>>> +	 *
>>>>> +	 * So, that would be nice to keep it the same as before.
>>>>> +	 */
>>>>> +	return ((res->desc != IORES_DESC_NONE) &&
>>>>> +		(res->desc != IORES_DESC_RESERVED));
>>>>>  }
>>>>
>>>> Added Tom since he added the check function.  Is it possible to only
>>>> check explict valid desc types instead of exclude IORES_DESC_NONE?
>>>
>>> Sorry for the delay...
>>>
>>> The original intent of the check was to map most memory as encrypted under
>>> SEV if it was marked with a specific descriptor, since it was likely to
>>> not be MMIO. I tried converting most things that mapped memory to memremap
>>> vs ioremap, but ACPI was one area that I left alone and this check catches
>>> the mapping of the ACPI tables. I suppose it's possible to change this to
>>> check just for IORES_DESC_ACPI_* values, but I would have to do some
>>> testing.
>>
>> Recently, i tested it according to your advice, here it is really checking for the
>> 'IORES_DESC_ACPI_*' values.  If you agree to this change, i would add the following
>> patch into this patch set and post them again.
>>
>> [root@localhost linux]# git diff arch/x86/mm/ioremap.c
>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>> index 0029604af8a4..0e3ba620612d 100644
>> --- a/arch/x86/mm/ioremap.c
>> +++ b/arch/x86/mm/ioremap.c
>> @@ -83,7 +83,8 @@ static bool __ioremap_check_ram(struct resource *res)
>>  
>>  static int __ioremap_check_desc_other(struct resource *res)
>>  {
>> -       return (res->desc != IORES_DESC_NONE);
>> +       return ((res->desc == IORES_DESC_ACPI_TABLES) ||
>> +               (res->desc == IORES_DESC_ACPI_NV_STORAGE));
> 
> I'm not a big fan of this. I think you should leave it as the previous
> check you had for IORES_DESC_NONE and IORES_DESC_RESERVED. There's no
> telling what type of resources may be mapped in the future where this
> will break.
> 
> Adding a nice comment here about how IORES_DESC_NONE originally was to
> identify MMIO and reserved areas. Now IORES_DESC_RESERVED has been created
> for the reserved areas so the check needs to be expanded so that these
> areas aren't mapped encrypted when using ioremap.

Thanks for your comment.

It's OK for me. I'm not sure whether Boris has any suggestion.

Thanks.
Lianbo

> 
> Thanks,
> Tom
> 
>>  }
>>
>>
>> Thanks.
>> Lianbo
>>
>>>
>>> Thanks,
>>> Tom
>>>
>>>>
>>>>>  
>>>>>  static int __ioremap_res_check(struct resource *res, void *arg)
>>>>> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
>>>>> index da0ebaec25f0..6ed59de48bd5 100644
>>>>> --- a/include/linux/ioport.h
>>>>> +++ b/include/linux/ioport.h
>>>>> @@ -133,6 +133,7 @@ enum {
>>>>>  	IORES_DESC_PERSISTENT_MEMORY_LEGACY	= 5,
>>>>>  	IORES_DESC_DEVICE_PRIVATE_MEMORY	= 6,
>>>>>  	IORES_DESC_DEVICE_PUBLIC_MEMORY		= 7,
>>>>> +	IORES_DESC_RESERVED			= 8,
>>>>>  };
>>>>>  
>>>>>  /* helpers to define resources */
>>>>> diff --git a/kernel/resource.c b/kernel/resource.c
>>>>> index b0fbf685c77a..f34a632c4169 100644
>>>>> --- a/kernel/resource.c
>>>>> +++ b/kernel/resource.c
>>>>> @@ -994,7 +994,7 @@ __reserve_region_with_split(struct resource *root, resource_size_t start,
>>>>>  	res->start = start;
>>>>>  	res->end = end;
>>>>>  	res->flags = type | IORESOURCE_BUSY;
>>>>> -	res->desc = IORES_DESC_NONE;
>>>>> +	res->desc = IORES_DESC_RESERVED;
>>>>>  
>>>>>  	while (1) {
>>>>>  
>>>>> @@ -1029,7 +1029,7 @@ __reserve_region_with_split(struct resource *root, resource_size_t start,
>>>>>  				next_res->start = conflict->end + 1;
>>>>>  				next_res->end = end;
>>>>>  				next_res->flags = type | IORESOURCE_BUSY;
>>>>> -				next_res->desc = IORES_DESC_NONE;
>>>>> +				next_res->desc = IORES_DESC_RESERVED;
>>>>>  			}
>>>>>  		} else {
>>>>>  			res->start = conflict->end + 1;
>>>>> @@ -1477,7 +1477,7 @@ static int __init reserve_setup(char *str)
>>>>>  			res->start = io_start;
>>>>>  			res->end = io_start + io_num - 1;
>>>>>  			res->flags |= IORESOURCE_BUSY;
>>>>> -			res->desc = IORES_DESC_NONE;
>>>>> +			res->desc = IORES_DESC_RESERVED;
>>>>>  			res->child = NULL;
>>>>>  			if (request_resource(parent, res) == 0)
>>>>>  				reserved = x+1;
>>>>> -- 
>>>>> 2.17.1
>>>>>
>>>>
>>>>
>>>> There are a lot of places call region_intersects which use DESC_NONE,
>>>> I'm not sure if needed changes accordingly.  Cced Dan and Toshi.
>>>>
>>>>
>>>> Thanks
>>>> Dave
>>>>

Re: [PATCH 1/2 v8] resource: add the new I/O resource descriptor 'IORES_DESC_RESERVED'

[Boris Petkov]

On March 25, 2019 8:34:25 PM GMT+01:00, "Lendacky, Thomas" <Thomas.Lendacky@amd.com> wrote:
>On 3/16/19 2:31 AM, lijiang 
>Adding a nice comment here about how IORES_DESC_NONE originally was to
>identify MMIO and reserved areas. Now IORES_DESC_RESERVED has been
>created
>for the reserved areas so the check needs to be expanded

Expanded and made explicit *which* areas should not be mapped encrypted and then the function name corrected. 

Thx.

-- 
Sent from a small device: formatting sux and brevity is inevitable.