[PATCH RESEND 0/3] Xen PCI passthrough fixes

[PATCH RESEND 0/3] Xen PCI passthrough fixes

[Igor Druzhinin]

Resend to include xen-devel@lists.xenproject.org to CC

Igor Druzhinin (3):
  OvmfPkg/XenSupport: remove usage of prefetchable PCI host bridge
    aperture
  OvmfPkg/XenSupport: use a correct PCI host bridge aperture for BAR64
  OvmfPkg/XenSupport: turn off address decoding before BAR sizing

 OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 44 ++++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 7 deletions(-)

-- 
2.7.4

[PATCH RESEND 1/3] OvmfPkg/XenSupport: remove usage of prefetchable PCI host bridge aperture

[Igor Druzhinin]

This aperture doesn't exist in OVMF and trying to use it causes
failing assertions later in cases there are prefetchable and
non-prefetchable BARs following each other. This configuration is
quite likely with some PCI passthrough devices.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
 OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
index 9204179..c23c46d 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
+++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
@@ -129,11 +129,7 @@ PcatPciRootBridgeParseBars (
           //
           Length = ((~Length) + 1) & 0xffffffff;
 
-          if ((Value & BIT3) == BIT3) {
-            MemAperture = PMem;
-          } else {
-            MemAperture = Mem;
-          }
+          MemAperture = Mem;
         } else {
           //
           // 64bit
@@ -149,11 +145,7 @@ PcatPciRootBridgeParseBars (
           Length = Length | LShiftU64 ((UINT64) UpperValue, 32);
           Length = (~Length) + 1;
 
-          if ((Value & BIT3) == BIT3) {
-            MemAperture = PMemAbove4G;
-          } else {
-            MemAperture = MemAbove4G;
-          }
+          MemAperture = MemAbove4G;
         }
 
         Limit = Base + Length - 1;
-- 
2.7.4

Re: [PATCH RESEND 1/3] OvmfPkg/XenSupport: remove usage of prefetchable PCI host bridge aperture

[Anthony PERARD]

Hi,

On Wed, Mar 06, 2019 at 12:40:54PM +0000, Igor Druzhinin wrote:
> This aperture doesn't exist in OVMF and trying to use it causes

I'm trying to understand what you mean by writing "doesn't exist in
OVMF". Are prefetchable BAR not handled by ScanForRootBridges() ?
Or is it the emulation of the config space that isn't correct?
Maybe QEMU should lies about a BAR been prefetchable?

This patch feels like a workaround, but maybe I'm wrong.

Thanks.

> failing assertions later in cases there are prefetchable and
> non-prefetchable BARs following each other. This configuration is
> quite likely with some PCI passthrough devices.

-- 
Anthony PERARD

Re: [PATCH RESEND 1/3] OvmfPkg/XenSupport: remove usage of prefetchable PCI host bridge aperture

[Igor Druzhinin]

On 14/03/2019 17:41, Anthony PERARD wrote:
> Hi,
> 
> On Wed, Mar 06, 2019 at 12:40:54PM +0000, Igor Druzhinin wrote:
>> This aperture doesn't exist in OVMF and trying to use it causes
> 
> I'm trying to understand what you mean by writing "doesn't exist in
> OVMF". Are prefetchable BAR not handled by ScanForRootBridges() ?
> Or is it the emulation of the config space that isn't correct?
> Maybe QEMU should lies about a BAR been prefetchable?

The problem here is: hvmloader places BARs initially disregarding
prefetchable bit in an arbitrary order because essentially there is only
1 aperture for the host bridge in emulated system under Xen (and KVM as
well). In PcatPciRootBridgeParseBars() we construct apertures for high
level OVMF code by reading the BAR placement information after
hvmloader. It often appears that there are prefetchable and
non-prefetchable BARs coexist with each other and make prefetchable and
non-prefetchable apertures overlap. This eventually triggers an
assertion in high level OVMF code because that shouldn't happen.

OVMF for KVM is not using prefetchable BAR at all - see
PciHostBridgeGetRootBridges() in which it passes mNonExistAperture dummy
object to high level code. I think it's wrong to construct a
prefetchable aperture for Xen and this code should be removed as it's
done for QEMU-KVM. Do you think this patch needs to do that?

Igor

Re: [PATCH RESEND 1/3] OvmfPkg/XenSupport: remove usage of prefetchable PCI host bridge aperture

[Anthony PERARD]

On Thu, Mar 14, 2019 at 07:45:56PM +0000, Igor Druzhinin wrote:
> On 14/03/2019 17:41, Anthony PERARD wrote:
> > Hi,
> > 
> > On Wed, Mar 06, 2019 at 12:40:54PM +0000, Igor Druzhinin wrote:
> >> This aperture doesn't exist in OVMF and trying to use it causes
> > 
> > I'm trying to understand what you mean by writing "doesn't exist in
> > OVMF". Are prefetchable BAR not handled by ScanForRootBridges() ?
> > Or is it the emulation of the config space that isn't correct?
> > Maybe QEMU should lies about a BAR been prefetchable?
> 
> The problem here is: hvmloader places BARs initially disregarding
> prefetchable bit in an arbitrary order because essentially there is only
> 1 aperture for the host bridge in emulated system under Xen (and KVM as
> well). In PcatPciRootBridgeParseBars() we construct apertures for high
> level OVMF code by reading the BAR placement information after
> hvmloader. It often appears that there are prefetchable and
> non-prefetchable BARs coexist with each other and make prefetchable and
> non-prefetchable apertures overlap. This eventually triggers an
> assertion in high level OVMF code because that shouldn't happen.

Thanks for the explanation. Could you add it to the patch description?

> OVMF for KVM is not using prefetchable BAR at all - see
> PciHostBridgeGetRootBridges() in which it passes mNonExistAperture dummy
> object to high level code. I think it's wrong to construct a
> prefetchable aperture for Xen and this code should be removed as it's
> done for QEMU-KVM. Do you think this patch needs to do that?

It would be nice to remove the code that isn't useful so feel free to do
it and/or keep the current patch with the description updated.

Thanks,

-- 
Anthony PERARD

Re: [PATCH RESEND 1/3] OvmfPkg/XenSupport: remove usage of prefetchable PCI host bridge aperture

[Laszlo Ersek]

On 03/19/19 15:03, Anthony PERARD wrote:
> On Thu, Mar 14, 2019 at 07:45:56PM +0000, Igor Druzhinin wrote:
>> On 14/03/2019 17:41, Anthony PERARD wrote:
>>> Hi,
>>>
>>> On Wed, Mar 06, 2019 at 12:40:54PM +0000, Igor Druzhinin wrote:
>>>> This aperture doesn't exist in OVMF and trying to use it causes
>>>
>>> I'm trying to understand what you mean by writing "doesn't exist in
>>> OVMF". Are prefetchable BAR not handled by ScanForRootBridges() ?
>>> Or is it the emulation of the config space that isn't correct?
>>> Maybe QEMU should lies about a BAR been prefetchable?
>>
>> The problem here is: hvmloader places BARs initially disregarding
>> prefetchable bit in an arbitrary order because essentially there is only
>> 1 aperture for the host bridge in emulated system under Xen (and KVM as
>> well). In PcatPciRootBridgeParseBars() we construct apertures for high
>> level OVMF code by reading the BAR placement information after
>> hvmloader. It often appears that there are prefetchable and
>> non-prefetchable BARs coexist with each other and make prefetchable and
>> non-prefetchable apertures overlap. This eventually triggers an
>> assertion in high level OVMF code because that shouldn't happen.
> 
> Thanks for the explanation. Could you add it to the patch description?
> 
>> OVMF for KVM is not using prefetchable BAR at all - see
>> PciHostBridgeGetRootBridges() in which it passes mNonExistAperture dummy
>> object to high level code. I think it's wrong to construct a
>> prefetchable aperture for Xen and this code should be removed as it's
>> done for QEMU-KVM. Do you think this patch needs to do that?
> 
> It would be nice to remove the code that isn't useful so feel free to do
> it and/or keep the current patch with the description updated.

Right -- I'd like to add one keyword here, for background: EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM. (It's documented in both edk2 [MdePkg/Include/Protocol/PciHostBridgeResourceAllocation.h] and the PI spec [EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL.GetAllocAttributes()].)

