Re: [flashrom] [PATCH 6/8] use getpagesize() to determine physmap's length in nicintel_spi.c

[Carl-Daniel Hailfinger]

Am 15.03.2011 16:29 schrieb Stefan Tauner:
> Signed-off-by: Stefan Tauner

Right, this is a really good idea.

Could you patch physmap instead to round down the requested address and 
round up the end of the range? Then we could just request the amount we 
need without having to care in each programmer driver about page size.
I think somewhere in flashrom we even have rounding code for physmap, it 
just needs to be moved from a driver to the generic physmap.

Your code looks solid.
> nicintel_spi.c |    7 +++++--
>   1 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/nicintel_spi.c b/nicintel_spi.c
> index 5a6e04d..811ed6e 100644
> --- a/nicintel_spi.c
> +++ b/nicintel_spi.c
> @@ -26,11 +26,14 @@
>    */
>
> #include<stdlib.h>
> +#include<unistd.h>
>   #include "flash.h"
>   #include "programmer.h"
>
> #define PCI_VENDOR_ID_INTEL 0x8086
>
> +#define MEMMAP_SIZE getpagesize()
> #define EECD	0x10
> #define FLA	0x1c
>
> @@ -148,7 +151,7 @@ int nicintel_spi_init(void)
>    io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, nics_intel_spi);
>
> nicintel_spibar = physmap("Intel Gigabit NIC w/ SPI flash",
> 		  io_base_addr, 4096);
> 		  io_base_addr, MEMMAP_SIZE);
> tmp = pci_mmio_readl(nicintel_spibar + EECD);
> tmp&= ~FLASH_WRITES_DISABLED;
> tmp |= FLASH_WRITES_ENABLED;
>
> @@ -173,7 +176,7 @@ int nicintel_spi_shutdown(void)
>    tmp |= FLASH_WRITES_DISABLED;
>    pci_mmio_writel(tmp, nicintel_spibar + EECD);
> physunmap(nicintel_spibar, 4096);
> physunmap(nicintel_spibar, MEMMAP_SIZE);
> pci_cleanup(pacc);
> release_io_perms();

Regards,
Carl-Daniel

[PATCH 6/8] use getpagesize() to determine physmap's length in nicintel_spi.c

[Stefan Tauner]

On Thu, 31 Mar 2011 08:45:39 +0200
Carl-Daniel Hailfinger  wrote:
> Could you patch physmap instead to round down the requested address
> and round up the end of the range? Then we could just request the
> amount we need without having to care in each programmer driver about
> page size. I think somewhere in flashrom we even have rounding code
> for physmap, it just needs to be moved from a driver to the generic
> physmap.

oh. steep learning curve there. :)
not because the rounding is complicated, but because i am not sure i am
aware of all side effects.
i guess the existing rounding code you was referring to is that in
sb600spi.c?

what i can do/propose for now:
change physmap_common as follows:
instead of the two check including getpagesize():
- round down the physical address requested later from phys_to_virt to
  the nearest getpagesize()-aligned address
- round up the length requested later accordingly i.e. request a
  physical window that is aligned to pages and includes at least the
  memory described by phys_addr + len
- return virt_addr after adding the offset from rounding down the
  physical address
- change nicintel_spi.c accordingly

i would rather not touch the other drivers for now. i think i know what
we are doing here in theory, but i fear i would break something :)

ps: i can't fully test this, because i broke my intel nic when i tried
to unsolder the flash (to add a smd zif socket for further tinkering).

[PATCH 6/8] use getpagesize() to determine physmap's length in nicintel_spi.c

[Stefan Tauner]

On Thu, 31 Mar 2011 13:03:54 +0200
Stefan Tauner  wrote:
> On Thu, 31 Mar 2011 08:45:39 +0200
> Carl-Daniel Hailfinger  wrote:
> > Could you patch physmap instead to round down the requested address
> > and round up the end of the range? Then we could just request the
> > amount we need without having to care in each programmer driver
> > about page size. I think somewhere in flashrom we even have
> > rounding code for physmap, it just needs to be moved from a driver
> > to the generic physmap.
>
> oh. steep learning curve there. :)
> not because the rounding is complicated, but because i am not sure i
> am aware of all side effects.
> i guess the existing rounding code you was referring to is that in
> sb600spi.c?
>
> what i can do/propose for now:
> change physmap_common as follows:
> instead of the two check including getpagesize():
> round down the physical address requested later from phys_to_virt to
> the nearest getpagesize()-aligned address
> round up the length requested later accordingly i.e. request a
> physical window that is aligned to pages and includes at least the
> memory described by phys_addr + len
> return virt_addr after adding the offset from rounding down the
> physical address
> change nicintel_spi.c accordingly

