Re: Patch to align virt and phys spaces on i386

On Mar 8, 2006, at 1:19 PM, Glenn Brown wrote:
> The attached patch increases i386 virt_offset alignment to 4kB, so
> the bottom 12 bits of corresponding virt and bus addrs will be  
> identical.

Hi Glenn,

Do you have a SourceForge ID?  I would like to add you to the  
Developers section of Etherboot so you can use CVS to apply your  
patch.  If you'd rather not do it yourself, we can do it for you, but  
if you are willing, mail it to me privately, and I'll add you.

I think that perhaps we should go back to the policy of generally  
having people post patches before CVSing them to make sure everyone  
keeps up with what is going on.  Obviously there would have to be  
some exceptions, but it's so good to be able to look at code before  
it enters the production codebase.  We're still a small enough group  
we can do that.

"Unreviewed CVS commits have spoiled good conversation and the  
opportunity for public embarrassment!!"

> This change facilitates porting drivers to Etherboot, as the vast  
> majority of driver execution environments have this virt/bus  
> alignment relationship, and non-Etherboot drivers often assume bus  
> addresses are aligned if the corresponding virtual addrs are aligned.
> PCI Express requires DMAs not cross 4kB boundaries, and this large  
> alignment is required by my forthcoming myri10ge.c 10Gb PCI Express  
> driver, which is a minimal port of our Linux driver.  Other Linux  
> driver ports in the future would also surely benefit from this patch.

I noticed the PCI Express requirement the other day, and also noticed  
that EFI was requiring some more stringent alignment.

> I have successully tested this approach with our myri10ge.c driver  
> on all 8 PCI express motherboards I have available to me,  
> representing the following hardware:
>     CPUs: AMD Opteron and Intel EM64T
>     Chipsets: Serverworks, Invidia, VIA, Intel, SiS
>     BIOSes: AMI, Award, IBM, Phoenix AwardBIOS, and Phoenix  
> TrustedCore.

Great.  Thanks for being rigorous.

This looks awfully familiar :)

> +		unsigned long delta = (((unsigned long) _text - addr)
> +				      & (VIRT_ALIGN-1));
> +		addr += delta;
> +		eaddr += delta;

> The "HAVE_VIRT_OFFSET" lines of the patch are not strictly  
> necessary, but are there for clarity and to avoid changing non-i386  
> behaviour,
> since I have no way to test non-i386.

Thanks for your work.

Marty

> Eager for feeback,
> --Glenn
> Index: src/arch/i386/include/io.h
> ===================================================================
> RCS file: /repository/etherboot/src/arch/i386/include/io.h,v
> retrieving revision 1.1.1.1
> retrieving revision 1.2
> diff -a -u -a -u -r1.1.1.1 -r1.2
> --- src/arch/i386/include/io.h	27 Nov 2005 20:48:44 -0000	1.1.1.1
> +++ src/arch/i386/include/io.h	7 Mar 2006 23:30:28 -0000	1.2
> @@ -4,6 +4,7 @@
>
>  /* Amount of relocation etherboot is experiencing */
>  extern unsigned long virt_offset;
> +#define HAVE_VIRT_OFFSET 1
>
>  /* Don't require identity mapped physical memory,
>   * osloader.c is the only valid user at the moment.
> Index: src/bin/.keepme
> ===================================================================
> RCS file: src/bin/.keepme
> diff -N src/bin/.keepme
> Index: src/core/relocate.c
> ===================================================================
> RCS file: /repository/etherboot/src/core/relocate.c,v
> retrieving revision 1.1
> retrieving revision 1.4
> diff -a -u -a -u -r1.1 -r1.4
> --- src/core/relocate.c	27 Nov 2005 21:13:50 -0000	1.1
> +++ src/core/relocate.c	7 Mar 2006 23:31:47 -0000	1.4
> @@ -15,6 +15,14 @@
>   *
>   */
>
> +/* Align any virt_offset on this power-of-two boundary, so
> + * virt_to_phys() et al. will not alter the less significant bits.
> + * This should match the maximum DMA alignment that might be required
> + * by on the system, so drivers can assume that aligned virtual
> + * addresses will be similarly aligned in bus address space.
> + */
> +#define VIRT_ALIGN 4096 /* PCI Express forbids DMA's across 4KB  
> boundaries */
> +
>  void relocate(void)
>  {
>  	unsigned long addr, eaddr, size;
> @@ -30,7 +38,11 @@
>  	addr = virt_to_phys(_text);
>  	eaddr = virt_to_phys(_end);
>  	size = (eaddr - addr + 0xf) & ~0xf;
> -
> +#ifdef HAVE_VIRT_OFFSET
> +	/* Allocate an oversized block so we can realign within it. */
> +	size += VIRT_ALIGN;
> +#endif /* HAVE_VIRT_OFFSET */
> +
>  	/* If the current etherboot is beyond MAX_ADDR pretend it is
>  	 * at the lowest possible address.
>  	 */
> @@ -85,9 +97,26 @@
>  		}
>  		if (eaddr < r_end - size) {
>  			addr = r_end - size;
> -			eaddr = r_end;
> +			eaddr = addr + (_end - _text);
>  		}
>  	}
> +
> +#ifdef HAVE_VIRT_OFFSET
> +	/* Adjust the relocation to align virt_offset.
> +	 *
> +	 * relocate_to(addr) sets virt_offset=addr-_text, so without
> +	 * any adjustment, the misalignment would be
> +	 * (addr-_text)&(VIRT_ALIGN-1).  We force this misalignment to
> +	 * be zero here.
> +	 */
> +	{
> +		unsigned long delta = (((unsigned long) _text - addr)
> +				      & (VIRT_ALIGN-1));
> +		addr += delta;
> +		eaddr += delta;
> +	}
> +#endif /* HAVE_VIRT_OFFSET */
> +	
>  	if (addr != virt_to_phys(_text)) {
>  		unsigned long old_addr = virt_to_phys(_text);
>  		printf("Relocating _text from: [%lx,%lx) to [%lx,%lx)\n",



-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642