| Message ID | 20190619092352.23583-4-shmuel.eiderman@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Qemu to SeaBIOS LCHS interface | expand |
On Wed, Jun 19, 2019 at 12:23:51PM +0300, Sam Eiderman wrote: > Adding the following utility functions: > > * boot_lchs_find_pci_device > * boot_lchs_find_scsi_device > * boot_lchs_find_ata_device FWIW, this leads to a bit of code duplication. I think it would be preferable to refactor the bootprio_find_XYZ() calls. Instead of returning an 'int prio' they could return a znprintf'd 'char *devpath' instead. Then the boot_add_XYZ() calls could directly call find_prio(devpath). The boot_add_hd() could then directly populate drive->lchs or call setup_translation(). -Kevin
Sounds reasonable, how do purpose to deal with: config BIOS_GEOMETRY config BOOTORDER precompiler optouts? If we don’t need any of them we also don’t need to call “get_scsi_devpath", “get_ata_devpath”, “get_pci_dev_path”. I’ll see what can be done. > On 20 Jun 2019, at 17:37, Kevin O'Connor <kevin@koconnor.net> wrote: > > On Wed, Jun 19, 2019 at 12:23:51PM +0300, Sam Eiderman wrote: >> Adding the following utility functions: >> >> * boot_lchs_find_pci_device >> * boot_lchs_find_scsi_device >> * boot_lchs_find_ata_device > > FWIW, this leads to a bit of code duplication. I think it would be > preferable to refactor the bootprio_find_XYZ() calls. Instead of > returning an 'int prio' they could return a znprintf'd 'char *devpath' > instead. Then the boot_add_XYZ() calls could directly call > find_prio(devpath). The boot_add_hd() could then directly populate > drive->lchs or call setup_translation(). > > -Kevin
On Fri, Jun 21, 2019 at 08:42:28PM +0300, Sam Eiderman wrote: > Sounds reasonable, how do purpose to deal with: > > config BIOS_GEOMETRY > config BOOTORDER > > precompiler optouts? I think you can stick them both under BOOTORDER. That option is only there in case someone wants to reduce the size of the SeaBIOS binary. I can't think of a reasonable situation where one cares that much about binary size, yet wants to override legacy disk translations.. > If we don’t need any of them we also don’t need to call “get_scsi_devpath", “get_ata_devpath”, “get_pci_dev_path”. > > I’ll see what can be done. Thanks. -Kevin
But maybe someone wants bootorder but doesn’t want to override legacy disk translations…
I’m thinking of maybe adding
if (!CONFIG_BOOTORDER || !CONFIG_BIOS_GEOMETRY)
return NULL;
In each of the get_*_devpath functions (which will normally return an allocated string, not on stack).
Another approach can be make CONFIG_BIOS_GEOMETRY depend on CONFIG_BOOTORDER.
Then we should only keep:
if (!CONFIG_BOOTORDER)
return NULL;
In the get_*_devpath functions.
I think the first approach will look better when reading the code - will not require the reader to
analize dependancies in the Kconfig file.
Sam
> On 21 Jun 2019, at 21:59, Kevin O'Connor <kevin@koconnor.net> wrote:
>
> On Fri, Jun 21, 2019 at 08:42:28PM +0300, Sam Eiderman wrote:
>> Sounds reasonable, how do purpose to deal with:
>>
>> config BIOS_GEOMETRY
>> config BOOTORDER
>>
>> precompiler optouts?
>
> I think you can stick them both under BOOTORDER. That option is only
> there in case someone wants to reduce the size of the SeaBIOS binary.
> I can't think of a reasonable situation where one cares that much
> about binary size, yet wants to override legacy disk translations..
>
>> If we don’t need any of them we also don’t need to call “get_scsi_devpath", “get_ata_devpath”, “get_pci_dev_path”.
>>
>> I’ll see what can be done.
>
> Thanks.
> -Kevin
On Sat, Jun 22, 2019 at 11:51:48AM +0300, Sam Eiderman wrote: > But maybe someone wants bootorder but doesn’t want to override legacy disk translations… > > I’m thinking of maybe adding > > if (!CONFIG_BOOTORDER || !CONFIG_BIOS_GEOMETRY) > return NULL; That's fine - though it's (!CONFIG_BOOTORDER && !CONFIG_BIOS_GEOMETRY). FYI, I think BIOS_GEOMETRY is a little confusing - maybe CUSTOM_DISK_GEOMETRY. -Kevin
> On 22 Jun 2019, at 18:27, Kevin O'Connor <kevin@koconnor.net> wrote: > > On Sat, Jun 22, 2019 at 11:51:48AM +0300, Sam Eiderman wrote: >> But maybe someone wants bootorder but doesn’t want to override legacy disk translations… >> >> I’m thinking of maybe adding >> >> if (!CONFIG_BOOTORDER || !CONFIG_BIOS_GEOMETRY) >> return NULL; > > That's fine - though it's (!CONFIG_BOOTORDER && !CONFIG_BIOS_GEOMETRY). Yes of course, my bad > > FYI, I think BIOS_GEOMETRY is a little confusing - maybe > CUSTOM_DISK_GEOMETRY. The thing is that disk geometry is actually (physical geometry, reported by the disk controller) and here bios geometry stands for the geometry reported from bios int13. Also “bios geometry” === “logical geometry” === “lchs”. So following previous discussion with Gerd, maybe CONFIG_HOST_BIOS_GEOMETRY is better? Sam > > -Kevin
Kevin,
Rethinking this change (where we construct the device path from outside and call boot_prio_find()),
this is pretty tricky to implement since we need to take care of csm_bootprio_ata() and
csm_bootprio_pci() which do not work with device path.
In addition,
bootprio_find_fdc_device
bootprio_find_pci_rom
bootprio_find_named_rom
bootprio_find_usb
Will not require this change and this will just result in too much refactoring.
Maybe simply introduce build_scsi_path() and build_ata_path() functions and then,
for instance, make booprio_find_scsi_device() and boot_lchs_find_scsi_device()
call the same build_scsi_path() function, resulting in less code duplication.
Sam
> On 22 Jun 2019, at 20:33, Sam Eiderman <shmuel.eiderman@oracle.com> wrote:
>
>
>
>> On 22 Jun 2019, at 18:27, Kevin O'Connor <kevin@koconnor.net> wrote:
>>
>> On Sat, Jun 22, 2019 at 11:51:48AM +0300, Sam Eiderman wrote:
>>> But maybe someone wants bootorder but doesn’t want to override legacy disk translations…
>>>
>>> I’m thinking of maybe adding
>>>
>>> if (!CONFIG_BOOTORDER || !CONFIG_BIOS_GEOMETRY)
>>> return NULL;
>>
>> That's fine - though it's (!CONFIG_BOOTORDER && !CONFIG_BIOS_GEOMETRY).
>
> Yes of course, my bad
>
>>
>> FYI, I think BIOS_GEOMETRY is a little confusing - maybe
>> CUSTOM_DISK_GEOMETRY.
>
> The thing is that disk geometry is actually (physical geometry, reported by the disk controller) and here
> bios geometry stands for the geometry reported from bios int13.
> Also “bios geometry” === “logical geometry” === “lchs”.
> So following previous discussion with Gerd, maybe CONFIG_HOST_BIOS_GEOMETRY is better?
>
> Sam
>
>>
>> -Kevin
>
diff --git a/src/Kconfig b/src/Kconfig index 55a87cb7..0b4c1c0d 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -72,6 +72,13 @@ endchoice help Support controlling of the boot order via the fw_cfg/CBFS "bootorder" file. + config BIOS_GEOMETRY + depends on BOOT + bool "Boot device bios geometry override" + default y + help + Support overriding bios (logical) geometry of boot devices via the + fw_cfg/CBFS "bios-geometry" file. config COREBOOT_FLASH depends on COREBOOT diff --git a/src/boot.c b/src/boot.c index 70f639f4..308d9559 100644 --- a/src/boot.c +++ b/src/boot.c @@ -105,6 +105,8 @@ parse_u32(char *cur, u32 *n) static void loadBiosGeometry(void) { + if (!CONFIG_BIOS_GEOMETRY) + return; char *f = romfile_loadfile("bios-geometry", NULL); if (!f) return; @@ -144,6 +146,75 @@ loadBiosGeometry(void) } while (f); } +// Search the bios-geometry list for the given glob pattern. +static BootDeviceLCHS * +boot_lchs_find(const char *glob) +{ + dprintf(1, "Searching bios-geometry for: %s\n", glob); + int i; + for (i = 0; i < BiosGeometryCount; i++) + if (glob_prefix(glob, BiosGeometry[i].name)) + return &BiosGeometry[i]; + return NULL; +} + +int boot_lchs_find_pci_device(struct pci_device *pci, struct chs_s *chs) +{ + if (!CONFIG_BIOS_GEOMETRY) + return -1; + char desc[256]; + build_pci_path(desc, sizeof(desc), "*", pci); + BootDeviceLCHS *b = boot_lchs_find(desc); + if (!b) + return -1; + chs->cylinder = (u16)b->lcyls; + chs->head = (u16)b->lheads; + chs->sector = (u16)b->lsecs; + return 0; +} + +int boot_lchs_find_scsi_device(struct pci_device *pci, int target, int lun, + struct chs_s *chs) +{ + if (!CONFIG_BIOS_GEOMETRY) + return -1; + if (!pci) + // support only pci machine for now + return -1; + // Find scsi drive - for example: /pci@i0cf8/scsi@5/channel@0/disk@1,0 + char desc[256], *p; + p = build_pci_path(desc, sizeof(desc), "*", pci); + snprintf(p, desc+sizeof(desc)-p, "/*@0/*@%x,%x", target, lun); + BootDeviceLCHS *b = boot_lchs_find(desc); + if (!b) + return -1; + chs->cylinder = (u16)b->lcyls; + chs->head = (u16)b->lheads; + chs->sector = (u16)b->lsecs; + return 0; +} + +int boot_lchs_find_ata_device(struct pci_device *pci, int chanid, int slave, + struct chs_s *chs) +{ + if (!CONFIG_BIOS_GEOMETRY) + return -1; + if (!pci) + // support only pci machine for now + return -1; + // Find ata drive - for example: /pci@i0cf8/ide@1,1/drive@1/disk@0 + char desc[256], *p; + p = build_pci_path(desc, sizeof(desc), "*", pci); + snprintf(p, desc+sizeof(desc)-p, "/drive@%x/disk@%x", chanid, slave); + BootDeviceLCHS *b = boot_lchs_find(desc); + if (!b) + return -1; + chs->cylinder = (u16)b->lcyls; + chs->head = (u16)b->lheads; + chs->sector = (u16)b->lsecs; + return 0; +} + /**************************************************************** * Boot priority ordering diff --git a/src/util.h b/src/util.h index e2afc80c..b173fa88 100644 --- a/src/util.h +++ b/src/util.h @@ -38,6 +38,12 @@ struct usbdevice_s; int bootprio_find_usb(struct usbdevice_s *usbdev, int lun); int get_keystroke_full(int msec); int get_keystroke(int msec); +struct chs_s; +int boot_lchs_find_pci_device(struct pci_device *pci, struct chs_s *chs); +int boot_lchs_find_scsi_device(struct pci_device *pci, int target, int lun, + struct chs_s *chs); +int boot_lchs_find_ata_device(struct pci_device *pci, int chanid, int slave, + struct chs_s *chs); // bootsplash.c void enable_vga_console(void);
Adding the following utility functions: * boot_lchs_find_pci_device * boot_lchs_find_scsi_device * boot_lchs_find_ata_device These will be used to apply LCHS values received through fw_cfg. Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> Reviewed-by: Arbel Moshe <arbel.moshe@oracle.com> Signed-off-by: Sam Eiderman <shmuel.eiderman@oracle.com> --- src/Kconfig | 7 ++++++ src/boot.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util.h | 6 ++++++ 3 files changed, 84 insertions(+)