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

2021-01-17 Thread 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], >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], , drive);
> +bda->hdcount = 1;
> +} else {
> +add_drive(IDMap[EXTTYPE_HD], >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
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org


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

2021-01-17 Thread 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
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org


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

2021-01-17 Thread 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
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org


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

2021-01-17 Thread 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 = 
>  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
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org