Re: [edk2] [PATCH] OvmfPkg: prevent 64-bit MMIO BAR degradation if there is no CSM

2016-05-25 Thread Laszlo Ersek 
On 05/24/16 22:53, Jordan Justen wrote:
> Reviewed-by: Jordan Justen 

Thank you guys for the reviews. Commit
855743f7177459bea95798e59b6b18dab867710c.

Cheers
Laszlo

> On 2016-05-18 15:12:53, Laszlo Ersek wrote:
>> According to edk2 commit
>>
>>   "MdeModulePkg/PciBus: do not improperly degrade resource"
>>
>> and to the EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL definition in the
>> Platform Init 1.4a specification, a platform can provide such a protocol
>> in order to influence the PCI resource allocation performed by the PCI Bus
>> driver.
>>
>> In particular it is possible instruct the PCI Bus driver, with a
>> "wildcard" hint, to allocate the 64-bit MMIO BARs of a device in 64-bit
>> address space, regardless of whether the device features an option ROM.
>>
>> (By default, the PCI Bus driver considers an option ROM reason enough for
>> allocating the 64-bit MMIO BARs in 32-bit address space. It cannot know if
>> BDS will launch a legacy boot option, and under legacy boot, a legacy BIOS
>> binary from a combined option ROM could be dispatched, and fail to access
>> MMIO BARs in 64-bit address space.)
>>
>> In platform code we can ascertain whether a CSM is present or not. If not,
>> then legacy BIOS binaries in option ROMs can't be dispatched, hence the
>> BAR degradation is detrimental, and we should prevent it. This is expected
>> to conserve the 32-bit address space for 32-bit MMIO BARs.
>>
>> The driver added in this patch could be simplified based on the following
>> facts:
>>
>> - In the Ia32 build, the 64-bit MMIO aperture is always zero-size, hence
>>   the driver will exit immediately. Therefore the driver could be omitted
>>   from the Ia32 build.
>>
>> - In the Ia32X64 and X64 builds, the driver could be omitted if CSM_ENABLE
>>   was defined (because in that case the degradation would be justified).
>>   On the other hand, if CSM_ENABLE was undefined, then the driver could be
>>   included, and it could provide the hint unconditionally (without looking
>>   for the Legacy BIOS protocol).
>>
>> These short-cuts are not taken because they would increase the differences
>> between the OVMF DSC/FDF files. If we can manage without extreme
>> complexity, we should use dynamic logic (vs. build time configuration),
>> plus keep conditional compilation to a minimum.
>>
>> Cc: Jordan Justen 
>> Cc: Ruiyu Ni 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek 
>> ---
>>
>> Notes:
>> This patch will make a difference only when Ray's patch at
>> 
>> is committed.
>>
>>  OvmfPkg/OvmfPkgIa32.dsc  |  
>>  1 +
>>  OvmfPkg/OvmfPkgIa32X64.dsc   |  
>>  1 +
>>  OvmfPkg/OvmfPkgX64.dsc   |  
>>  1 +
>>  OvmfPkg/OvmfPkgIa32.fdf  |  
>>  1 +
>>  OvmfPkg/OvmfPkgIa32X64.fdf   |  
>>  1 +
>>  OvmfPkg/OvmfPkgX64.fdf   |  
>>  1 +
>>  OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf |  
>> 50 +++
>>  OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c   | 
>> 353 
>>  8 files changed, 409 insertions(+)
>>
>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>> index 45569f2875b5..1becb65cd878 100644
>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>> @@ -552,6 +552,7 @@ [Components]
>>UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
>>UefiCpuPkg/CpuDxe/CpuDxe.inf
>>PcAtChipsetPkg/8254TimerDxe/8254Timer.inf
>> +  OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
>>MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
>>  
>>PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>> index 05ff1c914455..3d838cdcb125 100644
>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>> @@ -561,6 +561,7 @@ [Components.X64]
>>UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
>>UefiCpuPkg/CpuDxe/CpuDxe.inf
>>PcAtChipsetPkg/8254TimerDxe/8254Timer.inf
>> +  OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
>>MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
>>  
>>PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index 54d4413f11f6..16d58bad7865 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -559,6 +559,7 @@ [Components]
>>UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
>>UefiCpuPkg/CpuDxe/CpuDxe.inf
>>PcAtChipsetPkg/8254TimerDxe/8254Timer.inf
>>

