The AtaAtapiPassThru driver maps three system memory regions for Bus
Master Common Buffer operation on the following call path, if the
controller has PCI_CLASS_MASS_STORAGE_SATADPA class code:
AtaAtapiPassThruStart()
EnumerateAttachedDevice()
AhciModeInitialization()
AhciCreateTransferDescriptor()
The device is disabled (including Bus Master DMA) when the controller is
unbound, in AtaAtapiPassThruStop(). Then the regions are unmapped.
The former step should also be done when we exit the boot services, and
the OS gains ownership of system memory.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h | 6 ++
MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 59 +++++++++++++++++++-
2 files changed, 64 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
index 85e5a5553953..8d6eac706c0b 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
@@ -119,10 +119,16 @@ typedef struct {
//
// For Non-blocking.
//
EFI_EVENT TimerEvent;
LIST_ENTRY NonBlockingTaskList;
+
+ //
+ // For disabling the device (especially Bus Master DMA) at
+ // ExitBootServices().
+ //
+ EFI_EVENT ExitBootEvent;
} ATA_ATAPI_PASS_THRU_INSTANCE;
//
// Task for Non-blocking mode.
//
diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
index a48b295d26aa..09064dda18b7 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
@@ -102,11 +102,12 @@ ATA_ATAPI_PASS_THRU_INSTANCE gAtaAtapiPassThruInstanceTemplate = {
0, // PreviousLun
NULL, // Timer event
{ // NonBlocking TaskList
NULL,
NULL
- }
+ },
+ NULL, // ExitBootEvent
};
ATAPI_DEVICE_PATH mAtapiDevicePathTemplate = {
{
MESSAGING_DEVICE_PATH,
@@ -476,10 +477,42 @@ InitializeAtaAtapiPassThru (
ASSERT_EFI_ERROR (Status);
return Status;
}
+/**
+ Disable the device (especially Bus Master DMA) when exiting the boot
+ services.
+
+ @param[in] Event Event for which this notification function is being
+ called.
+ @param[in] Context Pointer to the ATA_ATAPI_PASS_THRU_INSTANCE that
+ represents the HBA.
+**/
+VOID
+EFIAPI
+AtaPassThruExitBootServices (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ )
+{
+ ATA_ATAPI_PASS_THRU_INSTANCE *Instance;
+ EFI_PCI_IO_PROTOCOL *PciIo;
+
+ DEBUG ((DEBUG_VERBOSE, "%a: Context=0x%p\n", __FUNCTION__, Context));
+
+ Instance = Context;
+ PciIo = Instance->PciIo;
+
+ PciIo->Attributes (
+ PciIo,
+ EfiPciIoAttributeOperationDisable,
+ Instance->EnabledPciAttributes,
+ NULL
+ );
+}
+
/**
Tests to see if this driver supports a given controller. If a child device is provided,
it further tests to see if this driver supports creating a handle for the specified child device.
This function checks to see if the driver specified by This supports the device specified by
@@ -755,10 +788,21 @@ AtaAtapiPassThruStart (
Instance->AtaPassThru.Mode = &Instance->AtaPassThruMode;
Instance->ExtScsiPassThru.Mode = &Instance->ExtScsiPassThruMode;
InitializeListHead(&Instance->DeviceList);
InitializeListHead(&Instance->NonBlockingTaskList);
+ Status = gBS->CreateEvent (
+ EVT_SIGNAL_EXIT_BOOT_SERVICES,
+ TPL_CALLBACK,
+ AtaPassThruExitBootServices,
+ Instance,
+ &Instance->ExitBootEvent
+ );
+ if (EFI_ERROR (Status)) {
+ goto ErrorExit;
+ }
+
Instance->TimerEvent = NULL;
Status = gBS->CreateEvent (
EVT_TIMER | EVT_NOTIFY_SIGNAL,
TPL_NOTIFY,
@@ -808,10 +852,14 @@ ErrorExit:
if ((Instance != NULL) && (Instance->TimerEvent != NULL)) {
gBS->CloseEvent (Instance->TimerEvent);
}
+ if ((Instance != NULL) && (Instance->ExitBootEvent != NULL)) {
+ gBS->CloseEvent (Instance->ExitBootEvent);
+ }
+
//
// Remove all inserted ATA devices.
//
DestroyDeviceInfoList(Instance);
@@ -906,10 +954,19 @@ AtaAtapiPassThruStop (
if (Instance->TimerEvent != NULL) {
gBS->CloseEvent (Instance->TimerEvent);
Instance->TimerEvent = NULL;
}
DestroyAsynTaskList (Instance, FALSE);
+
+ //
+ // Close event signaled at gBS->ExitBootServices().
+ //
+ if (Instance->ExitBootEvent != NULL) {
+ gBS->CloseEvent (Instance->ExitBootEvent);
+ Instance->ExitBootEvent = NULL;
+ }
+
//
// Free allocated resource
//
DestroyDeviceInfoList (Instance);
--
2.14.1.3.gb7cf6e02401b
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Hi Star, Eric,
On 09/08/17 00:41, Laszlo Ersek wrote:
> The AtaAtapiPassThru driver maps three system memory regions for Bus
> Master Common Buffer operation on the following call path, if the
> controller has PCI_CLASS_MASS_STORAGE_SATADPA class code:
>
> AtaAtapiPassThruStart()
> EnumerateAttachedDevice()
> AhciModeInitialization()
> AhciCreateTransferDescriptor()
>
> The device is disabled (including Bus Master DMA) when the controller is
> unbound, in AtaAtapiPassThruStop(). Then the regions are unmapped.
>
> The former step should also be done when we exit the boot services, and
> the OS gains ownership of system memory.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h | 6 ++
> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 59 +++++++++++++++++++-
> 2 files changed, 64 insertions(+), 1 deletion(-)
this patch -- that is, commit 6fb8ddd36bde
("MdeModulePkg/AtaAtapiPassThru: disable the device at
ExitBootServices()", 2017-09-03) -- has caused a performance regression
in OVMF, in booting Windows installer ISOs from emulated IDE CD-ROMs.
Interestingly, the performance regression only affects the "traditional"
IDE controller of the "pc" (i440fx) machine type of QEMU; it does not
affect the AHCI/SATA controller of the "q35" machine type. My measurements:
QEMU machine OVMF time from launch to
type first install screen
-------- ------- --------------------- --------------------
v2.8.1.1 pc 704b71d7e11f 117 s
v2.8.1.1 pc 704b71d7e11f\6fb8ddd36bde 44 s
v2.8.1.1 q35 704b71d7e11f 9 s
v2.8.1.1 q35 704b71d7e11f\6fb8ddd36bde 9 s
v2.10.1 pc 704b71d7e11f 119 s
v2.10.1 pc 704b71d7e11f\6fb8ddd36bde 46 s
v2.10.1 q35 704b71d7e11f 10 s
v2.10.1 q35 704b71d7e11f\6fb8ddd36bde 9 s
(Here "704b71d7e11f" means "OVMF built at current upstream master,
704b71d7e11f"; and "\6fb8ddd36bde" means that commit 6fb8ddd36bde was
reverted on top.)
This issue was reported in:
https://bugs.launchpad.net/ubuntu/+source/edk2/+bug/1725560
Personally I think that commit 6fb8ddd36bde is correct. Under the
LaunchPad report, I wrote:
> To me this totally looks like a Windows bug; edk2 commit 6fb8ddd36bde
> does the right thing. The patch disables PCI bus master DMA for the
> IDE/AHCI controller in question at ExitBootServices(), i.e. when the
> OS gains control of the system from the firmware. At that point
> ownership of the RAM is transferred to the OS, and in-flight DMA has
> to be aborted (otherwise DMA pending from the firmware could overwrite
> RAM that is now owned by the OS). Whether a controller is passed --
> from firmware to the OS -- with DMA enabled vs. DMA disabled in the
> PCI command register, should have zero consequence on how the OS
> drives the controller subsequently, with its own native driver.
Furthermore, the increased load time of the Windows ISO is *not* only
due to the genuinely lower performance of IDE, compared to SATA; there
is at least one very long wait where Windows doesn't seem to be doing
anything at all, when the machine type is "pc" (i440fx) and this patch
is applied:
> BTW, I've also noticed that a large chunk of the delay, with i440fx,
> is not even spent loading data from the IDE CD-ROM. IDE emulation
> means host CPU load, but in this case, Windows just sits there with
> the empty purplish/bluish screen, and there is zero host CPU load --
> nothing happens. It's as if Windows was waiting for some timer to
> expire.
Is my understanding correct that you guys have never seen the same
performance regression on physical hardware?
Is that perhaps because, in practice, physical computers only use SATA
controllers these days, not traditional IDE?
I'm in a difficult situation, because (a) the patch is obviously right,
(b) the correctness of the patch does not help the QEMU / OVMF users
that suffer from the performance regression.
I'm thinking maybe we can add some "bug compatibility" code to
AtaPassThruExitBootServices(). A dynamic PCD would technically work, but
I totally understand that MdeModulePkg only accepts new PCDs if there is
absolutely no other way to solve an issue. So, what do you guys think?
Meanwhile I'll also ask my colleagues for help with debugging QEMU /
Windows, to see why exactly this change slows down Windows (on
traditional IDE controllers). It will take a while though, my team is
busy at the KVM Forum 2017.
Thanks!
Laszlo
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
> index 85e5a5553953..8d6eac706c0b 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
> @@ -119,10 +119,16 @@ typedef struct {
> //
> // For Non-blocking.
> //
> EFI_EVENT TimerEvent;
> LIST_ENTRY NonBlockingTaskList;
> +
> + //
> + // For disabling the device (especially Bus Master DMA) at
> + // ExitBootServices().
> + //
> + EFI_EVENT ExitBootEvent;
> } ATA_ATAPI_PASS_THRU_INSTANCE;
>
> //
> // Task for Non-blocking mode.
> //
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> index a48b295d26aa..09064dda18b7 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> @@ -102,11 +102,12 @@ ATA_ATAPI_PASS_THRU_INSTANCE gAtaAtapiPassThruInstanceTemplate = {
> 0, // PreviousLun
> NULL, // Timer event
> { // NonBlocking TaskList
> NULL,
> NULL
> - }
> + },
> + NULL, // ExitBootEvent
> };
>
> ATAPI_DEVICE_PATH mAtapiDevicePathTemplate = {
> {
> MESSAGING_DEVICE_PATH,
> @@ -476,10 +477,42 @@ InitializeAtaAtapiPassThru (
> ASSERT_EFI_ERROR (Status);
>
> return Status;
> }
>
> +/**
> + Disable the device (especially Bus Master DMA) when exiting the boot
> + services.
> +
> + @param[in] Event Event for which this notification function is being
> + called.
> + @param[in] Context Pointer to the ATA_ATAPI_PASS_THRU_INSTANCE that
> + represents the HBA.
> +**/
> +VOID
> +EFIAPI
> +AtaPassThruExitBootServices (
> + IN EFI_EVENT Event,
> + IN VOID *Context
> + )
> +{
> + ATA_ATAPI_PASS_THRU_INSTANCE *Instance;
> + EFI_PCI_IO_PROTOCOL *PciIo;
> +
> + DEBUG ((DEBUG_VERBOSE, "%a: Context=0x%p\n", __FUNCTION__, Context));
> +
> + Instance = Context;
> + PciIo = Instance->PciIo;
> +
> + PciIo->Attributes (
> + PciIo,
> + EfiPciIoAttributeOperationDisable,
> + Instance->EnabledPciAttributes,
> + NULL
> + );
> +}
> +
> /**
> Tests to see if this driver supports a given controller. If a child device is provided,
> it further tests to see if this driver supports creating a handle for the specified child device.
>
> This function checks to see if the driver specified by This supports the device specified by
> @@ -755,10 +788,21 @@ AtaAtapiPassThruStart (
> Instance->AtaPassThru.Mode = &Instance->AtaPassThruMode;
> Instance->ExtScsiPassThru.Mode = &Instance->ExtScsiPassThruMode;
> InitializeListHead(&Instance->DeviceList);
> InitializeListHead(&Instance->NonBlockingTaskList);
>
> + Status = gBS->CreateEvent (
> + EVT_SIGNAL_EXIT_BOOT_SERVICES,
> + TPL_CALLBACK,
> + AtaPassThruExitBootServices,
> + Instance,
> + &Instance->ExitBootEvent
> + );
> + if (EFI_ERROR (Status)) {
> + goto ErrorExit;
> + }
> +
> Instance->TimerEvent = NULL;
>
> Status = gBS->CreateEvent (
> EVT_TIMER | EVT_NOTIFY_SIGNAL,
> TPL_NOTIFY,
> @@ -808,10 +852,14 @@ ErrorExit:
>
> if ((Instance != NULL) && (Instance->TimerEvent != NULL)) {
> gBS->CloseEvent (Instance->TimerEvent);
> }
>
> + if ((Instance != NULL) && (Instance->ExitBootEvent != NULL)) {
> + gBS->CloseEvent (Instance->ExitBootEvent);
> + }
> +
> //
> // Remove all inserted ATA devices.
> //
> DestroyDeviceInfoList(Instance);
>
> @@ -906,10 +954,19 @@ AtaAtapiPassThruStop (
> if (Instance->TimerEvent != NULL) {
> gBS->CloseEvent (Instance->TimerEvent);
> Instance->TimerEvent = NULL;
> }
> DestroyAsynTaskList (Instance, FALSE);
> +
> + //
> + // Close event signaled at gBS->ExitBootServices().
> + //
> + if (Instance->ExitBootEvent != NULL) {
> + gBS->CloseEvent (Instance->ExitBootEvent);
> + Instance->ExitBootEvent = NULL;
> + }
> +
> //
> // Free allocated resource
> //
> DestroyDeviceInfoList (Instance);
>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 25 October 2017 at 16:26, Laszlo Ersek <lersek@redhat.com> wrote:
> Hi Star, Eric,
>
> On 09/08/17 00:41, Laszlo Ersek wrote:
>> The AtaAtapiPassThru driver maps three system memory regions for Bus
>> Master Common Buffer operation on the following call path, if the
>> controller has PCI_CLASS_MASS_STORAGE_SATADPA class code:
>>
>> AtaAtapiPassThruStart()
>> EnumerateAttachedDevice()
>> AhciModeInitialization()
>> AhciCreateTransferDescriptor()
>>
>> The device is disabled (including Bus Master DMA) when the controller is
>> unbound, in AtaAtapiPassThruStop(). Then the regions are unmapped.
>>
>> The former step should also be done when we exit the boot services, and
>> the OS gains ownership of system memory.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h | 6 ++
>> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 59 +++++++++++++++++++-
>> 2 files changed, 64 insertions(+), 1 deletion(-)
>
> this patch -- that is, commit 6fb8ddd36bde
> ("MdeModulePkg/AtaAtapiPassThru: disable the device at
> ExitBootServices()", 2017-09-03) -- has caused a performance regression
> in OVMF, in booting Windows installer ISOs from emulated IDE CD-ROMs.
>
> Interestingly, the performance regression only affects the "traditional"
> IDE controller of the "pc" (i440fx) machine type of QEMU; it does not
> affect the AHCI/SATA controller of the "q35" machine type.
Does it make any difference if you only disable memory decoding and
bus mastering, but leave I/O port decoding enabled?
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Good point.
Could we find out what change causes the performance regression? Bus Master disable / Memory Space disable / IO Space disable?
How about to only disable Bus Master in the exit boot service event notification? It seems the key point suggested by UEFI Driver_Writer_Guide_V1.0.1_120308.pdf.
7.7
Examples from the EDK II that use this feature are the PCI device drivers for USB Host
Controllers. Some USB Host Controllers are PCI Bus Masters that continuously access a
memory buffer to poll for operation requests. Access to this memory buffer by a USB
Host Controller may be required to boot an operation system, but this activity must be
terminated when the OS calls ExitBootServices(). *The typical action in the Exit Boot
Services Event for these types of drivers is to disable the PCI bus master* and place the
USB Host Controller into a halted state
Thanks,
Star
-----Original Message-----
From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
Sent: Thursday, October 26, 2017 4:12 AM
To: Laszlo Ersek <lersek@redhat.com>
Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Yao, Jiewen <jiewen.yao@intel.com>; Brijesh Singh <brijesh.singh@amd.com>
Subject: Re: [edk2] [PATCH 03/10] MdeModulePkg/AtaAtapiPassThru: disable the device at ExitBootServices()
On 25 October 2017 at 16:26, Laszlo Ersek <lersek@redhat.com> wrote:
> Hi Star, Eric,
>
> On 09/08/17 00:41, Laszlo Ersek wrote:
>> The AtaAtapiPassThru driver maps three system memory regions for Bus
>> Master Common Buffer operation on the following call path, if the
>> controller has PCI_CLASS_MASS_STORAGE_SATADPA class code:
>>
>> AtaAtapiPassThruStart()
>> EnumerateAttachedDevice()
>> AhciModeInitialization()
>> AhciCreateTransferDescriptor()
>>
>> The device is disabled (including Bus Master DMA) when the controller
>> is unbound, in AtaAtapiPassThruStop(). Then the regions are unmapped.
>>
>> The former step should also be done when we exit the boot services,
>> and the OS gains ownership of system memory.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h | 6 ++
>> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 59
>> +++++++++++++++++++-
>> 2 files changed, 64 insertions(+), 1 deletion(-)
>
> this patch -- that is, commit 6fb8ddd36bde
> ("MdeModulePkg/AtaAtapiPassThru: disable the device at
> ExitBootServices()", 2017-09-03) -- has caused a performance
> regression in OVMF, in booting Windows installer ISOs from emulated IDE CD-ROMs.
>
> Interestingly, the performance regression only affects the "traditional"
> IDE controller of the "pc" (i440fx) machine type of QEMU; it does not
> affect the AHCI/SATA controller of the "q35" machine type.
Does it make any difference if you only disable memory decoding and bus mastering, but leave I/O port decoding enabled?
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard, Star,
(CC Igor)
On 10/26/17 07:08, Zeng, Star wrote:
> Good point.
>
> Could we find out what change causes the performance regression? Bus Master disable / Memory Space disable / IO Space disable?
> How about to only disable Bus Master in the exit boot service event notification? It seems the key point suggested by UEFI Driver_Writer_Guide_V1.0.1_120308.pdf.
>
> 7.7
> Examples from the EDK II that use this feature are the PCI device drivers for USB Host
> Controllers. Some USB Host Controllers are PCI Bus Masters that continuously access a
> memory buffer to poll for operation requests. Access to this memory buffer by a USB
> Host Controller may be required to boot an operation system, but this activity must be
> terminated when the OS calls ExitBootServices(). *The typical action in the Exit Boot
> Services Event for these types of drivers is to disable the PCI bus master* and place the
> USB Host Controller into a halted state
thank you for the ideas.
* Disabling only EFI_PCI_IO_ATTRIBUTE_BUS_MASTER at EBS mitigates the
symptom.
* Disabling (EFI_PCI_IO_ATTRIBUTE_BUS_MASTER | EFI_PCI_IO_ATTRIBUTE_IO)
at EBS preserves the symptom.
* Disabling
(EFI_PCI_IO_ATTRIBUTE_BUS_MASTER | EFI_PCI_IO_ATTRIBUTE_MEMORY) at EBS
also mitigates the symptom.
So it is as Ard suspected, disabling IO port decoding is what tickles
the bug in Windows.
(Now I'm vaguely recalling an earlier discussion from qemu-devel that
Windows has a bug in that, if any given PCI device is disabled at boot,
then Windows will not load drivers for it, or some such. I'm struggling
to recall the context; maybe it was related to ACPI generation in QEMU.
I'm CC'ing Igor; maybe he remembers better.)
I will post a patch, for disabling EFI_PCI_IO_ATTRIBUTE_BUS_MASTER only.
First, that's going to follow the driver writers' guide verbatim;
second, disabling BMDMA and MMIO, but not IO, would look weird in the
code. :/
Thank you both for the help!
Laszlo
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Thursday, October 26, 2017 4:12 AM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Yao, Jiewen <jiewen.yao@intel.com>; Brijesh Singh <brijesh.singh@amd.com>
> Subject: Re: [edk2] [PATCH 03/10] MdeModulePkg/AtaAtapiPassThru: disable the device at ExitBootServices()
>
> On 25 October 2017 at 16:26, Laszlo Ersek <lersek@redhat.com> wrote:
>> Hi Star, Eric,
>>
>> On 09/08/17 00:41, Laszlo Ersek wrote:
>>> The AtaAtapiPassThru driver maps three system memory regions for Bus
>>> Master Common Buffer operation on the following call path, if the
>>> controller has PCI_CLASS_MASS_STORAGE_SATADPA class code:
>>>
>>> AtaAtapiPassThruStart()
>>> EnumerateAttachedDevice()
>>> AhciModeInitialization()
>>> AhciCreateTransferDescriptor()
>>>
>>> The device is disabled (including Bus Master DMA) when the controller
>>> is unbound, in AtaAtapiPassThruStop(). Then the regions are unmapped.
>>>
>>> The former step should also be done when we exit the boot services,
>>> and the OS gains ownership of system memory.
>>>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>> Cc: Star Zeng <star.zeng@intel.com>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h | 6 ++
>>> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 59
>>> +++++++++++++++++++-
>>> 2 files changed, 64 insertions(+), 1 deletion(-)
>>
>> this patch -- that is, commit 6fb8ddd36bde
>> ("MdeModulePkg/AtaAtapiPassThru: disable the device at
>> ExitBootServices()", 2017-09-03) -- has caused a performance
>> regression in OVMF, in booting Windows installer ISOs from emulated IDE CD-ROMs.
>>
>> Interestingly, the performance regression only affects the "traditional"
>> IDE controller of the "pc" (i440fx) machine type of QEMU; it does not
>> affect the AHCI/SATA controller of the "q35" machine type.
>
> Does it make any difference if you only disable memory decoding and bus mastering, but leave I/O port decoding enabled?
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 26 October 2017 at 15:09, Laszlo Ersek <lersek@redhat.com> wrote: > Ard, Star, > > (CC Igor) > > On 10/26/17 07:08, Zeng, Star wrote: >> Good point. >> >> Could we find out what change causes the performance regression? Bus Master disable / Memory Space disable / IO Space disable? >> How about to only disable Bus Master in the exit boot service event notification? It seems the key point suggested by UEFI Driver_Writer_Guide_V1.0.1_120308.pdf. >> >> 7.7 >> Examples from the EDK II that use this feature are the PCI device drivers for USB Host >> Controllers. Some USB Host Controllers are PCI Bus Masters that continuously access a >> memory buffer to poll for operation requests. Access to this memory buffer by a USB >> Host Controller may be required to boot an operation system, but this activity must be >> terminated when the OS calls ExitBootServices(). *The typical action in the Exit Boot >> Services Event for these types of drivers is to disable the PCI bus master* and place the >> USB Host Controller into a halted state > > thank you for the ideas. > > * Disabling only EFI_PCI_IO_ATTRIBUTE_BUS_MASTER at EBS mitigates the > symptom. > > * Disabling (EFI_PCI_IO_ATTRIBUTE_BUS_MASTER | EFI_PCI_IO_ATTRIBUTE_IO) > at EBS preserves the symptom. > > * Disabling > (EFI_PCI_IO_ATTRIBUTE_BUS_MASTER | EFI_PCI_IO_ATTRIBUTE_MEMORY) at EBS > also mitigates the symptom. > Excellent! > So it is as Ard suspected, disabling IO port decoding is what tickles > the bug in Windows. > > (Now I'm vaguely recalling an earlier discussion from qemu-devel that > Windows has a bug in that, if any given PCI device is disabled at boot, > then Windows will not load drivers for it, or some such. I'm struggling > to recall the context; maybe it was related to ACPI generation in QEMU. > I'm CC'ing Igor; maybe he remembers better.) > > I will post a patch, for disabling EFI_PCI_IO_ATTRIBUTE_BUS_MASTER only. > First, that's going to follow the driver writers' guide verbatim; > second, disabling BMDMA and MMIO, but not IO, would look weird in the > code. :/ > > Thank you both for the help! Anytime. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.