we also need something similar for physunmap(...).
would the same rounding be sufficient?
how should we implement that? a function void getWindow(*addr, *len)
that sets the addr and len and will be called by physmap and physunmap?
or is that too complicated?

[PATCH 6/8] use getpagesize() to determine physmap's length in nicintel_spi.c

[Stefan Tauner]

On Thu, 31 Mar 2011 22:31:17 +0200
Stefan Tauner  wrote:
> On Thu, 31 Mar 2011 13:03:54 +0200
> Stefan Tauner  wrote:
> > On Thu, 31 Mar 2011 08:45:39 +0200
> > Carl-Daniel Hailfinger  wrote:
> > > Could you patch physmap instead to round down the requested address
> > > and round up the end of the range? Then we could just request the
> > > amount we need without having to care in each programmer driver
> > > about page size. I think somewhere in flashrom we even have
> > > rounding code for physmap, it just needs to be moved from a driver
> > > to the generic physmap.
> >
> > oh. steep learning curve there. :)
> > not because the rounding is complicated, but because i am not sure i
> > am aware of all side effects.
> > i guess the existing rounding code you was referring to is that in
> > sb600spi.c?
> >
> > what i can do/propose for now:
> > change physmap_common as follows:
> > instead of the two check including getpagesize():
> > round down the physical address requested later from phys_to_virt to
> > the nearest getpagesize()-aligned address
> > round up the length requested later accordingly i.e. request a
> > physical window that is aligned to pages and includes at least the
> > memory described by phys_addr + len
> > return virt_addr after adding the offset from rounding down the
> > physical address
> > change nicintel_spi.c accordingly
>
> we also need something similar for physunmap(...).
> would the same rounding be sufficient?
> how should we implement that? a function void getWindow(*addr, *len)
> that sets the addr and len and will be called by physmap and physunmap?
> or is that too complicated?

carldani: ping!
i have started to mimic what the mei kernel driver does and i think i
cant continue due to unaligned memory mappings. so it would be a good
moment to push for what you have suggested. even if that is not the
problem i currently face, i would like to work on this sometimes so
please comment my proposal above.

[PATCH 6/8] use getpagesize() to determine physmap's length in nicintel_spi.c

[Stefan Tauner]

On Sun, 29 May 2011 06:17:15 +0200
Stefan Tauner  wrote:
> On Thu, 31 Mar 2011 22:31:17 +0200
> Stefan Tauner  wrote:
> > On Thu, 31 Mar 2011 13:03:54 +0200
> > Stefan Tauner  wrote:
> > > On Thu, 31 Mar 2011 08:45:39 +0200
> > > Carl-Daniel Hailfinger  wrote:
> > > > Could you patch physmap instead to round down the requested address
> > > > and round up the end of the range? Then we could just request the
> > > > amount we need without having to care in each programmer driver
> > > > about page size. I think somewhere in flashrom we even have
> > > > rounding code for physmap, it just needs to be moved from a driver
> > > > to the generic physmap.
> > >
> > > oh. steep learning curve there. :)
> > > not because the rounding is complicated, but because i am not sure i
> > > am aware of all side effects.
> > > i guess the existing rounding code you was referring to is that in
> > > sb600spi.c?
> > >
> > > what i can do/propose for now:
> > > change physmap_common as follows:
> > > instead of the two check including getpagesize():
> > > round down the physical address requested later from phys_to_virt to
> > > the nearest getpagesize()-aligned address
> > > round up the length requested later accordingly i.e. request a
> > > physical window that is aligned to pages and includes at least the
> > > memory described by phys_addr + len
> > > return virt_addr after adding the offset from rounding down the
> > > physical address
> > > change nicintel_spi.c accordingly
> >
> > we also need something similar for physunmap(...).
> > would the same rounding be sufficient?
> > how should we implement that? a function void getWindow(*addr, *len)
> > that sets the addr and len and will be called by physmap and physunmap?
> > or is that too complicated?
>
> carldani: ping!
> i have started to mimic what the mei kernel driver does and i think i
> cant continue due to unaligned memory mappings. so it would be a good
> moment to push for what you have suggested. even if that is not the
> problem i currently face, i would like to work on this sometimes so
> please comment my proposal above.