Re: [edk2] [PATCH] OvmfPkg: prevent 64-bit MMIO BAR degradation if there is no CSM

2016-05-24 Thread Jordan Justen 
Reviewed-by: Jordan Justen 

On 2016-05-18 15:12:53, Laszlo Ersek wrote:
> According to edk2 commit
> 
>   "MdeModulePkg/PciBus: do not improperly degrade resource"
> 
> and to the EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL definition in the
> Platform Init 1.4a specification, a platform can provide such a protocol
> in order to influence the PCI resource allocation performed by the PCI Bus
> driver.
> 
> In particular it is possible instruct the PCI Bus driver, with a
> "wildcard" hint, to allocate the 64-bit MMIO BARs of a device in 64-bit
> address space, regardless of whether the device features an option ROM.
> 
> (By default, the PCI Bus driver considers an option ROM reason enough for
> allocating the 64-bit MMIO BARs in 32-bit address space. It cannot know if
> BDS will launch a legacy boot option, and under legacy boot, a legacy BIOS
> binary from a combined option ROM could be dispatched, and fail to access
> MMIO BARs in 64-bit address space.)
> 
> In platform code we can ascertain whether a CSM is present or not. If not,
> then legacy BIOS binaries in option ROMs can't be dispatched, hence the
> BAR degradation is detrimental, and we should prevent it. This is expected
> to conserve the 32-bit address space for 32-bit MMIO BARs.
> 
> The driver added in this patch could be simplified based on the following
> facts:
> 
> - In the Ia32 build, the 64-bit MMIO aperture is always zero-size, hence
>   the driver will exit immediately. Therefore the driver could be omitted
>   from the Ia32 build.
> 
> - In the Ia32X64 and X64 builds, the driver could be omitted if CSM_ENABLE
>   was defined (because in that case the degradation would be justified).
>   On the other hand, if CSM_ENABLE was undefined, then the driver could be
>   included, and it could provide the hint unconditionally (without looking
>   for the Legacy BIOS protocol).
> 
> These short-cuts are not taken because they would increase the differences
> between the OVMF DSC/FDF files. If we can manage without extreme
> complexity, we should use dynamic logic (vs. build time configuration),
> plus keep conditional compilation to a minimum.
> 
> Cc: Jordan Justen 
> Cc: Ruiyu Ni 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek 
> ---
> 
> Notes:
> This patch will make a difference only when Ray's patch at
> 
> is committed.
> 
>  OvmfPkg/OvmfPkgIa32.dsc  |   
> 1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc   |   
> 1 +
>  OvmfPkg/OvmfPkgX64.dsc   |   
> 1 +
>  OvmfPkg/OvmfPkgIa32.fdf  |   
> 1 +
>  OvmfPkg/OvmfPkgIa32X64.fdf   |   
> 1 +
>  OvmfPkg/OvmfPkgX64.fdf   |   
> 1 +
>  OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf |  
> 50 +++
>  OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c   | 
> 353 
>  8 files changed, 409 insertions(+)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 45569f2875b5..1becb65cd878 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -552,6 +552,7 @@ [Components]
>UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
>UefiCpuPkg/CpuDxe/CpuDxe.inf
>PcAtChipsetPkg/8254TimerDxe/8254Timer.inf
> +  OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
>MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
>  
>PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 05ff1c914455..3d838cdcb125 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -561,6 +561,7 @@ [Components.X64]
>UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
>UefiCpuPkg/CpuDxe/CpuDxe.inf
>PcAtChipsetPkg/8254TimerDxe/8254Timer.inf
> +  OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
>MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
>  
>PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 54d4413f11f6..16d58bad7865 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -559,6 +559,7 @@ [Components]
>UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
>UefiCpuPkg/CpuDxe/CpuDxe.inf
>PcAtChipsetPkg/8254TimerDxe/8254Timer.inf
> +  OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
>MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
>  
>PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>

Re: [edk2] [PATCH] OvmfPkg: prevent 64-bit MMIO BAR degradation if there is no CSM

2016-05-19 Thread Laszlo Ersek 
On 05/19/16 10:42, Ni, Ruiyu wrote:
> I agree.
> 
> Reviewed-by: Ruiyu Ni <ruiyu...@intel.com>

Thanks Ray!

I'll await Jordan's feedback too.

Cheers!
Laszlo

