[SeaBIOS] Re: [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: 
<2021043917.i6tsrnaymcly5...@sirius.home.kraxel.org>)?



Kind regards,

Paul
___
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

[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 = 
>>   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

[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 -- 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