Re: [edk2] [PATCH v4 05/10] OvmfPkg: SataControllerDxe: enable IO / mem access and DMA when binding

[Paolo Bonzini]

On 16/09/2015 04:57, Laszlo Ersek wrote:
> (OvmfPkg's copy of SataControllerDxe must differ from the same in DuetPkg
> because Duet inherits a pre-configured SATA controller from the BIOS, as
> explained by Feng.)

It's not true that it _must_ differ.  Duet _may_ avoid setting the bits
because it's using the pre-configured SATA controller, but it also _may_
set them.

Definitely both Windows and Linux set the bits on their own, for example.

In fact if the BIOS doesn't have an AHCI driver (you can try with
SeaBIOS) Duet would need exactly the same patches 5 and 6.

Paolo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v4 05/10] OvmfPkg: SataControllerDxe: enable IO / mem access and DMA when binding

[Tian, Feng]

Hi, Laszlo

There is a macro EFI_PCI_DEVICE_ENABLE, which is (EFI_PCI_IO_ATTRIBUTE_IO | 
EFI_PCI_IO_ATTRIBUTE_MEMORY | EFI_PCI_IO_ATTRIBUTE_BUS_MASTER)

Suggest to use this macro. Others look good to me. Reviewed-by: Feng Tian 
<feng.t...@intel.com>

Thanks
Feng

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
Ersek
Sent: Wednesday, September 16, 2015 10:57
To: edk2-de...@ml01.01.org
Cc: Tian, Feng; Justen, Jordan L; Gabriel L. Somlo; Alexander Graf; Hannes 
Reinecke; Reza Jelveh
Subject: [edk2] [PATCH v4 05/10] OvmfPkg: SataControllerDxe: enable IO / mem 
access and DMA when binding

When we bind the SATA controller in SataControllerStart(), we read the NP 
("Number of Ports") bitfield from the CAP ("HBA Capabilities") register of the 
controller. (See the AHCI 1.3.1 spec.)

This register is memory mapped. If we'd like to access it, we must at least 
enable memory space access for the device. In addition, Feng Tian recommended 
enabling Bus Master DMA in 
<http://thread.gmane.org/gmane.comp.bios.tianocore.devel/10545/focus=10659>.
We also enable IO space access for completeness.

Further, because we change the PCI attributes of the device with the above when 
binding it, we must also restore its original PCI attributes when unbinding it. 
See the Driver Writer's Guide for UEFI 2.3.1 v1.01, section
18.3 "PCI drivers" | 18.3.2 "Start() and Stop()".

(OvmfPkg's copy of SataControllerDxe must differ from the same in DuetPkg 
because Duet inherits a pre-configured SATA controller from the BIOS, as 
explained by Feng.)

Cc: Alexander Graf <ag...@suse.de>
Cc: Reza Jelveh <reza.jel...@tuhh.de>
Cc: Jordan Justen <jordan.l.jus...@intel.com>
Cc: Hannes Reinecke <h...@suse.de>
Cc: Gabriel L. Somlo <so...@cmu.edu>
Cc: Feng Tian <feng.t...@intel.com>
Suggested-by: Feng Tian <feng.t...@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <ler...@redhat.com>
---
 OvmfPkg/SataControllerDxe/SataController.h |  5 +++  
OvmfPkg/SataControllerDxe/SataController.c | 39 +++-
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/SataControllerDxe/SataController.h 
b/OvmfPkg/SataControllerDxe/SataController.h
index a6c6c16..e5b719e 100644
--- a/OvmfPkg/SataControllerDxe/SataController.h
+++ b/OvmfPkg/SataControllerDxe/SataController.h
@@ -86,6 +86,11 @@ typedef struct _EFI_SATA_CONTROLLER_PRIVATE_DATA {
   EFI_PCI_IO_PROTOCOL   *PciIo;
 
   //
+  // Original PCI attributes
+  //
+  UINT64OriginalPciAttributes;
+
+  //
   // The number of devices that are supported by this channel
   //
   UINT8 DeviceCount;
diff --git a/OvmfPkg/SataControllerDxe/SataController.c 
b/OvmfPkg/SataControllerDxe/SataController.c
index 5e7e23b..275b527 100644
--- a/OvmfPkg/SataControllerDxe/SataController.c
+++ b/OvmfPkg/SataControllerDxe/SataController.c
@@ -390,6 +390,7 @@ SataControllerStart (  {
   EFI_STATUSStatus;
   EFI_PCI_IO_PROTOCOL   *PciIo;
+  UINT64OriginalPciAttributes;
   PCI_TYPE00PciData;
   EFI_SATA_CONTROLLER_PRIVATE_DATA  *SataPrivateData;
   UINT32Data32;
@@ -415,12 +416,33 @@ SataControllerStart (
   }
 
   //
+  // Save original PCI attributes, and enable IO space access, memory 
+ space  // access, and Bus Master (DMA).
+  //
+  Status = PciIo->Attributes (PciIo, EfiPciIoAttributeOperationGet, 0,
+);  if (EFI_ERROR (Status)) {
+goto ClosePciIo;
+  }
+  Status = PciIo->Attributes (
+PciIo,
+EfiPciIoAttributeOperationEnable,
+(EFI_PCI_IO_ATTRIBUTE_IO |
+ EFI_PCI_IO_ATTRIBUTE_MEMORY |
+ EFI_PCI_IO_ATTRIBUTE_BUS_MASTER),
+NULL
+);
+  if (EFI_ERROR (Status)) {
+goto ClosePciIo;
+  }
+
+  //
   // Allocate Sata Private Data structure
   //
   SataPrivateData = AllocateZeroPool (sizeof 
(EFI_SATA_CONTROLLER_PRIVATE_DATA));
   if (SataPrivateData == NULL) {
 Status = EFI_OUT_OF_RESOURCES;
-goto ClosePciIo;
+goto RestorePciAttributes;
   }
 
   //
@@ -428,6 +450,7 @@ SataControllerStart (
   //
   SataPrivateData->Signature = SATA_CONTROLLER_SIGNATURE;
   SataPrivateData->PciIo = PciIo;
+  SataPrivateData->OriginalPciAttributes = OriginalPciAttributes;
   SataPrivateData->IdeInit.GetChannelInfo = IdeInitGetChannelInfo;
   SataPrivateData->IdeInit.NotifyPhase = IdeInitNotifyPhase;
   SataPrivateData->IdeInit.SubmitData = IdeInitSubmitData; @@ -512,6 +535,10 
@@ FreeDisqualifiedModes:
 FreeSataPrivateData:
   FreePool (SataPrivateData);
 
+RestorePciAttributes:
+  PciIo->Attributes (PciIo, EfiPciIoAttributeOperati