On 08/15/14 09:12, Laszlo Ersek wrote: > On 08/15/14 02:19, reza.jel...@tuhh.de wrote: >> From: Reza Jelveh <reza.jel...@tuhh.de> >> >> Removed: >> - IntelFrameworkModulePkg/Bus/Pci/IdeBusDxe/IdeBusDxe.inf >> (provides gEfiDiskInfoProtocolGuid, gEfiBlockIoProtocolGuid) >> - PcAtChipsetPkg/Bus/Pci/IdeControllerDxe/IdeControllerDxe.inf >> (provides gEfiIdeControllerInitProtocolGuid) >> >> Added: >> - MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf >> (provides gEfiDiskInfoProtocolGuid, gEfiBlockIoProtocolGuid, >> >> \--> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf >> (provides gEfiAtaPassThruProtocolGuid, >> gEfiExtScsiPassThruProtocolGuid) >> gEfiBlockIo2ProtocolGuid, gEfiStorageSecurityCommandProtocolGuid) >> >> \--> PcAtChipsetPkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf >> (provides gEfiIdeControllerInitProtocolGuid) >> >> AtaBusDxe enumerates all ports. AtaAtapiPassthru then enumerates the ATA >> devices in IDE and AHCI mode seperately on those ports. It then notifies >> the SataController. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Reza Jelveh <reza.jel...@tuhh.de> >> --- >> OvmfPkg/OvmfPkgIa32.dsc | 5 +++-- >> OvmfPkg/OvmfPkgIa32.fdf | 5 +++-- >> OvmfPkg/OvmfPkgIa32X64.dsc | 5 +++-- >> OvmfPkg/OvmfPkgIa32X64.fdf | 5 +++-- >> OvmfPkg/OvmfPkgX64.dsc | 5 +++-- >> OvmfPkg/OvmfPkgX64.fdf | 5 +++-- >> 6 files changed, 18 insertions(+), 12 deletions(-) >> >> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc >> index f7064b7..9049455 100644 >> --- a/OvmfPkg/OvmfPkgIa32.dsc >> +++ b/OvmfPkg/OvmfPkgIa32.dsc >> @@ -452,8 +452,9 @@ >> MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf >> MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf >> MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf >> - IntelFrameworkModulePkg/Bus/Pci/IdeBusDxe/IdeBusDxe.inf >> - PcAtChipsetPkg/Bus/Pci/IdeControllerDxe/IdeControllerDxe.inf >> + PcAtChipsetPkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf >> + MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf >> + MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf >> MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf >> MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf >> MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf >> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf >> index 243cff3..ce9e642 100644 >> --- a/OvmfPkg/OvmfPkgIa32.fdf >> +++ b/OvmfPkg/OvmfPkgIa32.fdf >> @@ -250,8 +250,9 @@ INF >> MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf >> INF MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf >> INF MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf >> INF MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf >> -INF IntelFrameworkModulePkg/Bus/Pci/IdeBusDxe/IdeBusDxe.inf >> -INF PcAtChipsetPkg/Bus/Pci/IdeControllerDxe/IdeControllerDxe.inf >> +INF PcAtChipsetPkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf >> +INF MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf >> +INF MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf >> INF MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf >> INF MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf >> INF MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf >> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc >> index 26d1132..67c520a 100644 >> --- a/OvmfPkg/OvmfPkgIa32X64.dsc >> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc >> @@ -459,8 +459,9 @@ >> MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf >> MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf >> MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf >> - IntelFrameworkModulePkg/Bus/Pci/IdeBusDxe/IdeBusDxe.inf >> - PcAtChipsetPkg/Bus/Pci/IdeControllerDxe/IdeControllerDxe.inf >> + PcAtChipsetPkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf >> + MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf >> + MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf >> MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf >> MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf >> MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf >> diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf >> index 67c5f9c..e2198e1 100644 >> --- a/OvmfPkg/OvmfPkgIa32X64.fdf >> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf >> @@ -250,8 +250,9 @@ INF >> MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf >> INF MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf >> INF MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf >> INF MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf >> -INF IntelFrameworkModulePkg/Bus/Pci/IdeBusDxe/IdeBusDxe.inf >> -INF PcAtChipsetPkg/Bus/Pci/IdeControllerDxe/IdeControllerDxe.inf >> +INF PcAtChipsetPkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf >> +INF MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf >> +INF MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf >> INF MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf >> INF MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf >> INF MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf >> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc >> index 66459c2..161f783 100644 >> --- a/OvmfPkg/OvmfPkgX64.dsc >> +++ b/OvmfPkg/OvmfPkgX64.dsc >> @@ -457,8 +457,9 @@ >> MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf >> MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf >> MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf >> - IntelFrameworkModulePkg/Bus/Pci/IdeBusDxe/IdeBusDxe.inf >> - PcAtChipsetPkg/Bus/Pci/IdeControllerDxe/IdeControllerDxe.inf >> + PcAtChipsetPkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf >> + MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf >> + MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf >> MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf >> MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf >> MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf >> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf >> index 1b029b8..cebdf29 100644 >> --- a/OvmfPkg/OvmfPkgX64.fdf >> +++ b/OvmfPkg/OvmfPkgX64.fdf >> @@ -250,8 +250,9 @@ INF >> MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf >> INF MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf >> INF MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf >> INF MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf >> -INF IntelFrameworkModulePkg/Bus/Pci/IdeBusDxe/IdeBusDxe.inf >> -INF PcAtChipsetPkg/Bus/Pci/IdeControllerDxe/IdeControllerDxe.inf >> +INF PcAtChipsetPkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf >> +INF MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf >> +INF MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf >> INF MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf >> INF MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf >> INF MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf >> > > Looks good to me, if patch 2/6 is accepted by the DuetPkg and > PcAtChipsetPkg maintainers. > > I have an additional request: > > (1a) Please build an OVMF binary *without* the patch. > (1b) Enabling DEBUG_VERBOSE (0x00400000) in PcdDebugPrintErrorLevel. > (1c) Then install a UEFI guest OS (eg. Fedora) on a (traditional) IDE > drive, and boot it. Make sure that you use ,bootindex=... on the > qemu command line for the IDE disk device. > (1d) Please capture the debug console output from OVMF, especially the > section between "SetBootOrderFromQemu: FwCfg:" and > "SetBootOrderFromQemu: setting BootOrder: success". > > Then, > > (2a) Rebuild OVMF with your patch. > (2b) DEBUG_VERBOSE should remain enabled. > (2c) Repeat (1c), but this time the same IDE disk should be hanging off > a SATA port. Make sure that again you use ,bootindex=... > (2d) Please capture the log again. > > This exercise targets the following: I'd like to see if the IDE device, > now driven by the SATA controller driver, gets the exact same UEFI > device path as before in edk2, *and* if QEMU generates the exact same > OpenFirmware device path as before for the IDE device in the "bootorder" > fw_cfg file. > > Quoting "OvmfPkg/Library/PlatformBdsLib/QemuBootOrder.c", function > TranslateOfwNodes(): > > // > // OpenFirmware device path (IDE disk, IDE CD-ROM): > // > // /pci@i0cf8/ide@1,1/drive@0/disk@0 > // ^ ^ ^ ^ ^ > // | | | | master or slave > // | | | primary or secondary > // | PCI slot & function holding IDE controller > // PCI root at system bus port, PIO > // > // UEFI device path: > // > // PciRoot(0x0)/Pci(0x1,0x1)/Ata(Primary,Master,0x0) > // ^ > // fixed LUN > // > > If after your patch the IDE device gets a different OFW devpath from > QEMU, and/or it gets a different UEFI devpath in edk2, then the above > prefix matching in TranslateOfwNodes() will have to be extended; a new > branch will be needed that maps the new OFW devpath to the new UEFI devpath. > > Otherwise OVMF won't be able to reorder UEFI boot options according to > the presence of the IDE device in the "bootorder" fw_cfg file.
... actually, I guess the goal of this patch is to enable AHCI mode ATA devices while keeping IDE mode ATA devices working. So, not only should we ensure that IDE mode ATA devices remain in working state wrt. device paths (ie. TranslateOfwNodes()) -- no regressions --, we should also see if (and how) the *new*, AHCI mode ATA devices are listed by QEMU, and listed by edk2. IOW, the old OFW devpath -> UEFI devpath fragment should be kept working (for IDE mode devices), and (likely) a new mapping should be added for AHCI mode devices. I know zilch about SATA, so *maybe* the AHCI mode devices will be covered by one of the existent mappings (eg. the one for virtio-scsi disks?...) Thanks, Laszlo ------------------------------------------------------------------------------ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
