[SeaBIOS] Re: [RESEND PATCH] fw/pciinit: don't misalign large BARs

[Philippe Mathieu-Daudé]

On 15/4/24 08:38, Daniil Tatianin wrote:

Hey there!

Do we need more reviewed-bys before this can be merged?

Thanks!

On 4/11/24 10:51 PM, Daniil Tatianin wrote:

Previously we would unconditionally lower the alignment for large BARs
in case their alignment was greater than "pci_mem64_top >> 11", this
would make it impossible to use these devices by the kernel:
 [   13.821108] pci :9c:00.0: can't claim BAR 1 [mem 
0x660-0x67f 64bit pref]: no compatible bridge window
 [   13.823492] pci :9d:00.0: can't claim BAR 1 [mem 
0x640-0x65f 64bit pref]: no compatible bridge window
 [   13.824218] pci :9e:00.0: can't claim BAR 1 [mem 
0x620-0x63f 64bit pref]: no compatible bridge window
 [   13.828322] pci :8a:00.0: can't claim BAR 1 [mem 
0x6e0-0x6ff 64bit pref]: no compatible bridge window
 [   13.830691] pci :8b:00.0: can't claim BAR 1 [mem 
0x6c0-0x6df 64bit pref]: no compatible bridge window
 [   13.832218] pci :8c:00.0: can't claim BAR 1 [mem 
0x6a0-0x6bf 64bit pref]: no compatible bridge window


Fix it by only overwriting the alignment in case it's actually greater
than the desired by the BAR window.

Fixes: 96a8d130a8c ("be less conservative with the 64bit pci io window")
Signed-off-by: Daniil Tatianin 
Reviewed-by: Gerd Hoffmann 
---
  src/fw/pciinit.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH v4 5/6] qemu: log reservations in fw_cfg e820 table

[Philippe Mathieu-Daudé]

On 24/8/23 10:57, Gerd Hoffmann wrote:

With loglevel 1 (same we use for RAM entries),
so it is included in the firmware log by default.

Signed-off-by: Gerd Hoffmann 
---
  src/fw/paravirt.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Reviewed-by: Philippe Mathieu-Daudé 

___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH v3 1/3] esp-scsi: flush FIFO before sending SCSI command

[Philippe Mathieu-Daudé]

On 7/8/23 08:52, Mark Cave-Ayland wrote:

The ESP FIFO is used as a buffer for DMA requests and so isn't guaranteed to
be empty in the case of SCSI errors or a mixed DMA/non-DMA request. Flush the
FIFO before sending a SCSI command to guarantee that it is correctly
positioned at the start of the FIFO.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Paolo Bonzini 
---
  src/hw/esp-scsi.c | 4 
  1 file changed, 4 insertions(+)


Reviewed-by: Philippe Mathieu-Daudé 


___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH v2 1/2] esp-scsi: flush FIFO before sending SCSI command

[Philippe Mathieu-Daudé]

On 2/8/23 11:48, Mark Cave-Ayland wrote:

The ESP FIFO is used as a buffer for DMA requests and so isn't guaranteed to
be empty in the case of SCSI errors or a mixed DMA/non-DMA request. Flush the
FIFO before sending a SCSI command to guarantee that it is correctly
positioned at the start of the FIFO.

Signed-off-by: Mark Cave-Ayland 
---
  src/hw/esp-scsi.c | 4 
  1 file changed, 4 insertions(+)


Reviewed-by: Philippe Mathieu-Daudé 

___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH 1/2] esp-scsi: flush FIFO before sending SCSI command

[Philippe Mathieu-Daudé]

On 29/7/23 15:04, Mark Cave-Ayland wrote:

The ESP FIFO is used as a buffer for DMA requests and so isn't guaranteed to
be empty in the case of SCSI errors or a mixed DMA/non-DMA request. Flush the
FIFO before sending a SCSI command to guarantee that it is correctly
positioned at the start of the FIFO.

Signed-off-by: Mark Cave-Ayland 
---
  src/hw/esp-scsi.c | 4 
  1 file changed, 4 insertions(+)


Reviewed-by: Philippe Mathieu-Daudé 

___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: From header field rewrite used as git author (was: Re: [PATCH] ahci: handle TFES irq correctly)

[Philippe Mathieu-Daudé]

On 3/7/23 12:51, Paul Menzel wrote:

Just a note, Mailman’s rewrite of the `From` header field resulted in 
the author information below in the SeaBIOS git archive [1][2]:


 > Niklas Cassel via SeaBIOS 

No idea, how to solve this.


Thanks for reporting. Candidate patch using git .mailmap
(https://git-scm.com/docs/gitmailmap):

https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/thread/6QUBPHALQ6NURG2553FZJPDUO4I3SRGZ/

It would be better if some pre-apply script checks for this
locally or during merge.

Regards,

Phil.
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] [PATCH] git: Add .mailmap entries for patches claiming list authorship

[Philippe Mathieu-Daudé]

Similarly to QEMU's commit 3bd2608db7 ("maint: Add .mailmap
entries for patches claiming list authorship") [*], add a
.mailmap file to SeaBIOS repository to fix the following
commits:

- 1281e34 ("ahci: handle TFES irq correctly")
  authored by Niklas Cassel 

- cd93345 ("virtio-blk: Fix integer overflow for large max IO sizes")
  authored by Lukas Stockner 

- dc776a2 ("nvme: avoid use-after-free in nvme_controller_enable()")
  authored by Jan Beulich 

- 54082c8 ("nvme: fix missing newline on sq full print")
  authored by Alex Martens 

- b0d61ec ("usb-hid: Increase MAX_KBD_EVENT")
  authored by Stefan Ott 

[*] https://gitlab.com/qemu-project/qemu/-/commit/3bd2608db72

Fixes: 1281e34 ("ahci: handle TFES irq correctly")
Fixes: cd93345 ("virtio-blk: Fix integer overflow for large max IO sizes")
Fixes: dc776a2 ("nvme: avoid use-after-free in nvme_controller_enable()")
Fixes: 54082c8 ("nvme: fix missing newline on sq full print")
Fixes: b0d61ec ("usb-hid: Increase MAX_KBD_EVENT")
Reported-by: Paul Menzel 
Signed-off-by: Philippe Mathieu-Daudé 
---
 .mailmap | 23 +++
 1 file changed, 23 insertions(+)
 create mode 100644 .mailmap

diff --git a/.mailmap b/.mailmap
new file mode 100644
index 000..1411ae5
--- /dev/null
+++ b/.mailmap
@@ -0,0 +1,23 @@
+# This mailmap fixes up author names/addresses.
+#
+# If you are adding to this file consider if a similar change needs to
+# be made to contrib/gitdm/aliases. They are not however completely
+# analogous. .mailmap is concerned with fixing up damaged author
+# fields where as the gitdm equivalent is more concerned with making
+# sure multiple email addresses get mapped onto the same author.
+#
+# From man git-shortlog the forms are:
+#
+#  Proper Name 
+#   
+#  Proper Name  
+#  Proper Name  Commit Name 
+#
+
+# Translate a few commits where mailman rewrote the From: line due to
+# strict SPF, although we prefer to avoid adding more entries like that.
+Niklas Cassel  Niklas Cassel via SeaBIOS 

+Lukas Stockner  Lukas Stockner via SeaBIOS 

+Jan Beulich  Jan Beulich via SeaBIOS 
+Alex Martens  Alex Martens via SeaBIOS 

+Stefan Ott  Stefan Ott via SeaBIOS 
-- 
2.38.1

___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH 2/2] dsdt: add support for pnp ids as strings

[Philippe Mathieu-Daudé]

On 9/30/20 1:12 PM, Gerd Hoffmann wrote:
> PNP devices can be declared using eisaid encoding ...
> 
>   Name (_HID, EisaId ("PNP0103"))
> 
> ... or as string ...
> 
>   Name (_HID, "PNP0A06")
> 
> .. so lets support both variants.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  src/fw/dsdt_parser.c | 21 +
>  1 file changed, 13 insertions(+), 8 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH 1/2] output: add support for uppercase hex numbers

[Philippe Mathieu-Daudé]

On 9/30/20 1:12 PM, Gerd Hoffmann wrote:
> ... via "%X" format string.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  src/output.c | 49 +++--
>  1 file changed, 27 insertions(+), 22 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH] smbios: avoid integer overflow adding SMBIOS type 0 table

[Philippe Mathieu-Daudé]

On 9/8/20 5:21 PM, Daniel P. Berrangé wrote:
> SeaBIOS implements the SMBIOS 2.1 entry point which is limited to a
> maximum length of 0x. If the SMBIOS data received from QEMU is large
> enough, then adding the type 0 table will cause integer overflow. This
> results in fun behaviour such as a KVM crash, or hangs in SeaBIOS.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/fw/biostables.c | 14 ++
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/src/fw/biostables.c b/src/fw/biostables.c
> index 0c07833..794b5be 100644
> --- a/src/fw/biostables.c
> +++ b/src/fw/biostables.c
> @@ -462,10 +462,16 @@ smbios_romfile_setup(void)
>  /* common case: add our own type 0, with 3 strings and 4 '\0's */
>  u16 t0_len = sizeof(struct smbios_type_0) + strlen(BIOS_NAME) +
>   strlen(VERSION) + strlen(BIOS_DATE) + 4;
> -ep.structure_table_length += t0_len;
> -if (t0_len > ep.max_structure_size)
> -ep.max_structure_size = t0_len;
> -ep.number_of_structures++;
> +if (t0_len > (0x - ep.structure_table_length)) {
> +dprintf(1, "Insufficient space (%d bytes) to add SMBIOS type 0 
> table (%d bytes)\n",
> +0x - ep.structure_table_length, t0_len);
> +need_t0 = 0;
> +} else {
> +ep.structure_table_length += t0_len;
> +if (t0_len > ep.max_structure_size)
> +ep.max_structure_size = t0_len;
> +ep.number_of_structures++;
> +    }
>  }
>  
>  /* allocate final blob and record its address in the entry point */
> 

Reviewed-by: Philippe Mathieu-Daudé 
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH] hw/nvme: Increase `nvme_cmd_readwrite()` message log level from 3 to 5

[Philippe Mathieu-Daudé]

On 7/27/20 2:59 PM, Paul Menzel wrote:
> Currently, setting SeaBIOS debug level to 3, the log is filled with
> messages like below.
> 
> ns 1 read lba 11346288+8: 0
> ns 1 read lba 11346296+4: 0
> ns 1 read lba 11346300+4: 0
> ns 1 read lba 11346304+8: 0
> ns 1 read lba 11346312+8: 0
> ns 1 read lba 11346320+8: 0
> ns 1 read lba 11346328+8: 0
> ns 1 read lba 11346336+8: 0
> 
> With SeaBIOS as coreboot payload, this fills up the CBMEM console
> buffer.
> 
> So, increase the debug level to 5, so possible console buffer do not
> overflow.
> 
> Signed-off-by: Paul Menzel 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  src/hw/nvme.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/hw/nvme.c b/src/hw/nvme.c
> index 41f3b76..6a01204 100644
> --- a/src/hw/nvme.c
> +++ b/src/hw/nvme.c
> @@ -669,7 +669,7 @@ nvme_cmd_readwrite(struct nvme_namespace *ns, struct 
> disk_op_s *op, int write)
>  }
>  
>  res = nvme_io_readwrite(ns, op->lba + i, ns->dma_buffer, blocks, 
> write);
> -dprintf(3, "ns %u %s lba %llu+%u: %d\n", ns->ns_id, write ? "write"
> +dprintf(5, "ns %u %s lba %llu+%u: %d\n", ns->ns_id, write ? "write"
>: "read",
>  op->lba + i, blocks, res);
>  
> 
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH] cdrom: Demote `scsi_is_ready` return print to debug level

[Philippe Mathieu-Daudé]

On 5/18/20 6:58 PM, Paul Menzel wrote:

Printing the return value of `scsi_is_ready()` is a debug message, so
change the log level from 1 to 5.

 Booting from DVD/CD...
 Device reports MEDIUM NOT PRESENT
 scsi_is_ready returned -1
 Boot failed: Could not read from CDROM (code 0003)

Signed-off-by: Paul Menzel 
---
  src/cdrom.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/cdrom.c b/src/cdrom.c
index 577d69d..a77671a 100644
--- a/src/cdrom.c
+++ b/src/cdrom.c
@@ -142,7 +142,7 @@ cdrom_boot(struct drive_s *drive)
  
  int ret = scsi_is_ready();

  if (ret)
-dprintf(1, "scsi_is_ready returned %d\n", ret);
+dprintf(5, "scsi_is_ready returned %d\n", ret);
  
  // Read the Boot Record Volume Descriptor

  u8 buffer[CDROM_SECTOR_SIZE];



Reviewed-by: Philippe Mathieu-Daudé 
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH 3/3] qemu: check rtc presence before reading cpu count from cmos

[Philippe Mathieu-Daudé]

On 5/8/20 1:37 PM, Gerd Hoffmann wrote:

Read month register which should never have a value larger than 12.
In case the read returns 0xff assume the rtc isn't there.
Don't try to read the cpu count from cmos without rtc.

Signed-off-by: Gerd Hoffmann 


Reviewed-by: Philippe Mathieu-Daudé 


---
  src/fw/paravirt.c | 13 ++---
  1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
index 3465f97ec0b0..e76fa65d87a0 100644
--- a/src/fw/paravirt.c
+++ b/src/fw/paravirt.c
@@ -447,6 +447,11 @@ qemu_get_romfile_key(struct romfile_s *file)
  return qfile->select;
  }
  
+static int rtc_present(void)

+{
+return rtc_read(CMOS_RTC_MONTH) != 0xff;
+}
+
  u16
  qemu_get_present_cpus_count(void)
  {
@@ -454,9 +459,11 @@ qemu_get_present_cpus_count(void)
  if (qemu_cfg_enabled()) {
  qemu_cfg_read_entry(_count, QEMU_CFG_NB_CPUS, sizeof(smp_count));
  }
-u16 cmos_cpu_count = rtc_read(CMOS_BIOS_SMP_COUNT) + 1;
-if (smp_count < cmos_cpu_count) {
-smp_count = cmos_cpu_count;
+if (rtc_present()) {
+u16 cmos_cpu_count = rtc_read(CMOS_BIOS_SMP_COUNT) + 1;
+if (smp_count < cmos_cpu_count) {
+smp_count = cmos_cpu_count;
+}
  }
  return smp_count;
  }


___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH 3/3] kvm: add support for reading tsc frequency via cpuid.

[Philippe Mathieu-Daudé]

On 3/6/20 4:44 PM, Gerd Hoffmann wrote:

Signed-off-by: Gerd Hoffmann 
---
  src/fw/paravirt.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
index 4fcd8f570673..8b463af96c3e 100644
--- a/src/fw/paravirt.c
+++ b/src/fw/paravirt.c
@@ -67,6 +67,11 @@ static void kvm_detect(void)
  if (strcmp(signature, "KVMKVMKVM") == 0) {
  dprintf(1, "Running on KVM\n");
  PlatformRunningOn |= PF_KVM;
+if (eax >= KVM_CPUID_SIGNATURE + 0x10) {
+cpuid(KVM_CPUID_SIGNATURE + 0x10, , , , );
+dprintf(1, "kvm: have invtsc, freq %u kHz\n", eax);


What is "invtsc"? "tsc" alone maybe?

Reviewed-by: Philippe Mathieu-Daudé 


+tsctimer_setfreq(eax);
+}
  }
  }
  


___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH 1/3] timer: factor out tsctimer_configure()

[Philippe Mathieu-Daudé]

On 3/6/20 4:44 PM, Gerd Hoffmann wrote:

Factor out TimerKHz and ShiftTSC calculation to tsctimer_configure().

Signed-off-by: Gerd Hoffmann 
---
  src/hw/timer.c | 21 +
  1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/src/hw/timer.c b/src/hw/timer.c
index bdcb3bfca211..ce3d63cd75f6 100644
--- a/src/hw/timer.c
+++ b/src/hw/timer.c
@@ -58,6 +58,18 @@ u8 ShiftTSC VARFSEG;
   * Internal timer setup
   /
  
+static void

+tsctimer_configure(u64 t)
+{
+while (t >= (1<<24)) {
+ShiftTSC++;
+t = (t + 1) >> 1;
+}
+TimerKHz = DIV_ROUND_UP((u32)t, 1000 * PMTIMER_TO_PIT);
+TimerPort = 0;
+dprintf(1, "CPU Mhz=%u\n", (TimerKHz << ShiftTSC) / 1000);
+}
+
  #define CALIBRATE_COUNT 0x800   // Approx 1.7ms
  
  // Calibrate the CPU time-stamp-counter

@@ -87,14 +99,7 @@ tsctimer_setup(void)
  dprintf(6, "tsc calibrate start=%u end=%u diff=%u\n"
  , (u32)start, (u32)end, (u32)diff);
  u64 t = DIV_ROUND_UP(diff * PMTIMER_HZ, CALIBRATE_COUNT);
