[PATCH v2 0/3] Xen PCI passthrough fixes

[PATCH v2 0/3] Xen PCI passthrough fixes

[Igor Druzhinin]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 0/3] Xen PCI passthrough fixes

[Igor Druzhinin]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Igor Druzhinin]

This aperture doesn't exist in QEMU-XEN and hvmloader places BARs
in arbitrary order disregarding prefetchable bit. This makes
prefetchable and non-prefetchable BARs to follow each other that's
quite likely with PCI passthrough devices. In that case, the existing
code, that tries to work out aperture boundaries by reading hvmloader
BAR placement, will report a bogus prefetchable aperture which overlaps
with the regular one. It will eventually trigger an assertion in
DXE PCI initialization code.

Do the same thing as OVMF on QEMU-KVM and pass a non-existing aperture
there. It's not necessary to pass additional allocation flags as we set
ResourceAssigned flag on the root bridge which means they will be ignored.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
Changes in v2:
* remove usage of prefetchable aperture entirely
* explained rationale for the change in the description
---
 OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 34 ++++++++++-----------------
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
index 9204179..5ea63f7 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
+++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
@@ -66,9 +66,7 @@ PcatPciRootBridgeParseBars (
   IN UINTN                          BarOffsetEnd,
   IN PCI_ROOT_BRIDGE_APERTURE       *Io,
   IN PCI_ROOT_BRIDGE_APERTURE       *Mem,
-  IN PCI_ROOT_BRIDGE_APERTURE       *MemAbove4G,
-  IN PCI_ROOT_BRIDGE_APERTURE       *PMem,
-  IN PCI_ROOT_BRIDGE_APERTURE       *PMemAbove4G
+  IN PCI_ROOT_BRIDGE_APERTURE       *MemAbove4G
 
 )
 {
@@ -129,11 +127,7 @@ PcatPciRootBridgeParseBars (
           //
           Length = ((~Length) + 1) & 0xffffffff;
 
-          if ((Value & BIT3) == BIT3) {
-            MemAperture = PMem;
-          } else {
-            MemAperture = Mem;
-          }
+          MemAperture = Mem;
         } else {
           //
           // 64bit
@@ -149,11 +143,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;
@@ -170,6 +160,8 @@ PcatPciRootBridgeParseBars (
   }
 }
 
+STATIC PCI_ROOT_BRIDGE_APERTURE mNonExistAperture = { MAX_UINT64, 0 };
+
 PCI_ROOT_BRIDGE *
 ScanForRootBridges (
   UINTN      *NumberOfRootBridges
@@ -186,7 +178,7 @@ ScanForRootBridges (
   UINT64     Base;
   UINT64     Limit;
   UINT64     Value;
-  PCI_ROOT_BRIDGE_APERTURE Io, Mem, MemAbove4G, PMem, PMemAbove4G, *MemAperture;
+  PCI_ROOT_BRIDGE_APERTURE Io, Mem, MemAbove4G, *MemAperture;
   PCI_ROOT_BRIDGE *RootBridges;
   UINTN      BarOffsetEnd;
 
@@ -206,9 +198,7 @@ ScanForRootBridges (
     ZeroMem (&Io, sizeof (Io));
     ZeroMem (&Mem, sizeof (Mem));
     ZeroMem (&MemAbove4G, sizeof (MemAbove4G));
-    ZeroMem (&PMem, sizeof (PMem));
-    ZeroMem (&PMemAbove4G, sizeof (PMemAbove4G));
-    Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base = PMemAbove4G.Base = MAX_UINT64;
+    Io.Base = Mem.Base = MemAbove4G.Base = MAX_UINT64;
     //
     // Scan all the PCI devices on the primary bus of the PCI root bridge
     //
@@ -313,16 +303,17 @@ ScanForRootBridges (
 
           //
           // Get the Prefetchable Memory range that the PPB is decoding
+          // and merge it into Memory range
           //
           Value = Pci.Bridge.PrefetchableMemoryBase & 0x0f;
           Base = ((UINT32) Pci.Bridge.PrefetchableMemoryBase & 0xfff0) << 16;
           Limit = (((UINT32) Pci.Bridge.PrefetchableMemoryLimit & 0xfff0)
                    << 16) | 0xfffff;
-          MemAperture = &PMem;
+          MemAperture = &Mem;
           if (Value == BIT0) {
             Base |= LShiftU64 (Pci.Bridge.PrefetchableBaseUpper32, 32);
             Limit |= LShiftU64 (Pci.Bridge.PrefetchableLimitUpper32, 32);
-            MemAperture = &PMemAbove4G;
+            MemAperture = &MemAbove4G;
           }
           if (Base < Limit) {
             if (MemAperture->Base > Base) {
@@ -373,8 +364,7 @@ ScanForRootBridges (
           OFFSET_OF (PCI_TYPE00, Device.Bar),
           BarOffsetEnd,
           &Io,
-          &Mem, &MemAbove4G,
-          &PMem, &PMemAbove4G
+          &Mem, &MemAbove4G
         );
 
         //
@@ -446,7 +436,7 @@ ScanForRootBridges (
       InitRootBridge (
         Attributes, Attributes, 0,
         (UINT8) PrimaryBus, (UINT8) SubBus,
-        &Io, &Mem, &MemAbove4G, &PMem, &PMemAbove4G,
+        &Io, &Mem, &MemAbove4G, &mNonExistAperture, &mNonExistAperture,
         &RootBridges[*NumberOfRootBridges]
       );
       RootBridges[*NumberOfRootBridges].ResourceAssigned = TRUE;
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Igor Druzhinin]

This aperture doesn't exist in QEMU-XEN and hvmloader places BARs
in arbitrary order disregarding prefetchable bit. This makes
prefetchable and non-prefetchable BARs to follow each other that's
quite likely with PCI passthrough devices. In that case, the existing
code, that tries to work out aperture boundaries by reading hvmloader
BAR placement, will report a bogus prefetchable aperture which overlaps
with the regular one. It will eventually trigger an assertion in
DXE PCI initialization code.

Do the same thing as OVMF on QEMU-KVM and pass a non-existing aperture
there. It's not necessary to pass additional allocation flags as we set
ResourceAssigned flag on the root bridge which means they will be ignored.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
Changes in v2:
* remove usage of prefetchable aperture entirely
* explained rationale for the change in the description
---
 OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 34 ++++++++++-----------------
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
index 9204179..5ea63f7 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
+++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
@@ -66,9 +66,7 @@ PcatPciRootBridgeParseBars (
   IN UINTN                          BarOffsetEnd,
   IN PCI_ROOT_BRIDGE_APERTURE       *Io,
   IN PCI_ROOT_BRIDGE_APERTURE       *Mem,
-  IN PCI_ROOT_BRIDGE_APERTURE       *MemAbove4G,
-  IN PCI_ROOT_BRIDGE_APERTURE       *PMem,
-  IN PCI_ROOT_BRIDGE_APERTURE       *PMemAbove4G
+  IN PCI_ROOT_BRIDGE_APERTURE       *MemAbove4G
 
 )
 {
@@ -129,11 +127,7 @@ PcatPciRootBridgeParseBars (
           //
           Length = ((~Length) + 1) & 0xffffffff;
 
-          if ((Value & BIT3) == BIT3) {
-            MemAperture = PMem;
-          } else {
-            MemAperture = Mem;
-          }
+          MemAperture = Mem;
         } else {
           //
           // 64bit
@@ -149,11 +143,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;
@@ -170,6 +160,8 @@ PcatPciRootBridgeParseBars (
   }
 }
 
+STATIC PCI_ROOT_BRIDGE_APERTURE mNonExistAperture = { MAX_UINT64, 0 };
+
 PCI_ROOT_BRIDGE *
 ScanForRootBridges (
   UINTN      *NumberOfRootBridges
@@ -186,7 +178,7 @@ ScanForRootBridges (
   UINT64     Base;
   UINT64     Limit;
   UINT64     Value;
-  PCI_ROOT_BRIDGE_APERTURE Io, Mem, MemAbove4G, PMem, PMemAbove4G, *MemAperture;
+  PCI_ROOT_BRIDGE_APERTURE Io, Mem, MemAbove4G, *MemAperture;
   PCI_ROOT_BRIDGE *RootBridges;
   UINTN      BarOffsetEnd;
 
@@ -206,9 +198,7 @@ ScanForRootBridges (
     ZeroMem (&Io, sizeof (Io));
     ZeroMem (&Mem, sizeof (Mem));
     ZeroMem (&MemAbove4G, sizeof (MemAbove4G));
-    ZeroMem (&PMem, sizeof (PMem));
-    ZeroMem (&PMemAbove4G, sizeof (PMemAbove4G));
-    Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base = PMemAbove4G.Base = MAX_UINT64;
+    Io.Base = Mem.Base = MemAbove4G.Base = MAX_UINT64;
     //
     // Scan all the PCI devices on the primary bus of the PCI root bridge
     //
@@ -313,16 +303,17 @@ ScanForRootBridges (
 
           //
           // Get the Prefetchable Memory range that the PPB is decoding
+          // and merge it into Memory range
           //
           Value = Pci.Bridge.PrefetchableMemoryBase & 0x0f;
           Base = ((UINT32) Pci.Bridge.PrefetchableMemoryBase & 0xfff0) << 16;
           Limit = (((UINT32) Pci.Bridge.PrefetchableMemoryLimit & 0xfff0)
                    << 16) | 0xfffff;
-          MemAperture = &PMem;
+          MemAperture = &Mem;
           if (Value == BIT0) {
             Base |= LShiftU64 (Pci.Bridge.PrefetchableBaseUpper32, 32);
             Limit |= LShiftU64 (Pci.Bridge.PrefetchableLimitUpper32, 32);
-            MemAperture = &PMemAbove4G;
+            MemAperture = &MemAbove4G;
           }
           if (Base < Limit) {
             if (MemAperture->Base > Base) {
@@ -373,8 +364,7 @@ ScanForRootBridges (
           OFFSET_OF (PCI_TYPE00, Device.Bar),
           BarOffsetEnd,
           &Io,
-          &Mem, &MemAbove4G,
-          &PMem, &PMemAbove4G
+          &Mem, &MemAbove4G
         );
 
         //
@@ -446,7 +436,7 @@ ScanForRootBridges (
       InitRootBridge (
         Attributes, Attributes, 0,
         (UINT8) PrimaryBus, (UINT8) SubBus,
-        &Io, &Mem, &MemAbove4G, &PMem, &PMemAbove4G,
+        &Io, &Mem, &MemAbove4G, &mNonExistAperture, &mNonExistAperture,
         &RootBridges[*NumberOfRootBridges]
       );
       RootBridges[*NumberOfRootBridges].ResourceAssigned = TRUE;
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 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
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
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 5ea63f7..f763134 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
+++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
@@ -143,7 +143,11 @@ PcatPciRootBridgeParseBars (
           Length = Length | LShiftU64 ((UINT64) UpperValue, 32);
           Length = (~Length) + 1;
 
-          MemAperture = MemAbove4G;
+          if (Base < BASE_4GB) {
+            MemAperture = Mem;
+          } else {
+            MemAperture = MemAbove4G;
+          }
         }
 
         Limit = Base + Length - 1;
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 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
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
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 5ea63f7..f763134 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
+++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
@@ -143,7 +143,11 @@ PcatPciRootBridgeParseBars (
           Length = Length | LShiftU64 ((UINT64) UpperValue, 32);
           Length = (~Length) + 1;
 
-          MemAperture = MemAbove4G;
+          if (Base < BASE_4GB) {
+            MemAperture = Mem;
+          } else {
+            MemAperture = MemAbove4G;
+          }
         }
 
         Limit = Base + Length - 1;
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 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>
---
Changes in v2:
* coding style issues and minor suggestions
---
 OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 35 +++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
index f763134..67a37cd 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
+++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
@@ -55,6 +55,35 @@ PcatPciRootBridgeBarExisted (
   EnableInterrupts ();
 }
 
+#define 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 & PCI_COMMAND_DECODE) {
+      PciWrite16 (Address, *OriginalValue & ~PCI_COMMAND_DECODE);
+    }
+  } else {
+    if (Value & PCI_COMMAND_DECODE) {
+      PciWrite16 (Address, Value);
+    }
+  }
+}
+
 STATIC
 VOID
 PcatPciRootBridgeParseBars (
@@ -81,6 +110,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, (UINT16 *)&OriginalValue
+  );
+
   for (Offset = BarOffsetBase; Offset < BarOffsetEnd; Offset += sizeof (UINT32)) {
     PcatPciRootBridgeBarExisted (
       PCI_LIB_ADDRESS (Bus, Device, Function, Offset),
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 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>
---
Changes in v2:
* coding style issues and minor suggestions
---
 OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 35 +++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
index f763134..67a37cd 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
+++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
@@ -55,6 +55,35 @@ PcatPciRootBridgeBarExisted (
   EnableInterrupts ();
 }
 
+#define 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 & PCI_COMMAND_DECODE) {
+      PciWrite16 (Address, *OriginalValue & ~PCI_COMMAND_DECODE);
+    }
+  } else {
+    if (Value & PCI_COMMAND_DECODE) {
+      PciWrite16 (Address, Value);
+    }
+  }
+}
+
 STATIC
 VOID
 PcatPciRootBridgeParseBars (
@@ -81,6 +110,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, (UINT16 *)&OriginalValue
+  );
+
   for (Offset = BarOffsetBase; Offset < BarOffsetEnd; Offset += sizeof (UINT32)) {
     PcatPciRootBridgeBarExisted (
       PCI_LIB_ADDRESS (Bus, Device, Function, Offset),
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Laszlo Ersek]

On 04/09/19 05:12, 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>
> ---
> Changes in v2:
> * coding style issues and minor suggestions
> ---
>  OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 35 +++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
> index f763134..67a37cd 100644
> --- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
> +++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
> @@ -55,6 +55,35 @@ PcatPciRootBridgeBarExisted (
>    EnableInterrupts ();
>  }
>  
> +#define PCI_COMMAND_DECODE ((UINT16)(EFI_PCI_COMMAND_IO_SPACE | \
> +                                     EFI_PCI_COMMAND_MEMORY_SPACE))

Thanks for this update (= dropping the EFI prefix).

> +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 & PCI_COMMAND_DECODE) {
> +      PciWrite16 (Address, *OriginalValue & ~PCI_COMMAND_DECODE);
> +    }
> +  } else {
> +    if (Value & PCI_COMMAND_DECODE) {
> +      PciWrite16 (Address, Value);
> +    }
> +  }
> +}
> +
>  STATIC
>  VOID
>  PcatPciRootBridgeParseBars (
> @@ -81,6 +110,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, (UINT16 *)&OriginalValue
> +  );
> +
>    for (Offset = BarOffsetBase; Offset < BarOffsetEnd; Offset += sizeof (UINT32)) {
>      PcatPciRootBridgeBarExisted (
>        PCI_LIB_ADDRESS (Bus, Device, Function, Offset),
> 


(1) I don't understand the purpose of the "OriginalValue" parameter in
PcatPciRootBridgeDecoding().

- The caller (i.e. PcatPciRootBridgeParseBars()) never uses the value
output in it, AFAICS.

- I also dislike the (UINT16 *) cast, in the caller.

- In addition, the very first assignment (to Value) in
PcatPciRootBridgeDecoding() reads an indeterminate value from
*OriginalValue, which is undefined behavior.

- Furthermore, PcatPciRootBridgeDecoding() annotates OriginalValue as
OUT, not IN OUT, hence it shouldn't start out by reading *OriginalValue.

I suggest dropping the parameter (in favor of a similar local variable
in PcatPciRootBridgeDecoding()).


(2) The expression

  *OriginalValue & ~PCI_COMMAND_DECODE

is not optimal. In the operand of the "~" operator, PCI_COMMAND_DECODE
is converted to "int" (INT32 in edk2), as part of the "default integer
promotions". The "~" operator flips the sign bit as well, and
(~PCI_COMMAND_DECODE) evaluates to a signed integer with negative value.

(*OriginalValue) is also of type UINT16 -- in
PcatPciRootBridgeDecoding() --; the same default integer promotion
applies to it. Therefore the "&" operator is applied to two signed
integers, of which one has negative value.

While none of the above is undefined behavior per se (and in practice
the expression will work OK), interpreting the expression for what it
*really* means, is laborious. Write the following instead, please:

  *OriginalValue & ~(UINT32)PCI_COMMAND_DECODE

In this case, the operand of "~" will be of type "unsigned int", and
will not undergo a promotion.

(*OriginalValue) will undergo the same promotion as before (to int /
INT32). Then, it will be converted to "unsigned int" / UINT32, as part
of the "usual arithmetic conversions", given the type of the other
operand of "&".

This way, both the bit-neg and the bit-and operators will only have
"unsigned int" (UINT32) operands, which is a lot cleaner.

... I admit that seeing bitwise operators applied to signed integers is
one of my pet peeves; sorry about that.


If you get positive test results from others (mainly on xen-devel I
guess) for this v2 patch, and the Xen reviewers of OvmfPkg are otherwise
fine with this v2 patch, then I think the above-requested changes,
hopefully addressed in v3, should not invalidate any feedback tags given
for v2.

Thanks!
Laszlo

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 3/3] OvmfPkg/XenSupport: turn off address decoding before BAR sizing

[Laszlo Ersek]

On 04/09/19 05:12, 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>
> ---
> Changes in v2:
> * coding style issues and minor suggestions
> ---
>  OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 35 +++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
> index f763134..67a37cd 100644
> --- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
> +++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
> @@ -55,6 +55,35 @@ PcatPciRootBridgeBarExisted (
>    EnableInterrupts ();
>  }
>  
> +#define PCI_COMMAND_DECODE ((UINT16)(EFI_PCI_COMMAND_IO_SPACE | \
> +                                     EFI_PCI_COMMAND_MEMORY_SPACE))

Thanks for this update (= dropping the EFI prefix).

> +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 & PCI_COMMAND_DECODE) {
> +      PciWrite16 (Address, *OriginalValue & ~PCI_COMMAND_DECODE);
> +    }
> +  } else {
> +    if (Value & PCI_COMMAND_DECODE) {
> +      PciWrite16 (Address, Value);
> +    }
> +  }
> +}
> +
>  STATIC
>  VOID
>  PcatPciRootBridgeParseBars (
> @@ -81,6 +110,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, (UINT16 *)&OriginalValue
> +  );
> +
>    for (Offset = BarOffsetBase; Offset < BarOffsetEnd; Offset += sizeof (UINT32)) {
>      PcatPciRootBridgeBarExisted (
>        PCI_LIB_ADDRESS (Bus, Device, Function, Offset),
> 


(1) I don't understand the purpose of the "OriginalValue" parameter in
PcatPciRootBridgeDecoding().

- The caller (i.e. PcatPciRootBridgeParseBars()) never uses the value
output in it, AFAICS.

- I also dislike the (UINT16 *) cast, in the caller.

- In addition, the very first assignment (to Value) in
PcatPciRootBridgeDecoding() reads an indeterminate value from
*OriginalValue, which is undefined behavior.

- Furthermore, PcatPciRootBridgeDecoding() annotates OriginalValue as
OUT, not IN OUT, hence it shouldn't start out by reading *OriginalValue.

I suggest dropping the parameter (in favor of a similar local variable
in PcatPciRootBridgeDecoding()).


(2) The expression

  *OriginalValue & ~PCI_COMMAND_DECODE

is not optimal. In the operand of the "~" operator, PCI_COMMAND_DECODE
is converted to "int" (INT32 in edk2), as part of the "default integer
promotions". The "~" operator flips the sign bit as well, and
(~PCI_COMMAND_DECODE) evaluates to a signed integer with negative value.

(*OriginalValue) is also of type UINT16 -- in
PcatPciRootBridgeDecoding() --; the same default integer promotion
applies to it. Therefore the "&" operator is applied to two signed
integers, of which one has negative value.

While none of the above is undefined behavior per se (and in practice
the expression will work OK), interpreting the expression for what it
*really* means, is laborious. Write the following instead, please:

  *OriginalValue & ~(UINT32)PCI_COMMAND_DECODE

In this case, the operand of "~" will be of type "unsigned int", and
will not undergo a promotion.

(*OriginalValue) will undergo the same promotion as before (to int /
INT32). Then, it will be converted to "unsigned int" / UINT32, as part
of the "usual arithmetic conversions", given the type of the other
operand of "&".

This way, both the bit-neg and the bit-and operators will only have
"unsigned int" (UINT32) operands, which is a lot cleaner.

... I admit that seeing bitwise operators applied to signed integers is
one of my pet peeves; sorry about that.


If you get positive test results from others (mainly on xen-devel I
guess) for this v2 patch, and the Xen reviewers of OvmfPkg are otherwise
fine with this v2 patch, then I think the above-requested changes,
hopefully addressed in v3, should not invalidate any feedback tags given
for v2.

Thanks!
Laszlo

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 0/3] Xen PCI passthrough fixes

[Laszlo Ersek]

Hi Igor,

On 04/09/19 05:12, Igor Druzhinin wrote:
> 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(-)
> 

some general administrativa:

(1) the edk2-devel mailing list has moved. Please subscribe at
<https://edk2.groups.io/g/devel>, and resend the series. Apologies about
the inconvenience.

(2) By the time we push this series,
<https://bugzilla.tianocore.org/show_bug.cgi?id=1373> will have been
fixed most likely. That means that you will no longer need to add the
"Contributed-under" line to your signoffs, on all patches.

Thanks!
Laszlo

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 0/3] Xen PCI passthrough fixes

[Laszlo Ersek]

Hi Igor,

On 04/09/19 05:12, Igor Druzhinin wrote:
> 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(-)
> 

some general administrativa:

(1) the edk2-devel mailing list has moved. Please subscribe at
<https://edk2.groups.io/g/devel>, and resend the series. Apologies about
the inconvenience.

(2) By the time we push this series,
<https://bugzilla.tianocore.org/show_bug.cgi?id=1373> will have been
fixed most likely. That means that you will no longer need to add the
"Contributed-under" line to your signoffs, on all patches.

Thanks!
Laszlo

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Anthony PERARD]

On Tue, Apr 09, 2019 at 04:12:38AM +0100, Igor Druzhinin wrote:
> This aperture doesn't exist in QEMU-XEN and hvmloader places BARs
> in arbitrary order disregarding prefetchable bit. This makes
> prefetchable and non-prefetchable BARs to follow each other that's
> quite likely with PCI passthrough devices. In that case, the existing
> code, that tries to work out aperture boundaries by reading hvmloader
> BAR placement, will report a bogus prefetchable aperture which overlaps
> with the regular one. It will eventually trigger an assertion in
> DXE PCI initialization code.
> 
> Do the same thing as OVMF on QEMU-KVM and pass a non-existing aperture
> there. It's not necessary to pass additional allocation flags as we set
> ResourceAssigned flag on the root bridge which means they will be ignored.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> ---
> Changes in v2:
> * remove usage of prefetchable aperture entirely
> * explained rationale for the change in the description
> ---

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Anthony PERARD]

On Tue, Apr 09, 2019 at 04:12:38AM +0100, Igor Druzhinin wrote:
> This aperture doesn't exist in QEMU-XEN and hvmloader places BARs
> in arbitrary order disregarding prefetchable bit. This makes
> prefetchable and non-prefetchable BARs to follow each other that's
> quite likely with PCI passthrough devices. In that case, the existing
> code, that tries to work out aperture boundaries by reading hvmloader
> BAR placement, will report a bogus prefetchable aperture which overlaps
> with the regular one. It will eventually trigger an assertion in
> DXE PCI initialization code.
> 
> Do the same thing as OVMF on QEMU-KVM and pass a non-existing aperture
> there. It's not necessary to pass additional allocation flags as we set
> ResourceAssigned flag on the root bridge which means they will be ignored.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> ---
> Changes in v2:
> * remove usage of prefetchable aperture entirely
> * explained rationale for the change in the description
> ---

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Andrew Cooper]