>>-Original Message-
>>From: Laszlo Ersek [mailto:ler...@redhat.com]
>>Sent: Thursday, May 19, 2016 3:38 PM
>>To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel-01 <edk2-de...@ml01.01.org>
>>Cc: Justen, Jordan L <jordan.l.jus...@intel.com>
>>Subject: Re: [edk2] [PATCH] OvmfPkg: prevent 64-bit MMIO BAR degradation if 
>>there is no CSM
>>
>>On 05/19/16 04:02, Ni, Ruiyu wrote:
>>> Laszlo,
>>> All are good except: can you use EfiCreateProtocolNotifyEvent()
>>> exposed by UefiLib?
>>
>>I am aware of that function, but I dislike it. The reason I dislike it
>>is that (in my opinion) its internal error handling is inferior. For the
>>return values of gBS->CreateEvent() and gBS->RegisterProtocolNotify(),
>>is only uses ASSERT_EFI_ERROR(), and to the return value of
>>gBS->SignalEvent(), it doesn't apply any check.
>>
>>I think the error handling in my code below is superior. CreateEvent()
>>can legitimately fail with EFI_OUT_OF_RESOURCES. So can
>>RegisterProtocolNotify().
>>
>>I wondered before how the error handling could be fixed in
>>EfiCreateProtocolNotifyEvent(). One problem I see is that it returns an
>>EFI_EVENT object, not an EFI_STATUS one.
>>
>>Given that EFI_EVENT is just a typedef for VOID*, we could return a NULL
>>if something fails. However, in the library class header file, such a
>>return value is not permitted:
>>
>>  @return The notification event that was created.
>>
>>This means that the ASSERT_EFI_ERROR() macro invocations in the function
>>body actually conform to the specification: namely,
>>EfiCreateProtocolNotifyEvent() is never supposed to fail (and I guess
>>all of its callers rely on this).
>>
>>Too bad that this function specification doesn't actually reflect
>>reality. (See the above EFI_OUT_OF_RESOURCES failure modes for
>>CreateEvent() and RegisterProtocolNotify().)
>>
>>Which is, I guess, my ultimate reason for never using this utility
>>function, and always open-coding CreateEvent() +
>>RegisterProtocolNotify() + SignalEvent() with proper error checking
>>(using an assert for SignalEvent(), which really can't fail, according
>>to the UEFI spec).
>>
>>Thanks
>>Laszlo
>>
>>>> -Original Message-
>>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
>>>> Laszlo Ersek
>>>> Sent: Thursday, May 19, 2016 6:13 AM
>>>> To: edk2-devel-01 <edk2-de...@ml01.01.org>
>>>> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Justen, Jordan L 
>>>> <jordan.l.jus...@intel.com>
>>>> Subject: [edk2] [PATCH] OvmfPkg: prevent 64-bit MMIO BAR degradation if 
>>>> there is no CSM
>>>>
>>>> According to edk2 commit
>>>>
>>>>  "MdeModulePkg/PciBus: do not improperly degrade resource"
>>>>
>>>> and to the EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL definition in the
>>>> Platform Init 1.4a specification, a platform can provide such a protocol
>>>> in order to influence the PCI resource allocation performed by the PCI Bus
>>>> driver.
>>>>
>>>> In particular it is possible instruct the PCI Bus driver, with a
>>>> "wildcard" hint, to allocate the 64-bit MMIO BARs of a device in 64-bit
>>>> address space, regardless of whether the device features an option ROM.
>>>>
>>>> (By default, the PCI Bus driver considers an option ROM reason enough for
>>>> allocating the 64-bit MMIO BARs in 32-bit address space. It cannot know if
>>>> BDS will launch a legacy boot option, and under legacy boot, a legacy BIOS
>>>> binary from a combined option ROM could be dispatched, and fail to access
>>>> MMIO BARs in 64-bit address space.)
>>>>
>>>> In platform code we can ascertain whether a CSM is present or not. If not,
>>>> then legacy BIOS binaries in option ROMs can't be dispatched, hence the
>>>> BAR degradation is detrimental, and we should prevent it. This is expected
>>>> to conserve the 32-bit address space for 32-bit MMIO BARs.
>>>>
>>>> The driver added in this patch could be simplified based on the following
>>>> facts:
>>>>
>>>> - In the Ia32 build, the 64-bit MMIO aperture is always zero-s

Re: [edk2] [PATCH] OvmfPkg: prevent 64-bit MMIO BAR degradation if there is no CSM

