| Message ID | 1408407718-10835-9-git-send-email-mdroth@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 08/19/2014 10:21 AM, Michael Roth wrote: > Some kernels program a 0 address for io regions. PCI 3.0 spec > section 6.2.5.1 doesn't seem to disallow this. I remember there was discussion about it but I forgot :) Why does it have to be a part of this patchset? Worth mentioning in the commit log I believe. > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > --- > hw/pci/pci.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 351d320..9578749 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -1035,7 +1035,7 @@ static pcibus_t pci_bar_address(PCIDevice *d, > /* Check if 32 bit BAR wraps around explicitly. > * TODO: make priorities correct and remove this work around. > */ > - if (last_addr <= new_addr || new_addr == 0 || last_addr >= UINT32_MAX) { > + if (last_addr <= new_addr || last_addr >= UINT32_MAX) { > return PCI_BAR_UNMAPPED; > } > return new_addr; >
On 19.08.14 02:21, Michael Roth wrote: > Some kernels program a 0 address for io regions. PCI 3.0 spec > section 6.2.5.1 doesn't seem to disallow this. > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> This patch does not need to be inside of this patch set. It also should go via Michael's tree. Alex > --- > hw/pci/pci.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 351d320..9578749 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -1035,7 +1035,7 @@ static pcibus_t pci_bar_address(PCIDevice *d, > /* Check if 32 bit BAR wraps around explicitly. > * TODO: make priorities correct and remove this work around. > */ > - if (last_addr <= new_addr || new_addr == 0 || last_addr >= UINT32_MAX) { > + if (last_addr <= new_addr || last_addr >= UINT32_MAX) { > return PCI_BAR_UNMAPPED; > } > return new_addr; >
On 26 August 2014 10:14, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > On 08/19/2014 10:21 AM, Michael Roth wrote: >> Some kernels program a 0 address for io regions. PCI 3.0 spec >> section 6.2.5.1 doesn't seem to disallow this. > > > I remember there was discussion about it but I forgot :) I think the conclusion we came to was that there may have been a note in the PCI 2.1 spec that implied that 0 addresses meant "disabled" but this seems to have gone from later versions, suggesting it was erroneous. Personally I'm happy for us to remove the "0 means disabled" check, but I'd prefer it if we do it consistently for both IO and MMIO regions -- this patch only changes the IO BAR code. thanks -- PMM
Quoting Alexey Kardashevskiy (2014-08-26 04:14:27) > On 08/19/2014 10:21 AM, Michael Roth wrote: > > Some kernels program a 0 address for io regions. PCI 3.0 spec > > section 6.2.5.1 doesn't seem to disallow this. > > > I remember there was discussion about it but I forgot :) Why does it have > to be a part of this patchset? Worth mentioning in the commit log I believe. Unfortunately with ppc guests the first bar allocation tends to be the 0-address case, so to me it seemed necessary for a testable series. Can simply document this in the series and re-send separately though. > > > > > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > > --- > > hw/pci/pci.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > index 351d320..9578749 100644 > > --- a/hw/pci/pci.c > > +++ b/hw/pci/pci.c > > @@ -1035,7 +1035,7 @@ static pcibus_t pci_bar_address(PCIDevice *d, > > /* Check if 32 bit BAR wraps around explicitly. > > * TODO: make priorities correct and remove this work around. > > */ > > - if (last_addr <= new_addr || new_addr == 0 || last_addr >= UINT32_MAX) { > > + if (last_addr <= new_addr || last_addr >= UINT32_MAX) { > > return PCI_BAR_UNMAPPED; > > } > > return new_addr; > > > > > -- > Alexey
On Mon, Aug 18, 2014 at 07:21:54PM -0500, Michael Roth wrote: > Some kernels program a 0 address for io regions. PCI 3.0 spec > section 6.2.5.1 doesn't seem to disallow this. > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> Yes the PCI spec does not care. But unfortunately as documented in the comment, at least for PC (maybe others) priorities aren't currently setup correctly, so programming PCI BAR at address zero (during sizing) conflicts with whatever else is there. To make address 0 work, you'll have to fix up the prioriorities for a bunch of machine types :( > --- > hw/pci/pci.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 351d320..9578749 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -1035,7 +1035,7 @@ static pcibus_t pci_bar_address(PCIDevice *d, > /* Check if 32 bit BAR wraps around explicitly. > * TODO: make priorities correct and remove this work around. > */ > - if (last_addr <= new_addr || new_addr == 0 || last_addr >= UINT32_MAX) { > + if (last_addr <= new_addr || last_addr >= UINT32_MAX) { > return PCI_BAR_UNMAPPED; > } > return new_addr; > -- > 1.9.1 >
Quoting Michael S. Tsirkin (2014-08-27 08:47:51) > On Mon, Aug 18, 2014 at 07:21:54PM -0500, Michael Roth wrote: > > Some kernels program a 0 address for io regions. PCI 3.0 spec > > section 6.2.5.1 doesn't seem to disallow this. > > > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > > Yes the PCI spec does not care. > > But unfortunately as documented in the comment, at > least for PC (maybe others) priorities aren't > currently setup correctly, so programming PCI BAR at > address zero (during sizing) conflicts with > whatever else is there. I'm not sure I understand: that note was included as part of the following fixup to 9f1a029abf15751e32a4b1df99ed2b8315f9072c: - if (last_addr <= new_addr || new_addr == 0) { + /* Check if BAR is being sized explicitly. + * TODO: make priorities correct and remove this work around. + */ + if (last_addr <= new_addr || new_addr == 0 || last_addr >= UINT32_MAX) which forces the BAR to PCI_BAR_UNMAPPED and unmaps the io region if the address range extends beyond UINT32_MAX (which would happen during sizing when guest writes -1...and I guess maybe last_addr <= new_addr covered the same case back when we used uint32_t for pcibus_t?) ... But the (new_addr == 0) seems to be something unrelated..., it means the guest actually attempted to program a 0 address, or... since pci_update_mappings unconditionally updates all IO regions for a device whenever a particular BAR is written to, it would prevent us from temporarily mapping all the IO regions to 0 (until guest re-assigns them) ... You mentioned in the past this could lead to dispatch tables getting permanantly corrupted, so maybe that's what the check was for? But I guess there's still a separate issue, where there's a high liklihood that a 0 address would conflict with some hard-wired IO address? Wouldn't this be a guest bug though? Well, I guess it would be a QEMU bug if the above scenario is a real one...but if we fix or verify that's not the case, would this be an acceptable change? > > To make address 0 work, you'll have to fix up the prioriorities for a > bunch of machine types :( > > > --- > > hw/pci/pci.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > index 351d320..9578749 100644 > > --- a/hw/pci/pci.c > > +++ b/hw/pci/pci.c > > @@ -1035,7 +1035,7 @@ static pcibus_t pci_bar_address(PCIDevice *d, > > /* Check if 32 bit BAR wraps around explicitly. > > * TODO: make priorities correct and remove this work around. > > */ > > - if (last_addr <= new_addr || new_addr == 0 || last_addr >= UINT32_MAX) { > > + if (last_addr <= new_addr || last_addr >= UINT32_MAX) { > > return PCI_BAR_UNMAPPED; > > } > > return new_addr; > > -- > > 1.9.1 > >
On 28 August 2014 22:21, Michael Roth <mdroth@linux.vnet.ibm.com> wrote: > But I guess there's still a separate issue, where there's a high liklihood that > a 0 address would conflict with some hard-wired IO address? Wouldn't this be a > guest bug though? Even if it's a guest bug, we should act like the hardware does if the guest does this. If that differs between PCI controllers then we need a flag so the host controller model can select the required behaviour. (The versatile PB PCI controller we model does have "address 0 is valid", and we'd need to have this working if we implemented DMA accesses properly.) -- PMM
On Thu, Aug 28, 2014 at 10:33:02PM +0100, Peter Maydell wrote: > On 28 August 2014 22:21, Michael Roth <mdroth@linux.vnet.ibm.com> wrote: > > But I guess there's still a separate issue, where there's a high liklihood that > > a 0 address would conflict with some hard-wired IO address? Wouldn't this be a > > guest bug though? Real hardware behaves in a specific way. the problem is we don't always emulate it properly, and PCI has some work arounds for that. > Even if it's a guest bug, we should act like the hardware does > if the guest does this. If that differs between PCI controllers > then we need a flag so the host controller model can select > the required behaviour. (The versatile PB PCI controller we > model does have "address 0 is valid", and we'd need to have > this working if we implemented DMA accesses properly.) > > -- PMM Exactly. Actually, I forgot that I might have already fixed it for PC: 83d08f2673504a299194dcac1657a13754b5932a pc: map PCI address space as catchall region for not mapped addresses But need to go back and re-check other systems.
diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 351d320..9578749 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -1035,7 +1035,7 @@ static pcibus_t pci_bar_address(PCIDevice *d, /* Check if 32 bit BAR wraps around explicitly. * TODO: make priorities correct and remove this work around. */ - if (last_addr <= new_addr || new_addr == 0 || last_addr >= UINT32_MAX) { + if (last_addr <= new_addr || last_addr >= UINT32_MAX) { return PCI_BAR_UNMAPPED; } return new_addr;
Some kernels program a 0 address for io regions. PCI 3.0 spec section 6.2.5.1 doesn't seem to disallow this. Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> --- hw/pci/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)