Thanks
Laszlo

Re: [Xen-devel] [PATCH RESEND 1/3] OvmfPkg/XenSupport: remove usage of prefetchable PCI host bridge aperture

[Laszlo Ersek]

On 03/22/19 09:33, Roger Pau Monné wrote:
> On Wed, Mar 06, 2019 at 12:40:54PM +0000, Igor Druzhinin wrote:
>> This aperture doesn't exist in OVMF and trying to use it causes
>> failing assertions later in cases there are prefetchable and
>> non-prefetchable BARs following each other. This configuration is
>> quite likely with some PCI passthrough devices.
> 
> According to the PCIe spec, it's fine to place prefetchable BARs in
> non-prefetchable memory space. There's a note that says that most
> implementations will only have 1G of non-prefetchable memory, and
> that most scalable platforms will map 32bit BARs into
> non-prefetchable memory regardless of the prefetchable bit value.
> 
> Shouldn't OVMF be fine with finding both prefetchable and
> non-prefetchable BARs, as long as the memory region is set to
> non-prefetchable?
> 
> Does OVMF have the capability to position BARs by itself? If so we
> could skip of the placement done by hvmloader and just let OVMF
> position things where it see fit.

The core PciBusDxe driver that is built into OVMF certainly does the
resource allocation/placement, but when OVMF is executed on Xen, this
functionality of PciBusDxe is dynamically disabled by setting
PcdPciDisableBusEnumeration to TRUE. (I'm not saying this is right vs.
wrong, just that it happens.)