2016-05-19 Thread Ni, Ruiyu 
I agree.

Reviewed-by: Ruiyu Ni <ruiyu...@intel.com>


>-Original Message-
>From: Laszlo Ersek [mailto:ler...@redhat.com]
>Sent: Thursday, May 19, 2016 3:38 PM
>To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel-01 <edk2-de...@ml01.01.org>
>Cc: Justen, Jordan L <jordan.l.jus...@intel.com>
>Subject: Re: [edk2] [PATCH] OvmfPkg: prevent 64-bit MMIO BAR degradation if 
>there is no CSM
>
>On 05/19/16 04:02, Ni, Ruiyu wrote:
>> Laszlo,
>> All are good except: can you use EfiCreateProtocolNotifyEvent()
>> exposed by UefiLib?
>
>I am aware of that function, but I dislike it. The reason I dislike it
>is that (in my opinion) its internal error handling is inferior. For the
>return values of gBS->CreateEvent() and gBS->RegisterProtocolNotify(),
>is only uses ASSERT_EFI_ERROR(), and to the return value of
>gBS->SignalEvent(), it doesn't apply any check.
>
>I think the error handling in my code below is superior. CreateEvent()
>can legitimately fail with EFI_OUT_OF_RESOURCES. So can
>RegisterProtocolNotify().
>
>I wondered before how the error handling could be fixed in
>EfiCreateProtocolNotifyEvent(). One problem I see is that it returns an
>EFI_EVENT object, not an EFI_STATUS one.
>
>Given that EFI_EVENT is just a typedef for VOID*, we could return a NULL
>if something fails. However, in the library class header file, such a
>return value is not permitted:
>
>  @return The notification event that was created.
>
>This means that the ASSERT_EFI_ERROR() macro invocations in the function
>body actually conform to the specification: namely,
>EfiCreateProtocolNotifyEvent() is never supposed to fail (and I guess
>all of its callers rely on this).
>
>Too bad that this function specification doesn't actually reflect
>reality. (See the above EFI_OUT_OF_RESOURCES failure modes for
>CreateEvent() and RegisterProtocolNotify().)
>
>Which is, I guess, my ultimate reason for never using this utility
>function, and always open-coding CreateEvent() +
>RegisterProtocolNotify() + SignalEvent() with proper error checking
>(using an assert for SignalEvent(), which really can't fail, according
>to the UEFI spec).
>
>Thanks
>Laszlo
>
>>> -Original Message-
>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
>>> Laszlo Ersek
>>> Sent: Thursday, May 19, 2016 6:13 AM
>>> To: edk2-devel-01 <edk2-de...@ml01.01.org>
>>> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Justen, Jordan L 
>>> <jordan.l.jus...@intel.com>
>>> Subject: [edk2] [PATCH] OvmfPkg: prevent 64-bit MMIO BAR degradation if 
>>> there is no CSM
>>>
>>> According to edk2 commit
>>>
>>>  "MdeModulePkg/PciBus: do not improperly degrade resource"
>>>
>>> and to the EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL definition in the
>>> Platform Init 1.4a specification, a platform can provide such a protocol
>>> in order to influence the PCI resource allocation performed by the PCI Bus
>>> driver.
>>>
>>> In particular it is possible instruct the PCI Bus driver, with a
>>> "wildcard" hint, to allocate the 64-bit MMIO BARs of a device in 64-bit
>>> address space, regardless of whether the device features an option ROM.
>>>
>>> (By default, the PCI Bus driver considers an option ROM reason enough for
>>> allocating the 64-bit MMIO BARs in 32-bit address space. It cannot know if
>>> BDS will launch a legacy boot option, and under legacy boot, a legacy BIOS
>>> binary from a combined option ROM could be dispatched, and fail to access
>>> MMIO BARs in 64-bit address space.)
>>>
>>> In platform code we can ascertain whether a CSM is present or not. If not,
>>> then legacy BIOS binaries in option ROMs can't be dispatched, hence the
>>> BAR degradation is detrimental, and we should prevent it. This is expected
>>> to conserve the 32-bit address space for 32-bit MMIO BARs.
>>>
>>> The driver added in this patch could be simplified based on the following
>>> facts:
>>>
>>> - In the Ia32 build, the 64-bit MMIO aperture is always zero-size, hence
>>>  the driver will exit immediately. Therefore the driver could be omitted
>>>  from the Ia32 build.
>>>
>>> - In the Ia32X64 and X64 builds, the driver could be omitted if CSM_ENABLE
>>>  was defined (because in that case the degradation would be justified).
>>>  On the other hand, if CSM_ENABLE was undefined, then the driver could be
>>>  included, and it could provide the hint unconditionally

Re: [edk2] [PATCH] OvmfPkg: prevent 64-bit MMIO BAR degradation if there is no CSM

2016-05-19 Thread Laszlo Ersek 
On 05/19/16 04:02, Ni, Ruiyu wrote:
> Laszlo,
> All are good except: can you use EfiCreateProtocolNotifyEvent()
> exposed by UefiLib?

I am aware of that function, but I dislike it. The reason I dislike it
is that (in my opinion) its internal error handling is inferior. For the
return values of gBS->CreateEvent() and gBS->RegisterProtocolNotify(),
is only uses ASSERT_EFI_ERROR(), and to the return value of
gBS->SignalEvent(), it doesn't apply any check.

I think the error handling in my code below is superior. CreateEvent()
can legitimately fail with EFI_OUT_OF_RESOURCES. So can
RegisterProtocolNotify().

I wondered before how the error handling could be fixed in
EfiCreateProtocolNotifyEvent(). One problem I see is that it returns an
EFI_EVENT object, not an EFI_STATUS one.

Given that EFI_EVENT is just a typedef for VOID*, we could return a NULL
if something fails. However, in the library class header file, such a
return value is not permitted:

  @return The notification event that was created.

This means that the ASSERT_EFI_ERROR() macro invocations in the function
body actually conform to the specification: namely,
EfiCreateProtocolNotifyEvent() is never supposed to fail (and I guess
all of its callers rely on this).

Too bad that this function specification doesn't actually reflect
reality. (See the above EFI_OUT_OF_RESOURCES failure modes for
CreateEvent() and RegisterProtocolNotify().)

Which is, I guess, my ultimate reason for never using this utility
function, and always open-coding CreateEvent() +
RegisterProtocolNotify() + SignalEvent() with proper error checking
(using an assert for SignalEvent(), which really can't fail, according
to the UEFI spec).

Thanks
Laszlo

>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
>> Laszlo Ersek
>> Sent: Thursday, May 19, 2016 6:13 AM
>> To: edk2-devel-01 <edk2-de...@ml01.01.org>
>> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Justen, Jordan L 
>> <jordan.l.jus...@intel.com>
>> Subject: [edk2] [PATCH] OvmfPkg: prevent 64-bit MMIO BAR degradation if 
>> there is no CSM
>>
>> According to edk2 commit
>>
>>  "MdeModulePkg/PciBus: do not improperly degrade resource"
>>
>> and to the EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL definition in the
>> Platform Init 1.4a specification, a platform can provide such a protocol
>> in order to influence the PCI resource allocation performed by the PCI Bus
>> driver.
>>
>> In particular it is possible instruct the PCI Bus driver, with a
>> "wildcard" hint, to allocate the 64-bit MMIO BARs of a device in 64-bit
>> address space, regardless of whether the device features an option ROM.
>>
>> (By default, the PCI Bus driver considers an option ROM reason enough for
>> allocating the 64-bit MMIO BARs in 32-bit address space. It cannot know if
>> BDS will launch a legacy boot option, and under legacy boot, a legacy BIOS
>> binary from a combined option ROM could be dispatched, and fail to access
>> MMIO BARs in 64-bit address space.)
>>
>> In platform code we can ascertain whether a CSM is present or not. If not,
>> then legacy BIOS binaries in option ROMs can't be dispatched, hence the
>> BAR degradation is detrimental, and we should prevent it. This is expected
>> to conserve the 32-bit address space for 32-bit MMIO BARs.
>>
>> The driver added in this patch could be simplified based on the following
>> facts:
>>
>> - In the Ia32 build, the 64-bit MMIO aperture is always zero-size, hence
>>  the driver will exit immediately. Therefore the driver could be omitted
>>  from the Ia32 build.
>>
>> - In the Ia32X64 and X64 builds, the driver could be omitted if CSM_ENABLE
>>  was defined (because in that case the degradation would be justified).
>>  On the other hand, if CSM_ENABLE was undefined, then the driver could be
>>  included, and it could provide the hint unconditionally (without looking
>>  for the Legacy BIOS protocol).
>>
>> These short-cuts are not taken because they would increase the differences
>> between the OVMF DSC/FDF files. If we can manage without extreme
>> complexity, we should use dynamic logic (vs. build time configuration),
>> plus keep conditional compilation to a minimum.
>>
>> Cc: Jordan Justen <jordan.l.jus...@intel.com>
>> Cc: Ruiyu Ni <ruiyu...@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>> ---
>>
>> Notes:
>>This patch will make a difference only when Ray's patch at
>><http://thread.gmane.org/gmane.c