On 09/04/2019 04:12, 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.

This is an HVMLoader bug, surely?

Why not fix it in HVMLoader by not enabling memory decoding in the first
place, and avoid having boots now take two sets of emulations to play
with memory decoding, when 0 will do?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 3/3] OvmfPkg/XenSupport: turn off address decoding before BAR sizing

[Andrew Cooper]

On 09/04/2019 04:12, 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.

This is an HVMLoader bug, surely?

Why not fix it in HVMLoader by not enabling memory decoding in the first
place, and avoid having boots now take two sets of emulations to play
with memory decoding, when 0 will do?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Igor Druzhinin]

On 11/04/2019 14:19, Andrew Cooper wrote:
> On 09/04/2019 04:12, 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.
> 
> This is an HVMLoader bug, surely?

I'll try to turn it off in HVMLoader but I'm not sure other BIOSes would
like it. Although they should not make any assumptions on whether memory
decoding is enabled or not (so as OVMF - hence this patch) it's still
risky from my point of view.

Igor

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 3/3] OvmfPkg/XenSupport: turn off address decoding before BAR sizing

[Igor Druzhinin]

On 11/04/2019 14:19, Andrew Cooper wrote:
> On 09/04/2019 04:12, 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.
> 
> This is an HVMLoader bug, surely?

