| Message ID | 1451081365-15190-14-git-send-email-toshi.kani@hpe.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Dec 25, 2015 at 03:09:23PM -0700, Toshi Kani wrote: > Change to call walk_iomem_res_desc() for searching resource entries > with the following names: > "ACPI Tables" > "ACPI Non-volatile Storage" > "Persistent Memory (legacy)" > "Crash kernel" > > Note, the caller of walk_iomem_res() with "GART" is left unchanged > because this entry may be initialized by out-of-tree drivers, which > do not have 'desc' set to IORES_DESC_GART. There's this out-of-tree bogus argument again. :\ Why do we care about out-of-tree drivers? You can just as well fix the "GART" case too and kill walk_iomem_res() altogether...
Ccing kexec maillist. On 12/25/15 at 03:09pm, Toshi Kani wrote: > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c > index c245085..e2bd737 100644 > --- a/kernel/kexec_file.c > +++ b/kernel/kexec_file.c > @@ -522,10 +522,10 @@ int kexec_add_buffer(struct kimage *image, char *buffer, unsigned long bufsz, > > /* Walk the RAM ranges and allocate a suitable range for the buffer */ > if (image->type == KEXEC_TYPE_CRASH) > - ret = walk_iomem_res("Crash kernel", > - IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY, > - crashk_res.start, crashk_res.end, kbuf, > - locate_mem_hole_callback); > + ret = walk_iomem_res_desc(IORES_DESC_CRASH_KERNEL, Since crashk_res's desc has been assigned to IORES_DESC_CRASH_KERNEL, it is better to use crashk_res.desc, instead of using IORES_DESC_CRASH_KERNEL directly. Thanks Minfei > + IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY, > + crashk_res.start, crashk_res.end, kbuf, > + locate_mem_hole_callback); > else > ret = walk_system_ram_res(0, -1, kbuf, > locate_mem_hole_callback);
+ cc: kexec list On 12/26/2015 3:38 AM, Borislav Petkov wrote: > On Fri, Dec 25, 2015 at 03:09:23PM -0700, Toshi Kani wrote: >> Change to call walk_iomem_res_desc() for searching resource entries >> with the following names: >> "ACPI Tables" >> "ACPI Non-volatile Storage" >> "Persistent Memory (legacy)" >> "Crash kernel" >> >> Note, the caller of walk_iomem_res() with "GART" is left unchanged >> because this entry may be initialized by out-of-tree drivers, which >> do not have 'desc' set to IORES_DESC_GART. > > There's this out-of-tree bogus argument again. :\ > > Why do we care about out-of-tree drivers? > > You can just as well fix the "GART" case too and kill walk_iomem_res() > altogether... Right, but I do not see any "GART" case in the upstream code, so I cannot change it... Thanks, -Toshi
On 12/26/2015 9:05 AM, Minfei Huang wrote: > Ccing kexec maillist. > > On 12/25/15 at 03:09pm, Toshi Kani wrote: >> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c >> index c245085..e2bd737 100644 >> --- a/kernel/kexec_file.c >> +++ b/kernel/kexec_file.c >> @@ -522,10 +522,10 @@ int kexec_add_buffer(struct kimage *image, char *buffer, unsigned long bufsz, >> >> /* Walk the RAM ranges and allocate a suitable range for the buffer */ >> if (image->type == KEXEC_TYPE_CRASH) >> - ret = walk_iomem_res("Crash kernel", >> - IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY, >> - crashk_res.start, crashk_res.end, kbuf, >> - locate_mem_hole_callback); >> + ret = walk_iomem_res_desc(IORES_DESC_CRASH_KERNEL, > > Since crashk_res's desc has been assigned to IORES_DESC_CRASH_KERNEL, it > is better to use crashk_res.desc, instead of using > IORES_DESC_CRASH_KERNEL directly. Sounds good. I will change it to use crashk_res.desc. Thanks, -Toshi
On 12/26/15 at 05:31pm, Toshi Kani wrote: > + cc: kexec list > > On 12/26/2015 3:38 AM, Borislav Petkov wrote: > >On Fri, Dec 25, 2015 at 03:09:23PM -0700, Toshi Kani wrote: > >>Change to call walk_iomem_res_desc() for searching resource entries > >>with the following names: > >> "ACPI Tables" > >> "ACPI Non-volatile Storage" > >> "Persistent Memory (legacy)" > >> "Crash kernel" > >> > >>Note, the caller of walk_iomem_res() with "GART" is left unchanged > >>because this entry may be initialized by out-of-tree drivers, which > >>do not have 'desc' set to IORES_DESC_GART. > > > >There's this out-of-tree bogus argument again. :\ > > > >Why do we care about out-of-tree drivers? > > > >You can just as well fix the "GART" case too and kill walk_iomem_res() > >altogether... > > Right, but I do not see any "GART" case in the upstream code, so I > cannot change it... Hi, Toshi. You can refer the below link that you may get a clue about GART. This is the fisrt time kexec-tools tried to support to ignore GART region in 2nd kernel. http://lists.infradead.org/pipermail/kexec/2008-December/003096.html Thanks Minfei
On Sun, Dec 27, 2015 at 10:12:57AM +0800, Minfei Huang wrote: > You can refer the below link that you may get a clue about GART. This is > the fisrt time kexec-tools tried to support to ignore GART region in 2nd > kernel. > > http://lists.infradead.org/pipermail/kexec/2008-December/003096.html So theoretically we could export that IORES_DESC* enum in an uapi header and move kexec-tools to use that. However, I'm fuzzy on how exactly the whole compatibility thing is done with kexec-tools and the kernel. We probably would have to support newer kexec-tools on an older kernel and vice versa so I'd guess we should mark walk_iomem_res() deprecated so that people are encouraged to upgrade to newer kexec-tools... Hmmm.
Hi, Toshi, On 12/25/15 at 03:09pm, Toshi Kani wrote: > Change to call walk_iomem_res_desc() for searching resource entries > with the following names: > "ACPI Tables" > "ACPI Non-volatile Storage" > "Persistent Memory (legacy)" > "Crash kernel" > > Note, the caller of walk_iomem_res() with "GART" is left unchanged > because this entry may be initialized by out-of-tree drivers, which > do not have 'desc' set to IORES_DESC_GART. Found below commit which initialize the GART entry: commit 56dd669a138c40ea6cdae487f233430d12372767 Author: Aaron Durbin <adurbin@google.com> Date: Tue Sep 26 10:52:40 2006 +0200 [PATCH] Insert GART region into resource map Patch inserts the GART region into the iomem resource map. The GART will then be visible within /proc/iomem. It will also allow for other users utilizing the GART to subreserve the region (agp or IOMMU). Signed-off-by: Aaron Durbin <adurbin@google.com> But later it was reverted: commit 707d4eefbdb31f8e588277157056b0ce637d6c68 Author: Bjorn Helgaas <bhelgaas@google.com> Date: Tue Mar 18 14:26:12 2014 -0600 Revert "[PATCH] Insert GART region into resource map" This reverts commit 56dd669a138c, which makes the GART visible in /proc/iomem. This fixes a regression: e501b3d87f00 ("agp: Support 64-bit APBASE") exposed an existing problem with a conflict between the GART region and a PCI BAR region. The GART addresses are bus addresses, not CPU addresses, and therefore should not be inserted in iomem_resource. On many machines, the GART region is addressable by the CPU as well as by an AGP master, but CPU addressability is not required by the spec. On some of these machines, the GART is mapped by a PCI BAR, and in that case, the PCI core automatically inserts it into iomem_resource, just as it does for all BARs. Inserting it here means we'll have a conflict if the PCI core later tries to claim the GART region, so let's drop the insertion here. The conflict indirectly causes X failures, as reported by Jouni in the bugzilla below. We detected the conflict even before e501b3d87f00, but after it the AGP code (fix_northbridge()) uses the PCI resource (which is zeroed because of the conflict) instead of reading the BAR again. Conflicts: arch/x86_64/kernel/aperture.c Fixes: e501b3d87f00 agp: Support 64-bit APBASE Link: https://bugzilla.kernel.org/show_bug.cgi?id=72201 Reported-and-tested-by: Jouni Mettälä <jtmettala@gmail.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> For amd64 agp, currently the region name is "aperture" instead: drivers/char/agp/amd64-agp.c: agp_aperture_valid() This may not be the only case, but I doubt that anyone is testing this since long time ago kexec-tools excluding the 'GART' region. Kexec-tools and kexec_file may need update to use "aperture" if someone can test it. I think adding an enum value for compatibility is reasonable, we do not care about third party drivers in mainline. Thanks Dave
Hi, Boris On 12/27/15 at 11:24am, Borislav Petkov wrote: > On Sun, Dec 27, 2015 at 10:12:57AM +0800, Minfei Huang wrote: > > You can refer the below link that you may get a clue about GART. This is > > the fisrt time kexec-tools tried to support to ignore GART region in 2nd > > kernel. > > > > http://lists.infradead.org/pipermail/kexec/2008-December/003096.html > > So theoretically we could export that IORES_DESC* enum in an uapi header and > move kexec-tools to use that. > > However, I'm fuzzy on how exactly the whole compatibility thing is done > with kexec-tools and the kernel. We probably would have to support > newer kexec-tools on an older kernel and vice versa so I'd guess we > should mark walk_iomem_res() deprecated so that people are encouraged to > upgrade to newer kexec-tools... Replied to Toshi old kernel will export the "GART" region for amd cards. So for old kernel and new kexec-tools we will have problem. I think add the GART desc for compitibility purpose is doable, no? Thanks Dave
On 01/04/16 at 05:25pm, Dave Young wrote: > Hi, Toshi, > > On 12/25/15 at 03:09pm, Toshi Kani wrote: > > Change to call walk_iomem_res_desc() for searching resource entries > > with the following names: > > "ACPI Tables" > > "ACPI Non-volatile Storage" > > "Persistent Memory (legacy)" > > "Crash kernel" > > > > Note, the caller of walk_iomem_res() with "GART" is left unchanged > > because this entry may be initialized by out-of-tree drivers, which > > do not have 'desc' set to IORES_DESC_GART. > > Found below commit which initialize the GART entry: > commit 56dd669a138c40ea6cdae487f233430d12372767 > Author: Aaron Durbin <adurbin@google.com> > Date: Tue Sep 26 10:52:40 2006 +0200 > > [PATCH] Insert GART region into resource map > > Patch inserts the GART region into the iomem resource map. The GART will then > be visible within /proc/iomem. It will also allow for other users > utilizing the GART to subreserve the region (agp or IOMMU). > > Signed-off-by: Aaron Durbin <adurbin@google.com> > > But later it was reverted: > commit 707d4eefbdb31f8e588277157056b0ce637d6c68 > Author: Bjorn Helgaas <bhelgaas@google.com> > Date: Tue Mar 18 14:26:12 2014 -0600 > > Revert "[PATCH] Insert GART region into resource map" > > This reverts commit 56dd669a138c, which makes the GART visible in > /proc/iomem. This fixes a regression: e501b3d87f00 ("agp: Support 64-bit > APBASE") exposed an existing problem with a conflict between the GART > region and a PCI BAR region. > > The GART addresses are bus addresses, not CPU addresses, and therefore > should not be inserted in iomem_resource. > > On many machines, the GART region is addressable by the CPU as well as by > an AGP master, but CPU addressability is not required by the spec. On some > of these machines, the GART is mapped by a PCI BAR, and in that case, the > PCI core automatically inserts it into iomem_resource, just as it does for > all BARs. > > Inserting it here means we'll have a conflict if the PCI core later tries > to claim the GART region, so let's drop the insertion here. > > The conflict indirectly causes X failures, as reported by Jouni in the > bugzilla below. We detected the conflict even before e501b3d87f00, but > after it the AGP code (fix_northbridge()) uses the PCI resource (which is > zeroed because of the conflict) instead of reading the BAR again. > > Conflicts: > arch/x86_64/kernel/aperture.c > > Fixes: e501b3d87f00 agp: Support 64-bit APBASE > Link: https://bugzilla.kernel.org/show_bug.cgi?id=72201 > Reported-and-tested-by: Jouni Mettälä <jtmettala@gmail.com> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > > For amd64 agp, currently the region name is "aperture" instead: > drivers/char/agp/amd64-agp.c: agp_aperture_valid() > > This may not be the only case, but I doubt that anyone is testing this since > long time ago kexec-tools excluding the 'GART' region. Kexec-tools and kexec_file > may need update to use "aperture" if someone can test it. > > I think adding an enum value for compatibility is reasonable, we do not care > about third party drivers in mainline. Hmm, rethink about it, the new kernel will not export "GART" regions thus the in-kernel loader should never see such regions as long as there's no other drivers exporting it. Thus dropping the code chunk in crash.c about 'GART' should be fine. Thanks Dave
On Mon, Jan 04, 2016 at 05:29:37PM +0800, Dave Young wrote: > Replied to Toshi old kernel will export the "GART" region for amd cards. > So for old kernel and new kexec-tools we will have problem. > > I think add the GART desc for compitibility purpose is doable, no? Just read your other mails too. If I see it correctly, there's only one place which has "GART": $ git grep -e \"GART\" arch/x86/kernel/crash.c:235: walk_iomem_res("GART", IORESOURCE_MEM, 0, -1, So crash.c only excludes this region but the kernel doesn't create it. Right? So we can kill that walk_iomem_res(), as you say. Which would be even nicer...
On Mon, 2016-01-04 at 13:26 +0100, Borislav Petkov wrote: > On Mon, Jan 04, 2016 at 05:29:37PM +0800, Dave Young wrote: > > Replied to Toshi old kernel will export the "GART" region for amd > > cards. > > So for old kernel and new kexec-tools we will have problem. > > > > I think add the GART desc for compitibility purpose is doable, no? > > Just read your other mails too. If I see it correctly, there's only one > place which has "GART": > > $ git grep -e \"GART\" > arch/x86/kernel/crash.c:235: walk_iomem_res("GART", IORESOURCE_MEM, 0, > -1, > > So crash.c only excludes this region but the kernel doesn't create it. > Right? > > So we can kill that walk_iomem_res(), as you say. Which would be even > nicer... Agreed. As Dave suggested in the other thread, we can simply remove the walk_iomem_res("GART",) call in crash.c. With this change, there will be no caller to walk_iomem_res(). Should we remove walk_iomem_res() altogether, or keep it for now as a deprecated func with the checkpatch check? Thanks, -Toshi
On Mon, Jan 04, 2016 at 10:57:40AM -0700, Toshi Kani wrote: > With this change, there will be no caller to walk_iomem_res(). Should we > remove walk_iomem_res() altogether, or keep it for now as a deprecated func > with the checkpatch check? Yes, kill it on the spot so that people don't get crazy ideas. Thanks!
On Mon, 2016-01-04 at 20:41 +0100, Borislav Petkov wrote: > On Mon, Jan 04, 2016 at 10:57:40AM -0700, Toshi Kani wrote: > > With this change, there will be no caller to walk_iomem_res(). Should > > we remove walk_iomem_res() altogether, or keep it for now as a > > deprecated func with the checkpatch check? > > Yes, kill it on the spot so that people don't get crazy ideas. Will do. Thanks! -Toshi
On 01/04/16 at 01:26pm, Borislav Petkov wrote: > On Mon, Jan 04, 2016 at 05:29:37PM +0800, Dave Young wrote: > > Replied to Toshi old kernel will export the "GART" region for amd cards. > > So for old kernel and new kexec-tools we will have problem. > > > > I think add the GART desc for compitibility purpose is doable, no? > > Just read your other mails too. If I see it correctly, there's only one > place which has "GART": > > $ git grep -e \"GART\" > arch/x86/kernel/crash.c:235: walk_iomem_res("GART", IORESOURCE_MEM, 0, -1, > > So crash.c only excludes this region but the kernel doesn't create it. > Right? Right. > > So we can kill that walk_iomem_res(), as you say. Which would be even > nicer... Yes, I think it is ok to kill walk_iomem_res() Thanks Dave
diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c index 2c1910f..082373b 100644 --- a/arch/x86/kernel/crash.c +++ b/arch/x86/kernel/crash.c @@ -588,12 +588,12 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params) /* Add ACPI tables */ cmd.type = E820_ACPI; flags = IORESOURCE_MEM | IORESOURCE_BUSY; - walk_iomem_res("ACPI Tables", flags, 0, -1, &cmd, + walk_iomem_res_desc(IORES_DESC_ACPI_TABLES, flags, 0, -1, &cmd, memmap_entry_callback); /* Add ACPI Non-volatile Storage */ cmd.type = E820_NVS; - walk_iomem_res("ACPI Non-volatile Storage", flags, 0, -1, &cmd, + walk_iomem_res_desc(IORES_DESC_ACPI_NV_STORAGE, flags, 0, -1, &cmd, memmap_entry_callback); /* Add crashk_low_res region */ diff --git a/arch/x86/kernel/pmem.c b/arch/x86/kernel/pmem.c index 14415af..92f7014 100644 --- a/arch/x86/kernel/pmem.c +++ b/arch/x86/kernel/pmem.c @@ -13,11 +13,11 @@ static int found(u64 start, u64 end, void *data) static __init int register_e820_pmem(void) { - char *pmem = "Persistent Memory (legacy)"; struct platform_device *pdev; int rc; - rc = walk_iomem_res(pmem, IORESOURCE_MEM, 0, -1, NULL, found); + rc = walk_iomem_res_desc(IORES_DESC_PERSISTENT_MEMORY_LEGACY, + IORESOURCE_MEM, 0, -1, NULL, found); if (rc <= 0) return 0; diff --git a/drivers/nvdimm/e820.c b/drivers/nvdimm/e820.c index b0045a5..95825b3 100644 --- a/drivers/nvdimm/e820.c +++ b/drivers/nvdimm/e820.c @@ -55,7 +55,7 @@ static int e820_pmem_probe(struct platform_device *pdev) for (p = iomem_resource.child; p ; p = p->sibling) { struct nd_region_desc ndr_desc; - if (strncmp(p->name, "Persistent Memory (legacy)", 26) != 0) + if (p->desc != IORES_DESC_PERSISTENT_MEMORY_LEGACY) continue; memset(&ndr_desc, 0, sizeof(ndr_desc)); diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c index c245085..e2bd737 100644 --- a/kernel/kexec_file.c +++ b/kernel/kexec_file.c @@ -522,10 +522,10 @@ int kexec_add_buffer(struct kimage *image, char *buffer, unsigned long bufsz, /* Walk the RAM ranges and allocate a suitable range for the buffer */ if (image->type == KEXEC_TYPE_CRASH) - ret = walk_iomem_res("Crash kernel", - IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY, - crashk_res.start, crashk_res.end, kbuf, - locate_mem_hole_callback); + ret = walk_iomem_res_desc(IORES_DESC_CRASH_KERNEL, + IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY, + crashk_res.start, crashk_res.end, kbuf, + locate_mem_hole_callback); else ret = walk_system_ram_res(0, -1, kbuf, locate_mem_hole_callback);
Change to call walk_iomem_res_desc() for searching resource entries with the following names: "ACPI Tables" "ACPI Non-volatile Storage" "Persistent Memory (legacy)" "Crash kernel" Note, the caller of walk_iomem_res() with "GART" is left unchanged because this entry may be initialized by out-of-tree drivers, which do not have 'desc' set to IORES_DESC_GART. Cc: Borislav Petkov <bp@alien8.de> Cc: Dan Williams <dan.j.williams@intel.com> Cc: Dave Young <dyoung@redhat.com> Cc: x86@kernel.org Cc: linux-nvdimm@lists.01.org Signed-off-by: Toshi Kani <toshi.kani@hpe.com> --- arch/x86/kernel/crash.c | 4 ++-- arch/x86/kernel/pmem.c | 4 ++-- drivers/nvdimm/e820.c | 2 +- kernel/kexec_file.c | 8 ++++---- 4 files changed, 9 insertions(+), 9 deletions(-)