Re: [edk2] [PATCH] OvmfPkg: prevent 64-bit MMIO BAR degradation if there is no CSM

2016-05-18 Thread Ni, Ruiyu 
Laszlo,
All are good except: can you use EfiCreateProtocolNotifyEvent()
exposed by UefiLib?

Regards,
Ray

>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
>Ersek
>Sent: Thursday, May 19, 2016 6:13 AM
>To: edk2-devel-01 <edk2-de...@ml01.01.org>
>Cc: Ni, Ruiyu <ruiyu...@intel.com>; Justen, Jordan L 
><jordan.l.jus...@intel.com>
>Subject: [edk2] [PATCH] OvmfPkg: prevent 64-bit MMIO BAR degradation if there 
>is no CSM
>
>According to edk2 commit
>
>  "MdeModulePkg/PciBus: do not improperly degrade resource"
>
>and to the EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL definition in the
>Platform Init 1.4a specification, a platform can provide such a protocol
>in order to influence the PCI resource allocation performed by the PCI Bus
>driver.
>
>In particular it is possible instruct the PCI Bus driver, with a
>"wildcard" hint, to allocate the 64-bit MMIO BARs of a device in 64-bit
>address space, regardless of whether the device features an option ROM.
>
>(By default, the PCI Bus driver considers an option ROM reason enough for
>allocating the 64-bit MMIO BARs in 32-bit address space. It cannot know if
>BDS will launch a legacy boot option, and under legacy boot, a legacy BIOS
>binary from a combined option ROM could be dispatched, and fail to access
>MMIO BARs in 64-bit address space.)
>
>In platform code we can ascertain whether a CSM is present or not. If not,
>then legacy BIOS binaries in option ROMs can't be dispatched, hence the
>BAR degradation is detrimental, and we should prevent it. This is expected
>to conserve the 32-bit address space for 32-bit MMIO BARs.
>
>The driver added in this patch could be simplified based on the following
>facts:
>
>- In the Ia32 build, the 64-bit MMIO aperture is always zero-size, hence
>  the driver will exit immediately. Therefore the driver could be omitted
>  from the Ia32 build.
>
>- In the Ia32X64 and X64 builds, the driver could be omitted if CSM_ENABLE
>  was defined (because in that case the degradation would be justified).
>  On the other hand, if CSM_ENABLE was undefined, then the driver could be
>  included, and it could provide the hint unconditionally (without looking
>  for the Legacy BIOS protocol).
>
>These short-cuts are not taken because they would increase the differences
>between the OVMF DSC/FDF files. If we can manage without extreme
>complexity, we should use dynamic logic (vs. build time configuration),
>plus keep conditional compilation to a minimum.
>
>Cc: Jordan Justen <jordan.l.jus...@intel.com>
>Cc: Ruiyu Ni <ruiyu...@intel.com>
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>---
>
>Notes:
>This patch will make a difference only when Ray's patch at
><http://thread.gmane.org/gmane.comp.bios.edk2.devel/12290/focus=12292>
>is committed.
>
> OvmfPkg/OvmfPkgIa32.dsc  |   
> 1 +
> OvmfPkg/OvmfPkgIa32X64.dsc   |   
> 1 +
> OvmfPkg/OvmfPkgX64.dsc   |   
> 1 +
> OvmfPkg/OvmfPkgIa32.fdf  |   
> 1 +
> OvmfPkg/OvmfPkgIa32X64.fdf   |   
> 1 +
> OvmfPkg/OvmfPkgX64.fdf   |   
> 1 +
> OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf |  
> 50 +++
> OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c   | 
> 353 
> 8 files changed, 409 insertions(+)
>
>diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>index 45569f2875b5..1becb65cd878 100644
>--- a/OvmfPkg/OvmfPkgIa32.dsc
>+++ b/OvmfPkg/OvmfPkgIa32.dsc
>@@ -552,6 +552,7 @@ [Components]
>   UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
>   UefiCpuPkg/CpuDxe/CpuDxe.inf
>   PcAtChipsetPkg/8254TimerDxe/8254Timer.inf
>+  OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
>   MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
> 
>   PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>index 05ff1c914455..3d838cdcb125 100644
>--- a/OvmfPkg/OvmfPkgIa32X64.dsc
>+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>@@ -561,6 +561,7 @@ [Components.X64]
>   UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
>   UefiCpuPkg/CpuDxe/CpuDxe.inf
>   PcAtChipsetPkg/8254TimerDxe/8254Timer.inf
>+  OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
>   MdeModulePkg/Bus/Pci/PciHostBridg