my current solution:
static void round_to_page_boundaries(uint32_t start_in, uint32_t len_in,
    			     uint32_t *start_out, uint32_t *len_out)
{
    uint32_t page_size = getpagesize();
    uint32_t end = start_in + len_in;;
    msg_pdbg("start_in= 0x%08x, len_in= 0x%08x, end_in= 0x%08x, page_size=0x%08x\n", start_in, len_in, start_in+len_in, page_size);
    *start_out = start_in & ~(page_size-1);
    end = (end & ~(page_size-1)) + page_size;
    *len_out = end - *start_out;
    msg_pdbg("start_out=0x%08x, len_out=0x%08x, end_out=0x%08x\n", *start_out, *len_out, *start_out+ *len_out);
}

seems to do what we want, but i am not sure about the interface (atm i
am using it in ichspi.c directly instead in physmap.c).

[PATCH 6/8] use getpagesize() to determine physmap's length in nicintel_spi.c

[Stefan Tauner]

ah.. and btw (sorry for the spam!) it would be nice if we could change
the behavior if the physmap function in the case of len=0.
the kernel interface has a nice property in this regard:
http://www.aoc.nrao.edu/~tjuerges/ALMA/Kernel/2.6.29.4/deviceiobook/re04.htm...
"If you want to get access to the complete BAR without checking for its
length first, pass 0."
i am not sure how the actual length is read out, but lspci shows it too
so it cant be that hard...
would you take a patch for that?

[PATCH] automatically round address ranges requested from physmap_common to page boundaries

[Stefan Tauner]

this patch also changes the display width of all addresses in physmap.c to 16 hex characters.

FIXME: what about unmappings?
munmap is safe.
djgpp's __dpmi_free_physical_address_mapping: unknown.
DirectHW's unmap_physical: unknown.

FIXME: jakllsch suggested using PRIx64 instead of x, because it's more portable

Signed-off-by: Stefan Tauner 
---
 physmap.c |   46 +++++++++++++++++++++++++++++++---------------
 1 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/physmap.c b/physmap.c
index cb035a5..3dbe387 100644
--- a/physmap.c
+++ b/physmap.c
@@ -99,7 +99,8 @@ void physunmap(void *virt_addr, size_t len)
    /* There is no known way to unmap the first 1 MB. The DPMI server will
     * do this for us on exit.
     */
-	if ((virt_addr >= realmem_map) && ((virt_addr + len) <= (realmem_map + (1024 * 1024)))) {
+	if ((virt_addr >= realmem_map) &&
+	    ((virt_addr + len) <= (realmem_map + (1024 * 1024)))) {
    	return;
    }

@@ -195,7 +196,8 @@ static void *sys_physmap_ro_cached(unsigned long phys_addr, size_t len)
    if (-1 == fd_mem_cached) {
    	/* Open the memory device CACHED. */
    	if (-1 == (fd_mem_cached = open(MEM_DEV, O_RDWR))) {
-			msg_perr("Critical error: open(" MEM_DEV "): %s", strerror(errno));
+			msg_perr("Critical error: open(" MEM_DEV "): %s",
+				 strerror(errno));
    		exit(2);
    	}
    }
@@ -221,26 +223,39 @@ void physunmap(void *virt_addr, size_t len)
 #define PHYSMAP_RW	0
 #define PHYSMAP_RO	1

+/* Round start to nearest page boundary below and set len so that the resulting
+ * address range ends at the lowest possible page boundary where the original
+ * address range is still entirely contained. It returns the difference between
+ * the rounded start address and the original start address. */
+static unsigned long round_to_page_boundaries(unsigned long *start, size_t *len)
+{
+	unsigned long page_size = getpagesize();
+	unsigned long page_mask = ~(page_size-1);
+	unsigned long end = *start + *len;
+	unsigned long old_start = *start;
+	msg_gspew("page_size=%lu\n", page_size);
+	msg_gspew("start=0x%016lx, len=0x%08lx, end=0x%016lx\n",
+		  *start, *len, end);
+	*start = *start & page_mask;
+	end = (end & page_mask) + (((end & page_mask) != end) ? page_size : 0);
+	*len = end - *start;
+	msg_gspew("start=0x%016lx, len=0x%08lx, end=0x%016lx\n",
+		  *start, *len, *start + *len);
+	return old_start - *start;
+}
+
 static void *physmap_common(const char *descr, unsigned long phys_addr, size_t len, int mayfail, int readonly)
 {
    void *virt_addr;
+	unsigned long  diff;

if (len == 0) {
-		msg_pspew("Not mapping %s, zero size at 0x%08lx.\n",
+		msg_pspew("Not mapping %s, zero size at 0x%016lx.\n",
    		     descr, phys_addr);
    	return ERROR_PTR;
    }
-		
-	if ((getpagesize() - 1) & len) {
-		msg_perr("Mapping %s at 0x%08lx, unaligned size 0x%lx.\n",
-			descr, phys_addr, (unsigned long)len);
-	}
-
-	if ((getpagesize() - 1) & phys_addr) {
-		msg_perr("Mapping %s, 0x%lx bytes at unaligned 0x%08lx.\n",
-			descr, (unsigned long)len, phys_addr);
-	}

+	diff = round_to_page_boundaries(&phys_addr, &len);
    if (readonly) {
    	virt_addr = sys_physmap_ro_cached(phys_addr, len);
    } else {
@@ -250,7 +265,8 @@ static void *physmap_common(const char *descr, unsigned long phys_addr, size_t l
    if (ERROR_PTR == virt_addr) {
    	if (NULL == descr)
    		descr = "memory";
-		msg_perr("Error accessing %s, 0x%lx bytes at 0x%08lx\n", descr, (unsigned long)len, phys_addr);
+		msg_perr("Error accessing %s, 0x%lx bytes at 0x%016lx\n", descr,
+			 (unsigned long)len, phys_addr);
    	perror(MEM_DEV " mmap failed");
 #ifdef __linux__
    	if (EINVAL == errno) {
@@ -269,7 +285,7 @@ static void *physmap_common(const char *descr, unsigned long phys_addr, size_t l
    		exit(3);
    }

-	return virt_addr;
+	return virt_addr + diff;
 }

void *physmap(const char *descr, unsigned long phys_addr, size_t len)

[RFC] automagical mmapping of pci bars (was: [PATCH 6/8] use getpagesize() to determine physmap's length in nicintel_spi.c)

[Stefan Tauner]

On Sun, 29 May 2011 23:38:18 +0200
Stefan Tauner  wrote:
> ah.. and btw (sorry for the spam!) it would be nice if we could change
> the behavior if the physmap function in the case of len=0.
> the kernel interface has a nice property in this regard:
> http://www.aoc.nrao.edu/~tjuerges/ALMA/Kernel/2.6.29.4/deviceiobook/re04.htm...
> "If you want to get access to the complete BAR without checking for its
> length first, pass 0."
> i am not sure how the actual length is read out, but lspci shows it too
> so it cant be that hard...
> would you take a patch for that?

the code is easy.
we need 3 variables here: the pci device vendor id, device id and the
pci bar index.
len will contain the total length of the pci bar.

struct pci_dev *p = pci_dev_find(vendor, device);
    pciaddr_t len = (p->known_fields & PCI_FILL_SIZES) ? p->size[bar_index] : 0;

we dont know this information inside physmap and thats probably better
anyway. :)

but(!) we could enhance pcidev_init to return an already mapped address
with the whole pci bar mmaped. i have not looked at the all use cases of
pcidev_init though. if some of its users really just want the physical
address of a device's bar we could refactor pcidev_init to retain this
functionality and share most of its code with the new function.
if we sometimes dont want to map the whole bar then there should be way
to do this too.

[PATCH 6/8] use getpagesize() to determine physmap's length in nicintel_spi.c

[Stefan Tauner]

On Thu, 31 Mar 2011 08:45:39 +0200
Carl-Daniel Hailfinger  wrote:
> Am 15.03.2011 16:29 schrieb Stefan Tauner:
> > Signed-off-by: Stefan Tauner
>
> Right, this is a really good idea.
>
> Could you patch physmap instead to round down the requested address and 
> round up the end of the range? Then we could just request the amount we 
> need without having to care in each programmer driver about page size.
> I think somewhere in flashrom we even have rounding code for physmap, it 
> just needs to be moved from a driver to the generic physmap.
>
> Your code looks solid.

Ooops, we forgot this one... more or less :)
committed in r1584