[SeaBIOS] Re: [PATCH 3/4] csm: Move pci_probe_devices() from InitializeYourself phase to PreparToBoot phase
+// 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: ><2021043917.i6tsrnaymcly5...@sirius.home.kraxel.org>)? 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 -- 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 4/4] boot: Support booting more than 2 hard drives
>> 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
___
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
>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
___
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
>> 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
___
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
> 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.
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
___
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
>
>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 =
>> 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
___
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
>
>
>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 -- 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
