Re: [edk2] [PATCH 3/6] OvmfPkg: enable SATA controller

Subject: Re: [edk2] [PATCH 3/6] OvmfPkg: enable SATA controller

From: Laszlo Ersek <lersek@redhat.com>

To: edk2-devel@lists.sourceforge.net

Date: 2014-08-15 17:12:35

On 08/15/14 02:19, reza.jelveh@tuhh.de wrote:
> From: Reza Jelveh 
> 
> 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 
> ---
>  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.

Thanks,
Laszlo

------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel