| Message ID | 1554779560-26204-4-git-send-email-igor.druzhinin@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Xen PCI passthrough fixes | expand |
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
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
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
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
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),
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(+)