[PATCH 0/2] remove timestamp from hvmloader for reproducible build

[PATCH 0/2] remove timestamp from hvmloader for reproducible build

[Olaf Hering]

Somehow these timestamps got into the tree a long time ago.
Now they prevent reproducible hvmloader binaries.

Olaf

Olaf Hering (2):
  hvmloader: remove timestamp from smbios
  hvmloader: remove timestamp from vgabios

 tools/firmware/hvmloader/Makefile |  1 -
 tools/firmware/hvmloader/smbios.c |  8 --------
 tools/firmware/vgabios/Makefile   | 11 ++++-------
 tools/firmware/vgabios/vgabios.c  |  5 -----
 4 files changed, 4 insertions(+), 21 deletions(-)

[PATCH 1/2] hvmloader: remove timestamp from smbios

[Olaf Hering]

Including a timestamp into the binary makes it impossible to get
reproducible binaries. Remove the timestamp because it carries no
valuable info.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/firmware/hvmloader/Makefile | 1 -
 tools/firmware/hvmloader/smbios.c | 8 --------
 2 files changed, 9 deletions(-)

diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
index b759e81..b4cdaae 100644
--- a/tools/firmware/hvmloader/Makefile
+++ b/tools/firmware/hvmloader/Makefile
@@ -88,7 +88,6 @@ all: subdirs-all
 	$(MAKE) hvmloader
 
 ovmf.o rombios.o seabios.o hvmloader.o: roms.inc
-smbios.o: CFLAGS += -D__SMBIOS_DATE__="\"$(shell date +%m/%d/%Y)\""
 
 hvmloader: $(OBJS) acpi/acpi.a
 	$(LD) $(LDFLAGS_DIRECT) -N -Ttext $(LOADADDR) -o hvmloader.tmp $^
