[ipxe-devel] Setup INT13 partition on usb images only ifdef CONSOLE_INT13

[Geert Stappers]

On Sun, Mar 01, 2020 at 11:05:59AM +0100, Geert Stappers wrote:
> On Thu, Feb 27, 2020 at 09:47:05AM +0900, Romain Guyard wrote:
> > On 2/27/20 7:09 AM, Geert Stappers wrote:
> > > On Sun, Feb 23, 2020 at 03:45:33PM -0800, Robien wrote:
> > >>> The patch feels wrong, surely **no** _Looks Good To Me_.
> > >>
> > >> Can you elaborate a bit so I can make it better?
> > >>
> > > 
> > > Find it attached
> > 
> > 
> > Thank you for your help.
> > 
> > I tried your patch and unfortunately it is not working on my hardware.
> 
> Noted.
> 
> 
> > When I try to boot, I got a black screen and my motherboard's speaker
> > screams some weird sound that I've never heard before. So thank you for
> > the musical discovery of the day.
> > 
> > The first idea of the patch was to not add any new code to avoid any
> > regression. That is why I took the assembly code from before the commit
> > that added CONSOLE_INT13 option and merged to the current version.
> > Can you explain your changes?
> 
> Here the `diff`,   "explain" in next message.
> 
> 
> --- a/src/arch/x86/prefix/usbdisk.S
> +++ b/src/arch/x86/prefix/usbdisk.S
> @@ -10,12 +10,11 @@ FILE_LICENCE ( GPL2_OR_LATER_OR_UBDL )
>  
>  #include "mbr.S"
>  
> -#ifdef CONSOLE_INT13
> -



>  /* Partition table: 64 heads, 32 sectors/track (ZIP-drive compatible) */
>  	.org 446
>  	.space 16
>  	.space 16
> +#ifdef CONSOLE_INT13


Moved the   #ifdef   to get less duplication in the source


>  	/* Partition 3: log partition (for CONSOLE_INT13) */
>  	.byte 0x00, 0x01, 0x01, 0x00
>  	.byte 0xe0, 0x3f, 0x20, 0x00
> @@ -38,21 +37,17 @@ FILE_LICENCE ( GPL2_OR_LATER_OR_UBDL )
>  	.org 2048 * 512
>  
>  #else /* CONSOLE_INT13 */
> -
> -/* Partition table: 64 heads, 32 sectors/track (ZIP-drive compatible) */
> -	.org 446
> -	.space 16
> -	.space 16

The removal of the unneeded duplicated lines

>  	.space 16
> +	/* Partition 4: boot partition */

Duplicate of comment from opposite #IFDEF choice


>  	.byte 0x80, 0x01, 0x01, 0x00
>  	.byte 0xeb, 0x3f, 0x20, 0x01
>  	.long 0x00000020
> -	.long 0x00000fe0
> +	.long 0x00000820

Same 0x800 size as the opposite #IFDEF Partion 4

 
>  	.org 510
>  	.byte 0x55, 0xaa
>  
> -/* Skip to start of partition */
> +/* Skip to start of boot partition */

Duplicate of comment from opposite #IFDEF choice


>  	.org 32 * 512
>  
>  #endif
> 


So not much changes.


> > >>> The patch feels wrong, surely **no** _Looks Good To Me_.

That "feels wrong"   is avoiding 

 -/* Partition table: 64 heads, 32 sectors/track (ZIP-drive compatible) */
 -	.org 446
 -	.space 16
 -	.space 16

duplication.


It doesn't  explain  the reported breakage.
The smaller partition could cause it. But we have no reports
about partition 4 being too small.



Other thing I want to mention:

0001-peerdist-Allow-for-the-use-of-a-hosted-cache-server.patch
0002-snp-Try-promiscuous-multicast-receive-filter-if-the-.patch
0003-snp-Set-EFI_SIMPLE_NETWORK_RECEIVE_MULTICAST-bit-as-.patch
0004-build-Construct-full-version-number-automatically-fr.patch
0005-travis-Ensure-that-most-recent-tag-is-always-availab.patch
0006-tftp-Eliminate-unnecessary-variable-length-stack-all.patch
0007-infiniband-Eliminate-variable-length-stack-allocatio.patch
0008-slam-Eliminate-variable-length-stack-allocation.patch
0009-slam-Allow-for-the-possibility-of-IPv6-multicast-add.patch
0010-settings-Eliminate-variable-length-stack-allocation.patch
0011-iscsi-Eliminate-variable-length-stack-allocations-in.patch
0012-iscsi-Eliminate-variable-length-stack-allocation-in-.patch
0013-Setup-INT13-partition-on-usb-images-only-if-CONSOLE_.patch


The reviewed patch was in a branch with other changes.




Regards
Geert Stappers
-- 
Silence is hard to parse