* Re: [PATCH] Updated /proc/iomem patch
2005-09-14 16:45 [PATCH] Updated /proc/iomem patch Khalid Aziz
@ 2005-09-14 18:32 ` Bjorn Helgaas
2005-09-15 16:26 ` Khalid Aziz
` (5 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Bjorn Helgaas @ 2005-09-14 18:32 UTC (permalink / raw)
To: linux-ia64
On Wednesday 14 September 2005 10:45 am, Khalid Aziz wrote:
> Here is an updated patch that makes /proc/iomem include information
> about system RAM as well. This makes /proc/iomem on ia64 look like it
> does on x86 and also enables kexec-tools to work on ia64.
> void
> efi_initialize_iomem_resources(struct resource *code_resource,
> struct resource *data_resource)
This function looks pretty generic. There *are* some small
differences between the i386 and ia64 versions, but I think
this would be a good opportunity to factor those out and move
the common code into drivers/firmware/efi.
There have already been bugs (i.e., broken dependencies on
memory descriptor size) that would have been avoided if i386
and ia64 used common code.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Updated /proc/iomem patch
2005-09-14 16:45 [PATCH] Updated /proc/iomem patch Khalid Aziz
2005-09-14 18:32 ` Bjorn Helgaas
@ 2005-09-15 16:26 ` Khalid Aziz
2005-09-15 16:46 ` Bjorn Helgaas
` (4 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Khalid Aziz @ 2005-09-15 16:26 UTC (permalink / raw)
To: linux-ia64
On Wed, 2005-09-14 at 10:45 -0600, Khalid Aziz wrote:
> Here is an updated patch that makes /proc/iomem include information
> about system RAM as well. This makes /proc/iomem on ia64 look like it
> does on x86 and also enables kexec-tools to work on ia64.
>
I am not sure if marking the memory allocated by bootloader for kernel,
inital ramdisk, EFI memory map and boot parameters as "reserved"
in /proc/iomem is the right thing to do. I have done it this way in the
patch but I would appreciate other opinions.
--
Khalid
==================================
Khalid Aziz Open Source and Linux Organization
(970)898-9214 Hewlett-Packard
khalid.aziz@hp.com Fort Collins, CO
"The Linux kernel is subject to relentless development"
- Alessandro Rubini
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Updated /proc/iomem patch
2005-09-14 16:45 [PATCH] Updated /proc/iomem patch Khalid Aziz
2005-09-14 18:32 ` Bjorn Helgaas
2005-09-15 16:26 ` Khalid Aziz
@ 2005-09-15 16:46 ` Bjorn Helgaas
2005-09-15 17:27 ` Khalid Aziz
` (3 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Bjorn Helgaas @ 2005-09-15 16:46 UTC (permalink / raw)
To: linux-ia64
On Thursday 15 September 2005 10:26 am, Khalid Aziz wrote:
> I am not sure if marking the memory allocated by bootloader for kernel,
> inital ramdisk, EFI memory map and boot parameters as "reserved"
> in /proc/iomem is the right thing to do. I have done it this way in the
> patch but I would appreciate other opinions.
I'm curious about how kexec-tools uses the information from iomem.
It tells you about physical address space, and you can't do much
with physical addresses from user-space. I suppose it's obvious
from the kexec-tools source -- do you have a pointer?
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Updated /proc/iomem patch
2005-09-14 16:45 [PATCH] Updated /proc/iomem patch Khalid Aziz
` (2 preceding siblings ...)
2005-09-15 16:46 ` Bjorn Helgaas
@ 2005-09-15 17:27 ` Khalid Aziz
2005-09-16 15:26 ` Khalid Aziz
` (2 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Khalid Aziz @ 2005-09-15 17:27 UTC (permalink / raw)
To: linux-ia64
On Thu, 2005-09-15 at 10:46 -0600, Bjorn Helgaas wrote:
> On Thursday 15 September 2005 10:26 am, Khalid Aziz wrote:
> > I am not sure if marking the memory allocated by bootloader for kernel,
> > inital ramdisk, EFI memory map and boot parameters as "reserved"
> > in /proc/iomem is the right thing to do. I have done it this way in the
> > patch but I would appreciate other opinions.
>
> I'm curious about how kexec-tools uses the information from iomem.
> It tells you about physical address space, and you can't do much
> with physical addresses from user-space. I suppose it's obvious
> from the kexec-tools source -- do you have a pointer?
As kexec tools reads various segments from the kernel image file, it
reads their final memory address where they should be loaded ina nd
attempts to verify if those addresses are valid. This is where it uses
information from /proc/iomem to determine if load addresses are indeed
real memory. This check prevents bad image from being loaded which kexec
will then try to execute later on kexec -e.
You can get kexec-tools sources from
<http://www.xmission.com/~ebiederm/files/kexec/>, or for Debian
unstable, simply "apt-get source kexec-tools" will pull the sources
down.
--
Khalid
==================================
Khalid Aziz Open Source and Linux Organization
(970)898-9214 Hewlett-Packard
khalid.aziz@hp.com Fort Collins, CO
"The Linux kernel is subject to relentless development"
- Alessandro Rubini
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Updated /proc/iomem patch
2005-09-14 16:45 [PATCH] Updated /proc/iomem patch Khalid Aziz
` (3 preceding siblings ...)
2005-09-15 17:27 ` Khalid Aziz
@ 2005-09-16 15:26 ` Khalid Aziz
2005-09-16 21:02 ` Luck, Tony
2005-09-28 21:31 ` Khalid Aziz
6 siblings, 0 replies; 8+ messages in thread
From: Khalid Aziz @ 2005-09-16 15:26 UTC (permalink / raw)
To: linux-ia64
[-- Attachment #1: Type: text/plain, Size: 1255 bytes --]
On Thu, 2005-09-15 at 10:26 -0600, Khalid Aziz wrote:
> On Wed, 2005-09-14 at 10:45 -0600, Khalid Aziz wrote:
> > Here is an updated patch that makes /proc/iomem include information
> > about system RAM as well. This makes /proc/iomem on ia64 look like it
> > does on x86 and also enables kexec-tools to work on ia64.
> >
>
> I am not sure if marking the memory allocated by bootloader for kernel,
> inital ramdisk, EFI memory map and boot parameters as "reserved"
> in /proc/iomem is the right thing to do. I have done it this way in the
> patch but I would appreciate other opinions.
>
The more I think about it, the more I am convinced memory type
EFI_LOADER_DATA should be marked simply "System RAM", not
"reserved". /proc/iomem is supposed to represent how physical address
space is used, not how physical memory is used.
Updated patch attached.
--
Khalid
====================================================================
Khalid Aziz Open Source and Linux Organization
(970)898-9214 Hewlett-Packard
khalid.aziz@hp.com Fort Collins, CO
"The Linux kernel is subject to relentless development"
- Alessandro Rubini
[-- Attachment #2: iomem.patch --]
[-- Type: text/x-patch, Size: 4058 bytes --]
diff -urNp linux-2.6.13.orig/arch/ia64/kernel/efi.c linux-2.6.13/arch/ia64/kernel/efi.c
--- linux-2.6.13.orig/arch/ia64/kernel/efi.c 2005-08-28 17:41:01.000000000 -0600
+++ linux-2.6.13/arch/ia64/kernel/efi.c 2005-09-14 09:37:47.000000000 -0600
@@ -862,3 +862,91 @@ efi_uart_console_only(void)
printk(KERN_ERR "Malformed %s value\n", name);
return 0;
}
+
+void
+efi_initialize_iomem_resources(struct resource *code_resource,
+ struct resource *data_resource)
+{
+ struct resource *res;
+ void *efi_map_start, *efi_map_end, *p;
+ efi_memory_desc_t *md;
+ u64 efi_desc_size;
+
+ efi_map_start = __va(ia64_boot_param->efi_memmap);
+ efi_map_end = efi_map_start + ia64_boot_param->efi_memmap_size;
+ efi_desc_size = ia64_boot_param->efi_memdesc_size;
+
+ res = NULL;
+
+ for (p = efi_map_start; p < efi_map_end; p += efi_desc_size) {
+ md = p;
+
+ /*
+ * Data was trimmed and may not be correct, look for bad data
+ * and skip it
+ */
+ if (md->num_pages == 0)
+ continue;
+
+ if ((res = kmalloc(sizeof(struct resource), GFP_KERNEL)) == NULL) {
+ printk("ERROR: failed to alocate resource for iomem\n");
+ return;
+ }
+ memset(res, 0, sizeof(*res));
+ res->flags = IORESOURCE_MEM;
+ switch (md->type) {
+
+ case EFI_MEMORY_MAPPED_IO:
+ case EFI_MEMORY_MAPPED_IO_PORT_SPACE:
+ kfree(res);
+ continue;
+ break;
+
+ case EFI_LOADER_CODE:
+ case EFI_LOADER_DATA:
+ case EFI_BOOT_SERVICES_DATA:
+ case EFI_BOOT_SERVICES_CODE:
+ case EFI_CONVENTIONAL_MEMORY:
+ if (md->attribute & EFI_MEMORY_WP) {
+ res->name = "System ROM";
+ res->flags |= IORESOURCE_READONLY;
+ } else {
+ res->name = "System RAM";
+ }
+ break;
+
+ case EFI_ACPI_MEMORY_NVS:
+ res->name = "ACPI Non-volatile Storage";
+ res->flags |= IORESOURCE_BUSY;
+ break;
+
+ case EFI_UNUSABLE_MEMORY:
+ res->name = "reserved";
+ res->flags |= IORESOURCE_BUSY | IORESOURCE_DISABLED;
+ break;
+
+ case EFI_RESERVED_TYPE:
+ case EFI_RUNTIME_SERVICES_CODE:
+ case EFI_RUNTIME_SERVICES_DATA:
+ case EFI_ACPI_RECLAIM_MEMORY:
+ default:
+ res->name = "reserved";
+ res->flags |= IORESOURCE_BUSY;
+ break;
+ }
+
+ res->start = md->phys_addr;
+ res->end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1;
+ if (insert_resource(&iomem_resource, res) < 0)
+ kfree(res);
+ else {
+ /*
+ * We don't know which region contains
+ * kernel data so we try it repeatedly and
+ * let the resource manager test it.
+ */
+ insert_resource(res, code_resource);
+ insert_resource(res, data_resource);
+ }
+ }
+}
diff -urNp linux-2.6.13.orig/arch/ia64/kernel/setup.c linux-2.6.13/arch/ia64/kernel/setup.c
--- linux-2.6.13.orig/arch/ia64/kernel/setup.c 2005-08-28 17:41:01.000000000 -0600
+++ linux-2.6.13/arch/ia64/kernel/setup.c 2005-09-14 09:16:50.000000000 -0600
@@ -78,6 +78,23 @@ struct screen_info screen_info;
unsigned long vga_console_iobase;
unsigned long vga_console_membase;
+static struct resource data_resource = {
+ .name = "Kernel data",
+ .start = 0,
+ .end = 0,
+ .flags = IORESOURCE_BUSY | IORESOURCE_MEM
+};
+
+static struct resource code_resource = {
+ .name = "Kernel code",
+ .start = 0,
+ .end = 0,
+ .flags = IORESOURCE_BUSY | IORESOURCE_MEM
+};
+extern void efi_initialize_iomem_resources(struct resource *,
+ struct resource *);
+extern char _text[], _edata[], _etext[];
+
unsigned long ia64_max_cacheline_size;
unsigned long ia64_iobase; /* virtual address for I/O accesses */
EXPORT_SYMBOL(ia64_iobase);
@@ -171,6 +188,20 @@ sort_regions (struct rsvd_region *rsvd_r
}
}
+/*
+ * Request address space for all standard resources
+ */
+static void __init register_memory(void)
+{
+ code_resource.start = ia64_tpa(_text);
+ code_resource.end = ia64_tpa(_etext) - 1;
+ data_resource.start = ia64_tpa(_etext);
+ data_resource.end = ia64_tpa(_edata) - 1;
+ efi_initialize_iomem_resources(&code_resource, &data_resource);
+}
+
+__initcall(register_memory);
+
/**
* reserve_memory - setup reserved memory areas
*
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: [PATCH] Updated /proc/iomem patch
2005-09-14 16:45 [PATCH] Updated /proc/iomem patch Khalid Aziz
` (4 preceding siblings ...)
2005-09-16 15:26 ` Khalid Aziz
@ 2005-09-16 21:02 ` Luck, Tony
2005-09-28 21:31 ` Khalid Aziz
6 siblings, 0 replies; 8+ messages in thread
From: Luck, Tony @ 2005-09-16 21:02 UTC (permalink / raw)
To: linux-ia64
[-- Attachment #1: Type: text/plain, Size: 1298 bytes --]
Just a couple of quibbles:
You still tested for md->num_pages == 0 with a comment saying
that this may be the result of trimming. With the clean-up
to efi_memmap_walk() we don't trim any more, so perhaps this
whole test can go? I've kept it with a toned down comment
(as paranoia against an implementation of EFI that hands us
a zero sized object).
What do you need from the "Kernel data" information? Right
now you use _edata as the end point ... which is the end of
initialized data from the kernel Elf file. But perhaps it
might be more sane to include all the other sections that
follow ... and use "_end" as the endpoint (actually _end
rounded up to a page boundary) ... which neatly fill in the
block of "System RAM" that the kernel is a sub-resource of
without leaving some nebulous hole at the end. I haven't
changed this yet ... pending your reply on what is needed.
And some minor nits (all of which I've fixed)
use kcalloc() rather than kmalloc+memset
delay calling the allocator until you know you need it
register_memory() throws a warning from the __initcall,
it needs to be an "int" returning function, not "void".
stylistically I don't like "continue; break;"
s/printk("ERROR/printk(KERN_ERR "/
trailing white space deleted
-Tony
[-- Attachment #2: newiomem.patch --]
[-- Type: application/octet-stream, Size: 3703 bytes --]
diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
--- a/arch/ia64/kernel/efi.c
+++ b/arch/ia64/kernel/efi.c
@@ -923,3 +923,90 @@ efi_memmap_init(unsigned long *s, unsign
*s = (u64)kern_memmap;
*e = (u64)++k;
}
+
+void
+efi_initialize_iomem_resources(struct resource *code_resource,
+ struct resource *data_resource)
+{
+ struct resource *res;
+ void *efi_map_start, *efi_map_end, *p;
+ efi_memory_desc_t *md;
+ u64 efi_desc_size;
+ char *name;
+ unsigned long flags;
+
+ efi_map_start = __va(ia64_boot_param->efi_memmap);
+ efi_map_end = efi_map_start + ia64_boot_param->efi_memmap_size;
+ efi_desc_size = ia64_boot_param->efi_memdesc_size;
+
+ res = NULL;
+
+ for (p = efi_map_start; p < efi_map_end; p += efi_desc_size) {
+ md = p;
+
+ if (md->num_pages == 0) /* should not happen */
+ continue;
+
+ flags = IORESOURCE_MEM;
+ switch (md->type) {
+
+ case EFI_MEMORY_MAPPED_IO:
+ case EFI_MEMORY_MAPPED_IO_PORT_SPACE:
+ continue;
+
+ case EFI_LOADER_CODE:
+ case EFI_LOADER_DATA:
+ case EFI_BOOT_SERVICES_DATA:
+ case EFI_BOOT_SERVICES_CODE:
+ case EFI_CONVENTIONAL_MEMORY:
+ if (md->attribute & EFI_MEMORY_WP) {
+ name = "System ROM";
+ flags |= IORESOURCE_READONLY;
+ } else {
+ name = "System RAM";
+ }
+ break;
+
+ case EFI_ACPI_MEMORY_NVS:
+ name = "ACPI Non-volatile Storage";
+ flags |= IORESOURCE_BUSY;
+ break;
+
+ case EFI_UNUSABLE_MEMORY:
+ name = "reserved";
+ flags |= IORESOURCE_BUSY | IORESOURCE_DISABLED;
+ break;
+
+ case EFI_RESERVED_TYPE:
+ case EFI_RUNTIME_SERVICES_CODE:
+ case EFI_RUNTIME_SERVICES_DATA:
+ case EFI_ACPI_RECLAIM_MEMORY:
+ default:
+ name = "reserved";
+ flags |= IORESOURCE_BUSY;
+ break;
+ }
+
+ if ((res = kcalloc(1, sizeof(struct resource), GFP_KERNEL)) == NULL) {
+ printk(KERN_ERR "failed to alocate resource for iomem\n");
+ return;
+ }
+
+ res->name = name;
+ res->start = md->phys_addr;
+ res->end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1;
+ res->flags = flags;
+
+ if (insert_resource(&iomem_resource, res) < 0)
+ kfree(res);
+ else {
+ /*
+ * We don't know which region contains
+ * kernel data so we try it repeatedly and
+ * let the resource manager test it.
+ */
+ insert_resource(res, code_resource);
+ insert_resource(res, data_resource);
+ }
+ }
+}
diff --git a/arch/ia64/kernel/setup.c b/arch/ia64/kernel/setup.c
--- a/arch/ia64/kernel/setup.c
+++ b/arch/ia64/kernel/setup.c
@@ -78,6 +78,23 @@ struct screen_info screen_info;
unsigned long vga_console_iobase;
unsigned long vga_console_membase;
+static struct resource data_resource = {
+ .name = "Kernel data",
+ .start = 0,
+ .end = 0,
+ .flags = IORESOURCE_BUSY | IORESOURCE_MEM
+};
+
+static struct resource code_resource = {
+ .name = "Kernel code",
+ .start = 0,
+ .end = 0,
+ .flags = IORESOURCE_BUSY | IORESOURCE_MEM
+};
+extern void efi_initialize_iomem_resources(struct resource *,
+ struct resource *);
+extern char _text[], _edata[], _etext[];
+
unsigned long ia64_max_cacheline_size;
unsigned long ia64_iobase; /* virtual address for I/O accesses */
EXPORT_SYMBOL(ia64_iobase);
@@ -171,6 +188,22 @@ sort_regions (struct rsvd_region *rsvd_r
}
}
+/*
+ * Request address space for all standard resources
+ */
+static int __init register_memory(void)
+{
+ code_resource.start = ia64_tpa(_text);
+ code_resource.end = ia64_tpa(_etext) - 1;
+ data_resource.start = ia64_tpa(_etext);
+ data_resource.end = ia64_tpa(_edata) - 1;
+ efi_initialize_iomem_resources(&code_resource, &data_resource);
+
+ return 0;
+}
+
+__initcall(register_memory);
+
/**
* reserve_memory - setup reserved memory areas
*
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: [PATCH] Updated /proc/iomem patch
2005-09-14 16:45 [PATCH] Updated /proc/iomem patch Khalid Aziz
` (5 preceding siblings ...)
2005-09-16 21:02 ` Luck, Tony
@ 2005-09-28 21:31 ` Khalid Aziz
6 siblings, 0 replies; 8+ messages in thread
From: Khalid Aziz @ 2005-09-28 21:31 UTC (permalink / raw)
To: linux-ia64
Sorry Tony. Your email got buried and I just saw it. My comments below.
On Fri, 2005-09-16 at 14:02 -0700, Luck, Tony wrote:
> Just a couple of quibbles:
>
> You still tested for md->num_pages = 0 with a comment saying
> that this may be the result of trimming. With the clean-up
> to efi_memmap_walk() we don't trim any more, so perhaps this
> whole test can go? I've kept it with a toned down comment
> (as paranoia against an implementation of EFI that hands us
> a zero sized object).
This was just a paranoidal check.
>
> What do you need from the "Kernel data" information? Right
> now you use _edata as the end point ... which is the end of
> initialized data from the kernel Elf file. But perhaps it
> might be more sane to include all the other sections that
> follow ... and use "_end" as the endpoint (actually _end
> rounded up to a page boundary) ... which neatly fill in the
> block of "System RAM" that the kernel is a sub-resource of
> without leaving some nebulous hole at the end. I haven't
> changed this yet ... pending your reply on what is needed.
>
Using _end as endpoint sounds reasonable. Feel free to change it if you
have not done so already.
> And some minor nits (all of which I've fixed)
>
> use kcalloc() rather than kmalloc+memset
>
> delay calling the allocator until you know you need it
>
> register_memory() throws a warning from the __initcall,
> it needs to be an "int" returning function, not "void".
>
> stylistically I don't like "continue; break;"
>
> s/printk("ERROR/printk(KERN_ERR "/
>
> trailing white space deleted
>
> -Tony
>
--
Khalid
==================================
Khalid Aziz Open Source and Linux Organization
(970)898-9214 Hewlett-Packard
khalid.aziz@hp.com Fort Collins, CO
"The Linux kernel is subject to relentless development"
- Alessandro Rubini
^ permalink raw reply [flat|nested] 8+ messages in thread