diff --git a/tools/firmware/hvmloader/smbios.c b/tools/firmware/hvmloader/smbios.c
index 4d3d692..f48465e 100644
--- a/tools/firmware/hvmloader/smbios.c
+++ b/tools/firmware/hvmloader/smbios.c
@@ -381,7 +381,6 @@ smbios_type_0_init(void *start, const char *xen_version,
                    uint32_t xen_major_version, uint32_t xen_minor_version)
 {
     struct smbios_type_0 *p = (struct smbios_type_0 *)start;
-    static const char *smbios_release_date = __SMBIOS_DATE__;
     const char *s;
     void *pts;
     uint32_t length;
@@ -427,9 +426,6 @@ smbios_type_0_init(void *start, const char *xen_version,
     strcpy((char *)start, s);
     start += strlen(s) + 1;
 
-    strcpy((char *)start, smbios_release_date);
-    start += strlen(smbios_release_date) + 1;
-
     *((uint8_t *)start) = 0;
     return start + 1;
 }
@@ -789,7 +785,6 @@ static void *
 smbios_type_22_init(void *start)
 {
     struct smbios_type_22 *p = (struct smbios_type_22 *)start;
-    static const char *smbios_release_date = __SMBIOS_DATE__;
     const char *s;
     void *pts;
     uint32_t length;
@@ -837,9 +832,6 @@ smbios_type_22_init(void *start)
     strcpy((char *)start, s);
     start += strlen(s) + 1;
 
-    strcpy((char *)start, smbios_release_date);
-    start += strlen(smbios_release_date) + 1;
-
     s = xenstore_read(HVM_XS_BATTERY_DEVICE_NAME, "XEN-VBAT");
     strcpy((char *)start, s);
     start += strlen(s) + 1;

[PATCH 2/2] hvmloader: remove timestamp from vgabios

[Olaf Hering]

Including a timestamp into the binary makes it impossible to get
reproducible binaries. Remove the timestamp because it carries no
valuable info.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/firmware/vgabios/Makefile  | 11 ++++-------
 tools/firmware/vgabios/vgabios.c |  5 -----
 2 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/tools/firmware/vgabios/Makefile b/tools/firmware/vgabios/Makefile
index 51d9e6e..0951a8a 100644
--- a/tools/firmware/vgabios/Makefile
+++ b/tools/firmware/vgabios/Makefile
@@ -5,11 +5,8 @@ BCC = bcc
 AS86 = as86
 
 RELEASE = `pwd | sed "s-.*/--"`
-RELDATE = `date '+%d %b %Y'`
 RELVERS = `pwd | sed "s-.*/--" | sed "s/vgabios//" | sed "s/-//"`
 
-VGABIOS_DATE = "-DVGABIOS_DATE=\"$(RELDATE)\""
-
 .PHONY: all
 all: bios cirrus-bios
 
@@ -40,7 +37,7 @@ release:
 	tar czvf ../$(RELEASE).tgz --exclude CVS -C .. $(RELEASE)/
 
 vgabios.bin: biossums vgabios.c vgabios.h vgafonts.h vgatables.h vbe.h vbe.c vbetables.h
-	$(GCC) -E -P vgabios.c $(VGABIOS_VERS) -DVBE $(VGABIOS_DATE) > _vgabios_.c
+	$(GCC) -E -P vgabios.c $(VGABIOS_VERS) -DVBE > _vgabios_.c
 	$(BCC) -o vgabios.s -C-c -D__i86__ -S -0 _vgabios_.c
 	sed -e 's/^\.text//' -e 's/^\.data//' vgabios.s > _vgabios_.s
 	$(AS86) _vgabios_.s -b vgabios.bin -u -w- -g -0 -j -O -l vgabios.txt
@@ -50,7 +47,7 @@ vgabios.bin: biossums vgabios.c vgabios.h vgafonts.h vgatables.h vbe.h vbe.c vbe
 	ls -l VGABIOS-lgpl-latest.bin
 
 vgabios.debug.bin: biossums vgabios.c vgabios.h vgafonts.h vgatables.h vbe.h vbe.c vbetables.h
-	$(GCC) -E -P vgabios.c $(VGABIOS_VERS) -DVBE -DDEBUG $(VGABIOS_DATE) > _vgabios-debug_.c
+	$(GCC) -E -P vgabios.c $(VGABIOS_VERS) -DVBE -DDEBUG > _vgabios-debug_.c
 	$(BCC) -o vgabios-debug.s -C-c -D__i86__ -S -0 _vgabios-debug_.c
 	sed -e 's/^\.text//' -e 's/^\.data//' vgabios-debug.s > _vgabios-debug_.s
 	$(AS86) _vgabios-debug_.s -b vgabios.debug.bin -u -w- -g -0 -j -O -l vgabios.debug.txt
@@ -60,7 +57,7 @@ vgabios.debug.bin: biossums vgabios.c vgabios.h vgafonts.h vgatables.h vbe.h vbe
 	ls -l VGABIOS-lgpl-latest.debug.bin
 
 vgabios-cirrus.bin: biossums vgabios.c vgabios.h vgafonts.h vgatables.h clext.c
-	$(GCC) -E -P vgabios.c $(VGABIOS_VERS) -DCIRRUS -DPCIBIOS $(VGABIOS_DATE) > _vgabios-cirrus_.c
+	$(GCC) -E -P vgabios.c $(VGABIOS_VERS) -DCIRRUS -DPCIBIOS > _vgabios-cirrus_.c
 	$(BCC) -o vgabios-cirrus.s -C-c -D__i86__ -S -0 _vgabios-cirrus_.c
 	sed -e 's/^\.text//' -e 's/^\.data//' vgabios-cirrus.s > _vgabios-cirrus_.s
 	$(AS86) _vgabios-cirrus_.s -b vgabios-cirrus.bin -u -w- -g -0 -j -O -l vgabios-cirrus.txt
@@ -70,7 +67,7 @@ vgabios-cirrus.bin: biossums vgabios.c vgabios.h vgafonts.h vgatables.h clext.c
 	ls -l VGABIOS-lgpl-latest.cirrus.bin
 
 vgabios-cirrus.debug.bin: biossums vgabios.c vgabios.h vgafonts.h vgatables.h clext.c
-	$(GCC) -E -P vgabios.c $(VGABIOS_VERS) -DCIRRUS -DCIRRUS_DEBUG -DPCIBIOS $(VGABIOS_DATE) > _vgabios-cirrus-debug_.c
+	$(GCC) -E -P vgabios.c $(VGABIOS_VERS) -DCIRRUS -DCIRRUS_DEBUG -DPCIBIOS > _vgabios-cirrus-debug_.c
 	$(BCC) -o vgabios-cirrus-debug.s -C-c -D__i86__ -S -0 _vgabios-cirrus-debug_.c
 	sed -e 's/^\.text//' -e 's/^\.data//' vgabios-cirrus-debug.s > _vgabios-cirrus-debug_.s
 	$(AS86) _vgabios-cirrus-debug_.s -b vgabios-cirrus.debug.bin -u -w- -g -0 -j -O -l vgabios-cirrus.debug.txt
diff --git a/tools/firmware/vgabios/vgabios.c b/tools/firmware/vgabios/vgabios.c
index a9dbe00..aed3e3a 100644
--- a/tools/firmware/vgabios/vgabios.c
+++ b/tools/firmware/vgabios/vgabios.c
@@ -175,11 +175,6 @@ vgabios_version:
 #endif
 .ascii	" "
 
-vgabios_date:
-.ascii  VGABIOS_DATE
-.byte   0x0a,0x0d
-.byte	0x00
-
 vgabios_copyright:
 .ascii	"(C) 2008 the LGPL VGABios developers Team"
 .byte	0x0a,0x0d

Re: [PATCH 1/2] hvmloader: remove timestamp from smbios

[Andrew Cooper]

On 03/02/15 15:54, Olaf Hering wrote:
> Including a timestamp into the binary makes it impossible to get
> reproducible binaries. Remove the timestamp because it carries no
> valuable info.
>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---

I agree with the sentiment, but this is not how to do it.

A release date is part of the SMBIOS spec, and the change below results
in a malformed smbios table (stale p->release_date_str = 3; pointer)

A better approach would be to derive the date from the commit date of
HEAD, which would be consistent across rebuilds.

~Andrew

>  tools/firmware/hvmloader/Makefile | 1 -
>  tools/firmware/hvmloader/smbios.c | 8 --------
>  2 files changed, 9 deletions(-)
>
> diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
> index b759e81..b4cdaae 100644
> --- a/tools/firmware/hvmloader/Makefile
> +++ b/tools/firmware/hvmloader/Makefile
> @@ -88,7 +88,6 @@ all: subdirs-all
>  	$(MAKE) hvmloader
>  
>  ovmf.o rombios.o seabios.o hvmloader.o: roms.inc
> -smbios.o: CFLAGS += -D__SMBIOS_DATE__="\"$(shell date +%m/%d/%Y)\""
>  
>  hvmloader: $(OBJS) acpi/acpi.a
>  	$(LD) $(LDFLAGS_DIRECT) -N -Ttext $(LOADADDR) -o hvmloader.tmp $^
> diff --git a/tools/firmware/hvmloader/smbios.c b/tools/firmware/hvmloader/smbios.c
> index 4d3d692..f48465e 100644
> --- a/tools/firmware/hvmloader/smbios.c
> +++ b/tools/firmware/hvmloader/smbios.c
> @@ -381,7 +381,6 @@ smbios_type_0_init(void *start, const char *xen_version,
>                     uint32_t xen_major_version, uint32_t xen_minor_version)
>  {
>      struct smbios_type_0 *p = (struct smbios_type_0 *)start;
> -    static const char *smbios_release_date = __SMBIOS_DATE__;
>      const char *s;
>      void *pts;
>      uint32_t length;
> @@ -427,9 +426,6 @@ smbios_type_0_init(void *start, const char *xen_version,
>      strcpy((char *)start, s);
>      start += strlen(s) + 1;
>  
> -    strcpy((char *)start, smbios_release_date);
> -    start += strlen(smbios_release_date) + 1;
> -
>      *((uint8_t *)start) = 0;
>      return start + 1;
>  }
> @@ -789,7 +785,6 @@ static void *
>  smbios_type_22_init(void *start)
>  {
>      struct smbios_type_22 *p = (struct smbios_type_22 *)start;
> -    static const char *smbios_release_date = __SMBIOS_DATE__;
>      const char *s;
>      void *pts;
>      uint32_t length;
> @@ -837,9 +832,6 @@ smbios_type_22_init(void *start)
>      strcpy((char *)start, s);
>      start += strlen(s) + 1;
>  
> -    strcpy((char *)start, smbios_release_date);
> -    start += strlen(smbios_release_date) + 1;
> -
>      s = xenstore_read(HVM_XS_BATTERY_DEVICE_NAME, "XEN-VBAT");
>      strcpy((char *)start, s);
>      start += strlen(s) + 1;

Re: [PATCH 1/2] hvmloader: remove timestamp from smbios

[Ian Campbell]

On Tue, 2015-02-03 at 16:54 +0100, Olaf Hering wrote:
> Including a timestamp into the binary makes it impossible to get
> reproducible binaries. Remove the timestamp because it carries no
> valuable info.

AFAICT this is changing the contents of a firmware table which is
exposed to the guest, so the changelog should come complete with a
pointer to the relevant spec and a discussion as to why this is ok to
omit from the tables.

I think you've also made the type 22 malformed, but you should check the
spec to see.

Ian.

Re: [PATCH 2/2] hvmloader: remove timestamp from vgabios

[Andrew Cooper]

On 03/02/15 15:54, Olaf Hering wrote:
> Including a timestamp into the binary makes it impossible to get
> reproducible binaries. Remove the timestamp because it carries no
> valuable info.
>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>

In this case, it would appear that the vgabios_date symbol is completely
unused inside the binary.  Good riddance!

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>  tools/firmware/vgabios/Makefile  | 11 ++++-------
>  tools/firmware/vgabios/vgabios.c |  5 -----
>  2 files changed, 4 insertions(+), 12 deletions(-)
>
> diff --git a/tools/firmware/vgabios/Makefile b/tools/firmware/vgabios/Makefile
> index 51d9e6e..0951a8a 100644
> --- a/tools/firmware/vgabios/Makefile
> +++ b/tools/firmware/vgabios/Makefile
> @@ -5,11 +5,8 @@ BCC = bcc
>  AS86 = as86
>  
>  RELEASE = `pwd | sed "s-.*/--"`
> -RELDATE = `date '+%d %b %Y'`
>  RELVERS = `pwd | sed "s-.*/--" | sed "s/vgabios//" | sed "s/-//"`
>  
> -VGABIOS_DATE = "-DVGABIOS_DATE=\"$(RELDATE)\""
> -
>  .PHONY: all
>  all: bios cirrus-bios
>  
> @@ -40,7 +37,7 @@ release:
>  	tar czvf ../$(RELEASE).tgz --exclude CVS -C .. $(RELEASE)/
>  
>  vgabios.bin: biossums vgabios.c vgabios.h vgafonts.h vgatables.h vbe.h vbe.c vbetables.h
> -	$(GCC) -E -P vgabios.c $(VGABIOS_VERS) -DVBE $(VGABIOS_DATE) > _vgabios_.c
> +	$(GCC) -E -P vgabios.c $(VGABIOS_VERS) -DVBE > _vgabios_.c
>  	$(BCC) -o vgabios.s -C-c -D__i86__ -S -0 _vgabios_.c
>  	sed -e 's/^\.text//' -e 's/^\.data//' vgabios.s > _vgabios_.s
>  	$(AS86) _vgabios_.s -b vgabios.bin -u -w- -g -0 -j -O -l vgabios.txt
> @@ -50,7 +47,7 @@ vgabios.bin: biossums vgabios.c vgabios.h vgafonts.h vgatables.h vbe.h vbe.c vbe
>  	ls -l VGABIOS-lgpl-latest.bin
>  
>  vgabios.debug.bin: biossums vgabios.c vgabios.h vgafonts.h vgatables.h vbe.h vbe.c vbetables.h
> -	$(GCC) -E -P vgabios.c $(VGABIOS_VERS) -DVBE -DDEBUG $(VGABIOS_DATE) > _vgabios-debug_.c
> +	$(GCC) -E -P vgabios.c $(VGABIOS_VERS) -DVBE -DDEBUG > _vgabios-debug_.c
>  	$(BCC) -o vgabios-debug.s -C-c -D__i86__ -S -0 _vgabios-debug_.c
>  	sed -e 's/^\.text//' -e 's/^\.data//' vgabios-debug.s > _vgabios-debug_.s
>  	$(AS86) _vgabios-debug_.s -b vgabios.debug.bin -u -w- -g -0 -j -O -l vgabios.debug.txt
> @@ -60,7 +57,7 @@ vgabios.debug.bin: biossums vgabios.c vgabios.h vgafonts.h vgatables.h vbe.h vbe
>  	ls -l VGABIOS-lgpl-latest.debug.bin
>  
>  vgabios-cirrus.bin: biossums vgabios.c vgabios.h vgafonts.h vgatables.h clext.c
> -	$(GCC) -E -P vgabios.c $(VGABIOS_VERS) -DCIRRUS -DPCIBIOS $(VGABIOS_DATE) > _vgabios-cirrus_.c
> +	$(GCC) -E -P vgabios.c $(VGABIOS_VERS) -DCIRRUS -DPCIBIOS > _vgabios-cirrus_.c
>  	$(BCC) -o vgabios-cirrus.s -C-c -D__i86__ -S -0 _vgabios-cirrus_.c
>  	sed -e 's/^\.text//' -e 's/^\.data//' vgabios-cirrus.s > _vgabios-cirrus_.s
>  	$(AS86) _vgabios-cirrus_.s -b vgabios-cirrus.bin -u -w- -g -0 -j -O -l vgabios-cirrus.txt
> @@ -70,7 +67,7 @@ vgabios-cirrus.bin: biossums vgabios.c vgabios.h vgafonts.h vgatables.h clext.c
>  	ls -l VGABIOS-lgpl-latest.cirrus.bin
>  
>  vgabios-cirrus.debug.bin: biossums vgabios.c vgabios.h vgafonts.h vgatables.h clext.c
> -	$(GCC) -E -P vgabios.c $(VGABIOS_VERS) -DCIRRUS -DCIRRUS_DEBUG -DPCIBIOS $(VGABIOS_DATE) > _vgabios-cirrus-debug_.c
> +	$(GCC) -E -P vgabios.c $(VGABIOS_VERS) -DCIRRUS -DCIRRUS_DEBUG -DPCIBIOS > _vgabios-cirrus-debug_.c
>  	$(BCC) -o vgabios-cirrus-debug.s -C-c -D__i86__ -S -0 _vgabios-cirrus-debug_.c
>  	sed -e 's/^\.text//' -e 's/^\.data//' vgabios-cirrus-debug.s > _vgabios-cirrus-debug_.s
>  	$(AS86) _vgabios-cirrus-debug_.s -b vgabios-cirrus.debug.bin -u -w- -g -0 -j -O -l vgabios-cirrus.debug.txt
> diff --git a/tools/firmware/vgabios/vgabios.c b/tools/firmware/vgabios/vgabios.c
> index a9dbe00..aed3e3a 100644
> --- a/tools/firmware/vgabios/vgabios.c
> +++ b/tools/firmware/vgabios/vgabios.c
> @@ -175,11 +175,6 @@ vgabios_version:
>  #endif
>  .ascii	" "
>  
> -vgabios_date:
> -.ascii  VGABIOS_DATE
> -.byte   0x0a,0x0d
> -.byte	0x00
> -
>  vgabios_copyright:
>  .ascii	"(C) 2008 the LGPL VGABios developers Team"
>  .byte	0x0a,0x0d
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 1/2] hvmloader: remove timestamp from smbios

[Jan Beulich]

>>> On 03.02.15 at 17:02, <andrew.cooper3@citrix.com> wrote:
> On 03/02/15 15:54, Olaf Hering wrote:
>> Including a timestamp into the binary makes it impossible to get
>> reproducible binaries. Remove the timestamp because it carries no
>> valuable info.
> 
> I agree with the sentiment, but this is not how to do it.
> 
> A release date is part of the SMBIOS spec, and the change below results
> in a malformed smbios table (stale p->release_date_str = 3; pointer)
> 
> A better approach would be to derive the date from the commit date of
> HEAD, which would be consistent across rebuilds.

Except that this information may not be available, and isn't really
relevant. Instead I'd suggest using the source time stamp of
smbios.c, as that's really the (almost) only thing controlling the
data presented to the guest.

Or maybe - considering that git checkouts use the current time
for file timestamps rather than the last modification time - use
HEAD's if available, and fall back to smbios.c's otherwise.

Jan

Re: [PATCH 2/2] hvmloader: remove timestamp from vgabios

[Jan Beulich]

>>> On 03.02.15 at 17:06, <andrew.cooper3@citrix.com> wrote:
> On 03/02/15 15:54, Olaf Hering wrote:
>> Including a timestamp into the binary makes it impossible to get
>> reproducible binaries. Remove the timestamp because it carries no
>> valuable info.
>>
>> Signed-off-by: Olaf Hering <olaf@aepfle.de>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
> 
> In this case, it would appear that the vgabios_date symbol is completely
> unused inside the binary.  Good riddance!

Let's hope there are no tools scanning the BIOS image for a date
string.

Jan

Re: [PATCH 2/2] hvmloader: remove timestamp from vgabios

[Ian Campbell]

On Tue, 2015-02-03 at 16:06 +0000, Andrew Cooper wrote:
> On 03/02/15 15:54, Olaf Hering wrote:
> > Including a timestamp into the binary makes it impossible to get
> > reproducible binaries. Remove the timestamp because it carries no
> > valuable info.
> >
> > Signed-off-by: Olaf Hering <olaf@aepfle.de>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> 
> In this case, it would appear that the vgabios_date symbol is completely
> unused inside the binary.  Good riddance!

I think it is actually used because the string at vgabios_version
(deliberately) falls through.

Olof, you just removed the nul terminator used there AFAICT. At best
this means that the vgabios_copyright is printed twice.

Ian.

Re: [PATCH 2/2] hvmloader: remove timestamp from vgabios

[Andrew Cooper]

On 03/02/15 16:18, Ian Campbell wrote:
> On Tue, 2015-02-03 at 16:06 +0000, Andrew Cooper wrote:
>> On 03/02/15 15:54, Olaf Hering wrote:
>>> Including a timestamp into the binary makes it impossible to get
>>> reproducible binaries. Remove the timestamp because it carries no
>>> valuable info.
>>>
>>> Signed-off-by: Olaf Hering <olaf@aepfle.de>
>>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>> Cc: Ian Campbell <ian.campbell@citrix.com>
>>> Cc: Wei Liu <wei.liu2@citrix.com>
>> In this case, it would appear that the vgabios_date symbol is completely
>> unused inside the binary.  Good riddance!
> I think it is actually used because the string at vgabios_version
> (deliberately) falls through.

Oh - so it does.  (I misread that as an .asciz).

>
> Olof, you just removed the nul terminator used there AFAICT. At best
> this means that the vgabios_copyright is printed twice.

In which case the 0a/0d and NUL characters need to stay, although the
single space above vgabios_date can be discarded.

~Andrew

Re: [PATCH 1/2] hvmloader: remove timestamp from smbios

[Andrew Cooper]

On 03/02/15 16:15, Jan Beulich wrote:
>>>> On 03.02.15 at 17:02, <andrew.cooper3@citrix.com> wrote:
>> On 03/02/15 15:54, Olaf Hering wrote:
>>> Including a timestamp into the binary makes it impossible to get
>>> reproducible binaries. Remove the timestamp because it carries no
>>> valuable info.
>> I agree with the sentiment, but this is not how to do it.
>>
>> A release date is part of the SMBIOS spec, and the change below results
>> in a malformed smbios table (stale p->release_date_str = 3; pointer)
>>
>> A better approach would be to derive the date from the commit date of
>> HEAD, which would be consistent across rebuilds.
> Except that this information may not be available, and isn't really
> relevant. Instead I'd suggest using the source time stamp of
> smbios.c, as that's really the (almost) only thing controlling the
> data presented to the guest.
>
> Or maybe - considering that git checkouts use the current time
> for file timestamps rather than the last modification time - use
> HEAD's if available, and fall back to smbios.c's otherwise.

Both good points.  That sounds like the best approach.

~Andrew

Re: [PATCH 2/2] hvmloader: remove timestamp from vgabios

[Ian Campbell]

On Tue, 2015-02-03 at 16:17 +0000, Jan Beulich wrote:
> >>> On 03.02.15 at 17:06, <andrew.cooper3@citrix.com> wrote:
> > On 03/02/15 15:54, Olaf Hering wrote:
> >> Including a timestamp into the binary makes it impossible to get
> >> reproducible binaries. Remove the timestamp because it carries no
> >> valuable info.
> >>
> >> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> >> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >> Cc: Ian Campbell <ian.campbell@citrix.com>
> >> Cc: Wei Liu <wei.liu2@citrix.com>
> > 
> > In this case, it would appear that the vgabios_date symbol is completely
> > unused inside the binary.  Good riddance!
> 
> Let's hope there are no tools scanning the BIOS image for a date
> string.

That occurred to me too and I forgot to mention it, it looks vaguely
like this stuff is trying to conform to something or other (e.g.
specific offset and some things which look a bit like signatures),
probably some ancient PC spec?

Ian.

Re: [PATCH 2/2] hvmloader: remove timestamp from vgabios

[Ian Campbell]

On Tue, 2015-02-03 at 16:23 +0000, Andrew Cooper wrote:
> On 03/02/15 16:18, Ian Campbell wrote:
> > On Tue, 2015-02-03 at 16:06 +0000, Andrew Cooper wrote:
> >> On 03/02/15 15:54, Olaf Hering wrote:
> >>> Including a timestamp into the binary makes it impossible to get
> >>> reproducible binaries. Remove the timestamp because it carries no
> >>> valuable info.
> >>>
> >>> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> >>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> >>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >>> Cc: Ian Campbell <ian.campbell@citrix.com>
> >>> Cc: Wei Liu <wei.liu2@citrix.com>
> >> In this case, it would appear that the vgabios_date symbol is completely
> >> unused inside the binary.  Good riddance!
> > I think it is actually used because the string at vgabios_version
> > (deliberately) falls through.
> 
> Oh - so it does.  (I misread that as an .asciz).

I'm not going to pretend I spotted it the first time either ;-)

> > Olof, you just removed the nul terminator used there AFAICT. At best
> > this means that the vgabios_copyright is printed twice.
> 
> In which case the 0a/0d and NUL characters need to stay, although the
> single space above vgabios_date can be discarded.

True. (Subject to what Jan wondered about scanning for BIOS strings...)

Ian.

Re: [PATCH 2/2] hvmloader: remove timestamp from vgabios

[Jan Beulich]

>>> On 03.02.15 at 17:28, <Ian.Campbell@citrix.com> wrote:
> On Tue, 2015-02-03 at 16:17 +0000, Jan Beulich wrote:
>> >>> On 03.02.15 at 17:06, <andrew.cooper3@citrix.com> wrote:
>> > On 03/02/15 15:54, Olaf Hering wrote:
>> >> Including a timestamp into the binary makes it impossible to get
>> >> reproducible binaries. Remove the timestamp because it carries no
>> >> valuable info.
>> >>
>> >> Signed-off-by: Olaf Hering <olaf@aepfle.de>
>> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> >> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> >> Cc: Ian Campbell <ian.campbell@citrix.com>
>> >> Cc: Wei Liu <wei.liu2@citrix.com>
>> > 
>> > In this case, it would appear that the vgabios_date symbol is completely
>> > unused inside the binary.  Good riddance!
>> 
>> Let's hope there are no tools scanning the BIOS image for a date
>> string.
> 
> That occurred to me too and I forgot to mention it, it looks vaguely
> like this stuff is trying to conform to something or other (e.g.
> specific offset and some things which look a bit like signatures),
> probably some ancient PC spec?

Exactly - that's what though of too. Just that I don't have any old
VGA BIOS spec around (anymore; if there ever was any). Ralph
Brown's once famous interrupt and memory documentation at least
doesn't mention anything, but however good and useful it was, it
never was official documentation in any way.

Jan

Re: [PATCH 1/2] hvmloader: remove timestamp from smbios

[Jan Beulich]

>>> On 03.02.15 at 17:23, <andrew.cooper3@citrix.com> wrote:
> On 03/02/15 16:15, Jan Beulich wrote:
>>>>> On 03.02.15 at 17:02, <andrew.cooper3@citrix.com> wrote:
>>> On 03/02/15 15:54, Olaf Hering wrote:
>>>> Including a timestamp into the binary makes it impossible to get
>>>> reproducible binaries. Remove the timestamp because it carries no
>>>> valuable info.
>>> I agree with the sentiment, but this is not how to do it.
>>>
>>> A release date is part of the SMBIOS spec, and the change below results
>>> in a malformed smbios table (stale p->release_date_str = 3; pointer)
>>>
>>> A better approach would be to derive the date from the commit date of
>>> HEAD, which would be consistent across rebuilds.
>> Except that this information may not be available, and isn't really
>> relevant. Instead I'd suggest using the source time stamp of
>> smbios.c, as that's really the (almost) only thing controlling the
>> data presented to the guest.
>>
>> Or maybe - considering that git checkouts use the current time
>> for file timestamps rather than the last modification time - use
>> HEAD's if available, and fall back to smbios.c's otherwise.
> 
> Both good points.  That sounds like the best approach.

Or actually, rather than HEAD's time stamp that of the most
recent commit touching smbios.c.

Jan

Re: [PATCH 1/2] hvmloader: remove timestamp from smbios

[Olaf Hering]

On Tue, Feb 03, Andrew Cooper wrote:

> A release date is part of the SMBIOS spec, and the change below results
> in a malformed smbios table (stale p->release_date_str = 3; pointer)

Good point. Thanks for review. Do you have a pointer to that spec?

> A better approach would be to derive the date from the commit date of
> HEAD, which would be consistent across rebuilds.

Releases dont have a HEAD. I will adjust the patch.

Olaf

Re: [PATCH 2/2] hvmloader: remove timestamp from vgabios

[Olaf Hering]

On Tue, Feb 03, Ian Campbell wrote:

> Olof, you just removed the nul terminator used there AFAICT. At best
> this means that the vgabios_copyright is printed twice.

My bad. Will resend. Thanks for the review.

Olaf

Re: [PATCH 1/2] hvmloader: remove timestamp from smbios

[Jan Beulich]

>>> On 03.02.15 at 19:41, <olaf@aepfle.de> wrote:
> On Tue, Feb 03, Andrew Cooper wrote:
> 
>> A release date is part of the SMBIOS spec, and the change below results
>> in a malformed smbios table (stale p->release_date_str = 3; pointer)
> 
> Good point. Thanks for review. Do you have a pointer to that spec?

http://dmtf.org/sites/default/files/standards/documents/DSP0134_2.8.0.pdf
(in particular section 7.1 and table 5)

Jan

Re: [PATCH 1/2] hvmloader: remove timestamp from smbios

[Olaf Hering]

On Wed, Feb 04, Jan Beulich wrote:

> http://dmtf.org/sites/default/files/standards/documents/DSP0134_2.8.0.pdf
> (in particular section 7.1 and table 5)

"The date string, if supplied, is in either mm/dd/yy or mm/dd/yyyy format."

To me it sounds like an optional thing. An empty string is as
informative as any other random date. 

But before the bikeshedding starts I think its best to turn the
assignment in the Makefiles from "VAR = VAL" into "VAR ?= VAL". This
preserves existing practice and gives us the opportunity to pass
01/01/1970 as fixed VAL.

Olaf

Re: [PATCH 1/2] hvmloader: remove timestamp from smbios

[Jan Beulich]

>>> On 04.02.15 at 09:42, <olaf@aepfle.de> wrote:
> On Wed, Feb 04, Jan Beulich wrote:
> 
>> http://dmtf.org/sites/default/files/standards/documents/DSP0134_2.8.0.pdf 
>> (in particular section 7.1 and table 5)
> 
> "The date string, if supplied, is in either mm/dd/yy or mm/dd/yyyy format."
> 
> To me it sounds like an optional thing. An empty string is as
> informative as any other random date. 

Please, again, be considerate of utilities looking for such information
even if the specification calls it optional. As pointed out before, there
are ways to achieve your goal of reproducible builds without dropping
bits of information.

> But before the bikeshedding starts I think its best to turn the
> assignment in the Makefiles from "VAR = VAL" into "VAR ?= VAL". This
> preserves existing practice and gives us the opportunity to pass
> 01/01/1970 as fixed VAL.

I don't think a BIOS date of 1970 will be taken well by various
components. ISTR some Linux versions blacklisting certain
functionality on too old BIOSes. Just grep for DMI_BIOS_DATE in
Linux'es arch/x86/pci/ for examples (there are further uses
elsewhere).

Jan