Re: [flashrom] [PATCH] DOS support for flashrom, first try

[Carl-Daniel Hailfinger]

On 09.03.2010 21:06, Carl-Daniel Hailfinger wrote:
> Rudolf Marek added some preliminary DOS support to flashrom. He kindly
> gave me the tarball, and I tried to generate a patch from it. Any
> omissions are my fault.
>
> If anyone wants a big (130 MB) tarball with cross compiler, matching
> libpci and other stuff, just ask me. The server hosting the tarball is
> has a transfer quota, and I'd rather avoid exhausting it with everybody
> downloading the thing in question.
>
> (It's Rudolf's code, so I'll let him sign off on it.)

Review follows.
If anyone feels up to the task, just repost the code with the requested
changes.
> Index: hwaccess.c
>
> --- hwaccess.c	(revision 927)
> +++ hwaccess.c	(working copy)
> @@ -37,7 +37,7 @@
>  #elif defined(__FreeBSD__) || defined (__DragonFly__)
>    if ((io_fd = open("/dev/io", O_RDWR)) < 0) {
>  #else
> if (iopl(3) != 0) {
> if (0) {
>
> #endif

This needs to be

#elif defined (__MSDOS__)
    if (0) {
#else
    if (iopl(3) != 0) {
#endif
> fprintf(stderr, "ERROR: Could not get I/O privileges (%s).\n"
> 	"You need to be root.\n", strerror(errno));
> Index: hwaccess.h
>
> --- hwaccess.h	(revision 927)
> +++ hwaccess.h	(working copy)
> @@ -67,12 +67,21 @@
>    #define INW  inw
>    #define INL  inl
>  #else

Use #elif defined (__MSDOS__) for all changes in this file.
> #define OUTB outb
> #define OUTW outw
> #define OUTL outl
> #define INB  inb
> #define INW  inw
> #define INL  inl
>
> +//#error "a"
>
> +//#include <pc.h>
> #define OUTB(x,y) outportb(y, x)
> #define OUTW(x,y) outportw(y, x)
> #define OUTL(x,y) outportl(y, x)
> #define INB  inportb
> #define INW  inportw
> #define INL  inportl
>
> #endif
>  #endif
>
> @@ -89,47 +98,6 @@
>      #endif
>    #include <stdint.h>
>
> -static inline void
> -outb(uint8_t value, uint16_t port)
> -{
> asm volatile ("outb %b0,%w1": :"a" (value), "Nd" (port));
>
> -}
>
> -static inline uint8_t
> -inb(uint16_t port)
> -{
> uint8_t value;
> asm volatile ("inb %w1,%0":"=a" (value):"Nd" (port));
> return value;
>
> -}
>
> -static inline void
> -outw(uint16_t value, uint16_t port)
> -{
> asm volatile ("outw %w0,%w1": :"a" (value), "Nd" (port));
>
> -}
>
> -static inline uint16_t
> -inw(uint16_t port)
> -{
> uint16_t value;
> asm volatile ("inw %w1,%0":"=a" (value):"Nd" (port));
> return value;
>
> -}
>
> -static inline void
> -outl(uint32_t value, uint16_t port)
> -{
> asm volatile ("outl %0,%w1": :"a" (value), "Nd" (port));
>
> -}
>
> -static inline uint32_t
> -inl(uint16_t port)
> -{
> uint32_t value;
> asm volatile ("inl %1,%0":"=a" (value):"Nd" (port));
> return value;
>
> -}
>    #endif
>  #endif
> Index: physmap.c
>
> --- physmap.c	(revision 927)
> +++ physmap.c	(working copy)
> @@ -58,21 +58,38 @@
>  static int fd_mem = -1;
>  static int fd_mem_cached = -1;

Use #if defined (__MSDOS__) for all changes in this file.
> +#define MAP_FAILED 0
>
> +//***************** call DPMI physical memory mapping and allocate LDT descriptor for the memory block

Comments should use /**/ instead of //
> +void *map_physical_memory(uint32_t phys_base, uint32_t size) // phys_base must be page aligned
> +{							     // size will be aligned automatically

Explanations for phys_base and size should be in a doxygen comment above
the function.
> void *p;
> if (size == 0)		       // if zero block size, do nothing

Comments above each line, not to the right of the code.
> return(NULL);
> if ((size & 0x0FFF) != 0)	       // if size is not page-aligned
> size = (size + 4096) & (~0x0FFF);  // make it page-aligned
> if ((p = valloc(size)) == NULL)      // allocate page-aligned memory (__djgpp_map_physical_memory required this)
> {				       // allocation failed
> printf("ERROR: cannot allocate %lu bytes for memory mapped device\n",size);

Please use msg_perr instead of printf, try to keep the 80 column limit
by breaking the string apart. gcc can handle stuff like below without
problems:

msg_perr("ERROR: foo bar "
     "baz bang\n");
> exit(-1);			       // exit program
> }
> if (__djgpp_map_physical_memory(p,size,phys_base))
> {				       // failed to map physmem
> printf("ERROR: failed to map physical memory range: 0x%08lX - 0x%08lX\nDPMI 1.0 server or support of DPMI function 0508h is needed.\nIt doesn't work under Windows, use win32 version instead.\n",phys_base,phys_base+size-1);

msg_perr instead of printf, 80 column limit.
> return MAP_FAILED;			       // exit program
> }
> return(p);			       // return pointer to memory block aliasing physical memory area
>
> +}
>
> /* For MMIO access. Must be uncached, doesn't make sense to restrict to ro. */
>  void *sys_physmap_rw_uncached(unsigned long phys_addr, size_t len)
>  {
>    void *virt_addr;
>
> if (-1 == fd_mem) {
> /* Open the memory device UNCACHED. Important for MMIO. */
> if (-1 == (fd_mem = open(MEM_DEV, O_RDWR | O_SYNC))) {
> 	perror("Critical error: open(" MEM_DEV ")");
> 	exit(2);
> }
>
> }
>
> virt_addr = mmap(0, len, PROT_WRITE | PROT_READ, MAP_SHARED,
> 	 fd_mem, (off_t)phys_addr);
> virt_addr = map_physical_memory(phys_addr, len)   ;
> return MAP_FAILED == virt_addr ? NULL : virt_addr;
>
> }
>
> @@ -83,17 +100,10 @@
>  {
>    void *virt_addr;
>
> if (-1 == fd_mem_cached) {
> /* Open the memory device CACHED. */
> if (-1 == (fd_mem_cached = open(MEM_DEV, O_RDWR))) {
> 	perror("Critical error: open(" MEM_DEV ")");
> 	exit(2);
> }
>
> }
>
> virt_addr = mmap(0, len, PROT_READ, MAP_SHARED,
> 	 fd_mem_cached, (off_t)phys_addr);
> virt_addr = map_physical_memory(phys_addr, len)    ;
> return MAP_FAILED == virt_addr ? NULL : virt_addr;
>
> }
>
> void physunmap(void *virt_addr, size_t len)
> @@ -103,7 +113,7 @@
>    	return;
>    }
> munmap(virt_addr, len);
>
> +//	munmap(virt_addr, len);
>  }
>  #endif
> Index: getopt.c
>
> --- getopt.c	(revision 0)
> +++ getopt.c	(revision 0)
> Index: getopt.h
> ===================================================================
> --- getopt.h	(revision 0)
> +++ getopt.h	(revision 0)

A copy of the getopt header. If anything, this needs to be merged
separately, but it would be preferable to just pick it up from some
generic include path.
> Index: pci/pci.h
>
> --- pci/pci.h	(revision 0)
> +++ pci/pci.h	(revision 0)
> Index: pci/types.h
> ===================================================================
> --- pci/types.h	(revision 0)
> +++ pci/types.h	(revision 0)
> Index: pci/config.h
> ===================================================================
> --- pci/config.h	(revision 0)
> +++ pci/config.h	(revision 0)
> Index: pci/header.h
> ===================================================================
> --- pci/header.h	(revision 0)
> +++ pci/header.h	(revision 0)

A copy of the important libpci headers. If anything, this needs to be
merged separately, but it would be preferable to just pick it up from
some generic include path.
> Index: Makefile
>
> --- Makefile	(revision 927)
> +++ Makefile	(working copy)

Superseded by my Makefile patch.

Regards,
Carl-Daniel