I'll try to turn it off in HVMLoader but I'm not sure other BIOSes would
like it. Although they should not make any assumptions on whether memory
decoding is enabled or not (so as OVMF - hence this patch) it's still
risky from my point of view.

Igor

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Igor Druzhinin]

On 11/04/2019 15:13, Igor Druzhinin wrote:
> On 11/04/2019 14:19, Andrew Cooper wrote:
>> On 09/04/2019 04:12, 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.
>>
>> This is an HVMLoader bug, surely?
> 
> I'll try to turn it off in HVMLoader but I'm not sure other BIOSes would
> like it. Although they should not make any assumptions on whether memory
> decoding is enabled or not (so as OVMF - hence this patch) it's still
> risky from my point of view.

Having tested ROMBIOS and SeaBIOS - none of them worked without
hvmloader enabling decoding. While it seems wrong - they obviously
plagued by the same assumption on system state as OVMF. This patch
addresses the issue in OVMF and I think that's correct. Addressing the
issue in other BIOSes might be considered an improvement for the future.

Igor

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 3/3] OvmfPkg/XenSupport: turn off address decoding before BAR sizing

[Igor Druzhinin]

On 11/04/2019 15:13, Igor Druzhinin wrote:
> On 11/04/2019 14:19, Andrew Cooper wrote:
>> On 09/04/2019 04:12, 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.
>>
>> This is an HVMLoader bug, surely?
> 
> I'll try to turn it off in HVMLoader but I'm not sure other BIOSes would
> like it. Although they should not make any assumptions on whether memory
> decoding is enabled or not (so as OVMF - hence this patch) it's still
> risky from my point of view.

Having tested ROMBIOS and SeaBIOS - none of them worked without
hvmloader enabling decoding. While it seems wrong - they obviously
plagued by the same assumption on system state as OVMF. This patch
addresses the issue in OVMF and I think that's correct. Addressing the
issue in other BIOSes might be considered an improvement for the future.

Igor

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel