hpwdt: clean up set_memory_x call for 32 bit

hpwdt: clean up set_memory_x call for 32 bit

[Maxim Uvarov]

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.

[PATCH] hpwdt: clean up set_memory_x call for 32 bit

[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

Re: [PATCH] hpwdt: clean up set_memory_x call for 32 bit

[Linus Torvalds]

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

Re: [PATCH] hpwdt: clean up set_memory_x call for 32 bit

[Wim Van Sebroeck]

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.

RE: [PATCH] hpwdt: clean up set_memory_x call for 32 bit

[Mingarelli, Thomas]

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.

Re: [PATCH] hpwdt: clean up set_memory_x call for 32 bit

[Maxim Uvarov]

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.
>
>

RE: [PATCH] hpwdt: clean up set_memory_x call for 32 bit

[Mingarelli, Thomas]

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.
>
>

Re: [PATCH] hpwdt: clean up set_memory_x call for 32 bit

[Wim Van Sebroeck]

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.

RE: [PATCH] hpwdt: clean up set_memory_x call for 32 bit

[Mingarelli, Thomas]

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.

Re: [PATCH] hpwdt: clean up set_memory_x call for 32 bit

[Maxim Uvarov]

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.