Re: [Qemu-ppc] [PATCH 08/12] pci: allow 0 address for PCI IO regions

[Michael Roth]

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 <address@hidden>
> 
> 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
> >