[edk2] [PATCH] OvmfPkg: prevent 64-bit MMIO BAR degradation if there is no CSM

2016-05-18 Thread Laszlo Ersek 
According to edk2 commit

  "MdeModulePkg/PciBus: do not improperly degrade resource"

and to the EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL definition in the
Platform Init 1.4a specification, a platform can provide such a protocol
in order to influence the PCI resource allocation performed by the PCI Bus
driver.

In particular it is possible instruct the PCI Bus driver, with a
"wildcard" hint, to allocate the 64-bit MMIO BARs of a device in 64-bit
address space, regardless of whether the device features an option ROM.

(By default, the PCI Bus driver considers an option ROM reason enough for
allocating the 64-bit MMIO BARs in 32-bit address space. It cannot know if
BDS will launch a legacy boot option, and under legacy boot, a legacy BIOS
binary from a combined option ROM could be dispatched, and fail to access
MMIO BARs in 64-bit address space.)

In platform code we can ascertain whether a CSM is present or not. If not,
then legacy BIOS binaries in option ROMs can't be dispatched, hence the
BAR degradation is detrimental, and we should prevent it. This is expected
to conserve the 32-bit address space for 32-bit MMIO BARs.

The driver added in this patch could be simplified based on the following
facts:

- In the Ia32 build, the 64-bit MMIO aperture is always zero-size, hence
  the driver will exit immediately. Therefore the driver could be omitted
  from the Ia32 build.

- In the Ia32X64 and X64 builds, the driver could be omitted if CSM_ENABLE
  was defined (because in that case the degradation would be justified).
  On the other hand, if CSM_ENABLE was undefined, then the driver could be
  included, and it could provide the hint unconditionally (without looking
  for the Legacy BIOS protocol).

These short-cuts are not taken because they would increase the differences
between the OVMF DSC/FDF files. If we can manage without extreme
complexity, we should use dynamic logic (vs. build time configuration),
plus keep conditional compilation to a minimum.

Cc: Jordan Justen 
Cc: Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
---

Notes:
This patch will make a difference only when Ray's patch at

is committed.

 OvmfPkg/OvmfPkgIa32.dsc  |   1 
+
 OvmfPkg/OvmfPkgIa32X64.dsc   |   1 
+
 OvmfPkg/OvmfPkgX64.dsc   |   1 
+
 OvmfPkg/OvmfPkgIa32.fdf  |   1 
+
 OvmfPkg/OvmfPkgIa32X64.fdf   |   1 
+
 OvmfPkg/OvmfPkgX64.fdf   |   1 
+
 OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf |  50 
+++
 OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c   | 353 

 8 files changed, 409 insertions(+)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 45569f2875b5..1becb65cd878 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -552,6 +552,7 @@ [Components]
   UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
   UefiCpuPkg/CpuDxe/CpuDxe.inf
   PcAtChipsetPkg/8254TimerDxe/8254Timer.inf
+  OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
   MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
 
   PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 05ff1c914455..3d838cdcb125 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -561,6 +561,7 @@ [Components.X64]
   UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
   UefiCpuPkg/CpuDxe/CpuDxe.inf
   PcAtChipsetPkg/8254TimerDxe/8254Timer.inf
+  OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
   MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
 
   PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 54d4413f11f6..16d58bad7865 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -559,6 +559,7 @@ [Components]
   UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
   UefiCpuPkg/CpuDxe/CpuDxe.inf
   PcAtChipsetPkg/8254TimerDxe/8254Timer.inf
+  OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
   MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
 
   PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index 594e84fd713b..8ab1633f832a 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -207,6 +207,7 @@ [FV.DXEFV]
 INF  UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
 INF  UefiCpuPkg/CpuDxe/CpuDxe.inf
 INF