Note that OVMF itself checks PcdPciDisableBusEnumeration for many things
(just grep OvmfPkg to see), so if we were to flip the PCD while running
on Xen, it would change the behavior of OVMF on Xen in a number of
areas. Can't offer a deeper treatise for now; all the related source
code locations would have to be audited (likely with "git blame" too).

Now, if PciBusDxe *is* allowed/requested to lay out the BARs, through
the PCD, then it (indirectly) depends on platform code to provide the
resource apertures -- of the root bridges -- out of which it can
allocate the BARs. My understanding is that XenSupport.c tries to detect
these apertures "retroactively", from the pre-existing BAR placements.
This was contributed by Ray in commit 49effaf26ec9
("OvmfPkg/PciHostBridgeLib: Scan for root bridges when running over
Xen", 2016-05-11), so I'll have to defer to him on the code.

I believe that, if we flipped the PCD to FALSE on Xen, and hvmloader
would stop pre-configuring the BARs (or OVMF would simply ignore that
pre-config), then this code (XenSupport.c) should be possible to
eliminate -- *however*, in that case, some other Xen-specific code would
become necessary, to expose the root bridge resource apertures (out of
which BARs should be allocated by PciBusDxe, see above).

In QEMU's case: all root bridges share the same apertures between each
other (given any specific resource type). They are communicated via
dynamic PCDs. The 32-bit MMIO aperture PCDs are set in PlatformPei
somewhat simply (based on QEMU machine type, IIRC). The 64-bit MMIO
aperture PCDs are also calculated in PlatformPei, but that calculation
is a *lot* more complex.

All in all, the "root" information is the set of apertures, i.e. what
parts of the GPA space can be used for what resource allocation.

Thanks,
Laszlo

Re: [Xen-devel] [PATCH RESEND 1/3] OvmfPkg/XenSupport: remove usage of prefetchable PCI host bridge aperture

[Igor Druzhinin]

On 24/03/2019 03:50, Roger Pau Monné wrote:
> On Fri, Mar 22, 2019 at 10:06:45AM +0100, Laszlo Ersek wrote:
>> The core PciBusDxe driver that is built into OVMF certainly does the
>> resource allocation/placement, but when OVMF is executed on Xen, this
>> functionality of PciBusDxe is dynamically disabled by setting
>> PcdPciDisableBusEnumeration to TRUE. (I'm not saying this is right vs.
>> wrong, just that it happens.)
>>
>> Note that OVMF itself checks PcdPciDisableBusEnumeration for many things
>> (just grep OvmfPkg to see), so if we were to flip the PCD while running
>> on Xen, it would change the behavior of OVMF on Xen in a number of
>> areas. Can't offer a deeper treatise for now; all the related source
>> code locations would have to be audited (likely with "git blame" too).
>>
>> Now, if PciBusDxe *is* allowed/requested to lay out the BARs, through
>> the PCD, then it (indirectly) depends on platform code to provide the
>> resource apertures -- of the root bridges -- out of which it can
>> allocate the BARs. My understanding is that XenSupport.c tries to detect
>> these apertures "retroactively", from the pre-existing BAR placements.
>> This was contributed by Ray in commit 49effaf26ec9
>> ("OvmfPkg/PciHostBridgeLib: Scan for root bridges when running over
>> Xen", 2016-05-11), so I'll have to defer to him on the code.
>>
>> I believe that, if we flipped the PCD to FALSE on Xen, and hvmloader
>> would stop pre-configuring the BARs (or OVMF would simply ignore that
>> pre-config), then this code (XenSupport.c) should be possible to
>> eliminate -- *however*, in that case, some other Xen-specific code would
>> become necessary, to expose the root bridge resource apertures (out of
>> which BARs should be allocated by PciBusDxe, see above).
>>
>> In QEMU's case: all root bridges share the same apertures between each
>> other (given any specific resource type). They are communicated via
>> dynamic PCDs. The 32-bit MMIO aperture PCDs are set in PlatformPei
>> somewhat simply (based on QEMU machine type, IIRC). The 64-bit MMIO
>> aperture PCDs are also calculated in PlatformPei, but that calculation
>> is a *lot* more complex.
>>
>> All in all, the "root" information is the set of apertures, i.e. what
>> parts of the GPA space can be used for what resource allocation.
> 
> Thanks for the detailed explanation. IMO it would be better to let
> OVMF do the BAR placement instead of having to do it in hvmloader,
> this just causes code duplication between projects and there's nothing
> Xen-specific about the PCI resource allocation.
> 
> I will try to find some time to look into this, albeit I'm not going
> to be able to work in this immediately. I'm more than happy if someone
> else has spare time and wants to pick this up.

I was thinking about that as well since most of the issues I found stem
from the fact hvmloader does its own things behind OVMF's back. But
decided that for now it'd be better to have a quick relief than try to
change the approach drastically as Laszlo pointed out there are many
implications of that change.

So for this patch I prefer to stick to removing only prefetchable
aperture all together (as dead code) and marking the host bridge as not
supporting it (as Laszlo suggested) since it reflects the reality and
done similarly for QEMU-KVM. More code removals and changes could be
done later.

Igor

[PATCH RESEND 2/3] OvmfPkg/XenSupport: use a correct PCI host bridge aperture for BAR64

[Igor Druzhinin]

In case BAR64 is placed below 4G choose the correct aperture.
This fixes a failed assertion down the code path.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
 OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
index c23c46d..408fb24 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
+++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
@@ -145,7 +145,11 @@ PcatPciRootBridgeParseBars (
           Length = Length | LShiftU64 ((UINT64) UpperValue, 32);
           Length = (~Length) + 1;
 
-          MemAperture = MemAbove4G;
+          if (Base < 0x100000000) {
+            MemAperture = Mem;
+          } else {
+            MemAperture = MemAbove4G;
+          }
         }
 
         Limit = Base + Length - 1;
-- 
2.7.4

Re: [PATCH RESEND 2/3] OvmfPkg/XenSupport: use a correct PCI host bridge aperture for BAR64

[Anthony PERARD]

On Wed, Mar 06, 2019 at 12:40:55PM +0000, Igor Druzhinin wrote:
> In case BAR64 is placed below 4G choose the correct aperture.
> This fixes a failed assertion down the code path.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> ---
>  OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
> index c23c46d..408fb24 100644
> --- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
> +++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
> @@ -145,7 +145,11 @@ PcatPciRootBridgeParseBars (
>            Length = Length | LShiftU64 ((UINT64) UpperValue, 32);
>            Length = (~Length) + 1;
>  
> -          MemAperture = MemAbove4G;
> +          if (Base < 0x100000000) {

You could use the macro BASE_4GB to replace this 1 followed by a looong
list of 0.

And with that changed:
Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

[PATCH RESEND 3/3] OvmfPkg/XenSupport: turn off address decoding before BAR sizing

[Igor Druzhinin]

On Xen, hvmloader firmware leaves address decoding enabled for
enumerated PCI device before jumping into OVMF. OVMF seems to
expect it to be disabled and tries to size PCI BARs in several places
without disabling it which causes BAR64, for example, being
incorrectly placed by QEMU.

Fix it by disabling PCI address decoding explicitly before the
first attempt to size BARs on Xen.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
 OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 34 +++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
index 408fb24..9940335 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
+++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
@@ -55,6 +55,33 @@ PcatPciRootBridgeBarExisted (
   EnableInterrupts ();
 }
 
+#define EFI_PCI_COMMAND_DECODE ((UINT16)(EFI_PCI_COMMAND_IO_SPACE | \
+                                         EFI_PCI_COMMAND_MEMORY_SPACE))
+STATIC
+VOID
+PcatPciRootBridgeDecoding (
+  IN  UINTN                          Address,
+  IN  BOOLEAN                        Enabled,
+  OUT UINT16                         *OriginalValue
+  )
+{
+  UINT16                            Value;
+
+  //
+  // Preserve the original value
+  //
+  Value = *OriginalValue;
+  *OriginalValue = PciRead16 (Address);
+
+  if (!Enabled) {
+    if (*OriginalValue & EFI_PCI_COMMAND_DECODE)
+       PciWrite16 (Address, *OriginalValue & ~EFI_PCI_COMMAND_DECODE);
+  } else {
+    if (Value & EFI_PCI_COMMAND_DECODE)
+      PciWrite16 (Address, Value);
+  }
+}
+
 STATIC
 VOID
 PcatPciRootBridgeParseBars (
@@ -76,6 +103,7 @@ PcatPciRootBridgeParseBars (
   UINT32                            Value;
   UINT32                            OriginalUpperValue;
   UINT32                            UpperValue;
+  UINT16                            OriginalCommand;
   UINT64                            Mask;
   UINTN                             Offset;
   UINT64                            Base;
@@ -83,6 +111,12 @@ PcatPciRootBridgeParseBars (
   UINT64                            Limit;
   PCI_ROOT_BRIDGE_APERTURE          *MemAperture;
 
+  // Disable address decoding for every device before OVMF starts sizing it
+  PcatPciRootBridgeDecoding (
+    PCI_LIB_ADDRESS (Bus, Device, Function, PCI_COMMAND_OFFSET),
+    FALSE, &OriginalCommand
+  );
+
   for (Offset = BarOffsetBase; Offset < BarOffsetEnd; Offset += sizeof (UINT32)) {
     PcatPciRootBridgeBarExisted (
       PCI_LIB_ADDRESS (Bus, Device, Function, Offset),
-- 
2.7.4

Re: [PATCH RESEND 3/3] OvmfPkg/XenSupport: turn off address decoding before BAR sizing

[Laszlo Ersek]

On 03/06/19 13:40, Igor Druzhinin wrote:
> On Xen, hvmloader firmware leaves address decoding enabled for
> enumerated PCI device before jumping into OVMF. OVMF seems to
> expect it to be disabled and tries to size PCI BARs in several places
> without disabling it which causes BAR64, for example, being
> incorrectly placed by QEMU.
> 
> Fix it by disabling PCI address decoding explicitly before the
> first attempt to size BARs on Xen.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> ---
>  OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 34 +++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
> index 408fb24..9940335 100644
> --- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
> +++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
> @@ -55,6 +55,33 @@ PcatPciRootBridgeBarExisted (
>    EnableInterrupts ();
>  }
>  
> +#define EFI_PCI_COMMAND_DECODE ((UINT16)(EFI_PCI_COMMAND_IO_SPACE | \
> +                                         EFI_PCI_COMMAND_MEMORY_SPACE))

I thought I asked you not to define a macro here that started with the
"EFI_" prefix :/

http://mid.mail-archive.com/dd8e3c9e-cb76-d3fe-6a10-c0a41c714b56@redhat.com

Laszlo

> +STATIC
> +VOID
> +PcatPciRootBridgeDecoding (
> +  IN  UINTN                          Address,
> +  IN  BOOLEAN                        Enabled,
> +  OUT UINT16                         *OriginalValue
> +  )
> +{
> +  UINT16                            Value;
> +
> +  //
> +  // Preserve the original value
> +  //
> +  Value = *OriginalValue;
> +  *OriginalValue = PciRead16 (Address);
> +
> +  if (!Enabled) {
> +    if (*OriginalValue & EFI_PCI_COMMAND_DECODE)
> +       PciWrite16 (Address, *OriginalValue & ~EFI_PCI_COMMAND_DECODE);
> +  } else {
> +    if (Value & EFI_PCI_COMMAND_DECODE)
> +      PciWrite16 (Address, Value);
> +  }
> +}
> +
>  STATIC
>  VOID
>  PcatPciRootBridgeParseBars (
> @@ -76,6 +103,7 @@ PcatPciRootBridgeParseBars (
>    UINT32                            Value;
>    UINT32                            OriginalUpperValue;
>    UINT32                            UpperValue;
> +  UINT16                            OriginalCommand;
>    UINT64                            Mask;
>    UINTN                             Offset;
>    UINT64                            Base;
> @@ -83,6 +111,12 @@ PcatPciRootBridgeParseBars (
>    UINT64                            Limit;
>    PCI_ROOT_BRIDGE_APERTURE          *MemAperture;
>  
> +  // Disable address decoding for every device before OVMF starts sizing it
> +  PcatPciRootBridgeDecoding (
> +    PCI_LIB_ADDRESS (Bus, Device, Function, PCI_COMMAND_OFFSET),
> +    FALSE, &OriginalCommand
> +  );
> +
>    for (Offset = BarOffsetBase; Offset < BarOffsetEnd; Offset += sizeof (UINT32)) {
>      PcatPciRootBridgeBarExisted (
>        PCI_LIB_ADDRESS (Bus, Device, Function, Offset),
> 

Re: [PATCH RESEND 3/3] OvmfPkg/XenSupport: turn off address decoding before BAR sizing

[Igor Druzhinin]

On 06/03/2019 13:22, Laszlo Ersek wrote:
> On 03/06/19 13:40, Igor Druzhinin wrote:
>> On Xen, hvmloader firmware leaves address decoding enabled for
>> enumerated PCI device before jumping into OVMF. OVMF seems to
>> expect it to be disabled and tries to size PCI BARs in several places
>> without disabling it which causes BAR64, for example, being
>> incorrectly placed by QEMU.
>>
>> Fix it by disabling PCI address decoding explicitly before the
>> first attempt to size BARs on Xen.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>> ---
>>  OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 34 +++++++++++++++++++++++++++
>>  1 file changed, 34 insertions(+)
>>
>> diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
>> index 408fb24..9940335 100644
>> --- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
>> +++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
>> @@ -55,6 +55,33 @@ PcatPciRootBridgeBarExisted (
>>    EnableInterrupts ();
>>  }
>>  
>> +#define EFI_PCI_COMMAND_DECODE ((UINT16)(EFI_PCI_COMMAND_IO_SPACE | \
>> +                                         EFI_PCI_COMMAND_MEMORY_SPACE))
> 
> I thought I asked you not to define a macro here that started with the
> "EFI_" prefix :/
> 
> http://mid.mail-archive.com/dd8e3c9e-cb76-d3fe-6a10-c0a41c714b56@redhat.com
> 

This is a resend of v1 patch series to get Xen folks into CC and gather
comments. I expect v2.

Igor

Re: [PATCH RESEND 3/3] OvmfPkg/XenSupport: turn off address decoding before BAR sizing

[Laszlo Ersek]

On 03/06/19 15:26, Igor Druzhinin wrote:
> On 06/03/2019 13:22, Laszlo Ersek wrote:
>> On 03/06/19 13:40, Igor Druzhinin wrote:
>>> On Xen, hvmloader firmware leaves address decoding enabled for
>>> enumerated PCI device before jumping into OVMF. OVMF seems to
>>> expect it to be disabled and tries to size PCI BARs in several places
>>> without disabling it which causes BAR64, for example, being
>>> incorrectly placed by QEMU.
>>>
>>> Fix it by disabling PCI address decoding explicitly before the
>>> first attempt to size BARs on Xen.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>>> ---
>>>  OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 34 +++++++++++++++++++++++++++
>>>  1 file changed, 34 insertions(+)
>>>
>>> diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
>>> index 408fb24..9940335 100644
>>> --- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
>>> +++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
>>> @@ -55,6 +55,33 @@ PcatPciRootBridgeBarExisted (
>>>    EnableInterrupts ();
>>>  }
>>>  
>>> +#define EFI_PCI_COMMAND_DECODE ((UINT16)(EFI_PCI_COMMAND_IO_SPACE | \
>>> +                                         EFI_PCI_COMMAND_MEMORY_SPACE))
>>
>> I thought I asked you not to define a macro here that started with the
>> "EFI_" prefix :/
>>
>> http://mid.mail-archive.com/dd8e3c9e-cb76-d3fe-6a10-c0a41c714b56@redhat.com
>>
> 
> This is a resend of v1 patch series to get Xen folks into CC and gather
> comments. I expect v2.

My bad, thanks for the clarification. On edk2-devel we very rarely post
RESENDs without updates, and so I missed the implications now.

Thanks
Laszlo

Re: [PATCH RESEND 3/3] OvmfPkg/XenSupport: turn off address decoding before BAR sizing

[Anthony PERARD]

On Wed, Mar 06, 2019 at 12:40:56PM +0000, Igor Druzhinin wrote:
> On Xen, hvmloader firmware leaves address decoding enabled for
> enumerated PCI device before jumping into OVMF. OVMF seems to
> expect it to be disabled and tries to size PCI BARs in several places
> without disabling it which causes BAR64, for example, being
> incorrectly placed by QEMU.
> 
> Fix it by disabling PCI address decoding explicitly before the
> first attempt to size BARs on Xen.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> ---
>  OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 34 +++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
> index 408fb24..9940335 100644
> --- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
> +++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
> @@ -55,6 +55,33 @@ PcatPciRootBridgeBarExisted (
>    EnableInterrupts ();
>  }
>  
> +#define EFI_PCI_COMMAND_DECODE ((UINT16)(EFI_PCI_COMMAND_IO_SPACE | \
> +                                         EFI_PCI_COMMAND_MEMORY_SPACE))
> +STATIC
> +VOID
> +PcatPciRootBridgeDecoding (
> +  IN  UINTN                          Address,
> +  IN  BOOLEAN                        Enabled,
> +  OUT UINT16                         *OriginalValue
> +  )
> +{
> +  UINT16                            Value;
> +
> +  //
> +  // Preserve the original value
> +  //
> +  Value = *OriginalValue;
> +  *OriginalValue = PciRead16 (Address);
> +
> +  if (!Enabled) {
> +    if (*OriginalValue & EFI_PCI_COMMAND_DECODE)
> +       PciWrite16 (Address, *OriginalValue & ~EFI_PCI_COMMAND_DECODE);

The edk2 coding style ask to always use { } with if:
https://github.com/tianocore/tianocore.github.io/wiki/Code-Style-C

> +  } else {
> +    if (Value & EFI_PCI_COMMAND_DECODE)
> +      PciWrite16 (Address, Value);

here too.

> +  }
> +}
> +
>  STATIC
>  VOID
>  PcatPciRootBridgeParseBars (
> @@ -76,6 +103,7 @@ PcatPciRootBridgeParseBars (
>    UINT32                            Value;
>    UINT32                            OriginalUpperValue;
>    UINT32                            UpperValue;
> +  UINT16                            OriginalCommand;
>    UINT64                            Mask;
>    UINTN                             Offset;
>    UINT64                            Base;
> @@ -83,6 +111,12 @@ PcatPciRootBridgeParseBars (
>    UINT64                            Limit;
>    PCI_ROOT_BRIDGE_APERTURE          *MemAperture;
>  
> +  // Disable address decoding for every device before OVMF starts sizing it
> +  PcatPciRootBridgeDecoding (
> +    PCI_LIB_ADDRESS (Bus, Device, Function, PCI_COMMAND_OFFSET),
> +    FALSE, &OriginalCommand

Is it on purpose that the original register value is stored in
OriginalCommand, but never used again?

> +  );
> +
>    for (Offset = BarOffsetBase; Offset < BarOffsetEnd; Offset += sizeof (UINT32)) {
>      PcatPciRootBridgeBarExisted (
>        PCI_LIB_ADDRESS (Bus, Device, Function, Offset),

Thanks,

-- 
Anthony PERARD

Re: [PATCH RESEND 3/3] OvmfPkg/XenSupport: turn off address decoding before BAR sizing

[Igor Druzhinin]

On 14/03/2019 17:55, Anthony PERARD wrote:
> On Wed, Mar 06, 2019 at 12:40:56PM +0000, Igor Druzhinin wrote:
>> On Xen, hvmloader firmware leaves address decoding enabled for
>> enumerated PCI device before jumping into OVMF. OVMF seems to
>> expect it to be disabled and tries to size PCI BARs in several places
>> without disabling it which causes BAR64, for example, being
>> incorrectly placed by QEMU.
>>
>> Fix it by disabling PCI address decoding explicitly before the
>> first attempt to size BARs on Xen.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>> ---
>>  OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 34 +++++++++++++++++++++++++++
>>  1 file changed, 34 insertions(+)
>>
>> diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
>> index 408fb24..9940335 100644
>> --- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
>> +++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
>> @@ -55,6 +55,33 @@ PcatPciRootBridgeBarExisted (
>>    EnableInterrupts ();
>>  }
>>  
>> +#define EFI_PCI_COMMAND_DECODE ((UINT16)(EFI_PCI_COMMAND_IO_SPACE | \
>> +                                         EFI_PCI_COMMAND_MEMORY_SPACE))
>> +STATIC
>> +VOID
>> +PcatPciRootBridgeDecoding (
>> +  IN  UINTN                          Address,
>> +  IN  BOOLEAN                        Enabled,
>> +  OUT UINT16                         *OriginalValue
>> +  )
>> +{
>> +  UINT16                            Value;
>> +
>> +  //
>> +  // Preserve the original value
>> +  //
>> +  Value = *OriginalValue;
>> +  *OriginalValue = PciRead16 (Address);
>> +
>> +  if (!Enabled) {
>> +    if (*OriginalValue & EFI_PCI_COMMAND_DECODE)
>> +       PciWrite16 (Address, *OriginalValue & ~EFI_PCI_COMMAND_DECODE);
> 
> The edk2 coding style ask to always use { } with if:
> https://github.com/tianocore/tianocore.github.io/wiki/Code-Style-C
> 
>> +  } else {
>> +    if (Value & EFI_PCI_COMMAND_DECODE)
>> +      PciWrite16 (Address, Value);
> 
> here too.
> 
>> +  }
>> +}
>> +
>>  STATIC
>>  VOID
>>  PcatPciRootBridgeParseBars (
>> @@ -76,6 +103,7 @@ PcatPciRootBridgeParseBars (
>>    UINT32                            Value;
>>    UINT32                            OriginalUpperValue;
>>    UINT32                            UpperValue;
>> +  UINT16                            OriginalCommand;
>>    UINT64                            Mask;
>>    UINTN                             Offset;
>>    UINT64                            Base;
>> @@ -83,6 +111,12 @@ PcatPciRootBridgeParseBars (
>>    UINT64                            Limit;
>>    PCI_ROOT_BRIDGE_APERTURE          *MemAperture;
>>  
>> +  // Disable address decoding for every device before OVMF starts sizing it
>> +  PcatPciRootBridgeDecoding (
>> +    PCI_LIB_ADDRESS (Bus, Device, Function, PCI_COMMAND_OFFSET),
>> +    FALSE, &OriginalCommand
> 
> Is it on purpose that the original register value is stored in
> OriginalCommand, but never used again?

Those are probably remnants of extended patch version for XenServer -
will remove together with other suggestions from you and Laszlo.

Igor