* hpwdt: clean up set_memory_x call for 32 bit
@ 2012-01-16 4:02 Maxim Uvarov
2012-01-16 4:02 ` [PATCH] " Maxim Uvarov
0 siblings, 1 reply; 10+ messages in thread
From: Maxim Uvarov @ 2012-01-16 4:02 UTC (permalink / raw)
To: linux-kernel; +Cc: akpm, wim, torvalds, stable, Thomas.Mingarelli
Hello,
Please find clean up patch for hpwdt.
There were 2 patches for the issue. Both do the same
but in different ways:
commit 0305d4e376508720be61a6500ded9b9390c9a136
Author: Maxim Uvarov <maxim.uvarov@oracle.com>
Date: Thu Dec 22 16:03:34 2011 +1100
drivers/watchdog/hpwdt.c: mark page executable
commit e67d668e147c3b4fec638c9e0ace04319f5ceccd
Author: Mingarelli, Thomas <Thomas.Mingarelli@hp.com>
Date: Mon Nov 7 10:59:00 2011 +0100
watchdog: hpwdt: Changes to handle NX secure bit in 32bit path
We decided to drop my patch(0305d4e37) and add author's (e67d668e1).
Both patches do the same things. But we missed that e67d668e1 calls
set_memory_x with wrong parameters. Right parameters are address aligned to
page and number of pages. Not address and size. First parameter with be auto-aligned
and set_memory_x() print WARN, but second parameter is serious error.
We need to think how to avoid changing NX for not our pages in future and add additional
warning or call panic() in set_memory_x(). Maybe try to read the latest byte or all bytes
for that memory.
Best regards,
Maxim Uvarov.
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH] hpwdt: clean up set_memory_x call for 32 bit 2012-01-16 4:02 hpwdt: clean up set_memory_x call for 32 bit Maxim Uvarov @ 2012-01-16 4:02 ` Maxim Uvarov 2012-01-24 20:20 ` Linus Torvalds 0 siblings, 1 reply; 10+ messages in thread From: Maxim Uvarov @ 2012-01-16 4:02 UTC (permalink / raw) To: linux-kernel; +Cc: akpm, wim, torvalds, stable, Thomas.Mingarelli, Maxim Uvarov 1. addess has to be page aligned. 2. set_memory_x uses page size argument, not size. Bug causes with following commit: commit da28179b4e90dda56912ee825c7eaa62fc103797 Author: Mingarelli, Thomas <Thomas.Mingarelli@hp.com> Date: Mon Nov 7 10:59:00 2011 +0100 watchdog: hpwdt: Changes to handle NX secure bit in 32bit path commit e67d668e147c3b4fec638c9e0ace04319f5ceccd upstream. This patch makes use of the set_memory_x() kernel API in order to make necessary BIOS calls to source NMIs. Signed-off-by: Maxim Uvarov <maxim.uvarov@oracle.com> --- drivers/watchdog/hpwdt.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c index 9cb60df..7f04d37 100644 --- a/drivers/watchdog/hpwdt.c +++ b/drivers/watchdog/hpwdt.c @@ -216,7 +216,7 @@ static int __devinit cru_detect(unsigned long map_entry, cmn_regs.u1.reax = CRU_BIOS_SIGNATURE_VALUE; - set_memory_x((unsigned long)bios32_entrypoint, (2 * PAGE_SIZE)); + set_memory_x((unsigned long)bios32_entrypoint & PAGE_MASK, 2); asminline_call(&cmn_regs, bios32_entrypoint); if (cmn_regs.u1.ral != 0) { @@ -235,7 +235,7 @@ static int __devinit cru_detect(unsigned long map_entry, cru_rom_addr = ioremap(cru_physical_address, cru_length); if (cru_rom_addr) { - set_memory_x((unsigned long)cru_rom_addr, cru_length); + set_memory_x((unsigned long)cru_rom_addr & PAGE_MASK, cru_length >> PAGE_SHIFT); retval = 0; } } -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] hpwdt: clean up set_memory_x call for 32 bit 2012-01-16 4:02 ` [PATCH] " Maxim Uvarov @ 2012-01-24 20:20 ` Linus Torvalds 2012-01-24 20:37 ` Wim Van Sebroeck 2012-01-27 16:49 ` Wim Van Sebroeck 0 siblings, 2 replies; 10+ messages in thread From: Linus Torvalds @ 2012-01-24 20:20 UTC (permalink / raw) To: Maxim Uvarov; +Cc: linux-kernel, akpm, wim, stable, Thomas.Mingarelli So I don't know who is supposed to be handling this (Wim?), but the patch itself looks suspicious. On Sun, Jan 15, 2012 at 8:02 PM, Maxim Uvarov <maxim.uvarov@oracle.com> wrote: > - set_memory_x((unsigned long)bios32_entrypoint, (2 * PAGE_SIZE)); > + set_memory_x((unsigned long)bios32_entrypoint & PAGE_MASK, 2); If it wasn't page-aligned to begin with, then maybe it needs three pages now? > - set_memory_x((unsigned long)cru_rom_addr, cru_length); > + set_memory_x((unsigned long)cru_rom_addr & PAGE_MASK, cru_length >> PAGE_SHIFT); Same here. If we align the start address down, we should fix up the length. And should we not align the number of pages up? In general, a "start/length" conversion to a "page/nr" model needs to be roughly len += start & ~PAGE_MASK; start &= PAGE_MASK; nr_pages = (len + PAGE_SIZE - 1) >> PAGE_SHIFT; to do things right. But I don't know where those magic numbers come from. Maybe the "2" is already due to the code possibly traversing a page boundary, and has already been fixed up. Somebody who knows the driver and the requirements should take a look at this. Linus ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hpwdt: clean up set_memory_x call for 32 bit 2012-01-24 20:20 ` Linus Torvalds @ 2012-01-24 20:37 ` Wim Van Sebroeck 2012-01-24 21:05 ` Mingarelli, Thomas 2012-01-27 16:49 ` Wim Van Sebroeck 1 sibling, 1 reply; 10+ messages in thread From: Wim Van Sebroeck @ 2012-01-24 20:37 UTC (permalink / raw) To: Linus Torvalds Cc: Maxim Uvarov, linux-kernel, akpm, stable, Thomas.Mingarelli, dann frazier Hi Linus, > So I don't know who is supposed to be handling this (Wim?), but the > patch itself looks suspicious. I asked Tom to look at Maxim's patch and see what it does. Tom was going to look at the patch and I'm waiting on feedback from him first. (That's why I din't sent it upstream yet). > On Sun, Jan 15, 2012 at 8:02 PM, Maxim Uvarov <maxim.uvarov@oracle.com> wrote: > > - set_memory_x((unsigned long)bios32_entrypoint, (2 * PAGE_SIZE)); > > + set_memory_x((unsigned long)bios32_entrypoint & PAGE_MASK, 2); > > If it wasn't page-aligned to begin with, then maybe it needs three pages now? > > > - set_memory_x((unsigned long)cru_rom_addr, cru_length); > > + set_memory_x((unsigned long)cru_rom_addr & PAGE_MASK, cru_length >> PAGE_SHIFT); > > Same here. If we align the start address down, we should fix up the > length. And should we not align the number of pages up? > > In general, a "start/length" conversion to a "page/nr" model needs to be roughly > > len += start & ~PAGE_MASK; > start &= PAGE_MASK; > nr_pages = (len + PAGE_SIZE - 1) >> PAGE_SHIFT; > > to do things right. But I don't know where those magic numbers come > from. Maybe the "2" is already due to the code possibly traversing a > page boundary, and has already been fixed up. Somebody who knows the > driver and the requirements should take a look at this. Valid comments indeed. Tom please take Linus comments with you when you look at the patch. Dan: I put you in Cc: also so that you can have a look at it also. Kind regards, Wim. ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] hpwdt: clean up set_memory_x call for 32 bit 2012-01-24 20:37 ` Wim Van Sebroeck @ 2012-01-24 21:05 ` Mingarelli, Thomas 2012-01-25 23:21 ` Maxim Uvarov 0 siblings, 1 reply; 10+ messages in thread From: Mingarelli, Thomas @ 2012-01-24 21:05 UTC (permalink / raw) To: Wim Van Sebroeck, Linus Torvalds Cc: Maxim Uvarov, linux-kernel, akpm, stable, dann frazier Yes I agree that Maxim's patch is correct. The original set_memory_x call for 64 bit was done correctly and the newer calls are wrong. The 2 pages for the BIOS SD is a known value so it should be safe to use as is. Tom -----Original Message----- From: Wim Van Sebroeck [mailto:wim@iguana.be] Sent: Tuesday, January 24, 2012 2:38 PM To: Linus Torvalds Cc: Maxim Uvarov; linux-kernel@vger.kernel.org; akpm@linux-foundation.org; stable@vger.kernel.org; Mingarelli, Thomas; dann frazier Subject: Re: [PATCH] hpwdt: clean up set_memory_x call for 32 bit Hi Linus, > So I don't know who is supposed to be handling this (Wim?), but the > patch itself looks suspicious. I asked Tom to look at Maxim's patch and see what it does. Tom was going to look at the patch and I'm waiting on feedback from him first. (That's why I din't sent it upstream yet). > On Sun, Jan 15, 2012 at 8:02 PM, Maxim Uvarov <maxim.uvarov@oracle.com> wrote: > > - set_memory_x((unsigned long)bios32_entrypoint, (2 * PAGE_SIZE)); > > + set_memory_x((unsigned long)bios32_entrypoint & PAGE_MASK, 2); > > If it wasn't page-aligned to begin with, then maybe it needs three pages now? > > > - set_memory_x((unsigned long)cru_rom_addr, cru_length); > > + set_memory_x((unsigned long)cru_rom_addr & PAGE_MASK, cru_length >> PAGE_SHIFT); > > Same here. If we align the start address down, we should fix up the > length. And should we not align the number of pages up? > > In general, a "start/length" conversion to a "page/nr" model needs to be roughly > > len += start & ~PAGE_MASK; > start &= PAGE_MASK; > nr_pages = (len + PAGE_SIZE - 1) >> PAGE_SHIFT; > > to do things right. But I don't know where those magic numbers come > from. Maybe the "2" is already due to the code possibly traversing a > page boundary, and has already been fixed up. Somebody who knows the > driver and the requirements should take a look at this. Valid comments indeed. Tom please take Linus comments with you when you look at the patch. Dan: I put you in Cc: also so that you can have a look at it also. Kind regards, Wim. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hpwdt: clean up set_memory_x call for 32 bit 2012-01-24 21:05 ` Mingarelli, Thomas @ 2012-01-25 23:21 ` Maxim Uvarov 2012-01-26 0:01 ` Mingarelli, Thomas 0 siblings, 1 reply; 10+ messages in thread From: Maxim Uvarov @ 2012-01-25 23:21 UTC (permalink / raw) To: Mingarelli, Thomas Cc: Wim Van Sebroeck, Linus Torvalds, linux-kernel, akpm, stable, dann frazier Thomas, will you be able to test patch accoring to Linus's nr_page note? Maxim. On 01/24/2012 01:05 PM, Mingarelli, Thomas wrote: > Yes I agree that Maxim's patch is correct. The original set_memory_x call for 64 bit was done correctly and the newer calls are wrong. > > The 2 pages for the BIOS SD is a known value so it should be safe to use as is. > > > > Tom > -----Original Message----- > From: Wim Van Sebroeck [mailto:wim@iguana.be] > Sent: Tuesday, January 24, 2012 2:38 PM > To: Linus Torvalds > Cc: Maxim Uvarov; linux-kernel@vger.kernel.org; akpm@linux-foundation.org; stable@vger.kernel.org; Mingarelli, Thomas; dann frazier > Subject: Re: [PATCH] hpwdt: clean up set_memory_x call for 32 bit > > Hi Linus, > >> So I don't know who is supposed to be handling this (Wim?), but the >> patch itself looks suspicious. > > I asked Tom to look at Maxim's patch and see what it does. Tom was going to look at the patch and > I'm waiting on feedback from him first. (That's why I din't sent it upstream yet). > >> On Sun, Jan 15, 2012 at 8:02 PM, Maxim Uvarov<maxim.uvarov@oracle.com> wrote: >>> - set_memory_x((unsigned long)bios32_entrypoint, (2 * PAGE_SIZE)); >>> + set_memory_x((unsigned long)bios32_entrypoint& PAGE_MASK, 2); >> >> If it wasn't page-aligned to begin with, then maybe it needs three pages now? >> >>> - set_memory_x((unsigned long)cru_rom_addr, cru_length); >>> + set_memory_x((unsigned long)cru_rom_addr& PAGE_MASK, cru_length>> PAGE_SHIFT); >> >> Same here. If we align the start address down, we should fix up the >> length. And should we not align the number of pages up? >> >> In general, a "start/length" conversion to a "page/nr" model needs to be roughly >> >> len += start& ~PAGE_MASK; >> start&= PAGE_MASK; >> nr_pages = (len + PAGE_SIZE - 1)>> PAGE_SHIFT; >> >> to do things right. But I don't know where those magic numbers come >> from. Maybe the "2" is already due to the code possibly traversing a >> page boundary, and has already been fixed up. Somebody who knows the >> driver and the requirements should take a look at this. > > Valid comments indeed. Tom please take Linus comments with you when you look at the patch. > > Dan: I put you in Cc: also so that you can have a look at it also. > > Kind regards, > Wim. > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] hpwdt: clean up set_memory_x call for 32 bit 2012-01-25 23:21 ` Maxim Uvarov @ 2012-01-26 0:01 ` Mingarelli, Thomas 0 siblings, 0 replies; 10+ messages in thread From: Mingarelli, Thomas @ 2012-01-26 0:01 UTC (permalink / raw) To: Maxim Uvarov Cc: Wim Van Sebroeck, Linus Torvalds, linux-kernel, akpm, stable, dann frazier Yes I will do this before the end of the week. Tom -----Original Message----- From: Maxim Uvarov [mailto:maxim.uvarov@oracle.com] Sent: Wednesday, January 25, 2012 5:21 PM To: Mingarelli, Thomas Cc: Wim Van Sebroeck; Linus Torvalds; linux-kernel@vger.kernel.org; akpm@linux-foundation.org; stable@vger.kernel.org; dann frazier Subject: Re: [PATCH] hpwdt: clean up set_memory_x call for 32 bit Thomas, will you be able to test patch accoring to Linus's nr_page note? Maxim. On 01/24/2012 01:05 PM, Mingarelli, Thomas wrote: > Yes I agree that Maxim's patch is correct. The original set_memory_x call for 64 bit was done correctly and the newer calls are wrong. > > The 2 pages for the BIOS SD is a known value so it should be safe to use as is. > > > > Tom > -----Original Message----- > From: Wim Van Sebroeck [mailto:wim@iguana.be] > Sent: Tuesday, January 24, 2012 2:38 PM > To: Linus Torvalds > Cc: Maxim Uvarov; linux-kernel@vger.kernel.org; akpm@linux-foundation.org; stable@vger.kernel.org; Mingarelli, Thomas; dann frazier > Subject: Re: [PATCH] hpwdt: clean up set_memory_x call for 32 bit > > Hi Linus, > >> So I don't know who is supposed to be handling this (Wim?), but the >> patch itself looks suspicious. > > I asked Tom to look at Maxim's patch and see what it does. Tom was going to look at the patch and > I'm waiting on feedback from him first. (That's why I din't sent it upstream yet). > >> On Sun, Jan 15, 2012 at 8:02 PM, Maxim Uvarov<maxim.uvarov@oracle.com> wrote: >>> - set_memory_x((unsigned long)bios32_entrypoint, (2 * PAGE_SIZE)); >>> + set_memory_x((unsigned long)bios32_entrypoint& PAGE_MASK, 2); >> >> If it wasn't page-aligned to begin with, then maybe it needs three pages now? >> >>> - set_memory_x((unsigned long)cru_rom_addr, cru_length); >>> + set_memory_x((unsigned long)cru_rom_addr& PAGE_MASK, cru_length>> PAGE_SHIFT); >> >> Same here. If we align the start address down, we should fix up the >> length. And should we not align the number of pages up? >> >> In general, a "start/length" conversion to a "page/nr" model needs to be roughly >> >> len += start& ~PAGE_MASK; >> start&= PAGE_MASK; >> nr_pages = (len + PAGE_SIZE - 1)>> PAGE_SHIFT; >> >> to do things right. But I don't know where those magic numbers come >> from. Maybe the "2" is already due to the code possibly traversing a >> page boundary, and has already been fixed up. Somebody who knows the >> driver and the requirements should take a look at this. > > Valid comments indeed. Tom please take Linus comments with you when you look at the patch. > > Dan: I put you in Cc: also so that you can have a look at it also. > > Kind regards, > Wim. > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hpwdt: clean up set_memory_x call for 32 bit 2012-01-24 20:20 ` Linus Torvalds 2012-01-24 20:37 ` Wim Van Sebroeck @ 2012-01-27 16:49 ` Wim Van Sebroeck 2012-01-27 16:55 ` Mingarelli, Thomas 2012-01-27 18:33 ` Maxim Uvarov 1 sibling, 2 replies; 10+ messages in thread From: Wim Van Sebroeck @ 2012-01-27 16:49 UTC (permalink / raw) To: Thomas.Mingarelli, Linus Torvalds Cc: Maxim Uvarov, linux-kernel, akpm, stable Hi Tom, > So I don't know who is supposed to be handling this (Wim?), but the > patch itself looks suspicious. > > On Sun, Jan 15, 2012 at 8:02 PM, Maxim Uvarov <maxim.uvarov@oracle.com> wrote: > > - set_memory_x((unsigned long)bios32_entrypoint, (2 * PAGE_SIZE)); > > + set_memory_x((unsigned long)bios32_entrypoint & PAGE_MASK, 2); > > If it wasn't page-aligned to begin with, then maybe it needs three pages now? I have been looking at the code again and basically we have for 32 bit the following sequence: 1) scan/search from 0x0f0000 through 0x0fffff, inclusive (in steps of 16 bytes) until we find the 32-bit BIOS Service Directory with signature == PCI_BIOS32_SD_VALUE (=0x5F32335F ="_32_"). 2) If we find this area then we first do a checksum check to see if it's a valid area. 3) if it's a valid area then we will check this area for a $CRU record. the code for this is as follows: /* * According to the spec, we're looking for the * first 4KB-aligned address below the entrypoint * listed in the header. The Service Directory code * is guaranteed to occupy no more than 2 4KB pages. */ map_entry = bios_32_ptr->entry_point & ~(PAGE_SIZE - 1); map_offset = bios_32_ptr->entry_point - map_entry; bios32_map = ioremap(map_entry, (2 * PAGE_SIZE)); if (bios32_map == NULL) return -ENODEV; bios32_entrypoint = bios32_map + map_offset; cmn_regs.u1.reax = CRU_BIOS_SIGNATURE_VALUE; set_memory_x((unsigned long)bios32_entrypoint, (2 * PAGE_SIZE)); asminline_call(&cmn_regs, bios32_entrypoint); => So if I understand it correctly then map_entry is page aligned. And thus bios32_map is also page aligned. Wouldn't it then not make more sense to do a: set_memory_x((unsigned long)bios32_map, 2); > > - set_memory_x((unsigned long)cru_rom_addr, cru_length); > > + set_memory_x((unsigned long)cru_rom_addr & PAGE_MASK, cru_length >> PAGE_SHIFT); > > Same here. If we align the start address down, we should fix up the > length. And should we not align the number of pages up? > > In general, a "start/length" conversion to a "page/nr" model needs to be roughly > > len += start & ~PAGE_MASK; > start &= PAGE_MASK; > nr_pages = (len + PAGE_SIZE - 1) >> PAGE_SHIFT; > > to do things right. But I don't know where those magic numbers come > from. Maybe the "2" is already due to the code possibly traversing a > page boundary, and has already been fixed up. Somebody who knows the > driver and the requirements should take a look at this. 4) if we then found the $CRU record then we do: physical_bios_base = cmn_regs.u2.rebx; physical_bios_offset = cmn_regs.u4.redx; cru_length = cmn_regs.u3.recx; cru_physical_address = physical_bios_base + physical_bios_offset; /* If the values look OK, then map it in. */ if (cru_physical_address) { cru_rom_addr = ioremap(cru_physical_address, cru_length); if (cru_rom_addr) { set_memory_x((unsigned long)cru_rom_addr, cru_length); retval = 0; } } => Which means that cru_physical_address and cru_rom_addr are not page-aligned. So if we follow the conversion model that Linus described we get: set_memory_x((unsigned long)cru_rom_addr & PAGE_MASK, (cru_length + PAGE_SIZE - 1) >> PAGE_SHIFT); Can you check this? Kind regards, Wim. ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] hpwdt: clean up set_memory_x call for 32 bit 2012-01-27 16:49 ` Wim Van Sebroeck @ 2012-01-27 16:55 ` Mingarelli, Thomas 2012-01-27 18:33 ` Maxim Uvarov 1 sibling, 0 replies; 10+ messages in thread From: Mingarelli, Thomas @ 2012-01-27 16:55 UTC (permalink / raw) To: Wim Van Sebroeck, Linus Torvalds; +Cc: Maxim Uvarov, linux-kernel, akpm, stable Your last step I have coded up but not tested yet. I will get that done today. Thanks, Tom -----Original Message----- From: Wim Van Sebroeck [mailto:wim@iguana.be] Sent: Friday, January 27, 2012 10:50 AM To: Mingarelli, Thomas; Linus Torvalds Cc: Maxim Uvarov; linux-kernel@vger.kernel.org; akpm@linux-foundation.org; stable@vger.kernel.org Subject: Re: [PATCH] hpwdt: clean up set_memory_x call for 32 bit Hi Tom, > So I don't know who is supposed to be handling this (Wim?), but the > patch itself looks suspicious. > > On Sun, Jan 15, 2012 at 8:02 PM, Maxim Uvarov <maxim.uvarov@oracle.com> wrote: > > - set_memory_x((unsigned long)bios32_entrypoint, (2 * PAGE_SIZE)); > > + set_memory_x((unsigned long)bios32_entrypoint & PAGE_MASK, 2); > > If it wasn't page-aligned to begin with, then maybe it needs three pages now? I have been looking at the code again and basically we have for 32 bit the following sequence: 1) scan/search from 0x0f0000 through 0x0fffff, inclusive (in steps of 16 bytes) until we find the 32-bit BIOS Service Directory with signature == PCI_BIOS32_SD_VALUE (=0x5F32335F ="_32_"). 2) If we find this area then we first do a checksum check to see if it's a valid area. 3) if it's a valid area then we will check this area for a $CRU record. the code for this is as follows: /* * According to the spec, we're looking for the * first 4KB-aligned address below the entrypoint * listed in the header. The Service Directory code * is guaranteed to occupy no more than 2 4KB pages. */ map_entry = bios_32_ptr->entry_point & ~(PAGE_SIZE - 1); map_offset = bios_32_ptr->entry_point - map_entry; bios32_map = ioremap(map_entry, (2 * PAGE_SIZE)); if (bios32_map == NULL) return -ENODEV; bios32_entrypoint = bios32_map + map_offset; cmn_regs.u1.reax = CRU_BIOS_SIGNATURE_VALUE; set_memory_x((unsigned long)bios32_entrypoint, (2 * PAGE_SIZE)); asminline_call(&cmn_regs, bios32_entrypoint); => So if I understand it correctly then map_entry is page aligned. And thus bios32_map is also page aligned. Wouldn't it then not make more sense to do a: set_memory_x((unsigned long)bios32_map, 2); > > - set_memory_x((unsigned long)cru_rom_addr, cru_length); > > + set_memory_x((unsigned long)cru_rom_addr & PAGE_MASK, cru_length >> PAGE_SHIFT); > > Same here. If we align the start address down, we should fix up the > length. And should we not align the number of pages up? > > In general, a "start/length" conversion to a "page/nr" model needs to be roughly > > len += start & ~PAGE_MASK; > start &= PAGE_MASK; > nr_pages = (len + PAGE_SIZE - 1) >> PAGE_SHIFT; > > to do things right. But I don't know where those magic numbers come > from. Maybe the "2" is already due to the code possibly traversing a > page boundary, and has already been fixed up. Somebody who knows the > driver and the requirements should take a look at this. 4) if we then found the $CRU record then we do: physical_bios_base = cmn_regs.u2.rebx; physical_bios_offset = cmn_regs.u4.redx; cru_length = cmn_regs.u3.recx; cru_physical_address = physical_bios_base + physical_bios_offset; /* If the values look OK, then map it in. */ if (cru_physical_address) { cru_rom_addr = ioremap(cru_physical_address, cru_length); if (cru_rom_addr) { set_memory_x((unsigned long)cru_rom_addr, cru_length); retval = 0; } } => Which means that cru_physical_address and cru_rom_addr are not page-aligned. So if we follow the conversion model that Linus described we get: set_memory_x((unsigned long)cru_rom_addr & PAGE_MASK, (cru_length + PAGE_SIZE - 1) >> PAGE_SHIFT); Can you check this? Kind regards, Wim. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hpwdt: clean up set_memory_x call for 32 bit 2012-01-27 16:49 ` Wim Van Sebroeck 2012-01-27 16:55 ` Mingarelli, Thomas @ 2012-01-27 18:33 ` Maxim Uvarov 1 sibling, 0 replies; 10+ messages in thread From: Maxim Uvarov @ 2012-01-27 18:33 UTC (permalink / raw) To: Wim Van Sebroeck Cc: Thomas.Mingarelli, Linus Torvalds, linux-kernel, akpm, stable On 01/27/2012 08:49 AM, Wim Van Sebroeck wrote: > Hi Tom, > >> So I don't know who is supposed to be handling this (Wim?), but the >> patch itself looks suspicious. >> >> On Sun, Jan 15, 2012 at 8:02 PM, Maxim Uvarov<maxim.uvarov@oracle.com> wrote: >>> - set_memory_x((unsigned long)bios32_entrypoint, (2 * PAGE_SIZE)); >>> + set_memory_x((unsigned long)bios32_entrypoint& PAGE_MASK, 2); >> >> If it wasn't page-aligned to begin with, then maybe it needs three pages now? > > I have been looking at the code again and basically we have for 32 bit the following sequence: > 1) scan/search from 0x0f0000 through 0x0fffff, inclusive (in steps of 16 bytes) until we find > the 32-bit BIOS Service Directory with signature == PCI_BIOS32_SD_VALUE (=0x5F32335F ="_32_"). > 2) If we find this area then we first do a checksum check to see if it's a valid area. > 3) if it's a valid area then we will check this area for a $CRU record. > > the code for this is as follows: > /* > * According to the spec, we're looking for the > * first 4KB-aligned address below the entrypoint > * listed in the header. The Service Directory code > * is guaranteed to occupy no more than 2 4KB pages. > */ > map_entry = bios_32_ptr->entry_point& ~(PAGE_SIZE - 1); > map_offset = bios_32_ptr->entry_point - map_entry; > > bios32_map = ioremap(map_entry, (2 * PAGE_SIZE)); > > if (bios32_map == NULL) > return -ENODEV; > > bios32_entrypoint = bios32_map + map_offset; > > cmn_regs.u1.reax = CRU_BIOS_SIGNATURE_VALUE; > > set_memory_x((unsigned long)bios32_entrypoint, (2 * PAGE_SIZE)); > asminline_call(&cmn_regs, bios32_entrypoint); > > => So if I understand it correctly then map_entry is page aligned. And thus bios32_map is also page aligned. > Wouldn't it then not make more sense to do a: > set_memory_x((unsigned long)bios32_map, 2); > >>> - set_memory_x((unsigned long)cru_rom_addr, cru_length); >>> + set_memory_x((unsigned long)cru_rom_addr& PAGE_MASK, cru_length>> PAGE_SHIFT); >> >> Same here. If we align the start address down, we should fix up the >> length. And should we not align the number of pages up? >> >> In general, a "start/length" conversion to a "page/nr" model needs to be roughly >> >> len += start& ~PAGE_MASK; >> start&= PAGE_MASK; >> nr_pages = (len + PAGE_SIZE - 1)>> PAGE_SHIFT; >> >> to do things right. But I don't know where those magic numbers come >> from. Maybe the "2" is already due to the code possibly traversing a >> page boundary, and has already been fixed up. Somebody who knows the >> driver and the requirements should take a look at this. > > 4) if we then found the $CRU record then we do: > physical_bios_base = cmn_regs.u2.rebx; > physical_bios_offset = cmn_regs.u4.redx; > cru_length = cmn_regs.u3.recx; > cru_physical_address = physical_bios_base + physical_bios_offset; > > /* If the values look OK, then map it in. */ > if (cru_physical_address) { > cru_rom_addr = ioremap(cru_physical_address, cru_length); > if (cru_rom_addr) { > set_memory_x((unsigned long)cru_rom_addr, cru_length); > retval = 0; > } > } > > => Which means that cru_physical_address and cru_rom_addr are not page-aligned. > So if we follow the conversion model that Linus described we get: > set_memory_x((unsigned long)cru_rom_addr& PAGE_MASK, > (cru_length + PAGE_SIZE - 1)>> PAGE_SHIFT); > > Can you check this? > > Kind regards, > Wim. > I got this warning if address was not aligned: WARNING: at arch/x86/mm/pageattr.c:877 change_page_attr_set_clr+0x21e/0x230() Hardware name: ProLiant DL580 G7 Pid: 1321, comm: modprobe Not tainted Call Trace: [<c043e3be>] ? change_page_attr_set_clr+0x21e/0x230 [<c045bd71>] warn_slowpath_common+0x81/0xa0 [<c043e3be>] ? change_page_attr_set_clr+0x21e/0x230 [<c045bdb2>] warn_slowpath_null+0x22/0x30 [<c043e3be>] change_page_attr_set_clr+0x21e/0x230 [<c043ec5f>] set_memory_x+0x5f/0x70 [<f8c515ce>] cru_detect+0x11e/0x130 [hpwdt] [<f8c51617>] bios32_present+0x37/0x40 [hpwdt] [<f8c5166e>] detect_cru_service+0x4e/0x70 [hpwdt] [<f8c5169f>] hpwdt_init_nmi_decoding+0xf/0xf0 [hpwdt] [<f8c5107d>] ? hpwdt_change_timer+0x2d/0x60 [hpwdt] [<f8c517f8>] hpwdt_init_one+0x78/0x190 [hpwdt] [<c06e8c71>] ? pm_runtime_enable+0x51/0x80 Maxim. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-01-27 18:35 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-01-16 4:02 hpwdt: clean up set_memory_x call for 32 bit Maxim Uvarov 2012-01-16 4:02 ` [PATCH] " Maxim Uvarov 2012-01-24 20:20 ` Linus Torvalds 2012-01-24 20:37 ` Wim Van Sebroeck 2012-01-24 21:05 ` Mingarelli, Thomas 2012-01-25 23:21 ` Maxim Uvarov 2012-01-26 0:01 ` Mingarelli, Thomas 2012-01-27 16:49 ` Wim Van Sebroeck 2012-01-27 16:55 ` Mingarelli, Thomas 2012-01-27 18:33 ` Maxim Uvarov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).