-while (t >= (1<<24)) {
-ShiftTSC++;
-t = (t + 1) >> 1;
-}
-TimerKHz = DIV_ROUND_UP((u32)t, 1000 * PMTIMER_TO_PIT);
-TimerPort = 0;
-
-dprintf(1, "CPU Mhz=%u\n", (TimerKHz << ShiftTSC) / 1000);
+tsctimer_configure(t);
  }
  
  // Setup internal timers.




Reviewed-by: Philippe Mathieu-Daudé 
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH] std/tcg: Replace zero-length array with flexible-array member

[Philippe Mathieu-Daudé]

On Tue, Mar 3, 2020 at 7:08 PM Philippe Mathieu-Daudé  wrote:
>
> Hello Paul,
>
> On 3/3/20 5:50 PM, Paul Menzel wrote:
> > Date: Tue, 3 Mar 2020 16:24:46 +0100
> >
> > GCC 10 gives the warnings below:
> >
> >  In file included from out/ccode32flat.o.tmp.c:54:
> >  ./src/tcgbios.c: In function 'tpm20_write_EfiSpecIdEventStruct':
> >  ./src/tcgbios.c:290:30: warning: array subscript '() + 
> > 4294967295' is outside the bounds of an interior zero-length array 'struct 
> > TCG_EfiSpecIdEventAlgorithmSize[0]' [-Wzero-length-bounds]
> >290 | event.hdr.digestSizes[count].algorithmId = 
> > be16_to_cpu(sel->hashAlg);
> >| ~^~~
> >  In file included from ./src/tcgbios.c:22,
> >   from out/ccode32flat.o.tmp.c:54:
> >  ./src/std/tcg.h:527:7: note: while referencing 'digestSizes'
> >527 | } digestSizes[0];
> >|   ^~~
> >  In file included from out/ccode32flat.o.tmp.c:54:
> >  ./src/tcgbios.c:291:30: warning: array subscript '() + 
> > 4294967295' is outside the bounds of an interior zero-length array 'struct 
> > TCG_EfiSpecIdEventAlgorithmSize[0]' [-Wzero-length-bounds]
> >291 | event.hdr.digestSizes[count].digestSize = hsize;
> >| ~^~~
> >  In file included from ./src/tcgbios.c:22,
> >   from out/ccode32flat.o.tmp.c:54:
> >  ./src/std/tcg.h:527:7: note: while referencing 'digestSizes'
> >527 | } digestSizes[0];
> >|   ^~~
> >
> > [Description copied from Gustavo A. R. Silva 
> > from his Linux kernel commits.]
> >
> > The current codebase makes use of the zero-length array language
> > extension to the C90 standard, but the preferred mechanism to declare
> > variable-length types such as these ones is a flexible array
> > member [1], introduced in C99:
> >
> >  struct foo {
> >  int stuff;
> >  struct boo array[];
> >  };
> >
> > By making use of the mechanism above, we will get a compiler warning
> > in case the flexible array does not occur last in the structure, which
> > will help us prevent some kind of undefined behavior bugs from being
> > inadvertently introduced[3] to the codebase from now on.
>
> Assuming this comment is also part of the Linux kernel commit, can we
> drop the '[3]' marker?

Assuming the maintainer can do this trivial change:
Reviewed-by: Philippe Mathieu-Daudé 

