[PATCH 1/4] pci: Allow scanning pci bus number up to 255 in CSM mode

[Bin Gao]

On real hardware especially server platforms, there might be multiple
root buses, thus pci bus number could run up to 255. This patch fixed
pci_probe_devices() by allowing to scan all 256 pci bus numbers(0-255).

Signed-off-by: Bin Gao 
---
 src/hw/pcidevice.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/hw/pcidevice.c b/src/hw/pcidevice.c
index 8853cf7..8d9c401 100644
--- a/src/hw/pcidevice.c
+++ b/src/hw/pcidevice.c
@@ -26,6 +26,14 @@ pci_probe_devices(void)
     struct hlist_node **pprev = &PCIDevices.first;
     int extraroots = romfile_loadint("etc/extra-pci-roots", 0);
     int bus = -1, lastbus = 0, rootbuses = 0, count=0;
+
+    // There might be multiple PCI root buses on physical hardware especially
+    // server platforms, thus bus number could run up to the top value,
+    // i.e. 0xff. Setting extraroots to 0xff to ensure we can enumerate all
+    // PCI bus numbers.
+    if (CONFIG_CSM)
+        extraroots = 0xff;
+
     while (bus < 0xff && (bus < MaxPCIBus || rootbuses < extraroots)) {
         bus++;
         int bdf;

[PATCH 2/4] csm: Enable boot from pci option rom

[Bin Gao]

In csm mode, the bev pointer of the pci option rom was not
added to the bootentry list, resulting in failure to boot
from pci option rom. This patch fixed it.
Also we enabled hw timer interrupt and clock update before
we call the option rom's init() funtion, so the clock tick
update at BDA address 40:6C will work during the rom's init()
execution. Withou this change, iPXE would hang on reading BDA
40:6C due to the value at this address is not changed.

Signed-off-by: Bin Gao 
---
 src/fw/csm.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/src/fw/csm.c b/src/fw/csm.c
index 8359bcb..f64bc73 100644
--- a/src/fw/csm.c
+++ b/src/fw/csm.c
@@ -19,6 +19,7 @@
 #include "std/acpi.h" // RSDP_SIGNATURE
 #include "std/bda.h" // struct bios_data_area_s
 #include "std/optionrom.h" // struct rom_header
+#include "std/pnpbios.h" // PNP_SIGNATURE
 #include "util.h" // copy_smbios

#define UINT8 u8
@@ -219,6 +220,7 @@ handle_csm_0005(struct bregs *regs)
 {
     EFI_DISPATCH_OPROM_TABLE *table = MAKE_FLATPTR(regs->es, regs->bx);
     struct rom_header *rom;
+    struct pnp_data *pnp;
     u16 bdf;

if (!CONFIG_OPTIONROMS) {
@@ -240,11 +242,36 @@ handle_csm_0005(struct bregs *regs)

rom_reserve(rom->size * 512);

+    // callrom() typically calls the rom's init() functions.
+    // If the rom's init() function does a delay by detecting the value
+    // at address 40:6C, it will fail because the value at 40:6C is
+    // updated only in clock_update() which is called in hw timer ISR.
+    // Thus we have to enable hw timer interrupt and set up clock before
+    // calling callrom().
+    pic_setup();
+    timer_setup();
+    clock_setup();
+
     // XX PnP seg/ofs should never be other than default
     callrom(rom, bdf);

rom_confirm(rom->size * 512);

+    // PnP rom - check for BEV and BCV boot capabilities.
+    pnp = (void*)((u8*)rom + rom->pnpoffset);
+    if (pnp->signature == PNP_SIGNATURE) {
+        struct pci_device pci_dev = {
+            .bdf = bdf,
+        };
+        int prio = csm_bootprio_pci(&pci_dev);
+        if (pnp->bev)
+            boot_add_bev(FLATPTR_TO_SEG(rom), pnp->bev, pnp->productname, prio);
+        else if (pnp->bcv)
+            boot_add_bcv(FLATPTR_TO_SEG(rom), pnp->bcv, pnp->productname, prio);
+    } else {
+        dprintf(3, "rom %p: not a PnP rom, no BEV or BCV capability\n", table);
+    }
+
     regs->bx = 0; // FIXME
     regs->ax = 0;
 }

[PATCH 2/4] csm: Enable boot from pci option rom

[Kevin O'Connor]

On Thu, Dec 31, 2020 at 07:08:24PM -0800, Bin Gao wrote:
> In csm mode, the bev pointer of the pci option rom was not
> added to the bootentry list, resulting in failure to boot
> from pci option rom. This patch fixed it.
> Also we enabled hw timer interrupt and clock update before
> we call the option rom's init() funtion, so the clock tick
> update at BDA address 40:6C will work during the rom's init()
> execution. Withou this change, iPXE would hang on reading BDA
> 40:6C due to the value at this address is not changed.
> Signed-off-by: Bin Gao
>
> src/fw/csm.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/src/fw/csm.c b/src/fw/csm.c
> index 8359bcb..f64bc73 100644
> --- a/src/fw/csm.c
> +++ b/src/fw/csm.c
> @@ -19,6 +19,7 @@
>  #include "std/acpi.h" // RSDP_SIGNATURE
>  #include "std/bda.h" // struct bios_data_area_s
>  #include "std/optionrom.h" // struct rom_header
> +#include "std/pnpbios.h" // PNP_SIGNATURE
>  #include "util.h" // copy_smbios
>
> #define UINT8 u8
> @@ -219,6 +220,7 @@ handle_csm_0005(struct bregs *regs)
>  {
>      EFI_DISPATCH_OPROM_TABLE *table = MAKE_FLATPTR(regs->es, regs->bx);
>      struct rom_header *rom;
>
> struct pnp_data *pnp;
> u16 bdf;
>
> if (!CONFIG_OPTIONROMS) {
>
> @@ -240,11 +242,36 @@ handle_csm_0005(struct bregs *regs)
>  rom_reserve(rom->size * 512);
>
> // callrom() typically calls the rom's init() functions.
> // If the rom's init() function does a delay by detecting the value
> // at address 40:6C, it will fail because the value at 40:6C is
> // updated only in clock_update() which is called in hw timer ISR.
> // Thus we have to enable hw timer interrupt and set up clock before
> // calling callrom().
> pic_setup();
> timer_setup();
> clock_setup();

This doesn't look correct to me.  The CSM code was originally designed
not to directly setup the PIC hardware - IIRC we don't know if the OS
is a "legacy OS" or not at this point.  Maybe David has a more clear
recollection.

In any case, it would not make sense to call clock_setup() and
timer_setup() on every option rom and then again in PrepareToBoot().

-Kevin

[PATCH 2/4] csm: Enable boot from pci option rom

[Gao, Bin]

> > In csm mode, the bev pointer of the pci option rom was not
> > added to the bootentry list, resulting in failure to boot
> > from pci option rom. This patch fixed it.
> > Also we enabled hw timer interrupt and clock update before
> > we call the option rom's init() funtion, so the clock tick
> > update at BDA address 40:6C will work during the rom's init()
> > execution. Withou this change, iPXE would hang on reading BDA
> > 40:6C due to the value at this address is not changed.
> > Signed-off-by: Bin Gao
> >
> > src/fw/csm.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/src/fw/csm.c b/src/fw/csm.c
> > index 8359bcb..f64bc73 100644
> > --- a/src/fw/csm.c
> > +++ b/src/fw/csm.c
> > @@ -19,6 +19,7 @@
> >  #include "std/acpi.h" // RSDP_SIGNATURE
> >  #include "std/bda.h" // struct bios_data_area_s
> >  #include "std/optionrom.h" // struct rom_header
> > +#include "std/pnpbios.h" // PNP_SIGNATURE
> >  #include "util.h" // copy_smbios
> >
> > #define UINT8 u8
> > @@ -219,6 +220,7 @@ handle_csm_0005(struct bregs *regs)
> >  {
> >      EFI_DISPATCH_OPROM_TABLE *table = MAKE_FLATPTR(regs->es, regs->bx);
> >      struct rom_header *rom;
> >
> > struct pnp_data *pnp;
> > u16 bdf;
> >
> > if (!CONFIG_OPTIONROMS) {
> >
> > @@ -240,11 +242,36 @@ handle_csm_0005(struct bregs *regs)
> >  rom_reserve(rom->size * 512);
> >
> > // callrom() typically calls the rom's init() functions.
> > // If the rom's init() function does a delay by detecting the value
> > // at address 40:6C, it will fail because the value at 40:6C is
> > // updated only in clock_update() which is called in hw timer ISR.
> > // Thus we have to enable hw timer interrupt and set up clock before
> > // calling callrom().
> > pic_setup();
> > timer_setup();
> > clock_setup();
>
> This doesn't look correct to me.  The CSM code was originally designed
> not to directly setup the PIC hardware - IIRC we don't know if the OS
> is a "legacy OS" or not at this point.  Maybe David has a more clear
> recollection.
>
> In any case, it would not make sense to call clock_setup() and
> timer_setup() on every option rom and then again in PrepareToBoot().

After each rom dispatch, the execution returns back to BIOS, so we lose all the hardware context from SeaBIOS. That's why I did a per-dispatch time init.
The ROM init() depends on the ticks updating if the ROM init() uses standard BDA 40:6C based time polling to implement a delay.
Probably no one really tried PCI oprom on physical hardware so this problem was not disclosed...
> -Kevin

[PATCH 3/4] csm: Move pci_probe_devices() from InitializeYourself phase to PreparToBoot phase

[Bin Gao]

On physical hardware, some PCI devices are not detectable when
UEFI invokes our InitializeYourself() function. But they
are guaranteed to be available before UEFI invokes our
PreparToBoot() function.

Signed-off-by: Bin Gao 
---
 src/fw/csm.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/fw/csm.c b/src/fw/csm.c
index f64bc73..1bd821e 100644
--- a/src/fw/csm.c
+++ b/src/fw/csm.c
@@ -64,7 +64,9 @@ static void
 csm_maininit(struct bregs *regs)
 {
     interface_init();
-    pci_probe_devices();
+
+    if (CONFIG_CSM && CONFIG_QEMU_HARDWARE)
+        pci_probe_devices();

csm_compat_table.PnPInstallationCheckSegment = SEG_BIOS;
     csm_compat_table.PnPInstallationCheckOffset = get_pnp_offset();
@@ -144,6 +146,13 @@ handle_csm_0002(struct bregs *regs)
         return;
     }

+    // On physical hardware, some PCI devices are not detectable when
+    // UEFI invokes our InitializeYourself() function. But they
+    // are guaranteed to be available before UEFI invokes our
+    // PreparToBoot() function.
+    if (CONFIG_CSM && !CONFIG_QEMU_HARDWARE)
+        pci_probe_devices();
+
     dprintf(3, "PrepareToBoot table %04x:%04x\n", regs->es, regs->bx);

struct e820entry *p = (void *)csm_compat_table.E820Pointer;

[PATCH 3/4] csm: Move pci_probe_devices() from InitializeYourself phase to PreparToBoot phase

[Paul Menzel]

Dear Bin,

Thank you for your patches.

Am 01.01.21 um 04:08 schrieb Bin Gao:
> On physical hardware, some PCI devices are not detectable when
> UEFI invokes our InitializeYourself() function.

It would be great, if you gave a concrete example of hardware (board, 
firmware version and PCI devices).
> But they are guaranteed to be available before UEFI invokes our
> PreparToBoot() function.

Nit: Small typo (also in summary and comment below): Prepar*e*ToBoot.
> Signed-off-by: Bin Gao
>
> src/fw/csm.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/src/fw/csm.c b/src/fw/csm.c
> index f64bc73..1bd821e 100644
> --- a/src/fw/csm.c
> +++ b/src/fw/csm.c
> @@ -64,7 +64,9 @@ static void
>   csm_maininit(struct bregs *regs)
>   {
>       interface_init();
> pci_probe_devices();
>
> if (CONFIG_CSM && CONFIG_QEMU_HARDWARE)
>    pci_probe_devices();
>
> csm_compat_table.PnPInstallationCheckSegment = SEG_BIOS;
> csm_compat_table.PnPInstallationCheckOffset = get_pnp_offset();
>
> @@ -144,6 +146,13 @@ handle_csm_0002(struct bregs *regs)
>           return;
>       }
>
> // On physical hardware, some PCI devices are not detectable when
>
> // UEFI invokes our InitializeYourself() function. But they
>
> // are guaranteed to be available before UEFI invokes our
>
> // PreparToBoot() function.
>
> if (CONFIG_CSM && !CONFIG_QEMU_HARDWARE)
>    pci_probe_devices();
> dprintf(3, "PrepareToBoot table %04x:%04x\n", regs->es, regs->bx);
>
> struct e820entry *p = (void *)csm_compat_table.E820Pointer;

Why don’t we want to do the same for QEMU?

Kind regards,

Paul

[PATCH 3/4] csm: Move pci_probe_devices() from InitializeYourself phase to PreparToBoot phase

[Gerd Hoffmann]

Hi,
> > // On physical hardware, some PCI devices are not detectable when
> > // UEFI invokes our InitializeYourself() function. But they
> > // are guaranteed to be available before UEFI invokes our
> > // PreparToBoot() function.
> > if (CONFIG_CSM && !CONFIG_QEMU_HARDWARE)
> >    pci_probe_devices();
> > dprintf(3, "PrepareToBoot table %04x:%04x\n", regs->es, regs->bx);
> > struct e820entry *p = (void *)csm_compat_table.E820Pointer;
>
> Why don’t we want to do the same for QEMU?

I think so, there is no reason why qemu should be handled in a different
way.  IIRC I've even mentioned that before.  Do you guys read the review
comments before submitting a new version of the patch set?

take care,
  Gerd

[PATCH 3/4] csm: Move pci_probe_devices() from InitializeYourself phase to PreparToBoot phase

[Gao, Bin]

> Dear Bin,
>
> Thank you for your patches.
>
> Am 01.01.21 um 04:08 schrieb Bin Gao:
> > On physical hardware, some PCI devices are not detectable when
> > UEFI invokes our InitializeYourself() function.
>
> It would be great, if you gave a concrete example of hardware (board,
> firmware version and PCI devices).

Will add this formation in my next revision.
> > But they are guaranteed to be available before UEFI invokes our
> > PreparToBoot() function.
>
> Nit: Small typo (also in summary and comment below): Prepar*e*ToBoot.

Will fix in next revision.
> > Signed-off-by: Bin Gao
> >
> > src/fw/csm.c | 11 ++++++++++-
> >   1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/fw/csm.c b/src/fw/csm.c
> > index f64bc73..1bd821e 100644
> > --- a/src/fw/csm.c
> > +++ b/src/fw/csm.c
> > @@ -64,7 +64,9 @@ static void
> >   csm_maininit(struct bregs *regs)
> >   {
> >       interface_init();
> > pci_probe_devices();
> >
> > if (CONFIG_CSM && CONFIG_QEMU_HARDWARE)
> >    pci_probe_devices();
> >
> > csm_compat_table.PnPInstallationCheckSegment = SEG_BIOS;
> > csm_compat_table.PnPInstallationCheckOffset = get_pnp_offset();
> >
> > @@ -144,6 +146,13 @@ handle_csm_0002(struct bregs *regs)
> >           return;
> >       }
> >
> > // On physical hardware, some PCI devices are not detectable when
> >
> > // UEFI invokes our InitializeYourself() function. But they
> >
> > // are guaranteed to be available before UEFI invokes our
> >
> > // PreparToBoot() function.
> >
> > if (CONFIG_CSM && !CONFIG_QEMU_HARDWARE)
> >    pci_probe_devices();
> > dprintf(3, "PrepareToBoot table %04x:%04x\n", regs->es, regs->bx);
> >
> > struct e820entry *p = (void *)csm_compat_table.E820Pointer;
>
> Why don’t we want to do the same for QEMU?

This problem was only seen on physical hardware, so I wanted this fix to be applied only to physical hardware.
> Kind regards,
>
> Paul
> _______________________________________________
> SeaBIOS mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]

[PATCH 3/4] csm: Move pci_probe_devices() from InitializeYourself phase to PreparToBoot phase

[Paul Menzel]

Dear Bin,

Am 13.01.21 um 04:43 schrieb Gao, Bin:
> > Am 01.01.21 um 04:08 schrieb Bin Gao:

[…]
> > > // On physical hardware, some PCI devices are not detectable when
> > >
> > > // UEFI invokes our InitializeYourself() function. But they
> > >
> > > // are guaranteed to be available before UEFI invokes our
> > >
> > > // PreparToBoot() function.
> > >
> > > if (CONFIG_CSM && !CONFIG_QEMU_HARDWARE)
> > >    pci_probe_devices();
> > >  dprintf(3, "PrepareToBoot table %04x:%04x\n", regs->es, regs->bx);
> > >
> > >  struct e820entry *p = (void *)csm_compat_table.E820Pointer;
> >
> > Why don’t we want to do the same for QEMU?
>
> This problem was only seen on physical hardware, so I wanted this
> fix to be applied only to physical hardware.

Yes, I understand, but less code “complexity” and having the same 
behavior on QEMU and real hardware is preferred. The maintainer Gerd 
replied yesterday. Did you get his reply (Message-ID: 
)?

Kind regards,

Paul

[PATCH 3/4] csm: Move pci_probe_devices() from InitializeYourself phase to PreparToBoot phase

[Gao, Bin]

> > > > // On physical hardware, some PCI devices are not detectable when
> > > >
> > > > // UEFI invokes our InitializeYourself() function. But they
> > > >
> > > > // are guaranteed to be available before UEFI invokes our
> > > >
> > > > // PreparToBoot() function.
> > > >
> > > > if (CONFIG_CSM && !CONFIG_QEMU_HARDWARE)
> > > >    pci_probe_devices();
> > > >  dprintf(3, "PrepareToBoot table %04x:%04x\n", regs->es, regs->bx);
> > > >
> > > >  struct e820entry *p = (void *)csm_compat_table.E820Pointer;
> > >
> > > Why don’t we want to do the same for QEMU?
> >
> > This problem was only seen on physical hardware, so I wanted this
> > fix to be applied only to physical hardware.
>
> Yes, I understand, but less code “complexity” and having the same
> behavior on QEMU and real hardware is preferred. The maintainer Gerd
> replied yesterday. Did you get his reply (Message-ID:
> )?

Thanks Paul and sorry for late catch-up.
Yes I saw Gerd's reply and we agreed to keep this change common to both
QEMU and bare metal.
> Kind regards,
>
> Paul
> _______________________________________________
> SeaBIOS mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]

[PATCH 3/4] csm: Move pci_probe_devices() from InitializeYourself phase to PreparToBoot phase

[Kevin O'Connor]

On Thu, Dec 31, 2020 at 07:08:25PM -0800, Bin Gao wrote:
> On physical hardware, some PCI devices are not detectable when
> UEFI invokes our InitializeYourself() function. But they
> are guaranteed to be available before UEFI invokes our
> PreparToBoot() function.
> Signed-off-by: Bin Gao
>
> src/fw/csm.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/src/fw/csm.c b/src/fw/csm.c
> index f64bc73..1bd821e 100644
> --- a/src/fw/csm.c
> +++ b/src/fw/csm.c
> @@ -64,7 +64,9 @@ static void
>  csm_maininit(struct bregs *regs)
>  {
>      interface_init();
> pci_probe_devices();
>
> if (CONFIG_CSM && CONFIG_QEMU_HARDWARE)
>    pci_probe_devices();
>
> csm_compat_table.PnPInstallationCheckSegment = SEG_BIOS;
>    csm_compat_table.PnPInstallationCheckOffset = get_pnp_offset();
>
> @@ -144,6 +146,13 @@ handle_csm_0002(struct bregs *regs)
>          return;
>      }
> // On physical hardware, some PCI devices are not detectable when
> // UEFI invokes our InitializeYourself() function. But they
> // are guaranteed to be available before UEFI invokes our
> // PreparToBoot() function.
> if (CONFIG_CSM && !CONFIG_QEMU_HARDWARE)
>    pci_probe_devices();

This looks like a hack.  I'm not sure what's being attempted, but a
more scalable solution will be needed.

-Kevin

[PATCH 3/4] csm: Move pci_probe_devices() from InitializeYourself phase to PreparToBoot phase

[Gao, Bin]

> On Thu, Dec 31, 2020 at 07:08:25PM -0800, Bin Gao wrote:
> > On physical hardware, some PCI devices are not detectable when
> > UEFI invokes our InitializeYourself() function. But they
> > are guaranteed to be available before UEFI invokes our
> > PreparToBoot() function.
> > Signed-off-by: Bin Gao
> >
> > src/fw/csm.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/fw/csm.c b/src/fw/csm.c
> > index f64bc73..1bd821e 100644
> > --- a/src/fw/csm.c
> > +++ b/src/fw/csm.c
> > @@ -64,7 +64,9 @@ static void
> >  csm_maininit(struct bregs *regs)
> >  {
> >      interface_init();
> > pci_probe_devices();
> >
> > if (CONFIG_CSM && CONFIG_QEMU_HARDWARE)
> >    pci_probe_devices();
> >
> > csm_compat_table.PnPInstallationCheckSegment = SEG_BIOS;
> >    csm_compat_table.PnPInstallationCheckOffset = get_pnp_offset();
> >
> > @@ -144,6 +146,13 @@ handle_csm_0002(struct bregs *regs)
> >          return;
> >      }
> > // On physical hardware, some PCI devices are not detectable when
> > // UEFI invokes our InitializeYourself() function. But they
> > // are guaranteed to be available before UEFI invokes our
> > // PreparToBoot() function.
> > if (CONFIG_CSM && !CONFIG_QEMU_HARDWARE)
> >    pci_probe_devices();
>
> This looks like a hack.  I'm not sure what's being attempted, but a
> more scalable solution will be needed.

As the commit message described, the goal is to make sure all the pci devices
can be found in pci_probe_devices(). Due to the BIOS internal logic, some PCI
devices may not be visible yet when InitializeYourself() is called. Thus we wanted
to run pci_probe_devices() later, i.e. in PrepareToBoot() instead of in
InitializeYourself().

As Paul and Gerd suggested, I will remove CONFIG_QEMU_HARDWARE and let the code
be common for both qemu and bare metal.
> -Kevin

[PATCH 4/4] boot: Support booting more than 2 hard drives

[Bin Gao]

By default SeaBIOS can map up to 2 hard disks, and more hard disks beyond
2 will be rejected. This restriction is caused by limited BDA slots.
This patch added support for mapping more than 2 hard disks by dynamically
mapping the hard disk right before we're about to boot the hard disk.

Signed-off-by: Bin Gao 
---
 src/Kconfig |  8 ++++++++
 src/block.c | 17 ++++++++++++++++-
 src/boot.c  | 14 +++++++++++---
 3 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/src/Kconfig b/src/Kconfig
index 3a8ffa1..d6031e2 100644
--- a/src/Kconfig
+++ b/src/Kconfig
@@ -380,6 +380,14 @@ menu "BIOS interfaces"
         help
             Support int13 disk/floppy drive functions.

+    config DYNAMIC_MAP_HD
+        depends on DRIVES
+        bool "Dynamically map hard drives"
+        default n
+        help
+            Support dynamically map hard drives so that the boot logic can
+            support more than 2 hard drives.
+
     config CDROM_BOOT
         depends on DRIVES
         bool "DVD/CDROM booting"
diff --git a/src/block.c b/src/block.c
index 1f600b8..904e5f4 100644
--- a/src/block.c
+++ b/src/block.c
@@ -253,6 +253,10 @@ add_drive(struct drive_s **idmap, u8 *count, struct drive_s *drive)
 }

// Map a hard drive
+//
+// By default SeaBIOS can map up to 2 hard disks, and more hard disks beyond
+// 2 will be rejected. This restriction is caused by limited BDA slots.
+// To support more than 2 hard drives, we need to enable CONFIG_DYNAMIC_MAP_HD.
 void
 map_hd_drive(struct drive_s *drive)
 {
@@ -260,7 +264,18 @@ map_hd_drive(struct drive_s *drive)
     struct bios_data_area_s *bda = MAKE_FLATPTR(SEG_BDA, 0);
     int hdid = bda->hdcount;
     dprintf(3, "Mapping hd drive %p to %d\n", drive, hdid);
-    add_drive(IDMap[EXTTYPE_HD], &bda->hdcount, drive);
+    // To implement dynamic hard drive maping, we constantly keep
+    // bda->hdcount = 1, and use hdid = 0 all the times.
+    // map_hd_drive() must be called each time before boot to a
+    // specific hard drive.
+    if (CONFIG_DYNAMIC_MAP_HD) {
+        hdid = 0; // 0 - always the first hard drive
+        u8 tmp = 0;
+        add_drive(IDMap[EXTTYPE_HD], &tmp, drive);
+        bda->hdcount = 1;
+    } else {
+        add_drive(IDMap[EXTTYPE_HD], &bda->hdcount, drive);
+    }

// Setup disk geometry translation.
     setup_translation(drive);
diff --git a/src/boot.c b/src/boot.c
index 1effd80..2e6e356 100644
--- a/src/boot.c
+++ b/src/boot.c
@@ -802,7 +802,7 @@ static int HaveHDBoot, HaveFDBoot;
 static void
 add_bev(int type, u32 vector)
 {
-    if (type == IPL_TYPE_HARDDISK && HaveHDBoot++)
+    if (type == IPL_TYPE_HARDDISK && HaveHDBoot++ && !CONFIG_DYNAMIC_MAP_HD)
         return;
     if (type == IPL_TYPE_FLOPPY && HaveFDBoot++)
         return;
@@ -837,8 +837,14 @@ bcv_prepboot(void)
             add_bev(IPL_TYPE_FLOPPY, 0);
             break;
         case IPL_TYPE_HARDDISK:
-            map_hd_drive(pos->drive);
-            add_bev(IPL_TYPE_HARDDISK, 0);
+            if (CONFIG_DYNAMIC_MAP_HD) {
+                // Pass the drive_s pointer to bev_s struct so that
+                // we can do dynamic hard disk map in do_boot().
+                add_bev(IPL_TYPE_HARDDISK, (u32)pos->drive);
+            } else {
+                map_hd_drive(pos->drive);
+                add_bev(IPL_TYPE_HARDDISK, 0);
+            }
             break;
         case IPL_TYPE_CDROM:
             map_cd_drive(pos->drive);
@@ -998,6 +1004,8 @@ do_boot(int seq_nr)
         break;
     case IPL_TYPE_HARDDISK:
         printf("Booting from Hard Disk...\n");
+        if (CONFIG_DYNAMIC_MAP_HD)
+            map_hd_drive((struct drive_s *)ie->vector);
         boot_disk(0x80, 1);
         break;
     case IPL_TYPE_CDROM:

[PATCH 4/4] boot: Support booting more than 2 hard drives

[Kevin O'Connor]

On Thu, Dec 31, 2020 at 07:08:26PM -0800, Bin Gao wrote:
> By default SeaBIOS can map up to 2 hard disks, and more hard disks beyond
> 2 will be rejected. This restriction is caused by limited BDA slots.
> This patch added support for mapping more than 2 hard disks by dynamically
> mapping the hard disk right before we're about to boot the hard disk.
> Signed-off-by: Bin Gao
>
> src/Kconfig |  8 ++++++++
>  src/block.c | 17 ++++++++++++++++-
>  src/boot.c  | 14 +++++++++++---
>  3 files changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/src/Kconfig b/src/Kconfig
> index 3a8ffa1..d6031e2 100644
> --- a/src/Kconfig
> +++ b/src/Kconfig
> @@ -380,6 +380,14 @@ menu "BIOS interfaces"
>          help
>              Support int13 disk/floppy drive functions.
> config DYNAMIC_MAP_HD
>    depends on DRIVES
>    bool "Dynamically map hard drives"
>    default n
>    help
>        Support dynamically map hard drives so that the boot logic can
>        support more than 2 hard drives.
> config CDROM_BOOT
>        depends on DRIVES
>        bool "DVD/CDROM booting"
>
> diff --git a/src/block.c b/src/block.c
> index 1f600b8..904e5f4 100644
> --- a/src/block.c
> +++ b/src/block.c
> @@ -253,6 +253,10 @@ add_drive(struct drive_s **idmap, u8 *count, struct drive_s *drive)
>  }
>
> // Map a hard drive
> +//
> +// By default SeaBIOS can map up to 2 hard disks, and more hard disks beyond
> +// 2 will be rejected. This restriction is caused by limited BDA slots.
> +// To support more than 2 hard drives, we need to enable CONFIG_DYNAMIC_MAP_HD.
>  void
>  map_hd_drive(struct drive_s *drive)
>  {
> @@ -260,7 +264,18 @@ map_hd_drive(struct drive_s *drive)
>      struct bios_data_area_s *bda = MAKE_FLATPTR(SEG_BDA, 0);
>      int hdid = bda->hdcount;
>      dprintf(3, "Mapping hd drive %p to %d\n", drive, hdid);
> add_drive(IDMap[EXTTYPE_HD], &bda->hdcount, drive);
>
> // To implement dynamic hard drive maping, we constantly keep
>
> // bda->hdcount = 1, and use hdid = 0 all the times.
>
> // map_hd_drive() must be called each time before boot to a
>
> // specific hard drive.
>
> if (CONFIG_DYNAMIC_MAP_HD) {
>    hdid = 0; // 0 - always the first hard drive
>    u8 tmp = 0;
>    add_drive(IDMap[EXTTYPE_HD], &tmp, drive);
>    bda->hdcount = 1;
>
> } else {
>    add_drive(IDMap[EXTTYPE_HD], &bda->hdcount, drive);
>
> }
>
> // Setup disk geometry translation.
> setup_translation(drive);
>
> diff --git a/src/boot.c b/src/boot.c
> index 1effd80..2e6e356 100644
> --- a/src/boot.c
> +++ b/src/boot.c
> @@ -802,7 +802,7 @@ static int HaveHDBoot, HaveFDBoot;
>  static void
>  add_bev(int type, u32 vector)
>  {
> if (type == IPL_TYPE_HARDDISK && HaveHDBoot++)
> if (type == IPL_TYPE_HARDDISK && HaveHDBoot++ && !CONFIG_DYNAMIC_MAP_HD)
>     return;
> if (type == IPL_TYPE_FLOPPY && HaveFDBoot++)
>     return;
>
> @@ -837,8 +837,14 @@ bcv_prepboot(void)
>              add_bev(IPL_TYPE_FLOPPY, 0);
>              break;
>          case IPL_TYPE_HARDDISK:
>        map_hd_drive(pos->drive);
>        add_bev(IPL_TYPE_HARDDISK, 0);
>        if (CONFIG_DYNAMIC_MAP_HD) {
>            // Pass the drive_s pointer to bev_s struct so that
>            // we can do dynamic hard disk map in do_boot().
>            add_bev(IPL_TYPE_HARDDISK, (u32)pos->drive);
>        } else {
>            map_hd_drive(pos->drive);
>            add_bev(IPL_TYPE_HARDDISK, 0);
>        }
>        break;
>    case IPL_TYPE_CDROM:
>        map_cd_drive(pos->drive);
>
> @@ -998,6 +1004,8 @@ do_boot(int seq_nr)
>          break;
>      case IPL_TYPE_HARDDISK:
>          printf("Booting from Hard Disk...\n");
>    if (CONFIG_DYNAMIC_MAP_HD)
>        map_hd_drive((struct drive_s *)ie->vector);
>    boot_disk(0x80, 1);

This is not valid.  Once we start the boot phase (via INT19), it's not
valid to change the hard drive mappings.  Once we invoke INT19, it's
possible for external code to alter memory - including any temporary
storage that SeaBIOS allocated during the post phase.  Indeed, on
normal QEMU the memory holding the active drives is marked as
read-only after the POST phase.  Also, external code can invoke the
SeaBIOS int19 handler multiple times.

In accordance with the "BIOS Boot Specification", SeaBIOS does all its
setup in the POST phase, and then just starts the boot process in the
"BOOT phase".

-Kevin

[PATCH 4/4] boot: Support booting more than 2 hard drives

[Gao, Bin]

> > diff --git a/src/boot.c b/src/boot.c
> > index 1effd80..2e6e356 100644
> > --- a/src/boot.c
> > +++ b/src/boot.c
> > @@ -802,7 +802,7 @@ static int HaveHDBoot, HaveFDBoot;
> >  static void
> >  add_bev(int type, u32 vector)
> >  {
> > if (type == IPL_TYPE_HARDDISK && HaveHDBoot++)
> > if (type == IPL_TYPE_HARDDISK && HaveHDBoot++ && !CONFIG_DYNAMIC_MAP_HD)
> >     return;
> > if (type == IPL_TYPE_FLOPPY && HaveFDBoot++)
> >     return;
> >
> > @@ -837,8 +837,14 @@ bcv_prepboot(void)
> >              add_bev(IPL_TYPE_FLOPPY, 0);
> >              break;
> >          case IPL_TYPE_HARDDISK:
> >        map_hd_drive(pos->drive);
> >        add_bev(IPL_TYPE_HARDDISK, 0);
> >        if (CONFIG_DYNAMIC_MAP_HD) {
> >            // Pass the drive_s pointer to bev_s struct so that
> >            // we can do dynamic hard disk map in do_boot().
> >            add_bev(IPL_TYPE_HARDDISK, (u32)pos->drive);
> >        } else {
> >            map_hd_drive(pos->drive);
> >            add_bev(IPL_TYPE_HARDDISK, 0);
> >        }
> >        break;
> >    case IPL_TYPE_CDROM:
> >        map_cd_drive(pos->drive);
> >
> > @@ -998,6 +1004,8 @@ do_boot(int seq_nr)
> >          break;
> >      case IPL_TYPE_HARDDISK:
> >          printf("Booting from Hard Disk...\n");
> >    if (CONFIG_DYNAMIC_MAP_HD)
> >        map_hd_drive((struct drive_s *)ie->vector);
> >    boot_disk(0x80, 1);
>
> This is not valid.  Once we start the boot phase (via INT19), it's not
> valid to change the hard drive mappings.  Once we invoke INT19, it's
> possible for external code to alter memory - including any temporary
> storage that SeaBIOS allocated during the post phase.  Indeed, on
> normal QEMU the memory holding the active drives is marked as
> read-only after the POST phase.  Also, external code can invoke the
> SeaBIOS int19 handler multiple times.
>
> In accordance with the "BIOS Boot Specification", SeaBIOS does all its
> setup in the POST phase, and then just starts the boot process in the
> "BOOT phase".

Essentially the map_hd_drive() function only touches struct drive_s{} 
and its parent container, IDMap[], BEV[] and BDA. The first one is typically 
malloc_high()ed, the last three are static/BSS. So if external code alters
memory it will not affect map_hd_drive(). With that said, the dynamic hd map
and the original static hd map don't have any difference, except that
dynamic map can support more hard drives. Also the new code is gated by a
configuration option CONFIG_DYNAMIC_MAP_HD and it's disabled by default,
so it won't break the BBS spec.
Would you consider this as an experiment feature?
> -Kevin

[Paul Menzel]

Dear Bin,

Thank you very much for your patches.

Am 01.01.21 um 04:08 schrieb Bin Gao:
> On real hardware especially server platforms, there might be multiple

Please give a concrete example of the hardware.
> root buses, thus pci bus number could run up to 255. This patch fixed

Nit: I’d use present tense in commit messages: This patch fixes ….
> pci_probe_devices() by allowing to scan all 256 pci bus numbers(0-255).

Nit: Space before (.
> Signed-off-by: Bin Gao
>
> src/hw/pcidevice.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/src/hw/pcidevice.c b/src/hw/pcidevice.c
> index 8853cf7..8d9c401 100644
> --- a/src/hw/pcidevice.c
> +++ b/src/hw/pcidevice.c
> @@ -26,6 +26,14 @@ pci_probe_devices(void)
>       struct hlist_node **pprev = &PCIDevices.first;
>       int extraroots = romfile_loadint("etc/extra-pci-roots", 0);
>       int bus = -1, lastbus = 0, rootbuses = 0, count=0;
> // There might be multiple PCI root buses on physical hardware especially
> // server platforms, thus bus number could run up to the top value,
> // i.e. 0xff. Setting extraroots to 0xff to ensure we can enumerate all
> // PCI bus numbers.
> if (CONFIG_CSM)
>    extraroots = 0xff;

Can there be a romfile with CSM? If yes, the configured value would be 
overwritten. Maybe add a log message in that case?
>   while (bus < 0xff && (bus < MaxPCIBus || rootbuses < extraroots)) {
>       bus++;
>       int bdf;

Kind regards,

Paul

[Gao, Bin]

> Dear Bin,
>
> Thank you very much for your patches.
>
> Am 01.01.21 um 04:08 schrieb Bin Gao:
> > On real hardware especially server platforms, there might be multiple
>
> Please give a concrete example of the hardware.

All Intel server processors have multiple PCI roots, called IIO.
> > root buses, thus pci bus number could run up to 255. This patch fixed
>
> Nit: I’d use present tense in commit messages: This patch fixes ….
> > pci_probe_devices() by allowing to scan all 256 pci bus numbers(0-255).
>
> Nit: Space before (.

Will fix this (and tense) in next revision.
> > Signed-off-by: Bin Gao
> >
> > src/hw/pcidevice.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> >
> > diff --git a/src/hw/pcidevice.c b/src/hw/pcidevice.c
> > index 8853cf7..8d9c401 100644
> > --- a/src/hw/pcidevice.c
> > +++ b/src/hw/pcidevice.c
> > @@ -26,6 +26,14 @@ pci_probe_devices(void)
> >       struct hlist_node **pprev = &PCIDevices.first;
> >       int extraroots = romfile_loadint("etc/extra-pci-roots", 0);
> >       int bus = -1, lastbus = 0, rootbuses = 0, count=0;
> > // There might be multiple PCI root buses on physical hardware especially
> > // server platforms, thus bus number could run up to the top value,
> > // i.e. 0xff. Setting extraroots to 0xff to ensure we can enumerate all
> > // PCI bus numbers.
> > if (CONFIG_CSM)
> >    extraroots = 0xff;
>
> Can there be a romfile with CSM? If yes, the configured value would be
> overwritten. Maybe add a log message in that case?

There is no romfile with CSM.
I do think the correct fix is to scan all the PCI buses, instead of to get a default max bus number. The fact is,
on physical hardware, we can't assume the max bus number. On typical server design, it easily consumes
up all the bus numbers, e.g. some PCIe bridges could pad up with many buses. On Intel server processors, for instance,
we may want to split the 256 bus number to all IIOs so it's almost sure that we can reach up the maximal possible
bus number (in a single PC segment).
Yes, I can add a log message in that case.
> >   while (bus < 0xff && (bus < MaxPCIBus || rootbuses < extraroots)) {
> >       bus++;
> >       int bdf;
>
> Kind regards,
>
> Paul

[Kevin O'Connor]

On Thu, Dec 31, 2020 at 07:08:23PM -0800, Bin Gao wrote:
> On real hardware especially server platforms, there might be multiple
> root buses, thus pci bus number could run up to 255. This patch fixed
> pci_probe_devices() by allowing to scan all 256 pci bus numbers(0-255).
> Signed-off-by: Bin Gao
>
> src/hw/pcidevice.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/src/hw/pcidevice.c b/src/hw/pcidevice.c
> index 8853cf7..8d9c401 100644
> --- a/src/hw/pcidevice.c
> +++ b/src/hw/pcidevice.c
> @@ -26,6 +26,14 @@ pci_probe_devices(void)
>      struct hlist_node **pprev = &PCIDevices.first;
>      int extraroots = romfile_loadint("etc/extra-pci-roots", 0);
>      int bus = -1, lastbus = 0, rootbuses = 0, count=0;
> // There might be multiple PCI root buses on physical hardware especially
> // server platforms, thus bus number could run up to the top value,
> // i.e. 0xff. Setting extraroots to 0xff to ensure we can enumerate all
> // PCI bus numbers.
> if (CONFIG_CSM)
>    extraroots = 0xff;

This doesn't look correct to me.  It would take a noticeable amount of
time to scan every possible PCI root bus.  It would introduce a
regression for any current users of CONFIG_CSM.

The "etc/extra-pci-roots" config is intended to allow users to make
these choices at runtime.  If more configurability is desired on
CONFIG_CSM, then it will be needed to figure out a way to pass options
to SeaBIOS when in that mode.

-Kevin

[Gao, Bin]

> On Thu, Dec 31, 2020 at 07:08:23PM -0800, Bin Gao wrote:
> > On real hardware especially server platforms, there might be multiple
> > root buses, thus pci bus number could run up to 255. This patch fixed
> > pci_probe_devices() by allowing to scan all 256 pci bus numbers(0-255).
> > Signed-off-by: Bin Gao
> >
> > src/hw/pcidevice.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/src/hw/pcidevice.c b/src/hw/pcidevice.c
> > index 8853cf7..8d9c401 100644
> > --- a/src/hw/pcidevice.c
> > +++ b/src/hw/pcidevice.c
> > @@ -26,6 +26,14 @@ pci_probe_devices(void)
> >      struct hlist_node **pprev = &PCIDevices.first;
> >      int extraroots = romfile_loadint("etc/extra-pci-roots", 0);
> >      int bus = -1, lastbus = 0, rootbuses = 0, count=0;
> > // There might be multiple PCI root buses on physical hardware especially
> > // server platforms, thus bus number could run up to the top value,
> > // i.e. 0xff. Setting extraroots to 0xff to ensure we can enumerate all
> > // PCI bus numbers.
> > if (CONFIG_CSM)
> >    extraroots = 0xff;
>
> This doesn't look correct to me.  It would take a noticeable amount of
> time to scan every possible PCI root bus.  It would introduce a
> regression for any current users of CONFIG_CSM.

On real hardware scanning every possible PCI root bus wouldn't be slow so it's not a problem. Would it be reasonable to scan all pci buses only on physical hardware?
if (CONFIG_CSM && ! CONFIG_QEMU_HARDWARE)
       extraroots = 0xff;
> The "etc/extra-pci-roots" config is intended to allow users to make
> these choices at runtime.  If more configurability is desired on
> CONFIG_CSM, then it will be needed to figure out a way to pass options
> to SeaBIOS when in that mode.

So far I only need this extraroots config on CONFIG_CSM.
I didn't see an easy way to pass options from UEFI BIOS to SeaBIOS, unless we extend the CSM protocol defined by Intel.
> -Kevin