Re: [Qemu-devel] [PATCH, RFC] pci: allow PCI devices to fix address space

[Michael S. Tsirkin]

On Wed, Dec 22, 2010 at 07:30:33AM +0100, Hervé Poussineau wrote:
> Isaku Yamahata a écrit :
> >On Wed, Dec 22, 2010 at 12:20:23AM +0100, Andreas Färber wrote:
> >>From: Hervé Poussineau <address@hidden>
> >>
> >>v1:
> >>* Rebased.
> >>
> >>Signed-off-by: Hervé Poussineau <address@hidden>
> >>Cc: Michael S. Tsirkin <address@hidden>
> >>Signed-off-by: Andreas Färber <address@hidden>
> >>---
> >> Hello Michael,
> >> Could you please take a look at this? I'm out of my field here.
> >> The intention of the first part appears to be to save (val & ~mask),
> >> whereas the inline helper would've returned (val & mask).
> >
> >Such behavior is intended.
> >The returned value is just discarded in this case.
> >test-and-clear means
> >     clear the bits
> >     return if those cleared bits were really set.
> >

What about this first chunk? Is it necessary.

> >> The second part makes existing code conditional on that value.
> >
> >What issue are you addressing?
> >Although the spec doesn't says about the default value of BAR registers
> >after reset, the current code assumes that almost all the pci devices clear
> >those registers.
> >Anyway after cold/warm reset firmware sets up BARs, so it doesn't matter.
> >You, however, seem to want to keep BARs over resets.
> >
> >thanks,
> >
> >
> As you have seen, the intend here is to be able to keep BARs over resets.
> It is required for some really specific devices, like a PCI to ISA
> bridge, where MMIO is always at the same address.
> In that case, the device keeps PCI_COMMAND_MEMORY and/or
> PCI_COMMAND_IO flags as read-only.
> 
> Hervé

Aha. Are the BARs still writeable?  If not maybe that's the right thing
to check? If yes maybe the device simply should have a reset
handler to rewrite them?


Also I would like the above text (maybe a bit shortened) to replace the comment:
            /* Reset region address, as it it is not read-only */
the general idea is to document the reason the code
is what it is, not restate what it does.

Something similar should also go into the commit message I think.


> >> Regards,
> >> Andreas
> >> hw/pci.c |   22 ++++++++++++++--------
> >> 1 files changed, 14 insertions(+), 8 deletions(-)
> >>
> >>diff --git a/hw/pci.c b/hw/pci.c
> >>index ef00d20..4db4b1f 100644
> >>--- a/hw/pci.c
> >>+++ b/hw/pci.c
> >>@@ -140,6 +140,7 @@ static void pci_update_irq_status(PCIDevice *dev)
> >> static void pci_device_reset(PCIDevice *dev)
> >> {
> >>     int r;
> >>+    uint16_t cmd;
> >>     /* TODO: call the below unconditionally once all pci devices
> >>      * are qdevified */
> >>     if (dev->qdev.info) {
> >>@@ -149,9 +150,10 @@ static void pci_device_reset(PCIDevice *dev)
> >>     dev->irq_state = 0;
> >>     pci_update_irq_status(dev);
> >>     /* Clear all writeable bits */
> >>-    pci_word_test_and_clear_mask(dev->config + PCI_COMMAND,
> >>-                                 pci_get_word(dev->wmask + PCI_COMMAND) |
> >>-                                 pci_get_word(dev->w1cmask + PCI_COMMAND));
> >>+    cmd = pci_get_word(dev->config + PCI_COMMAND);
> >>+    cmd &= ~(pci_get_word(dev->wmask + PCI_COMMAND) |
> >>+             pci_get_word(dev->w1cmask + PCI_COMMAND));
> >>+    pci_set_word(dev->config + PCI_COMMAND, cmd);
> >>     pci_word_test_and_clear_mask(dev->config + PCI_STATUS,
> >>                                  pci_get_word(dev->wmask + PCI_STATUS) |
> >>                                  pci_get_word(dev->w1cmask + PCI_STATUS));
> >>@@ -163,11 +165,15 @@ static void pci_device_reset(PCIDevice *dev)
> >>             continue;
> >>         }
> >>-        if (!(region->type & PCI_BASE_ADDRESS_SPACE_IO) &&
> >>-            region->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> >>-            pci_set_quad(dev->config + pci_bar(dev, r), region->type);
> >>-        } else {
> >>-            pci_set_long(dev->config + pci_bar(dev, r), region->type);
> >>+        if ((region->type == PCI_BASE_ADDRESS_SPACE_IO && !(cmd & 
> >>PCI_COMMAND_MEMORY)) ||
> >>+            (region->type == PCI_BASE_ADDRESS_SPACE_MEMORY && !(cmd & 
> >>PCI_COMMAND_IO))) {

These lines are too long.

> >>+            /* Reset region address, as it it is not read-only */

Checking wmask would be more straight-forward?


> >>+            if (!(region->type & PCI_BASE_ADDRESS_SPACE_IO) &&
> >>+                region->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> >>+                pci_set_quad(dev->config + pci_bar(dev, r), region->type);
> >>+            } else {
> >>+                pci_set_long(dev->config + pci_bar(dev, r), region->type);
> >>+            }
> >>         }
> >>     }
> >>     pci_update_mappings(dev);
> >>-- 
> >>1.7.3.4
> >>
> >>
> >