>
> >
> > Also, notice that, dynamic memory allocations won't be affected by
> > this change:
> >
> > "Flexible array members have incomplete type, and so the sizeof operator
> > may not be applied. As a quirk of the original implementation of
> > zero-length arrays, sizeof evaluates to zero."[1]
> >
> > This issue was found with the help of Coccinelle.
> >
> > [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> >
> > Signed-off-by: Paul Menzel 
> > ---
> >   src/std/tcg.h | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/std/tcg.h b/src/std/tcg.h
> > index 1cc1c92..1c9eeb4 100644
> > --- a/src/std/tcg.h
> > +++ b/src/std/tcg.h
> > @@ -524,7 +524,7 @@ struct TCG_EfiSpecIdEventStruct {
> >   struct TCG_EfiSpecIdEventAlgorithmSize {
> >   u16 algorithmId;
> >   u16 digestSize;
> > -} digestSizes[0];
> > +} digestSizes[];
> >   /*
> >   u8 vendorInfoSize;
> >   u8 vendorInfo[0];
> >
> >
> > ___
> > SeaBIOS mailing list -- seabios@seabios.org
> > To unsubscribe send an email to seabios-le...@seabios.org
> >
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH] std/tcg: Replace zero-length array with flexible-array member

[Philippe Mathieu-Daudé]

Hello Paul,

On 3/3/20 5:50 PM, Paul Menzel wrote:

Date: Tue, 3 Mar 2020 16:24:46 +0100

GCC 10 gives the warnings below:

 In file included from out/ccode32flat.o.tmp.c:54:
 ./src/tcgbios.c: In function 'tpm20_write_EfiSpecIdEventStruct':
 ./src/tcgbios.c:290:30: warning: array subscript '() + 
4294967295' is outside the bounds of an interior zero-length array 'struct 
TCG_EfiSpecIdEventAlgorithmSize[0]' [-Wzero-length-bounds]
   290 | event.hdr.digestSizes[count].algorithmId = 
be16_to_cpu(sel->hashAlg);
   | ~^~~
 In file included from ./src/tcgbios.c:22,
  from out/ccode32flat.o.tmp.c:54:
 ./src/std/tcg.h:527:7: note: while referencing 'digestSizes'
   527 | } digestSizes[0];
   |   ^~~
 In file included from out/ccode32flat.o.tmp.c:54:
 ./src/tcgbios.c:291:30: warning: array subscript '() + 
4294967295' is outside the bounds of an interior zero-length array 'struct 
TCG_EfiSpecIdEventAlgorithmSize[0]' [-Wzero-length-bounds]
   291 | event.hdr.digestSizes[count].digestSize = hsize;
   | ~^~~
 In file included from ./src/tcgbios.c:22,
  from out/ccode32flat.o.tmp.c:54:
 ./src/std/tcg.h:527:7: note: while referencing 'digestSizes'
   527 | } digestSizes[0];
   |   ^~~

[Description copied from Gustavo A. R. Silva 
from his Linux kernel commits.]

The current codebase makes use of the zero-length array language
extension to the C90 standard, but the preferred mechanism to declare
variable-length types such as these ones is a flexible array
member [1], introduced in C99:

 struct foo {
 int stuff;
 struct boo array[];
 };

By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on.


Assuming this comment is also part of the Linux kernel commit, can we 
drop the '[3]' marker?




Also, notice that, dynamic memory allocations won't be affected by
this change:

"Flexible array members have incomplete type, and so the sizeof operator
may not be applied. As a quirk of the original implementation of
zero-length arrays, sizeof evaluates to zero."[1]

This issue was found with the help of Coccinelle.

[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html

Signed-off-by: Paul Menzel 
---
  src/std/tcg.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/std/tcg.h b/src/std/tcg.h
index 1cc1c92..1c9eeb4 100644
--- a/src/std/tcg.h
+++ b/src/std/tcg.h
@@ -524,7 +524,7 @@ struct TCG_EfiSpecIdEventStruct {
  struct TCG_EfiSpecIdEventAlgorithmSize {
  u16 algorithmId;
  u16 digestSize;
-} digestSizes[0];
+} digestSizes[];
  /*
  u8 vendorInfoSize;
  u8 vendorInfo[0];


___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org


___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH 3/3] nvme: skip initializing non-bootable devices

[Philippe Mathieu-Daudé]

On 1/14/20 10:25 AM, Gerd Hoffmann wrote:

Check NVMe devices whenever they are bootable,
skip initialization in case they are not.

Signed-off-by: Gerd Hoffmann 
---
  src/hw/nvme.c | 7 +++
  1 file changed, 7 insertions(+)

diff --git a/src/hw/nvme.c b/src/hw/nvme.c
index 2e3aa38682c4..41f3b768528e 100644
--- a/src/hw/nvme.c
+++ b/src/hw/nvme.c
@@ -586,8 +586,15 @@ nvme_controller_enable(struct nvme_ctrl *ctrl)
  static void
  nvme_controller_setup(void *opaque)
  {
+u8 skip_nonbootable = is_bootprio_strict();
  struct pci_device *pci = opaque;
  
+if (skip_nonbootable && bootprio_find_pci_device(pci) < 0) {

+dprintf(1, "skipping init of a non-bootable NVMe at %pP\n",
+pci);
+goto err;
+}
+
  struct nvme_reg volatile *reg = pci_enable_membar(pci, 
PCI_BASE_ADDRESS_0);
  if (!reg)
  return;



Reviewed-by: Philippe Mathieu-Daudé 
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH 2/3] virtio-scsi: skip initializing non-bootable devices

[Philippe Mathieu-Daudé]

On 1/14/20 10:25 AM, Gerd Hoffmann wrote:

Check each disk attached to a virtio-scsi device whenever
it is bootable and skip initialization in case it isn't.

Signed-off-by: Gerd Hoffmann 


Reviewed-by: Philippe Mathieu-Daudé 


---
  src/hw/virtio-scsi.c | 11 ++-
  1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/hw/virtio-scsi.c b/src/hw/virtio-scsi.c
index a27bdc1cfbb7..a5332848b8c8 100644
--- a/src/hw/virtio-scsi.c
+++ b/src/hw/virtio-scsi.c
@@ -111,8 +111,18 @@ virtio_scsi_init_lun(struct virtio_lun_s *vlun, struct 
pci_device *pci,
  static int
  virtio_scsi_add_lun(u32 lun, struct drive_s *tmpl_drv)
  {
+u8 skip_nonbootable = is_bootprio_strict();
  struct virtio_lun_s *tmpl_vlun =
  container_of(tmpl_drv, struct virtio_lun_s, drive);
+int prio = bootprio_find_scsi_device(tmpl_vlun->pci, tmpl_vlun->target, 
tmpl_vlun->lun);
+
+if (skip_nonbootable && prio < 0) {
+dprintf(1, "skipping init of a non-bootable virtio-scsi dev at %pP,"
+" target %d, lun %d\n",
+tmpl_vlun->pci, tmpl_vlun->target, tmpl_vlun->lun);
+return -1;
+}
+
  struct virtio_lun_s *vlun = malloc_low(sizeof(*vlun));
  if (!vlun) {
  warn_noalloc();
@@ -123,7 +133,6 @@ virtio_scsi_add_lun(u32 lun, struct drive_s *tmpl_drv)
  
  boot_lchs_find_scsi_device(vlun->pci, vlun->target, vlun->lun,

 &(vlun->drive.lchs));
-int prio = bootprio_find_scsi_device(vlun->pci, vlun->target, vlun->lun);
  int ret = scsi_drive_setup(>drive, "virtio-scsi", prio);
  if (ret)
  goto fail;


___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH 1/3] boot: cache HALT priority

[Philippe Mathieu-Daudé]

On 1/14/20 10:25 AM, Gerd Hoffmann wrote:

Call find_prio("HALT") only once, on first is_bootprio_strict() call.
Store the result in a variable and reuse it on subsequent calls.

Signed-off-by: Gerd Hoffmann 


Reviewed-by: Philippe Mathieu-Daudé 


---
  src/boot.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/boot.c b/src/boot.c
index 5182ab426b9f..afeb36a3319a 100644
--- a/src/boot.c
+++ b/src/boot.c
@@ -297,7 +297,11 @@ find_prio(const char *glob)
  
  u8 is_bootprio_strict(void)

  {
-return find_prio("HALT") >= 0;
+static int prio_halt = -2;
+
+if (prio_halt == -2)
+prio_halt = find_prio("HALT");
+return prio_halt >= 0;
  }
  
  int bootprio_find_pci_device(struct pci_device *pci)



___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH] ahci: zero-initialize port struct

[Philippe Mathieu-Daudé]

Hi Sam,

On 11/13/19 4:03 PM, Sam Eiderman wrote:

Hi,

Does this fix a bug that actually happened?

I just noticed that in my lchs patches I assumed that lchs struct is
zeroed out in all devices (not only ahci):

9caa19be0e53 (geometry: Apply LCHS values for boot devices)

Seems like this is not the case but why only ahci is affected?

The list of devices is at least:

 * ata
 * ahci
 * scsi
 * esp
 * lsi
 * megasas
 * mpt
 * pvscsi
 * virtio
 * virtio-blk

As specified in the commit message.

Also Gerd it seems that my lchs patches were not committed in the
latest submitted version (v4)!!!
The ABI of the fw config key is completely broken.


What do you mean? Can you be more specific?
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH 2/2] tcgbios: Check for enough bytes returned from TPM2_GetCapability

[Philippe Mathieu-Daudé]

On 11/6/19 10:36 PM, Stefan Berger wrote:

When querying a TPM 2.0 for its PCRs, make sure that we get enough bytes
from it in a response that did not indicate a failure. Basically we are
defending against a TPM 2.0 sending responses that are not compliant to
the specs.

Signed-off-by: Stefan Berger 
---
  src/tcgbios.c | 13 +++--
  1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/src/tcgbios.c b/src/tcgbios.c
index 2e503f9..95c1e94 100644
--- a/src/tcgbios.c
+++ b/src/tcgbios.c
@@ -481,8 +481,17 @@ tpm20_get_pcrbanks(void)
  if (ret)
  return ret;
  
-u32 size = be32_to_cpu(trg->hdr.totlen) -

-   offsetof(struct tpm2_res_getcapability, data);
+/* defend against (broken) TPM sending packets that are too short */
+u32 resplen = be32_to_cpu(trg->hdr.totlen);
+if (resplen <= offsetof(struct tpm2_res_getcapability, data))
+return -1;
+
+u32 size = resplen - offsetof(struct tpm2_res_getcapability, data);
+/* we need a valid tpml_pcr_selection up to and including sizeOfSelect */
+if (size < offsetof(struct tpml_pcr_selection, selections) +
+   offsetof(struct tpms_pcr_selection, pcrSelect))
+return -1;
+
  tpm20_pcr_selection = malloc_high(size);
  if (tpm20_pcr_selection) {
  memcpy(tpm20_pcr_selection, >data, size);



Reviewed-by: Philippe Mathieu-Daudé 
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH 1/2] tpm: Require a response to have minimum size of a valid response header

[Philippe Mathieu-Daudé]

On 11/6/19 10:35 PM, Stefan Berger wrote:

Defend against a broken TPM 1.2 or TPM 2.0 that doesn't send at least
a full response header in the response but less than 10 bytes.

Signed-off-by: Stefan Berger 
---
  src/hw/tpm_drivers.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/hw/tpm_drivers.c b/src/hw/tpm_drivers.c
index e4770b3..2b5753c 100644
--- a/src/hw/tpm_drivers.c
+++ b/src/hw/tpm_drivers.c
@@ -620,7 +620,8 @@ tpmhw_transmit(u8 locty, struct tpm_req_header *req,
  return -1;
  
  irc = td->readresp(respbuffer, respbufferlen);

-if (irc != 0)
+if (irc != 0 ||
+*respbufferlen < sizeof(struct tpm_rsp_header))
  return -1;
  
  td->ready();




Reviewed-by: Philippe Mathieu-Daudé 
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH 3/5] lodepng: reduce warnings

[Philippe Mathieu-Daudé]

Hi Daniel,

On 10/29/19 6:42 PM, cyrev...@googlemail.com wrote:

From: Daniel Maslowski 


Can you explain the rational of this patch?
In particular why use signed vs unsigned.

Thanks,

Phil.


Signed-off-by: Daniel Maslowski 
---
  src/lodepng.c | 12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/lodepng.c b/src/lodepng.c
index d9c8443..490fc8e 100644
--- a/src/lodepng.c
+++ b/src/lodepng.c
@@ -1925,7 +1925,7 @@ static void Adam7_getpassvalues(unsigned passw[7], 
unsigned passh[7], size_t fil
  /* // 
*/
  
  /*read the information from the header and store it in the LodePNGInfo. return value is error*/

-unsigned lodepng_inspect(unsigned* w, unsigned* h, LodePNGState* state,
+unsigned lodepng_inspect(int* w, int* h, LodePNGState* state,
   const unsigned char* in, size_t insize) {
unsigned width, height;
LodePNGInfo* info = >info_png;
@@ -2275,7 +2275,7 @@ unsigned lodepng_inspect_chunk(LodePNGState* state, 
size_t pos,
  }
  
  /*read a PNG, the result will be in the same color type as the PNG (hence "generic")*/

-static void decodeGeneric(unsigned char** out, unsigned* w, unsigned* h,
+static void decodeGeneric(unsigned char** out, int* w, int* h,
LodePNGState* state,
const unsigned char* in, size_t insize) {
unsigned char IEND = 0;
@@ -2403,7 +2403,7 @@ static void decodeGeneric(unsigned char** out, unsigned* 
w, unsigned* h,
ucvector_cleanup();
  }
  
-unsigned lodepng_decode(unsigned char** out, unsigned* w, unsigned* h,

+unsigned lodepng_decode(unsigned char** out, int* w, int* h,
  LodePNGState* state,
  const unsigned char* in, size_t insize) {
*out = 0;
@@ -2441,7 +2441,7 @@ unsigned lodepng_decode(unsigned char** out, unsigned* w, 
unsigned* h,
return state->error;
  }
  
-unsigned lodepng_decode_memory(unsigned char** out, unsigned* w, unsigned* h, const unsigned char* in,

+unsigned lodepng_decode_memory(unsigned char** out, int* w, int* h, const 
unsigned char* in,
 size_t insize, LodePNGColorType colortype, 
unsigned bitdepth) {
unsigned error;
LodePNGState state;
@@ -2453,11 +2453,11 @@ unsigned lodepng_decode_memory(unsigned char** out, 
unsigned* w, unsigned* h, co
return error;
  }
  
-unsigned lodepng_decode32(unsigned char** out, unsigned* w, unsigned* h, const unsigned char* in, size_t insize) {

+unsigned lodepng_decode32(unsigned char** out, int* w, int* h, const unsigned 
char* in, size_t insize) {
return lodepng_decode_memory(out, w, h, in, insize, LCT_RGBA, 8);
  }
  
-unsigned lodepng_decode24(unsigned char** out, unsigned* w, unsigned* h, const unsigned char* in, size_t insize) {

+unsigned lodepng_decode24(unsigned char** out, int* w, int* h, const unsigned 
char* in, size_t insize) {
return lodepng_decode_memory(out, w, h, in, insize, LCT_RGB, 8);
  }
  


___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH 0/3] copyright & license updates

[Philippe Mathieu-Daudé]

On 10/23/19 7:50 AM, Gerd Hoffmann wrote:



Gerd Hoffmann (3):
   add copyright and license to bochsdisplay.c
   add copyright and license to ramfb.c
   add license to cp437.c

  src/cp437.c   | 2 ++
  vgasrc/bochsdisplay.c | 6 ++
  vgasrc/ramfb.c| 6 ++
  3 files changed, 14 insertions(+)


FWIW =)
Reviewed-by: Philippe Mathieu-Daudé 
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH v8 7/8] bootdevice: FW_CFG interface for LCHS values

[Philippe Mathieu-Daudé]

On 10/16/19 6:41 PM, Sam Eiderman wrote:

From: Sam Eiderman 

Using fw_cfg, supply logical CHS values directly from QEMU to the BIOS.

Non-standard logical geometries break under QEMU.

A virtual disk which contains an operating system which depends on
logical geometries (consistent values being reported from BIOS INT13
AH=08) will most likely break under QEMU/SeaBIOS if it has non-standard
logical geometries - for example 56 SPT (sectors per track).
No matter what QEMU will report - SeaBIOS, for large enough disks - will
use LBA translation, which will report 63 SPT instead.

In addition we cannot force SeaBIOS to rely on physical geometries at
all. A virtio-blk-pci virtual disk with 255 phyiscal heads cannot
report more than 16 physical heads when moved to an IDE controller,
since the ATA spec allows a maximum of 16 heads - this is an artifact of
virtualization.

By supplying the logical geometries directly we are able to support such
"exotic" disks.

We serialize this information in a similar way to the "bootorder"
interface.
The new fw_cfg entry is "bios-geometry".

Reviewed-by: Karl Heubaum 
Reviewed-by: Arbel Moshe 
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Sam Eiderman 
Signed-off-by: Sam Eiderman 
---
  bootdevice.c| 31 +++
  hw/nvram/fw_cfg.c   | 14 +++---
  include/sysemu/sysemu.h |  1 +
  3 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/bootdevice.c b/bootdevice.c
index 2cf6b37c57..03aaffcc8d 100644
--- a/bootdevice.c
+++ b/bootdevice.c
@@ -405,3 +405,34 @@ void del_boot_device_lchs(DeviceState *dev, const char 
*suffix)
  }
  }
  }
+
+char *get_boot_devices_lchs_list(size_t *size)
+{
+FWLCHSEntry *i;
+size_t total = 0;
+char *list = NULL;
+
+QTAILQ_FOREACH(i, _lchs, link) {
+char *bootpath;
+char *chs_string;
+size_t len;
+
+bootpath = get_boot_device_path(i->dev, false, i->suffix);
+chs_string = g_strdup_printf("%s %" PRIu32 " %" PRIu32 " %" PRIu32,
+ bootpath, i->lcyls, i->lheads, i->lsecs);
+
+if (total) {
+list[total - 1] = '\n';
+}
+len = strlen(chs_string) + 1;
+list = g_realloc(list, total + len);
+memcpy([total], chs_string, len);
+total += len;
+g_free(chs_string);
+g_free(bootpath);
+}
+


Hmm maybe assert(size != NULL) or if(size) {


+*size = total;


} or simply document "@size must not be NULL" in the declaration.

Can be a follow-up cleaning patch.

Regardless:
Tested-by: Philippe Mathieu-Daudé 


+
+return list;
+}
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 7dc3ac378e..18aff658c0 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -920,13 +920,21 @@ void *fw_cfg_modify_file(FWCfgState *s, const char 
*filename,
  
  static void fw_cfg_machine_reset(void *opaque)

  {
+MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
+FWCfgState *s = opaque;
  void *ptr;
  size_t len;
-FWCfgState *s = opaque;
-char *bootindex = get_boot_devices_list();
+char *buf;
  
-ptr = fw_cfg_modify_file(s, "bootorder", (uint8_t *)bootindex, len);

+buf = get_boot_devices_list();
+ptr = fw_cfg_modify_file(s, "bootorder", (uint8_t *)buf, len);
  g_free(ptr);
+
+if (!mc->legacy_fw_cfg_order) {
+buf = get_boot_devices_lchs_list();
+ptr = fw_cfg_modify_file(s, "bios-geometry", (uint8_t *)buf, len);
+g_free(ptr);
+}
  }
  
  static void fw_cfg_machine_ready(struct Notifier *n, void *data)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 5bc5c79cbc..80c57fdc4e 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -106,6 +106,7 @@ void validate_bootdevices(const char *devices, Error 
**errp);
  void add_boot_device_lchs(DeviceState *dev, const char *suffix,
uint32_t lcyls, uint32_t lheads, uint32_t lsecs);
  void del_boot_device_lchs(DeviceState *dev, const char *suffix);
+char *get_boot_devices_lchs_list(size_t *size);
  
  /* handler to set the boot_device order for a specific type of MachineClass */

  typedef void QEMUBootSetHandler(void *opaque, const char *boot_order,



Tested-by: Philippe Mathieu-Daudé 
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH v8 6/8] bootdevice: Refactor get_boot_devices_list

[Philippe Mathieu-Daudé]

On 10/16/19 6:41 PM, Sam Eiderman wrote:

From: Sam Eiderman 

Move device name construction to a separate function.

We will reuse this function in the following commit to pass logical CHS
parameters through fw_cfg much like we currently pass bootindex.

Reviewed-by: Karl Heubaum 
Reviewed-by: Arbel Moshe 
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Sam Eiderman 
Signed-off-by: Sam Eiderman 
---
  bootdevice.c | 61 +---
  1 file changed, 34 insertions(+), 27 deletions(-)

diff --git a/bootdevice.c b/bootdevice.c
index bc5e1c2de4..2cf6b37c57 100644
--- a/bootdevice.c
+++ b/bootdevice.c
@@ -202,6 +202,39 @@ DeviceState *get_boot_device(uint32_t position)
  return res;
  }
  
+static char *get_boot_device_path(DeviceState *dev, bool ignore_suffixes,

+  const char *suffix)
+{
+char *devpath = NULL, *s = NULL, *d, *bootpath;
+
+if (dev) {
+devpath = qdev_get_fw_dev_path(dev);
+assert(devpath);
+}
+
+if (!ignore_suffixes) {
+if (dev) {
+d = qdev_get_own_fw_dev_path_from_handler(dev->parent_bus, dev);
+if (d) {
+assert(!suffix);
+s = d;
+} else {
+s = g_strdup(suffix);
+}
+} else {
+s = g_strdup(suffix);
+}
+}
+
+bootpath = g_strdup_printf("%s%s",
+   devpath ? devpath : "",
+   s ? s : "");
+g_free(devpath);
+g_free(s);
+
+return bootpath;
+}
+
  /*
   * This function returns null terminated string that consist of new line
   * separated device paths.
@@ -218,36 +251,10 @@ char *get_boot_devices_list(size_t *size)
  bool ignore_suffixes = mc->ignore_boot_device_suffixes;
  
  QTAILQ_FOREACH(i, _boot_order, link) {

-char *devpath = NULL,  *suffix = NULL;
  char *bootpath;
-char *d;
  size_t len;
  
-if (i->dev) {

-devpath = qdev_get_fw_dev_path(i->dev);
-assert(devpath);
-}
-
-if (!ignore_suffixes) {
-if (i->dev) {
-d = qdev_get_own_fw_dev_path_from_handler(i->dev->parent_bus,
-  i->dev);
-if (d) {
-assert(!i->suffix);
-suffix = d;
-} else {
-suffix = g_strdup(i->suffix);
-}
-} else {
-suffix = g_strdup(i->suffix);
-}
-}
-
-bootpath = g_strdup_printf("%s%s",
-   devpath ? devpath : "",
-   suffix ? suffix : "");
-g_free(devpath);
-g_free(suffix);
+bootpath = get_boot_device_path(i->dev, ignore_suffixes, i->suffix);
  
  if (total) {

  list[total-1] = '\n';



Tested-by: Philippe Mathieu-Daudé 
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH v8 5/8] bootdevice: Gather LCHS from all relevant devices

[Philippe Mathieu-Daudé]

On 10/16/19 6:41 PM, Sam Eiderman wrote:

From: Sam Eiderman 

Relevant devices are:
 * ide-hd (and ide-cd, ide-drive)
 * scsi-hd (and scsi-cd, scsi-disk, scsi-block)
 * virtio-blk-pci

We do not call del_boot_device_lchs() for ide-* since we don't need to -
IDE block devices do not support unplugging.

Reviewed-by: Karl Heubaum 
Reviewed-by: Arbel Moshe 
Signed-off-by: Sam Eiderman 
Signed-off-by: Sam Eiderman 
---
  hw/block/virtio-blk.c |  6 ++
  hw/ide/qdev.c |  5 +
  hw/scsi/scsi-disk.c   | 12 
  3 files changed, 23 insertions(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index ed2ddebd2b..c56e905f80 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1200,6 +1200,11 @@ static void virtio_blk_device_realize(DeviceState *dev, 
Error **errp)
  blk_set_guest_block_size(s->blk, s->conf.conf.logical_block_size);
  
  blk_iostatus_enable(s->blk);

+
+add_boot_device_lchs(dev, "/disk@0,0",
+ conf->conf.lcyls,
+ conf->conf.lheads,
+ conf->conf.lsecs);
  }
  
  static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp)

@@ -1207,6 +1212,7 @@ static void virtio_blk_device_unrealize(DeviceState *dev, 
Error **errp)
  VirtIODevice *vdev = VIRTIO_DEVICE(dev);
  VirtIOBlock *s = VIRTIO_BLK(dev);
  
+del_boot_device_lchs(dev, "/disk@0,0");

  virtio_blk_data_plane_destroy(s->dataplane);
  s->dataplane = NULL;
  qemu_del_vm_change_state_handler(s->change);
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 6dd219944f..2ffd387a73 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -220,6 +220,11 @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind 
kind, Error **errp)
  
  add_boot_device_path(dev->conf.bootindex, >qdev,

   dev->unit ? "/disk@1" : "/disk@0");
+
+add_boot_device_lchs(>qdev, dev->unit ? "/disk@1" : "/disk@0",
+ dev->conf.lcyls,
+ dev->conf.lheads,
+ dev->conf.lsecs);
  }
  
  static void ide_dev_get_bootindex(Object *obj, Visitor *v, const char *name,

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 68b1675fd9..07fb5ebdf1 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -35,6 +35,7 @@
  #include "hw/block/block.h"
  #include "hw/qdev-properties.h"
  #include "sysemu/dma.h"
+#include "sysemu/sysemu.h"
  #include "qemu/cutils.h"
  #include "trace.h"
  
@@ -2414,6 +2415,16 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)

  blk_set_guest_block_size(s->qdev.conf.blk, s->qdev.blocksize);
  
  blk_iostatus_enable(s->qdev.conf.blk);

+
+add_boot_device_lchs(>qdev, NULL,
+ dev->conf.lcyls,
+ dev->conf.lheads,
+ dev->conf.lsecs);
+}
+
+static void scsi_unrealize(SCSIDevice *dev, Error **errp)
+{
+del_boot_device_lchs(>qdev, NULL);
  }
  
  static void scsi_hd_realize(SCSIDevice *dev, Error **errp)

@@ -3018,6 +3029,7 @@ static void scsi_hd_class_initfn(ObjectClass *klass, void 
*data)
  SCSIDeviceClass *sc = SCSI_DEVICE_CLASS(klass);
  
  sc->realize  = scsi_hd_realize;

+sc->unrealize= scsi_unrealize;
  sc->alloc_req    = scsi_new_request;
  sc->unit_attention_reported = scsi_disk_unit_attention_reported;
  dc->desc = "virtual SCSI disk";



Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH v8 4/8] scsi: Propagate unrealize() callback to scsi-hd

[Philippe Mathieu-Daudé]

On 10/16/19 6:41 PM, Sam Eiderman wrote:

From: Sam Eiderman 

We will need to add LCHS removal logic to scsi-hd's unrealize() in the
next commit.

Reviewed-by: Karl Heubaum 
Reviewed-by: Arbel Moshe 
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Sam Eiderman 
Signed-off-by: Sam Eiderman 
---
  hw/scsi/scsi-bus.c | 16 
  include/hw/scsi/scsi.h |  1 +
  2 files changed, 17 insertions(+)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index bccb7cc4c6..359d50d6d0 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -59,6 +59,14 @@ static void scsi_device_realize(SCSIDevice *s, Error **errp)
  }
  }
  
+static void scsi_device_unrealize(SCSIDevice *s, Error **errp)

+{
+SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(s);
+if (sc->unrealize) {
+sc->unrealize(s, errp);
+}
+}
+
  int scsi_bus_parse_cdb(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf,
 void *hba_private)
  {
@@ -217,12 +225,20 @@ static void scsi_qdev_realize(DeviceState *qdev, Error 
**errp)
  static void scsi_qdev_unrealize(DeviceState *qdev, Error **errp)
  {
  SCSIDevice *dev = SCSI_DEVICE(qdev);
+Error *local_err = NULL;
  
  if (dev->vmsentry) {

  qemu_del_vm_change_state_handler(dev->vmsentry);
  }
  
  scsi_device_purge_requests(dev, SENSE_CODE(NO_SENSE));

+
+scsi_device_unrealize(dev, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
+
  blockdev_mark_auto_del(dev->conf.blk);
  }
  
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h

index d77a92361b..332ef602f4 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -59,6 +59,7 @@ struct SCSIRequest {
  typedef struct SCSIDeviceClass {
  DeviceClass parent_class;
  void (*realize)(SCSIDevice *dev, Error **errp);
+void (*unrealize)(SCSIDevice *dev, Error **errp);
  int (*parse_cdb)(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf,
   void *hba_private);
  SCSIRequest *(*alloc_req)(SCSIDevice *s, uint32_t tag, uint32_t lun,



Tested-by: Philippe Mathieu-Daudé 
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH v8 3/8] bootdevice: Add interface to gather LCHS

[Philippe Mathieu-Daudé]

On 10/16/19 6:41 PM, Sam Eiderman wrote:

From: Sam Eiderman 

Add an interface to provide direct logical CHS values for boot devices.
We will use this interface in the next commits.

Reviewed-by: Karl Heubaum 
Reviewed-by: Arbel Moshe 
Signed-off-by: Sam Eiderman 
Signed-off-by: Sam Eiderman 
---
  bootdevice.c| 55 +
  include/sysemu/sysemu.h |  3 +++
  2 files changed, 58 insertions(+)

diff --git a/bootdevice.c b/bootdevice.c
index 1d225202f9..bc5e1c2de4 100644
--- a/bootdevice.c
+++ b/bootdevice.c
@@ -343,3 +343,58 @@ void device_add_bootindex_property(Object *obj, int32_t 
*bootindex,
  /* initialize devices' bootindex property to -1 */
  object_property_set_int(obj, -1, name, NULL);
  }
+
+typedef struct FWLCHSEntry FWLCHSEntry;
+
+struct FWLCHSEntry {
+QTAILQ_ENTRY(FWLCHSEntry) link;
+DeviceState *dev;
+char *suffix;
+uint32_t lcyls;
+uint32_t lheads;
+uint32_t lsecs;
+};
+
+static QTAILQ_HEAD(, FWLCHSEntry) fw_lchs =
+QTAILQ_HEAD_INITIALIZER(fw_lchs);
+
+void add_boot_device_lchs(DeviceState *dev, const char *suffix,
+  uint32_t lcyls, uint32_t lheads, uint32_t lsecs)
+{
+FWLCHSEntry *node;
+
+if (!lcyls && !lheads && !lsecs) {
+return;
+}
+
+assert(dev != NULL || suffix != NULL);
+
+node = g_malloc0(sizeof(FWLCHSEntry));
+node->suffix = g_strdup(suffix);
+node->dev = dev;
+node->lcyls = lcyls;
+node->lheads = lheads;
+node->lsecs = lsecs;
+
+QTAILQ_INSERT_TAIL(_lchs, node, link);
+}
+
+void del_boot_device_lchs(DeviceState *dev, const char *suffix)
+{
+FWLCHSEntry *i;
+
+if (dev == NULL) {
+return;
+}
+
+QTAILQ_FOREACH(i, _lchs, link) {
+if ((!suffix || !g_strcmp0(i->suffix, suffix)) &&
+ i->dev == dev) {
+QTAILQ_REMOVE(_lchs, i, link);
+g_free(i->suffix);
+g_free(i);
+
+break;
+}
+}
+}
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 44f18eb739..5bc5c79cbc 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -103,6 +103,9 @@ void device_add_bootindex_property(Object *obj, int32_t 
*bootindex,
 DeviceState *dev, Error **errp);
  void restore_boot_order(void *opaque);
  void validate_bootdevices(const char *devices, Error **errp);
+void add_boot_device_lchs(DeviceState *dev, const char *suffix,
+  uint32_t lcyls, uint32_t lheads, uint32_t lsecs);
+void del_boot_device_lchs(DeviceState *dev, const char *suffix);
  
  /* handler to set the boot_device order for a specific type of MachineClass */

  typedef void QEMUBootSetHandler(void *opaque, const char *boot_order,



Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH v8 2/8] block: Support providing LCHS from user

[Philippe Mathieu-Daudé]

On 10/16/19 6:41 PM, Sam Eiderman wrote:

From: Sam Eiderman 

Add logical geometry variables to BlockConf.

A user can now supply "lcyls", "lheads" & "lsecs" for any HD device
that supports CHS ("cyls", "heads", "secs").

These devices include:
 * ide-hd
 * scsi-hd
 * virtio-blk-pci

In future commits we will use the provided LCHS and pass it to the BIOS
through fw_cfg to be supplied using INT13 routines.

Reviewed-by: Karl Heubaum 
Reviewed-by: Arbel Moshe 
Signed-off-by: Sam Eiderman 
Signed-off-by: Sam Eiderman 


Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 


---
  include/hw/block/block.h | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index fd55a30bca..d7246f3862 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -26,6 +26,7 @@ typedef struct BlockConf {
  uint32_t discard_granularity;
  /* geometry, not all devices use this */
  uint32_t cyls, heads, secs;
+uint32_t lcyls, lheads, lsecs;
  OnOffAuto wce;
  bool share_rw;
  BlockdevOnError rerror;
@@ -65,7 +66,10 @@ static inline unsigned int get_physical_block_exp(BlockConf 
*conf)
  #define DEFINE_BLOCK_CHS_PROPERTIES(_state, _conf)  \
  DEFINE_PROP_UINT32("cyls", _state, _conf.cyls, 0),  \
  DEFINE_PROP_UINT32("heads", _state, _conf.heads, 0),\
-DEFINE_PROP_UINT32("secs", _state, _conf.secs, 0)
+DEFINE_PROP_UINT32("secs", _state, _conf.secs, 0),  \
+DEFINE_PROP_UINT32("lcyls", _state, _conf.lcyls, 0),\
+DEFINE_PROP_UINT32("lheads", _state, _conf.lheads, 0),  \
+DEFINE_PROP_UINT32("lsecs", _state, _conf.lsecs, 0)
  
  #define DEFINE_BLOCK_ERROR_PROPERTIES(_state, _conf)\

  DEFINE_PROP_BLOCKDEV_ON_ERROR("rerror", _state, _conf.rerror,   \


___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH] docs: Add developer-certificate-of-origin

[Philippe Mathieu-Daudé]

On 10/21/19 5:31 PM, Kevin O'Connor wrote:

Update the documentation to be explicit about the signed-off-by
convention.

Signed-off-by: Kevin O'Connor 


Reviewed-by: Philippe Mathieu-Daudé 


---
  docs/Contributing.md |  5 
  docs/developer-certificate-of-origin | 37 
  2 files changed, 42 insertions(+)
  create mode 100644 docs/developer-certificate-of-origin

diff --git a/docs/Contributing.md b/docs/Contributing.md
index d0f2b5b..8d7 100644
--- a/docs/Contributing.md
+++ b/docs/Contributing.md
@@ -18,3 +18,8 @@ submit patches. The SeaBIOS C code does follow a slightly 
different
  coding style from QEMU (eg, mixed code and C99 style variable
  declarations are encouraged, braces are not required around single
  statement blocks), however patches in the QEMU style are acceptable.
+
+As with QEMU, commits should contain a "Signed-off-by" line using your
+real name (sorry, no pseudonyms or anonymous contributions) and a
+current email address. It indicates agreement with the terms of the
+[developer certificate of 
origin](https://git.seabios.org/cgit/seabios.git/tree/docs/developer-certificate-of-origin).
diff --git a/docs/developer-certificate-of-origin 
b/docs/developer-certificate-of-origin
new file mode 100644
index 000..8201f99
--- /dev/null
+++ b/docs/developer-certificate-of-origin
@@ -0,0 +1,37 @@
+Developer Certificate of Origin
+Version 1.1
+
+Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
+1 Letterman Drive
+Suite D4700
+San Francisco, CA, 94129
+
+Everyone is permitted to copy and distribute verbatim copies of this
+license document, but changing it is not allowed.
+
+
+Developer's Certificate of Origin 1.1
+
+By making a contribution to this project, I certify that:
+
+(a) The contribution was created in whole or in part by me and I
+have the right to submit it under the open source license
+indicated in the file; or
+
+(b) The contribution is based upon previous work that, to the best
+of my knowledge, is covered under an appropriate open source
+license and I have the right under that license to submit that
+work with modifications, whether created in whole or in part
+by me, under the same open source license (unless I am
+permitted to submit under a different license), as indicated
+in the file; or
+
+(c) The contribution was provided directly to me by some other
+person who certified (a), (b) or (c) and I have not modified
+it.
+
+(d) I understand and agree that this project and the contribution
+are public and that a record of the contribution (including all
+personal information I submit with it, including my sign-off) is
+maintained indefinitely and may be redistributed consistent with
+this project or the open source license(s) involved.


___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values

[Philippe Mathieu-Daudé]

On 10/16/19 5:19 PM, Sam Eiderman wrote:

Sure!

Philippe withdrew his R-b on 7/8, as I explained 7/8 is fine (only
need to remove a bad comment) the problem was in the tests 8/8 -
should I include the original R/b?


I withdrew it because John was preparing his pull request, and I needed 
more time to review this again. But then Laszlo was quicker and figured 
out the problem is in the other patch, so please keep my original R-b.


Thanks to all 3 of you :)


I guess all other 1-6 are fine to add R/b...

On Wed, Oct 16, 2019 at 6:07 PM John Snow  wrote:




On 10/16/19 10:55 AM, Sam Eiderman wrote:

Thanks for the detailed comment Laszlo,

Indeed my e-mail has changed and I only received replies to the
commits where I added this new mail in the S-o-b section, should of
added in all of them.

So as you said it, the problem was actually in using qfw_cfg_get_u32
which assumes the value is encoded LE and has an additional
le32_to_cpu, should have used qfw_cfg_get directly like
qfw_cfg_get_file does.

Regarding qfw_cfg_get_file - I wrote this code when this function did
not exist yet, I think it was added 6 months ago. In any case, I will
use it instead.

Thanks for this.

I will resubmit this entire commit series:
* I will only change code in the last commit (tests)
* I will remove a comment which is now not true anymore
* I will add my new email in S-o-b

Sam



Philippe gave me a verbal tut-tut for not including his review tags in
my last pull request; when you re-spin could you be so kind as to
include any that still apply?

--js

___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values

[Philippe Mathieu-Daudé]

On 10/16/19 2:14 PM, Laszlo Ersek wrote:

Hi Sam,

On 10/16/19 13:02, Sam Eiderman wrote:

Gentle Ping,

Philippe, John?

Just wondering if the series is okay, as Gerd pointed out this series
is a blocker for the corresponding changes in SeaBIOS for v 1.13


The QEMU series is still not merged, due to a bug in the last patch
(namely, the test case, "hd-geo-test: Add tests for lchs override").

To my knowledge, SeaBIOS prefers to merge patches with the underlying
QEMU patches merged first, so you'll likely have to fix that QEMU issue
first.

I explained the bug in the QEMU test case here:

   http://mid.mail-archive.com/6b00dc74-7267-8ce8-3271-5db269edb1b7@redhat.com
   http://mid.mail-archive.com/700cd594-1446-e478-fb03-d2e6b862dc6c@redhat.com


Yes, I was expecting a respin with find_fw_cfg_file() fixed per Laszlo 
detailed review.



(Alternative links to the same:

   https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg01790.html
   https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg01793.html
)

I've never received feedback to those messages, and I think you must
have missed them.

FWIW, when I hit "Reply All" in that thread, you were on the "To:" list
with:

   Sam Eiderman 

but here you are present with

   Sam Eiderman 

In addition, when I posted those messages, I got the following
auto-response ("Undelivered Mail Returned to Sender"):


This is the mail system at host mx1.redhat.com.

I'm sorry to have to inform you that your message could not
be delivered to one or more recipients. It's attached below.

For further assistance, please send mail to postmaster.

If you do so, please include this problem report. You can
delete your own text from the attached returned message.

The mail system

: host
aserp2030.oracle.com[141.146.126.74] said:
 550 5.1.1 Unknown recipient address. (in reply to RCPT TO command)


That explains it :)



I didn't know your new address, so I could only hope you'd find my
feedback on qemu-devel.

Thanks
Laszlo


___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values

[Philippe Mathieu-Daudé]

Hi Sam,

On 9/29/19 12:13 PM, Sam Eiderman wrote:

Philippe, thanks for the fast review,


Fast is not always the friend of careful.



John, thanks for picking up this hot potato :-)

Sam

On Thu, Sep 26, 2019 at 10:16 PM Philippe Mathieu-Daudé 
mailto:phi...@redhat.com>> wrote:


On 9/26/19 9:09 PM, Philippe Mathieu-Daudé wrote:
 > On 9/26/19 8:26 PM, John Snow wrote:
 >> On 9/26/19 5:57 AM, Philippe Mathieu-Daudé wrote:
 >>> Hi Sam,
 >>>
 >>> On 9/25/19 1:06 PM, Sam Eiderman wrote:
 >>>> From: Sam Eiderman mailto:shmuel.eider...@oracle.com>>
 >>>>
 >>>> Using fw_cfg, supply logical CHS values directly from QEMU to
the BIOS.
 >>>>
 >>>> Non-standard logical geometries break under QEMU.
 >>>>
 >>>> A virtual disk which contains an operating system which depends on
 >>>> logical geometries (consistent values being reported from BIOS
INT13
 >>>> AH=08) will most likely break under QEMU/SeaBIOS if it has
non-standard
 >>>> logical geometries - for example 56 SPT (sectors per track).
 >>>> No matter what QEMU will report - SeaBIOS, for large enough
disks - will
 >>>> use LBA translation, which will report 63 SPT instead.
 >>>>
 >>>> In addition we cannot force SeaBIOS to rely on physical
geometries at
 >>>> all. A virtio-blk-pci virtual disk with 255 phyiscal heads cannot
 >>>> report more than 16 physical heads when moved to an IDE
controller,
 >>>> since the ATA spec allows a maximum of 16 heads - this is an
artifact of
 >>>> virtualization.
 >>>>
 >>>> By supplying the logical geometries directly we are able to
support such
 >>>> "exotic" disks.
 >>>>
 >>>> We serialize this information in a similar way to the "bootorder"
 >>>> interface.
 >>>> The new fw_cfg entry is "bios-geometry".
 >>>>
 >>>> Reviewed-by: Karl Heubaum mailto:karl.heub...@oracle.com>>
 >>>> Reviewed-by: Arbel Moshe mailto:arbel.mo...@oracle.com>>
 >>>> Signed-off-by: Sam Eiderman mailto:shmuel.eider...@oracle.com>>
 >>>> ---
 >>>>  bootdevice.c            | 32 
 >>>>  hw/nvram/fw_cfg.c       | 14 +++---
 >>>>  include/sysemu/sysemu.h |  1 +
 >>>>  3 files changed, 44 insertions(+), 3 deletions(-)
 >>>>
 >>>> diff --git a/bootdevice.c b/bootdevice.c
 >>>> index 2b12fb85a4..b034ad7bdc 100644
 >>>> --- a/bootdevice.c
 >>>> +++ b/bootdevice.c
 >>>> @@ -405,3 +405,35 @@ void del_boot_device_lchs(DeviceState
*dev, const char *suffix)
 >>>>          }
 >>>>      }
 >>>>  }
 >>>> +
 >>>> +/* Serialized as: (device name\0 + lchs struct) x devices */


I suppose the lchs struct is serialized in little-endian.


 >>>> +char *get_boot_devices_lchs_list(size_t *size)
 >>>> +{
 >>>> +    FWLCHSEntry *i;
 >>>> +    size_t total = 0;
 >>>> +    char *list = NULL;
 >>>> +
 >>>> +    QTAILQ_FOREACH(i, _lchs, link) {
 >>>> +        char *bootpath;
 >>>> +        char *chs_string;
 >>>> +        size_t len;
 >>>> +
 >>>> +        bootpath = get_boot_device_path(i->dev, false,
i->suffix);
 >>>> +        chs_string = g_strdup_printf("%s %" PRIu32 " %"
PRIu32 " %" PRIu32,
 >>>> +                                     bootpath, i->lcyls,
i->lheads, i->lsecs);


Sam. can you check if you don't need endianness conversion here?


 >>>
 >>> Hmm maybe we can g_free(bootpath) directly here.
 >>>
 >>
 >> I think it's okay to do it at the bottom of the loop. No real
benefit to
 >> being that eager to free resources in my mind. I expect setup at
the top
 >> of a block and teardown at the bottom of a block.
 >>
 >> Trying to do too much in the middle gets messy in my opinion,
not that
 >> it seems to matter here.
 >
 > No problem.
 >
 >>>> +
 >>>> +        if (total) {
 >>>> +            list[total - 1] =

[SeaBIOS] Re: [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values

[Philippe Mathieu-Daudé]

On 9/26/19 9:09 PM, Philippe Mathieu-Daudé wrote:
> On 9/26/19 8:26 PM, John Snow wrote:
>> On 9/26/19 5:57 AM, Philippe Mathieu-Daudé wrote:
>>> Hi Sam,
>>>
>>> On 9/25/19 1:06 PM, Sam Eiderman wrote:
>>>> From: Sam Eiderman 
>>>>
>>>> Using fw_cfg, supply logical CHS values directly from QEMU to the BIOS.
>>>>
>>>> Non-standard logical geometries break under QEMU.
>>>>
>>>> A virtual disk which contains an operating system which depends on
>>>> logical geometries (consistent values being reported from BIOS INT13
>>>> AH=08) will most likely break under QEMU/SeaBIOS if it has non-standard
>>>> logical geometries - for example 56 SPT (sectors per track).
>>>> No matter what QEMU will report - SeaBIOS, for large enough disks - will
>>>> use LBA translation, which will report 63 SPT instead.
>>>>
>>>> In addition we cannot force SeaBIOS to rely on physical geometries at
>>>> all. A virtio-blk-pci virtual disk with 255 phyiscal heads cannot
>>>> report more than 16 physical heads when moved to an IDE controller,
>>>> since the ATA spec allows a maximum of 16 heads - this is an artifact of
>>>> virtualization.
>>>>
>>>> By supplying the logical geometries directly we are able to support such
>>>> "exotic" disks.
>>>>
>>>> We serialize this information in a similar way to the "bootorder"
>>>> interface.
>>>> The new fw_cfg entry is "bios-geometry".
>>>>
>>>> Reviewed-by: Karl Heubaum 
>>>> Reviewed-by: Arbel Moshe 
>>>> Signed-off-by: Sam Eiderman 
>>>> ---
>>>>  bootdevice.c| 32 
>>>>  hw/nvram/fw_cfg.c   | 14 +++---
>>>>  include/sysemu/sysemu.h |  1 +
>>>>  3 files changed, 44 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/bootdevice.c b/bootdevice.c
>>>> index 2b12fb85a4..b034ad7bdc 100644
>>>> --- a/bootdevice.c
>>>> +++ b/bootdevice.c
>>>> @@ -405,3 +405,35 @@ void del_boot_device_lchs(DeviceState *dev, const 
>>>> char *suffix)
>>>>  }
>>>>  }
>>>>  }
>>>> +
>>>> +/* Serialized as: (device name\0 + lchs struct) x devices */
>>>> +char *get_boot_devices_lchs_list(size_t *size)
>>>> +{
>>>> +FWLCHSEntry *i;
>>>> +size_t total = 0;
>>>> +char *list = NULL;
>>>> +
>>>> +QTAILQ_FOREACH(i, _lchs, link) {
>>>> +char *bootpath;
>>>> +char *chs_string;
>>>> +size_t len;
>>>> +
>>>> +bootpath = get_boot_device_path(i->dev, false, i->suffix);
>>>> +chs_string = g_strdup_printf("%s %" PRIu32 " %" PRIu32 " %" 
>>>> PRIu32,
>>>> + bootpath, i->lcyls, i->lheads, 
>>>> i->lsecs);
>>>
>>> Hmm maybe we can g_free(bootpath) directly here.
>>>
>>
>> I think it's okay to do it at the bottom of the loop. No real benefit to
>> being that eager to free resources in my mind. I expect setup at the top
>> of a block and teardown at the bottom of a block.
>>
>> Trying to do too much in the middle gets messy in my opinion, not that
>> it seems to matter here.
> 
> No problem.
> 
>>>> +
>>>> +if (total) {
>>>> +list[total - 1] = '\n';
>>>> +}
>>>> +len = strlen(chs_string) + 1;
>>>> +list = g_realloc(list, total + len);
>>>> +memcpy([total], chs_string, len);
>>>> +total += len;
>>>> +g_free(chs_string);
>>>> +g_free(bootpath);
>>>> +}
>>>> +
>>>> +*size = total;
>>>> +
>>>> +return list;
>>>> +}
>>>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>>>> index 7dc3ac378e..18aff658c0 100644
>>>> --- a/hw/nvram/fw_cfg.c
>>>> +++ b/hw/nvram/fw_cfg.c
>>>> @@ -920,13 +920,21 @@ void *fw_cfg_modify_file(FWCfgState *s, const char 
>>>> *filename,
>>>>  
>>>>  static void fw_cfg_machine_reset(void *opaque)
>>>>  {
>>>> +MachineClass *mc = MACHINE_GET_CLASS(qdev_ge

[SeaBIOS] Re: [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values

[Philippe Mathieu-Daudé]

On 9/26/19 8:26 PM, John Snow wrote:
> On 9/26/19 5:57 AM, Philippe Mathieu-Daudé wrote:
>> Hi Sam,
>>
>> On 9/25/19 1:06 PM, Sam Eiderman wrote:
>>> From: Sam Eiderman 
>>>
>>> Using fw_cfg, supply logical CHS values directly from QEMU to the BIOS.
>>>
>>> Non-standard logical geometries break under QEMU.
>>>
>>> A virtual disk which contains an operating system which depends on
>>> logical geometries (consistent values being reported from BIOS INT13
>>> AH=08) will most likely break under QEMU/SeaBIOS if it has non-standard
>>> logical geometries - for example 56 SPT (sectors per track).
>>> No matter what QEMU will report - SeaBIOS, for large enough disks - will
>>> use LBA translation, which will report 63 SPT instead.
>>>
>>> In addition we cannot force SeaBIOS to rely on physical geometries at
>>> all. A virtio-blk-pci virtual disk with 255 phyiscal heads cannot
>>> report more than 16 physical heads when moved to an IDE controller,
>>> since the ATA spec allows a maximum of 16 heads - this is an artifact of
>>> virtualization.
>>>
>>> By supplying the logical geometries directly we are able to support such
>>> "exotic" disks.
>>>
>>> We serialize this information in a similar way to the "bootorder"
>>> interface.
>>> The new fw_cfg entry is "bios-geometry".
>>>
>>> Reviewed-by: Karl Heubaum 
>>> Reviewed-by: Arbel Moshe 
>>> Signed-off-by: Sam Eiderman 
>>> ---
>>>  bootdevice.c| 32 
>>>  hw/nvram/fw_cfg.c   | 14 +++---
>>>  include/sysemu/sysemu.h |  1 +
>>>  3 files changed, 44 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/bootdevice.c b/bootdevice.c
>>> index 2b12fb85a4..b034ad7bdc 100644
>>> --- a/bootdevice.c
>>> +++ b/bootdevice.c
>>> @@ -405,3 +405,35 @@ void del_boot_device_lchs(DeviceState *dev, const char 
>>> *suffix)
>>>  }
>>>  }
>>>  }
>>> +
>>> +/* Serialized as: (device name\0 + lchs struct) x devices */
>>> +char *get_boot_devices_lchs_list(size_t *size)
>>> +{
>>> +FWLCHSEntry *i;
>>> +size_t total = 0;
>>> +char *list = NULL;
>>> +
>>> +QTAILQ_FOREACH(i, _lchs, link) {
>>> +char *bootpath;
>>> +char *chs_string;
>>> +size_t len;
>>> +
>>> +bootpath = get_boot_device_path(i->dev, false, i->suffix);
>>> +chs_string = g_strdup_printf("%s %" PRIu32 " %" PRIu32 " %" PRIu32,
>>> + bootpath, i->lcyls, i->lheads, 
>>> i->lsecs);
>>
>> Hmm maybe we can g_free(bootpath) directly here.
>>
> 
> I think it's okay to do it at the bottom of the loop. No real benefit to
> being that eager to free resources in my mind. I expect setup at the top
> of a block and teardown at the bottom of a block.
> 
> Trying to do too much in the middle gets messy in my opinion, not that
> it seems to matter here.

No problem.

>>> +
>>> +if (total) {
>>> +list[total - 1] = '\n';
>>> +}
>>> +len = strlen(chs_string) + 1;
>>> +list = g_realloc(list, total + len);
>>> +memcpy([total], chs_string, len);
>>> +total += len;
>>> +g_free(chs_string);
>>> +g_free(bootpath);
>>> +}
>>> +
>>> +*size = total;
>>> +
>>> +return list;
>>> +}
>>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>>> index 7dc3ac378e..18aff658c0 100644
>>> --- a/hw/nvram/fw_cfg.c
>>> +++ b/hw/nvram/fw_cfg.c
>>> @@ -920,13 +920,21 @@ void *fw_cfg_modify_file(FWCfgState *s, const char 
>>> *filename,
>>>  
>>>  static void fw_cfg_machine_reset(void *opaque)
>>>  {
>>> +MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
>>> +FWCfgState *s = opaque;
>>>  void *ptr;
>>>  size_t len;
>>> -FWCfgState *s = opaque;
>>> -char *bootindex = get_boot_devices_list();
>>> +char *buf;
>>>  
>>> -ptr = fw_cfg_modify_file(s, "bootorder", (uint8_t *)bootindex, len);
>>> +buf = get_boot_devices_list();
>>> +ptr = fw_cfg_modify_file(s, "bootorder", (uint8_t

[SeaBIOS] Re: [PATCH v7 8/8] hd-geo-test: Add tests for lchs override

[Philippe Mathieu-Daudé]

On 9/25/19 1:06 PM, Sam Eiderman wrote:
> From: Sam Eiderman 
> 
> Add QTest tests to check the logical geometry override option.
> 
> The tests in hd-geo-test are out of date - they only test IDE and do not
> test interesting MBRs.
> 
> I added a few helper functions which will make adding more tests easier.
> 
> QTest's fw_cfg helper functions support only legacy fw_cfg, so I had to
> read the new fw_cfg layout on my own.
> 
> Creating qcow2 disks with specific size and MBR layout is currently
> unused - we only use a default empty MBR.
> 
> Reviewed-by: Karl Heubaum 
> Reviewed-by: Arbel Moshe 
> Signed-off-by: Sam Eiderman 
> ---
>  tests/Makefile.include |   2 +-
>  tests/hd-geo-test.c| 589 +
>  2 files changed, 590 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 479664f899..a5b92fea6a 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -780,7 +780,7 @@ tests/ide-test$(EXESUF): tests/ide-test.o 
> $(libqos-pc-obj-y)
>  tests/ahci-test$(EXESUF): tests/ahci-test.o $(libqos-pc-obj-y) 
> qemu-img$(EXESUF)
>  tests/ipmi-kcs-test$(EXESUF): tests/ipmi-kcs-test.o
>  tests/ipmi-bt-test$(EXESUF): tests/ipmi-bt-test.o
> -tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o
> +tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o $(libqos-obj-y)
>  tests/boot-order-test$(EXESUF): tests/boot-order-test.o $(libqos-obj-y)
>  tests/boot-serial-test$(EXESUF): tests/boot-serial-test.o $(libqos-obj-y)
>  tests/bios-tables-test$(EXESUF): tests/bios-tables-test.o \
> diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c
> index 62eb624726..458de99c31 100644
> --- a/tests/hd-geo-test.c
> +++ b/tests/hd-geo-test.c
> @@ -17,7 +17,12 @@
>  
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
> +#include "qemu/bswap.h"
> +#include "qapi/qmp/qlist.h"
>  #include "libqtest.h"
> +#include "libqos/fw_cfg.h"
> +#include "libqos/libqos.h"
> +#include "standard-headers/linux/qemu_fw_cfg.h"
>  
>  #define ARGV_SIZE 256
>  
> @@ -388,6 +393,575 @@ static void test_ide_drive_cd_0(void)
>  qtest_quit(qts);
>  }
>  
> +typedef struct {
> +bool active;
> +uint32_t head;
> +uint32_t sector;
> +uint32_t cyl;
> +uint32_t end_head;
> +uint32_t end_sector;
> +uint32_t end_cyl;
> +uint32_t start_sect;
> +uint32_t nr_sects;
> +} MBRpartitions[4];
> +
> +static MBRpartitions empty_mbr = { {false, 0, 0, 0, 0, 0, 0, 0, 0},
> +   {false, 0, 0, 0, 0, 0, 0, 0, 0},
> +   {false, 0, 0, 0, 0, 0, 0, 0, 0},
> +   {false, 0, 0, 0, 0, 0, 0, 0, 0} };
> +
> +static char *create_qcow2_with_mbr(MBRpartitions mbr, uint64_t sectors)
> +{
> +const char *template = "/tmp/qtest.XX";
> +char *raw_path = strdup(template);
> +char *qcow2_path = strdup(template);
> +char cmd[100 + 2 * PATH_MAX];
> +uint8_t buf[512];
> +int i, ret, fd, offset;
> +uint64_t qcow2_size = sectors * 512;
> +uint8_t status, parttype, head, sector, cyl;
> +char *qemu_img_path;
> +char *qemu_img_abs_path;
> +
> +offset = 0xbe;
> +
> +for (i = 0; i < 4; i++) {
> +status = mbr[i].active ? 0x80 : 0x00;
> +g_assert(mbr[i].head < 256);
> +g_assert(mbr[i].sector < 64);
> +g_assert(mbr[i].cyl < 1024);
> +head = mbr[i].head;
> +sector = mbr[i].sector + ((mbr[i].cyl & 0x300) >> 2);
> +cyl = mbr[i].cyl & 0xff;
> +
> +buf[offset + 0x0] = status;
> +buf[offset + 0x1] = head;
> +buf[offset + 0x2] = sector;
> +buf[offset + 0x3] = cyl;
> +
> +parttype = 0;
> +g_assert(mbr[i].end_head < 256);
> +g_assert(mbr[i].end_sector < 64);
> +g_assert(mbr[i].end_cyl < 1024);
> +head = mbr[i].end_head;
> +sector = mbr[i].end_sector + ((mbr[i].end_cyl & 0x300) >> 2);
> +cyl = mbr[i].end_cyl & 0xff;
> +
> +buf[offset + 0x4] = parttype;
> +buf[offset + 0x5] = head;
> +buf[offset + 0x6] = sector;
> +buf[offset + 0x7] = cyl;
> +
> +(*(uint32_t *)[offset + 0x8]) = cpu_to_le32(mbr[i].start_sect);
> +(*(uint32_t *)[offset + 0xc]) = cpu_to_le32(mbr[i].nr_sects);
> +
> +offset += 0x10;
> +}
> +
> +fd = mkstemp(raw_path);
> +g_assert(fd);
> +close(fd);
> +
> +fd = open(raw_path, O_WRONLY);
> +g_assert(fd >= 0);
> +ret = write(fd, buf, sizeof(buf));
> +g_assert(ret == sizeof(buf));
> +close(fd);
> +
> +fd = mkstemp(qcow2_path);
> +g_assert(fd);
> +close(fd);
> +
> +qemu_img_path = getenv("QTEST_QEMU_IMG");
> +g_assert(qemu_img_path);
> +qemu_img_abs_path = realpath(qemu_img_path, NULL);
> +g_assert(qemu_img_abs_path);
> +
> +ret = snprintf(cmd, sizeof(cmd),
> +   "%s convert -f raw -O qcow2 %s %s > /dev/null",
> +   qemu_img_abs_path,
> +

[SeaBIOS] Re: [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values

[Philippe Mathieu-Daudé]

Hi Sam,

On 9/25/19 1:06 PM, Sam Eiderman wrote:
> From: Sam Eiderman 
> 
> Using fw_cfg, supply logical CHS values directly from QEMU to the BIOS.
> 
> Non-standard logical geometries break under QEMU.
> 
> A virtual disk which contains an operating system which depends on
> logical geometries (consistent values being reported from BIOS INT13
> AH=08) will most likely break under QEMU/SeaBIOS if it has non-standard
> logical geometries - for example 56 SPT (sectors per track).
> No matter what QEMU will report - SeaBIOS, for large enough disks - will
> use LBA translation, which will report 63 SPT instead.
> 
> In addition we cannot force SeaBIOS to rely on physical geometries at
> all. A virtio-blk-pci virtual disk with 255 phyiscal heads cannot
> report more than 16 physical heads when moved to an IDE controller,
> since the ATA spec allows a maximum of 16 heads - this is an artifact of
> virtualization.
> 
> By supplying the logical geometries directly we are able to support such
> "exotic" disks.
> 
> We serialize this information in a similar way to the "bootorder"
> interface.
> The new fw_cfg entry is "bios-geometry".
> 
> Reviewed-by: Karl Heubaum 
> Reviewed-by: Arbel Moshe 
> Signed-off-by: Sam Eiderman 
> ---
>  bootdevice.c| 32 
>  hw/nvram/fw_cfg.c   | 14 +++---
>  include/sysemu/sysemu.h |  1 +
>  3 files changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/bootdevice.c b/bootdevice.c
> index 2b12fb85a4..b034ad7bdc 100644
> --- a/bootdevice.c
> +++ b/bootdevice.c
> @@ -405,3 +405,35 @@ void del_boot_device_lchs(DeviceState *dev, const char 
> *suffix)
>  }
>  }
>  }
> +
> +/* Serialized as: (device name\0 + lchs struct) x devices */
> +char *get_boot_devices_lchs_list(size_t *size)
> +{
> +FWLCHSEntry *i;
> +size_t total = 0;
> +char *list = NULL;
> +
> +QTAILQ_FOREACH(i, _lchs, link) {
> +char *bootpath;
> +char *chs_string;
> +size_t len;
> +
> +bootpath = get_boot_device_path(i->dev, false, i->suffix);
> +chs_string = g_strdup_printf("%s %" PRIu32 " %" PRIu32 " %" PRIu32,
> + bootpath, i->lcyls, i->lheads, 
> i->lsecs);

Hmm maybe we can g_free(bootpath) directly here.

> +
> +if (total) {
> +list[total - 1] = '\n';
> +}
> +len = strlen(chs_string) + 1;
> +list = g_realloc(list, total + len);
> +memcpy([total], chs_string, len);
> +total += len;
> +g_free(chs_string);
> +g_free(bootpath);
> +}
> +
> +*size = total;
> +
> +return list;
> +}
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 7dc3ac378e..18aff658c0 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -920,13 +920,21 @@ void *fw_cfg_modify_file(FWCfgState *s, const char 
> *filename,
>  
>  static void fw_cfg_machine_reset(void *opaque)
>  {
> +MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> +FWCfgState *s = opaque;
>  void *ptr;
>  size_t len;
> -FWCfgState *s = opaque;
> -char *bootindex = get_boot_devices_list();
> +char *buf;
>  
> -ptr = fw_cfg_modify_file(s, "bootorder", (uint8_t *)bootindex, len);
> +buf = get_boot_devices_list();
> +ptr = fw_cfg_modify_file(s, "bootorder", (uint8_t *)buf, len);
>  g_free(ptr);
> +
> +if (!mc->legacy_fw_cfg_order) {
> +buf = get_boot_devices_lchs_list();
> +ptr = fw_cfg_modify_file(s, "bios-geometry", (uint8_t *)buf, len);

OK. Can you add a test in tests/fw_cfg-test.c please?

> +g_free(ptr);
> +}
>  }
>  
>  static void fw_cfg_machine_ready(struct Notifier *n, void *data)
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 5bc5c79cbc..80c57fdc4e 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -106,6 +106,7 @@ void validate_bootdevices(const char *devices, Error 
> **errp);
>  void add_boot_device_lchs(DeviceState *dev, const char *suffix,
>uint32_t lcyls, uint32_t lheads, uint32_t lsecs);
>  void del_boot_device_lchs(DeviceState *dev, const char *suffix);
> +char *get_boot_devices_lchs_list(size_t *size);

Please add some documentation. At least 'size' must be non-NULL.

Ideally you should add doc for the other functions added in 3/8
"bootdevice: Add interface to gather LCHS" too.

John, what do you think about extracting the *boot_device* functions out
of "sysemu.h"?

Thanks,

Phil.

>  
>  /* handler to set the boot_device order for a specific type of MachineClass 
> */
>  typedef void QEMUBootSetHandler(void *opaque, const char *boot_order,
> 
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH v7 6/8] bootdevice: Refactor get_boot_devices_list

[Philippe Mathieu-Daudé]

On 9/25/19 1:06 PM, Sam Eiderman wrote:
> From: Sam Eiderman 
> 
> Move device name construction to a separate function.
> 
> We will reuse this function in the following commit to pass logical CHS
> parameters through fw_cfg much like we currently pass bootindex.
> 
> Reviewed-by: Karl Heubaum 
> Reviewed-by: Arbel Moshe 
> Signed-off-by: Sam Eiderman 
> ---
>  bootdevice.c | 61 +---
>  1 file changed, 34 insertions(+), 27 deletions(-)
> 
> diff --git a/bootdevice.c b/bootdevice.c
> index bc5e1c2de4..2b12fb85a4 100644
> --- a/bootdevice.c
> +++ b/bootdevice.c
> @@ -202,6 +202,39 @@ DeviceState *get_boot_device(uint32_t position)
>  return res;
>  }
>  
> +static char *get_boot_device_path(DeviceState *dev, bool ignore_suffixes,
> +  char *suffix)

Please update to 'const char *suffix'.
John, can you do it directly in your tree before sending the pull request?
With it:
Reviewed-by: Philippe Mathieu-Daudé 


> +{
> +char *devpath = NULL, *s = NULL, *d, *bootpath;
> +
> +if (dev) {
> +devpath = qdev_get_fw_dev_path(dev);
> +assert(devpath);
> +}
> +
> +if (!ignore_suffixes) {
> +if (dev) {
> +d = qdev_get_own_fw_dev_path_from_handler(dev->parent_bus, dev);
> +if (d) {
> +assert(!suffix);
> +s = d;
> +} else {
> +s = g_strdup(suffix);
> +}
> +} else {
> +s = g_strdup(suffix);
> +}
> +}
> +
> +bootpath = g_strdup_printf("%s%s",
> +   devpath ? devpath : "",
> +   s ? s : "");
> +g_free(devpath);
> +g_free(s);
> +
> +return bootpath;
> +}
> +
>  /*
>   * This function returns null terminated string that consist of new line
>   * separated device paths.
> @@ -218,36 +251,10 @@ char *get_boot_devices_list(size_t *size)
>  bool ignore_suffixes = mc->ignore_boot_device_suffixes;
>  
>  QTAILQ_FOREACH(i, _boot_order, link) {
> -char *devpath = NULL,  *suffix = NULL;
>  char *bootpath;
> -char *d;
>  size_t len;
>  
> -if (i->dev) {
> -devpath = qdev_get_fw_dev_path(i->dev);
> -assert(devpath);
> -}
> -
> -if (!ignore_suffixes) {
> -if (i->dev) {
> -d = qdev_get_own_fw_dev_path_from_handler(i->dev->parent_bus,
> -  i->dev);
> -if (d) {
> -assert(!i->suffix);
> -suffix = d;
> -} else {
> -suffix = g_strdup(i->suffix);
> -}
> -} else {
> -suffix = g_strdup(i->suffix);
> -}
> -}
> -
> -bootpath = g_strdup_printf("%s%s",
> -   devpath ? devpath : "",
> -   suffix ? suffix : "");
> -g_free(devpath);
> -g_free(suffix);
> +bootpath = get_boot_device_path(i->dev, ignore_suffixes, i->suffix);
>  
>  if (total) {
>  list[total-1] = '\n';
> 
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH v7 4/8] scsi: Propagate unrealize() callback to scsi-hd

[Philippe Mathieu-Daudé]

On 9/25/19 1:06 PM, Sam Eiderman wrote:
> From: Sam Eiderman 
> 
> We will need to add LCHS removal logic to scsi-hd's unrealize() in the
> next commit.
> 
> Signed-off-by: Sam Eiderman 
> Reviewed-by: Karl Heubaum 
> Reviewed-by: Arbel Moshe 
> Signed-off-by: Sam Eiderman 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  hw/scsi/scsi-bus.c | 16 
>  include/hw/scsi/scsi.h |  1 +
>  2 files changed, 17 insertions(+)
> 
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index bccb7cc4c6..359d50d6d0 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -59,6 +59,14 @@ static void scsi_device_realize(SCSIDevice *s, Error 
> **errp)
>  }
>  }
>  
> +static void scsi_device_unrealize(SCSIDevice *s, Error **errp)
> +{
> +SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(s);
> +if (sc->unrealize) {
> +sc->unrealize(s, errp);
> +}
> +}
> +
>  int scsi_bus_parse_cdb(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf,
> void *hba_private)
>  {
> @@ -217,12 +225,20 @@ static void scsi_qdev_realize(DeviceState *qdev, Error 
> **errp)
>  static void scsi_qdev_unrealize(DeviceState *qdev, Error **errp)
>  {
>  SCSIDevice *dev = SCSI_DEVICE(qdev);
> +Error *local_err = NULL;
>  
>  if (dev->vmsentry) {
>  qemu_del_vm_change_state_handler(dev->vmsentry);
>  }
>  
>  scsi_device_purge_requests(dev, SENSE_CODE(NO_SENSE));
> +
> +scsi_device_unrealize(dev, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +return;
> +}
> +
>  blockdev_mark_auto_del(dev->conf.blk);
>  }
>  
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index d77a92361b..332ef602f4 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -59,6 +59,7 @@ struct SCSIRequest {
>  typedef struct SCSIDeviceClass {
>  DeviceClass parent_class;
>  void (*realize)(SCSIDevice *dev, Error **errp);
> +void (*unrealize)(SCSIDevice *dev, Error **errp);
>  int (*parse_cdb)(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf,
>   void *hba_private);
>  SCSIRequest *(*alloc_req)(SCSIDevice *s, uint32_t tag, uint32_t lun,
> 
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH v7 1/8] block: Refactor macros - fix tabbing

[Philippe Mathieu-Daudé]

On 9/25/19 1:06 PM, Sam Eiderman wrote:
> From: Sam Eiderman 
> 
> Fixing tabbing in block related macros.
> 
> Reviewed-by: Karl Heubaum 
> Reviewed-by: Arbel Moshe 
> Signed-off-by: Sam Eiderman 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  hw/ide/qdev.c|  2 +-
>  include/hw/block/block.h | 16 
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> index 6fba6b62b8..6dd219944f 100644
> --- a/hw/ide/qdev.c
> +++ b/hw/ide/qdev.c
> @@ -290,7 +290,7 @@ static void ide_drive_realize(IDEDevice *dev, Error 
> **errp)
>  DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf),\
>  DEFINE_BLOCK_ERROR_PROPERTIES(IDEDrive, dev.conf),  \
>  DEFINE_PROP_STRING("ver",  IDEDrive, dev.version),  \
> -DEFINE_PROP_UINT64("wwn",  IDEDrive, dev.wwn, 0),\
> +DEFINE_PROP_UINT64("wwn",  IDEDrive, dev.wwn, 0),   \
>  DEFINE_PROP_STRING("serial",  IDEDrive, dev.serial),\
>  DEFINE_PROP_STRING("model", IDEDrive, dev.model)
>  
> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> index 607539057a..fd55a30bca 100644
> --- a/include/hw/block/block.h
> +++ b/include/hw/block/block.h
> @@ -50,21 +50,21 @@ static inline unsigned int 
> get_physical_block_exp(BlockConf *conf)
>_conf.logical_block_size),\
>  DEFINE_PROP_BLOCKSIZE("physical_block_size", _state,\
>_conf.physical_block_size),   \
> -DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0),  \
> +DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0),\
>  DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0),\
> -DEFINE_PROP_UINT32("discard_granularity", _state, \
> -   _conf.discard_granularity, -1), \
> -DEFINE_PROP_ON_OFF_AUTO("write-cache", _state, _conf.wce, \
> -ON_OFF_AUTO_AUTO), \
> +DEFINE_PROP_UINT32("discard_granularity", _state,   \
> +   _conf.discard_granularity, -1),  \
> +DEFINE_PROP_ON_OFF_AUTO("write-cache", _state, _conf.wce,   \
> +ON_OFF_AUTO_AUTO),  \
>  DEFINE_PROP_BOOL("share-rw", _state, _conf.share_rw, false)
>  
>  #define DEFINE_BLOCK_PROPERTIES(_state, _conf)  \
>  DEFINE_PROP_DRIVE("drive", _state, _conf.blk),  \
>  DEFINE_BLOCK_PROPERTIES_BASE(_state, _conf)
>  
> -#define DEFINE_BLOCK_CHS_PROPERTIES(_state, _conf)  \
> -DEFINE_PROP_UINT32("cyls", _state, _conf.cyls, 0),  \
> -DEFINE_PROP_UINT32("heads", _state, _conf.heads, 0), \
> +#define DEFINE_BLOCK_CHS_PROPERTIES(_state, _conf)  \
> +DEFINE_PROP_UINT32("cyls", _state, _conf.cyls, 0),  \
> +DEFINE_PROP_UINT32("heads", _state, _conf.heads, 0),\
>  DEFINE_PROP_UINT32("secs", _state, _conf.secs, 0)
>  
>  #define DEFINE_BLOCK_ERROR_PROPERTIES(_state, _conf)\
> 
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH] Makefile: Build with -Wno-address-of-packed-member

[Philippe Mathieu-Daudé]

On 8/21/19 3:21 PM, Kevin O'Connor wrote:
> Building with gcc v9 causes lots of warnings about pointers to packed
> variables.  However, SeaBIOS is limited to x86 where unaligned
> reads/writes are supported by the cpu.  So, disable that warning.
> 
> Signed-off-by: Kevin O'Connor 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  Makefile | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Makefile b/Makefile
> index d16d1ae..5f7d537 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -68,6 +68,7 @@ COMMONCFLAGS += $(call cc-option,$(CC),-fno-pie,)
>  COMMONCFLAGS += $(call cc-option,$(CC),-fno-stack-protector,)
>  COMMONCFLAGS += $(call cc-option,$(CC),-fno-stack-protector-all,)
>  COMMONCFLAGS += $(call cc-option,$(CC),-fstack-check=no,)
> +COMMONCFLAGS += $(call cc-option,$(CC),-Wno-address-of-packed-member,)
>  COMMA := ,
>  
>  CFLAGS32FLAT := $(COMMONCFLAGS) -DMODE16=0 -DMODESEGMENT=0
> 
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [Qemu-devel] Regression with floppy drive controller

[Philippe Mathieu-Daudé]

Cc'ing Eduardo, Paolo.

On 8/20/19 3:38 PM, Philippe Mathieu-Daudé wrote:
> On 8/20/19 3:12 PM, John Snow wrote:
>> On 8/20/19 6:25 AM, Philippe Mathieu-Daudé wrote:
>>> [cross posting QEMU & SeaBIOS]
>>>
>>> Hello,
>>>
>>> I'v been looking at a QEMU bug report [1] which bisection resulted in a
>>> SeaBIOS commit:
>>>
>>> 4a6dbcea3e412fe12effa2f812f50dd7eae90955 is the first bad commit
>>> commit 4a6dbcea3e412fe12effa2f812f50dd7eae90955
>>> Author: Nikolay Nikolov 
>>> Date:   Sun Feb 4 17:27:01 2018 +0200
>>>
>>> floppy: Use timer_check() in floppy_wait_irq()
>>>
>>> Use timer_check() instead of using floppy_motor_counter in BDA for the
>>> timeout check in floppy_wait_irq().
>>>
>>> The problem with using floppy_motor_counter was that, after it reaches
>>> 0, it immediately stops the floppy motors, which is not what is
>>> supposed to happen on real hardware. Instead, after a timeout (like in
>>> the end of every floppy operation, regardless of the result - success,
>>> timeout or error), the floppy motors must be kept spinning for
>>> additional 2 seconds (the FLOPPY_MOTOR_TICKS). So, now the
>>> floppy_motor_counter is initialized to 255 (the max value) in the
>>> beginning of the floppy operation. For IRQ timeouts, a different
>>> timeout is used, specified by the new FLOPPY_IRQ_TIMEOUT constant
>>> (currently set to 5 seconds - a fairly conservative value, but should
>>> work reliably on most floppies).
>>>
>>> After the floppy operation, floppy_drive_pio() resets the
>>> floppy_motor_counter to 2 seconds (FLOPPY_MOTOR_TICKS).
>>>
>>> This is also consistent with what other PC BIOSes do.
>>>
>>>
>>> This commit improve behavior with real hardware, so maybe QEMU is not
>>> modelling something or modelling it incorrectly?
> [...]
>>
>> Well, that's unfortunate.
>>
>> What version of QEMU shipped the SeaBIOS that caused the regression?
> 
> See https://bugs.launchpad.net/qemu/+bug/1840719/comments/3
> 
> QEMU commit 0b8f74488e, slighly before QEMU v3.1.0
> (previous tag is v3.0.0).
> 
> But you can use v4.1.0 too, simply change the SeaBIOS bios.bin, i.e.:
> 
>   qemu$ git checkout v4.1.0
> 
>   qemu$ (cd roms/seabios && git checkout 4a6dbcea3e4~) && \
> make -C roms bios
> 
> Now pc-bios/bios.bin is built using the last commit working,
> 
>   qemu$ (cd roms/seabios && git checkout 4a6dbcea3e4) && \
> make -C roms bios
> 
> And you can reproduce the error.

Back from here.

So the SeaBIOS patch is:

diff --git a/src/hw/floppy.c b/src/hw/floppy.c
index 77dbade..3012b3a 100644
--- a/src/hw/floppy.c
+++ b/src/hw/floppy.c
@@ -34,6 +34,7 @@
 #define FLOPPY_GAPLEN 0x1B
 #define FLOPPY_FORMAT_GAPLEN 0x6c
 #define FLOPPY_PIO_TIMEOUT 1000
+#define FLOPPY_IRQ_TIMEOUT 5000

 #define FLOPPY_DOR_MOTOR_D 0x80 // Set to turn drive 3's motor ON
 #define FLOPPY_DOR_MOTOR_C 0x40 // Set to turn drive 2's motor ON
@@ -221,8 +222,9 @@ floppy_wait_irq(void)
 {
 u8 frs = GET_BDA(floppy_recalibration_status);
 SET_BDA(floppy_recalibration_status, frs & ~FRS_IRQ);
+u32 end = timer_calc(FLOPPY_IRQ_TIMEOUT);
 for (;;) {
-if (!GET_BDA(floppy_motor_counter)) {
+if (timer_check(end)) {
 warn_timeout();
 floppy_disable_controller();
 return DISK_RET_ETIMEOUT;

timer_calc() unit is milliseconds, so this patch should wait upto
5seconds before failing, and it seems the timeout is not used at all.

SeaBIOS timer.c:

// Return the TSC value that is 'msecs' time in the future.
u32
timer_calc(u32 msecs)
{
return timer_read() + (GET_GLOBAL(TimerKHz) * msecs);
}

static u32
timer_read(void)
{
u16 port = GET_GLOBAL(TimerPort);
if (CONFIG_TSC_TIMER && !port)
// Read from CPU TSC
return rdtscll() >> GET_GLOBAL(ShiftTSC);
if (CONFIG_PMTIMER && port != PORT_PIT_COUNTER0)
// Read from PMTIMER
return timer_adjust_bits(inl(port), 0xff);
// Read from PIT.
outb(PM_SEL_READBACK | PM_READ_VALUE | PM_READ_COUNTER0, PORT_PIT_MODE);
u16 v = inb(PORT_PIT_COUNTER0) | (inb(PORT_PIT_COUNTER0) << 8);
return timer_adjust_bits(v, 0x);
}

Using the default QEMU config, we build SeaBIOS to use the TSC timer:

builds/seabios-128k/.config:CONFIG_TSC_TIMER=y
builds/seabios-256k/.config:CONFIG_TSC_TIMER=y

$ qemu-system-i386 -M isapc -cpu 486 \
  -fda Windows\ 98\ Second\ Edition\ Boot.img \
  -chardev stdio,id=seabios \
  -device isa-debugcon,iobase=0x40

[SeaBIOS] Re: [Qemu-devel] Regression with floppy drive controller

[Philippe Mathieu-Daudé]

[Sorry seabios@ list for the noise!]

On 8/20/19 4:54 PM, Philippe Mathieu-Daudé wrote:
> On 8/20/19 4:36 PM, Dr. David Alan Gilbert wrote:
>> * Philippe Mathieu-Daudé (phi...@redhat.com) wrote:
>>> On 8/20/19 3:38 PM, Philippe Mathieu-Daudé wrote:
>>>> On 8/20/19 3:12 PM, John Snow wrote:
>>>>> On 8/20/19 6:25 AM, Philippe Mathieu-Daudé wrote:
>>>>>> [cross posting QEMU & SeaBIOS]
>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> I'v been looking at a QEMU bug report [1] which bisection resulted in a
>>>>>> SeaBIOS commit:
>>>>>>
>>>>>> 4a6dbcea3e412fe12effa2f812f50dd7eae90955 is the first bad commit
>>>>>> commit 4a6dbcea3e412fe12effa2f812f50dd7eae90955
>>>>>> Author: Nikolay Nikolov 
>>>>>> Date:   Sun Feb 4 17:27:01 2018 +0200
>>>>>>
>>>>>> floppy: Use timer_check() in floppy_wait_irq()
>>>>>>
>>>>>> Use timer_check() instead of using floppy_motor_counter in BDA for 
>>>>>> the
>>>>>> timeout check in floppy_wait_irq().
>>>>>>
>>>>>> The problem with using floppy_motor_counter was that, after it 
>>>>>> reaches
>>>>>> 0, it immediately stops the floppy motors, which is not what is
>>>>>> supposed to happen on real hardware. Instead, after a timeout (like 
>>>>>> in
>>>>>> the end of every floppy operation, regardless of the result - 
>>>>>> success,
>>>>>> timeout or error), the floppy motors must be kept spinning for
>>>>>> additional 2 seconds (the FLOPPY_MOTOR_TICKS). So, now the
>>>>>> floppy_motor_counter is initialized to 255 (the max value) in the
>>>>>> beginning of the floppy operation. For IRQ timeouts, a different
>>>>>> timeout is used, specified by the new FLOPPY_IRQ_TIMEOUT constant
>>>>>> (currently set to 5 seconds - a fairly conservative value, but should
>>>>>> work reliably on most floppies).
>>>>>>
>>>>>> After the floppy operation, floppy_drive_pio() resets the
>>>>>> floppy_motor_counter to 2 seconds (FLOPPY_MOTOR_TICKS).
>>>>>>
>>>>>> This is also consistent with what other PC BIOSes do.
>>>>>>
>>>>>>
>>>>>> This commit improve behavior with real hardware, so maybe QEMU is not
>>>>>> modelling something or modelling it incorrectly?
> [...]
>>> Looking at the fdc timer I noticed it use a static '50 ns' magic value.
>>
>> That's not 50ns
>>
>>> Increasing this value allows the floppy image to boot again, using this
>>> snippet:
>>>
>>> -- >8 --
>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>> index 9b24cb9b85..5fc54073fd 100644
>>> --- a/hw/block/fdc.c
>>> +++ b/hw/block/fdc.c
>>> @@ -2134,7 +2134,7 @@ static void fdctrl_handle_readid(FDCtrl *fdctrl,
>>> int direction)
>>>
>>>  cur_drv->head = (fdctrl->fifo[1] >> 2) & 1;
>>>  timer_mod(fdctrl->result_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>>> - (NANOSECONDS_PER_SECOND / 50));
>>
>> That's 1/50th of a second in ns.
> 
> Just noticed that too, so we have here 20ms.
> 
>>> + (NANOSECONDS_PER_SECOND / 5000));
>>
>> I'm not too sure about readid; but assuming we're rotating at 360rpm,
>> that's 6 revolutions/second, and 18 sectors/track = 108 sectors/second
>> (half of that for a double density disk).
>>
>> So, the wait for a sector to spin around and read feels like it should
>> be in the region of 1/108 of a second + some latency - so 1/50th of a
>> second would seem to be in the ballpark or being right, where as 1/5000
>> of a second is way too fast for a poor old floppy.
> 
> The first command sent is READ_ID.
> 
> Reading the Intel 82077AA datasheet:
> 
>   The READ ID command is used to find the present
>   position of the recording heads. The 82077AA
>   stores the values from the first ID Field it is able to
>   read into its registers. If the 82077AA does not find
>   an ID Address Mark on the diskette after the second
>   occurrence of a pulse on the IDX pin, it then sets the
>   IC code in Status Register 0 to ‘‘01’’ (Abn

[SeaBIOS] Re: [Qemu-devel] Regression with floppy drive controller

[Philippe Mathieu-Daudé]

On 8/20/19 4:36 PM, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (phi...@redhat.com) wrote:
>> On 8/20/19 3:38 PM, Philippe Mathieu-Daudé wrote:
>>> On 8/20/19 3:12 PM, John Snow wrote:
>>>> On 8/20/19 6:25 AM, Philippe Mathieu-Daudé wrote:
>>>>> [cross posting QEMU & SeaBIOS]
>>>>>
>>>>> Hello,
>>>>>
>>>>> I'v been looking at a QEMU bug report [1] which bisection resulted in a
>>>>> SeaBIOS commit:
>>>>>
>>>>> 4a6dbcea3e412fe12effa2f812f50dd7eae90955 is the first bad commit
>>>>> commit 4a6dbcea3e412fe12effa2f812f50dd7eae90955
>>>>> Author: Nikolay Nikolov 
>>>>> Date:   Sun Feb 4 17:27:01 2018 +0200
>>>>>
>>>>> floppy: Use timer_check() in floppy_wait_irq()
>>>>>
>>>>> Use timer_check() instead of using floppy_motor_counter in BDA for the
>>>>> timeout check in floppy_wait_irq().
>>>>>
>>>>> The problem with using floppy_motor_counter was that, after it reaches
>>>>> 0, it immediately stops the floppy motors, which is not what is
>>>>> supposed to happen on real hardware. Instead, after a timeout (like in
>>>>> the end of every floppy operation, regardless of the result - success,
>>>>> timeout or error), the floppy motors must be kept spinning for
>>>>> additional 2 seconds (the FLOPPY_MOTOR_TICKS). So, now the
>>>>> floppy_motor_counter is initialized to 255 (the max value) in the
>>>>> beginning of the floppy operation. For IRQ timeouts, a different
>>>>> timeout is used, specified by the new FLOPPY_IRQ_TIMEOUT constant
>>>>> (currently set to 5 seconds - a fairly conservative value, but should
>>>>> work reliably on most floppies).
>>>>>
>>>>> After the floppy operation, floppy_drive_pio() resets the
>>>>> floppy_motor_counter to 2 seconds (FLOPPY_MOTOR_TICKS).
>>>>>
>>>>> This is also consistent with what other PC BIOSes do.
>>>>>
>>>>>
>>>>> This commit improve behavior with real hardware, so maybe QEMU is not
>>>>> modelling something or modelling it incorrectly?
[...]
>> Looking at the fdc timer I noticed it use a static '50 ns' magic value.
> 
> That's not 50ns
> 
>> Increasing this value allows the floppy image to boot again, using this
>> snippet:
>>
>> -- >8 --
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index 9b24cb9b85..5fc54073fd 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -2134,7 +2134,7 @@ static void fdctrl_handle_readid(FDCtrl *fdctrl,
>> int direction)
>>
>>  cur_drv->head = (fdctrl->fifo[1] >> 2) & 1;
>>  timer_mod(fdctrl->result_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>> - (NANOSECONDS_PER_SECOND / 50));
> 
> That's 1/50th of a second in ns.

Just noticed that too, so we have here 20ms.

>> + (NANOSECONDS_PER_SECOND / 5000));
> 
> I'm not too sure about readid; but assuming we're rotating at 360rpm,
> that's 6 revolutions/second, and 18 sectors/track = 108 sectors/second
> (half of that for a double density disk).
> 
> So, the wait for a sector to spin around and read feels like it should
> be in the region of 1/108 of a second + some latency - so 1/50th of a
> second would seem to be in the ballpark or being right, where as 1/5000
> of a second is way too fast for a poor old floppy.

The first command sent is READ_ID.

Reading the Intel 82077AA datasheet:

  The READ ID command is used to find the present
  position of the recording heads. The 82077AA
  stores the values from the first ID Field it is able to
  read into its registers. If the 82077AA does not find
  an ID Address Mark on the diskette after the second
  occurrence of a pulse on the IDX pin, it then sets the
  IC code in Status Register 0 to ‘‘01’’ (Abnormal ter-
  mination), sets the MA bit in Status Register 1 to
  ‘’1’’, and terminates the command.

Then later the SPECIFICATIONS table:

  nRD/nWR Pulse Width: min 90ns
  INDEX Pulse Width: min 5 'Internal Clock Period'

  The nominal values for the 'internal clock period' for the various
  data rates are:

1 Mbps:  3 * osc period = 125ns
  500 Kbps:  6 * osc period = 250ns
  300 Kbps: 10 * osc period = 420ns
  250 Kbps: 12 * osc period = 500ns

IIUC the model we have DATARATE SELECT REGISTER (DSR) = 0

So DRATESEL=0 => datarate = 500 Kbps

So we should wait at least 250ns.

Trying th

[SeaBIOS] Re: [Qemu-devel] Regression with floppy drive controller

[Philippe Mathieu-Daudé]

On 8/20/19 3:38 PM, Philippe Mathieu-Daudé wrote:
> On 8/20/19 3:12 PM, John Snow wrote:
>> On 8/20/19 6:25 AM, Philippe Mathieu-Daudé wrote:
>>> [cross posting QEMU & SeaBIOS]
>>>
>>> Hello,
>>>
>>> I'v been looking at a QEMU bug report [1] which bisection resulted in a
>>> SeaBIOS commit:
>>>
>>> 4a6dbcea3e412fe12effa2f812f50dd7eae90955 is the first bad commit
>>> commit 4a6dbcea3e412fe12effa2f812f50dd7eae90955
>>> Author: Nikolay Nikolov 
>>> Date:   Sun Feb 4 17:27:01 2018 +0200
>>>
>>> floppy: Use timer_check() in floppy_wait_irq()
>>>
>>> Use timer_check() instead of using floppy_motor_counter in BDA for the
>>> timeout check in floppy_wait_irq().
>>>
>>> The problem with using floppy_motor_counter was that, after it reaches
>>> 0, it immediately stops the floppy motors, which is not what is
>>> supposed to happen on real hardware. Instead, after a timeout (like in
>>> the end of every floppy operation, regardless of the result - success,
>>> timeout or error), the floppy motors must be kept spinning for
>>> additional 2 seconds (the FLOPPY_MOTOR_TICKS). So, now the
>>> floppy_motor_counter is initialized to 255 (the max value) in the
>>> beginning of the floppy operation. For IRQ timeouts, a different
>>> timeout is used, specified by the new FLOPPY_IRQ_TIMEOUT constant
>>> (currently set to 5 seconds - a fairly conservative value, but should
>>> work reliably on most floppies).
>>>
>>> After the floppy operation, floppy_drive_pio() resets the
>>> floppy_motor_counter to 2 seconds (FLOPPY_MOTOR_TICKS).
>>>
>>> This is also consistent with what other PC BIOSes do.
>>>
>>>
>>> This commit improve behavior with real hardware, so maybe QEMU is not
>>> modelling something or modelling it incorrectly?
> [...]
>>
>> Well, that's unfortunate.
>>
>> What version of QEMU shipped the SeaBIOS that caused the regression?
> 
> See https://bugs.launchpad.net/qemu/+bug/1840719/comments/3
> 
> QEMU commit 0b8f74488e, slighly before QEMU v3.1.0
> (previous tag is v3.0.0).
> 
> But you can use v4.1.0 too, simply change the SeaBIOS bios.bin, i.e.:
> 
>   qemu$ git checkout v4.1.0
> 
>   qemu$ (cd roms/seabios && git checkout 4a6dbcea3e4~) && \
> make -C roms bios
> 
> Now pc-bios/bios.bin is built using the last commit working,
> 
>   qemu$ (cd roms/seabios && git checkout 4a6dbcea3e4) && \
> make -C roms bios
> 
> And you can reproduce the error.

Looking at the fdc timer I noticed it use a static '50 ns' magic value.

Increasing this value allows the floppy image to boot again, using this
snippet:

-- >8 --
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 9b24cb9b85..5fc54073fd 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2134,7 +2134,7 @@ static void fdctrl_handle_readid(FDCtrl *fdctrl,
int direction)

 cur_drv->head = (fdctrl->fifo[1] >> 2) & 1;
 timer_mod(fdctrl->result_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
- (NANOSECONDS_PER_SECOND / 50));
+ (NANOSECONDS_PER_SECOND / 5000));
 }

 static void fdctrl_handle_format_track(FDCtrl *fdctrl, int direction)
---

Any idea what is the correct value to use here?

Regards,

Phil.
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [Qemu-devel] Regression with floppy drive controller

[Philippe Mathieu-Daudé]

On 8/20/19 3:12 PM, John Snow wrote:
> On 8/20/19 6:25 AM, Philippe Mathieu-Daudé wrote:
>> [cross posting QEMU & SeaBIOS]
>>
>> Hello,
>>
>> I'v been looking at a QEMU bug report [1] which bisection resulted in a
>> SeaBIOS commit:
>>
>> 4a6dbcea3e412fe12effa2f812f50dd7eae90955 is the first bad commit
>> commit 4a6dbcea3e412fe12effa2f812f50dd7eae90955
>> Author: Nikolay Nikolov 
>> Date:   Sun Feb 4 17:27:01 2018 +0200
>>
>> floppy: Use timer_check() in floppy_wait_irq()
>>
>> Use timer_check() instead of using floppy_motor_counter in BDA for the
>> timeout check in floppy_wait_irq().
>>
>> The problem with using floppy_motor_counter was that, after it reaches
>> 0, it immediately stops the floppy motors, which is not what is
>> supposed to happen on real hardware. Instead, after a timeout (like in
>> the end of every floppy operation, regardless of the result - success,
>> timeout or error), the floppy motors must be kept spinning for
>> additional 2 seconds (the FLOPPY_MOTOR_TICKS). So, now the
>> floppy_motor_counter is initialized to 255 (the max value) in the
>> beginning of the floppy operation. For IRQ timeouts, a different
>> timeout is used, specified by the new FLOPPY_IRQ_TIMEOUT constant
>> (currently set to 5 seconds - a fairly conservative value, but should
>> work reliably on most floppies).
>>
>> After the floppy operation, floppy_drive_pio() resets the
>> floppy_motor_counter to 2 seconds (FLOPPY_MOTOR_TICKS).
>>
>> This is also consistent with what other PC BIOSes do.
>>
>>
>> This commit improve behavior with real hardware, so maybe QEMU is not
>> modelling something or modelling it incorrectly?
[...]
> 
> Well, that's unfortunate.
> 
> What version of QEMU shipped the SeaBIOS that caused the regression?

See https://bugs.launchpad.net/qemu/+bug/1840719/comments/3

QEMU commit 0b8f74488e, slighly before QEMU v3.1.0
(previous tag is v3.0.0).

But you can use v4.1.0 too, simply change the SeaBIOS bios.bin, i.e.:

  qemu$ git checkout v4.1.0

  qemu$ (cd roms/seabios && git checkout 4a6dbcea3e4~) && \
make -C roms bios

Now pc-bios/bios.bin is built using the last commit working,

  qemu$ (cd roms/seabios && git checkout 4a6dbcea3e4) && \
make -C roms bios

And you can reproduce the error.

Regards,

Phil.
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Regression with floppy drive controller

[Philippe Mathieu-Daudé]

[cross posting QEMU & SeaBIOS]

Hello,

I'v been looking at a QEMU bug report [1] which bisection resulted in a
SeaBIOS commit:

4a6dbcea3e412fe12effa2f812f50dd7eae90955 is the first bad commit
commit 4a6dbcea3e412fe12effa2f812f50dd7eae90955
Author: Nikolay Nikolov 
Date:   Sun Feb 4 17:27:01 2018 +0200

floppy: Use timer_check() in floppy_wait_irq()

Use timer_check() instead of using floppy_motor_counter in BDA for the
timeout check in floppy_wait_irq().

The problem with using floppy_motor_counter was that, after it reaches
0, it immediately stops the floppy motors, which is not what is
supposed to happen on real hardware. Instead, after a timeout (like in
the end of every floppy operation, regardless of the result - success,
timeout or error), the floppy motors must be kept spinning for
additional 2 seconds (the FLOPPY_MOTOR_TICKS). So, now the
floppy_motor_counter is initialized to 255 (the max value) in the
beginning of the floppy operation. For IRQ timeouts, a different
timeout is used, specified by the new FLOPPY_IRQ_TIMEOUT constant
(currently set to 5 seconds - a fairly conservative value, but should
work reliably on most floppies).

After the floppy operation, floppy_drive_pio() resets the
floppy_motor_counter to 2 seconds (FLOPPY_MOTOR_TICKS).

This is also consistent with what other PC BIOSes do.


This commit improve behavior with real hardware, so maybe QEMU is not
modelling something or modelling it incorrectly?


Regards,

Phil.


PD: How to reproduce:

- Download Windows 98 SE floppy image from [2]

- Run QEMU using the 'isapc' machine:

  $ qemu-system-i386 -M isapc \
 -fda Windows\ 98\ Second\ Edition\ Boot.img

  SeaBIOS (version rel-1.11.0-11-g4a6dbce-prebuilt.qemu.org)
  Booting from Floppy...
  Boot failed: could not read the boot disk

[1] https://bugs.launchpad.net/qemu/+bug/1840719
[2] https://winworldpc.com/download/417d71c2-ae18-c39a-11c3-a4e284a2c3a5
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: [PATCH] build: Allow to use cross C preprocessor

[Philippe Mathieu-Daudé]

On 1/25/19 4:55 PM, Roman Bolshakov wrote:
> An attempt to cross-compile SeaBIOS on macOS can fail because host C
> preprocessor doesn't comprehend the command line options:
> 
>   Compiling IASL src/fw/acpi-dsdt.hex
> cpp -P -MD -MT src/fw/acpi-dsdt.hex src/fw/acpi-dsdt.dsl -o 
> out/src/fw/acpi-dsdt.dsl.i.orig
> clang: error: no input files
> make: *** [src/fw/acpi-dsdt.hex] Error 1
> 
> Signed-off-by: Roman Bolshakov 
> ---
>  Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index d2d11db..909c0a1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -23,7 +23,7 @@ OBJCOPY=$(CROSS_PREFIX)objcopy
>  OBJDUMP=$(CROSS_PREFIX)objdump
>  STRIP=$(CROSS_PREFIX)strip
>  PYTHON=python
> -CPP=cpp
> +CPP=$(CROSS_PREFIX)cpp
>  IASL:=iasl
>  LD32BIT_FLAG:=-melf_i386
>

Reviewed-by: Philippe Mathieu-Daudé 
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

[SeaBIOS] Re: Mailing list update

[Philippe Mathieu-Daudé]

On 1/9/19 7:01 PM, Mike Banon wrote:
> Please tell, will the old ASCII mailing list archive continue to work
> and get the new messages?
> I found it as really convenient to wget and observe with lightweight
> browsers or even console "links"

This is also useful when you want to reply to a thread previous to your
subscription date, since you can fetch the archive, import it in your
local mailer then reply.
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

Re: [SeaBIOS] [PATCH 1/4] x86: add readq()

[Philippe Mathieu-Daudé]

On 10/06/2017 11:33 AM, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lur...@redhat.com>
> 
> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>

> ---
>  src/x86.h | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/x86.h b/src/x86.h
> index 4aea65c..c7bb60d 100644
> --- a/src/x86.h
> +++ b/src/x86.h
> @@ -211,6 +211,11 @@ static inline void writeb(void *addr, u8 val) {
>  barrier();
>  *(volatile u8 *)addr = val;
>  }
> +static inline u64 readq(const void *addr) {
> +u64 val = *(volatile const u64 *)addr;
> +barrier();
> +return val;
> +}
>  static inline u32 readl(const void *addr) {
>  u32 val = *(volatile const u32 *)addr;
>  barrier();
> 

___
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 2/4] tpm: generalize init_timeout()

[Philippe Mathieu-Daudé]

On 10/06/2017 11:33 AM, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lur...@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>

> 
> It seems both TIS & CRB devices share the same timeout. Make
> initialization function generic for now.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> ---
>  src/hw/tpm_drivers.c | 25 +++--
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/src/hw/tpm_drivers.c b/src/hw/tpm_drivers.c
> index a137e62..0daaef2 100644
> --- a/src/hw/tpm_drivers.c
> +++ b/src/hw/tpm_drivers.c
> @@ -102,26 +102,31 @@ static TPMVersion tis_get_tpm_version(void)
>  return TPM_VERSION_1_2;
>  }
>  
> -static u32 tis_init(void)
> +static void init_timeout(int driver)
>  {
> -if (!CONFIG_TCGBIOS)
> -return 1;
> -
> -writeb(TIS_REG(0, TIS_REG_INT_ENABLE), 0);
> -
> -if (tpm_drivers[TIS_DRIVER_IDX].durations == NULL) {
> +if (tpm_drivers[driver].durations == NULL) {
>  u32 *durations = tpm_default_dur;
>  memcpy(durations, tpm_default_durations,
> sizeof(tpm_default_durations));
> -tpm_drivers[TIS_DRIVER_IDX].durations = durations;
> +tpm_drivers[driver].durations = durations;
>  }
>  
> -if (tpm_drivers[TIS_DRIVER_IDX].timeouts == NULL) {
> +if (tpm_drivers[driver].timeouts == NULL) {
>  u32 *timeouts = tpm_default_to;
>  memcpy(timeouts, tis_default_timeouts,
> sizeof(tis_default_timeouts));
> -tpm_drivers[TIS_DRIVER_IDX].timeouts = timeouts;
> +tpm_drivers[driver].timeouts = timeouts;
>  }
> +}
> +
> +static u32 tis_init(void)
> +{
> +if (!CONFIG_TCGBIOS)
> +return 1;
> +
> +writeb(TIS_REG(0, TIS_REG_INT_ENABLE), 0);
> +
> +init_timeout(TIS_DRIVER_IDX);
>  
>  return 1;
>  }
> 

___
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 4/4] SeaVGABios/cbvga: Advertise compatible VESA modes

[Philippe Mathieu-Daudé]

Hi Patrick,

On 03/17/2017 01:49 PM, Patrick Rudolph wrote:

Advertise compatible VESA modes, that are smaller or equal to
coreboot's active framebuffer. Only modes that have the same Bpp
are advertise and can be selected.

Allows the Windows 7 bootloader NTLDR to show up in VESA mode.
Allows to show the Windows 7 boot logo.
Allows Windows to boot in safe mode and in normal boot using
VgaSave driver with resolution up to 1600x1200.

This fixes most likely other bootloader and operating systems as well,
in case the are relying on VESA framebuffer support.

Known issues:
Windows replaces int 15h handler, resulting in memcpy_far not being available.
The call will prematurely exit and Windows won't show any picture at all.
All tests were done with clear screen on mode change disabled.

NTLDR is aware of the framebuffer's pitch, but the internal
clear screen method isn't. Some text artifacts might stay on screen.
That's nothing SeaVGABios can fix.

Signed-off-by: Patrick Rudolph 
---
 vgasrc/cbvga.c | 110 +
 1 file changed, 110 insertions(+)

diff --git a/vgasrc/cbvga.c b/vgasrc/cbvga.c
index 4c349c5..6be6936 100644
--- a/vgasrc/cbvga.c
+++ b/vgasrc/cbvga.c
@@ -18,8 +18,73 @@ static struct vgamode_s CBmodeinfo VAR16;
 static struct vgamode_s CBemulinfo VAR16;
 static u32 CBlinelength VAR16;

+static struct cbvga_mode_s


static "const" struct cbvga_mode_s


+{
+u16 mode;
+struct vgamode_s info;
+} cbvesa_modes[] VAR16 = {
+/* VESA 1.0 modes */
+{ 0x110, { MM_DIRECT, 640,  480,  15, 8, 16, SEG_GRAPH } },
+{ 0x111, { MM_DIRECT, 640,  480,  16, 8, 16, SEG_GRAPH } },
+{ 0x112, { MM_DIRECT, 640,  480,  24, 8, 16, SEG_GRAPH } },
+{ 0x113, { MM_DIRECT, 800,  600,  15, 8, 16, SEG_GRAPH } },
+{ 0x114, { MM_DIRECT, 800,  600,  16, 8, 16, SEG_GRAPH } },
+{ 0x115, { MM_DIRECT, 800,  600,  24, 8, 16, SEG_GRAPH } },
+{ 0x116, { MM_DIRECT, 1024, 768,  15, 8, 16, SEG_GRAPH } },
+{ 0x117, { MM_DIRECT, 1024, 768,  16, 8, 16, SEG_GRAPH } },
+{ 0x118, { MM_DIRECT, 1024, 768,  24, 8, 16, SEG_GRAPH } },
+{ 0x119, { MM_DIRECT, 1280, 1024, 15, 8, 16, SEG_GRAPH } },
+{ 0x11A, { MM_DIRECT, 1280, 1024, 16, 8, 16, SEG_GRAPH } },
+{ 0x11B, { MM_DIRECT, 1280, 1024, 24, 8, 16, SEG_GRAPH } },
+{ 0x11D, { MM_DIRECT, 1600, 1200, 15, 8, 16, SEG_GRAPH } },
+{ 0x11E, { MM_DIRECT, 1600, 1200, 16, 8, 16, SEG_GRAPH } },
+{ 0x11F, { MM_DIRECT, 1600, 1200, 24, 8, 16, SEG_GRAPH } },
+/* VESA 2.0 modes */
+{ 0x141, { MM_DIRECT, 640,  400,  32, 8, 16, SEG_GRAPH } },
+{ 0x142, { MM_DIRECT, 640,  480,  32, 8, 16, SEG_GRAPH } },
+{ 0x143, { MM_DIRECT, 800,  600,  32, 8, 16, SEG_GRAPH } },
+{ 0x144, { MM_DIRECT, 1024, 768,  32, 8, 16, SEG_GRAPH } },
+{ 0x145, { MM_DIRECT, 1280, 1024, 32, 8, 16, SEG_GRAPH } },
+{ 0x147, { MM_DIRECT, 1600, 1200, 32, 8, 16, SEG_GRAPH } },
+{ 0x149, { MM_DIRECT, 1152, 864,  15, 8, 16, SEG_GRAPH } },
+{ 0x14a, { MM_DIRECT, 1152, 864,  16, 8, 16, SEG_GRAPH } },
+{ 0x14b, { MM_DIRECT, 1152, 864,  24, 8, 16, SEG_GRAPH } },
+{ 0x14c, { MM_DIRECT, 1152, 864,  32, 8, 16, SEG_GRAPH } },
+{ 0x175, { MM_DIRECT, 1280, 768,  16, 8, 16, SEG_GRAPH } },
+{ 0x176, { MM_DIRECT, 1280, 768,  24, 8, 16, SEG_GRAPH } },
+{ 0x177, { MM_DIRECT, 1280, 768,  32, 8, 16, SEG_GRAPH } },
+{ 0x178, { MM_DIRECT, 1280, 800,  16, 8, 16, SEG_GRAPH } },
+{ 0x179, { MM_DIRECT, 1280, 800,  24, 8, 16, SEG_GRAPH } },
+{ 0x17a, { MM_DIRECT, 1280, 800,  32, 8, 16, SEG_GRAPH } },
+{ 0x17b, { MM_DIRECT, 1280, 960,  16, 8, 16, SEG_GRAPH } },
+{ 0x17c, { MM_DIRECT, 1280, 960,  24, 8, 16, SEG_GRAPH } },
+{ 0x17d, { MM_DIRECT, 1280, 960,  32, 8, 16, SEG_GRAPH } },
+{ 0x17e, { MM_DIRECT, 1440, 900,  16, 8, 16, SEG_GRAPH } },
+{ 0x17f, { MM_DIRECT, 1440, 900,  24, 8, 16, SEG_GRAPH } },
+{ 0x180, { MM_DIRECT, 1440, 900,  32, 8, 16, SEG_GRAPH } },
+{ 0x181, { MM_DIRECT, 1400, 1050, 16, 8, 16, SEG_GRAPH } },
+{ 0x182, { MM_DIRECT, 1400, 1050, 24, 8, 16, SEG_GRAPH } },
+{ 0x183, { MM_DIRECT, 1400, 1050, 32, 8, 16, SEG_GRAPH } },
+{ 0x184, { MM_DIRECT, 1680, 1050, 16, 8, 16, SEG_GRAPH } },
+{ 0x185, { MM_DIRECT, 1680, 1050, 24, 8, 16, SEG_GRAPH } },
+{ 0x186, { MM_DIRECT, 1680, 1050, 32, 8, 16, SEG_GRAPH } },
+{ 0x187, { MM_DIRECT, 1920, 1200, 16, 8, 16, SEG_GRAPH } },
+{ 0x188, { MM_DIRECT, 1920, 1200, 24, 8, 16, SEG_GRAPH } },
+{ 0x189, { MM_DIRECT, 1920, 1200, 32, 8, 16, SEG_GRAPH } },
+{ 0x18a, { MM_DIRECT, 2560, 1600, 16, 8, 16, SEG_GRAPH } },
+{ 0x18b, { MM_DIRECT, 2560, 1600, 24, 8, 16, SEG_GRAPH } },
+{ 0x18c, { MM_DIRECT, 2560, 1600, 32, 8, 16, SEG_GRAPH } },
+{ 0x18d, { MM_DIRECT, 1280, 720,  16, 8, 16, SEG_GRAPH } },
+{ 0x18e, { MM_DIRECT, 1280, 720,  24, 8, 16, SEG_GRAPH } },
+{ 0x18f, { MM_DIRECT, 1280, 720,  32, 8, 16, SEG_GRAPH } },
+{ 0x190, { MM_DIRECT, 1920, 1080, 

Re: [SeaBIOS] [PATCH 4/5] nvme: fix extraction of status code bits

[Philippe Mathieu-Daudé]

On 02/24/2017 03:27 AM, Daniel Verkamp wrote:

The status code field is 8 bits wide starting at bit 1; the previous
code would truncate the top bit.

Signed-off-by: Daniel Verkamp <dan...@drv.nu>


Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>


---
 src/hw/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/hw/nvme.c b/src/hw/nvme.c
index 97b05cb..9fda011 100644
--- a/src/hw/nvme.c
+++ b/src/hw/nvme.c
@@ -74,7 +74,7 @@ nvme_poll_cq(struct nvme_cq *cq)
 static int
 nvme_is_cqe_success(struct nvme_cqe const *cqe)
 {
-return (cqe->status & 0xFF) >> 1 == 0;
+return ((cqe->status >> 1) & 0xFF) == 0;
 }





___
SeaBIOS mailing list
SeaBIOS@seabios.org
https://www.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 3/5] nvme: fix reversed loop condition in cmd_readwrite

[Philippe Mathieu-Daudé]

On 02/24/2017 03:27 AM, Daniel Verkamp wrote:

It looks like the intent was to exit the loop if a command failed, but
the current code would actually continue looping in that case.

Signed-off-by: Daniel Verkamp <dan...@drv.nu>


Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>


---
 src/hw/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/hw/nvme.c b/src/hw/nvme.c
index c194f9f..97b05cb 100644
--- a/src/hw/nvme.c
+++ b/src/hw/nvme.c
@@ -571,7 +571,7 @@ nvme_cmd_readwrite(struct nvme_namespace *ns, struct 
disk_op_s *op, int write)
 u16 const max_blocks = NVME_PAGE_SIZE / ns->block_size;
 u16 i;

-for (i = 0; i < op->count || res != DISK_RET_SUCCESS;) {
+for (i = 0; i < op->count && res == DISK_RET_SUCCESS;) {
 u16 blocks_remaining = op->count - i;
 u16 blocks = blocks_remaining < max_blocks ? blocks_remaining
: max_blocks;



___
SeaBIOS mailing list
SeaBIOS@seabios.org
https://www.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH 5/5] nvme: fix copy-paste mistake in comment

[Philippe Mathieu-Daudé]

On 02/24/2017 03:27 AM, Daniel Verkamp wrote:

Signed-off-by: Daniel Verkamp <dan...@drv.nu>


Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>


---
 src/hw/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/hw/nvme.c b/src/hw/nvme.c
index 9fda011..60557ae 100644
--- a/src/hw/nvme.c
+++ b/src/hw/nvme.c
@@ -549,7 +549,7 @@ nvme_controller_setup(void *opaque)
 static void
 nvme_scan(void)
 {
-// Scan PCI bus for ATA adapters
+// Scan PCI bus for NVMe adapters
 struct pci_device *pci;

 foreachpci(pci) {



___
SeaBIOS mailing list
SeaBIOS@seabios.org
https://www.coreboot.org/mailman/listinfo/seabios