[Qemu-devel] Re: [PATCH 03/10] pci: fix pci_bus_reset() with 64bit BAR and several clean ups.

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Jun 17, 2010 at 03:15:45PM +0900, Isaku Yamahata wrote:
> fix pci_device_reset() with 64bit BAR.
> export pci_bus_reset(), pci_device_reset() and two helper functions
> for later use. And several clean ups.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/pci.c |   44 ++++++++++++++++++++++++++++++++++++--------
>  hw/pci.h |    5 +++++
>  2 files changed, 41 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 9ba62eb..87f5e6c 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -144,28 +144,50 @@ static void pci_update_irq_status(PCIDevice *dev)
>      }
>  }
>  
> -static void pci_device_reset(PCIDevice *dev)
> +void pci_device_reset_default(PCIDevice *dev)
>  {
>      int r;
>  
>      dev->irq_state = 0;
>      pci_update_irq_status(dev);
> -    dev->config[PCI_COMMAND] &= ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
> -                                  PCI_COMMAND_MASTER);
> +    pci_set_word(dev->config + PCI_COMMAND,
> +                 pci_get_word(dev->config + PCI_COMMAND) &
> +                 ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER));
>      dev->config[PCI_CACHE_LINE_SIZE] = 0x0;
>      dev->config[PCI_INTERRUPT_LINE] = 0x0;
>      for (r = 0; r < PCI_NUM_REGIONS; ++r) {
> -        if (!dev->io_regions[r].size) {
> +        PCIIORegion *region = &dev->io_regions[r];
> +        if (!region->size) {
>              continue;
>          }
> -        pci_set_long(dev->config + pci_bar(dev, r), dev->io_regions[r].type);
> +
> +        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);
>  }
>  

I applied the first hunk. Looking at it
made me notice that we don't clear interrupt disable
bit on reset, and we really should as it is read/write.
Rather than duplicating code, we should just use wmask.

I ended up with this:

commit b82d3876099c4f1fd009082f052e3bac7e3062e7
Author: Isaku Yamahata <yamahata@valinux.co.jp>
Date:   Thu Jun 17 15:15:45 2010 +0900

    pci: fix pci_device_reset
    
    Clear interrupt disable bit on reset, according to PCI spec.
    Fix pci_device_reset() with 64bit BAR.
    
    Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

diff --git a/hw/pci.c b/hw/pci.c
index 7787005..de33745 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -150,15 +150,24 @@ static void pci_device_reset(PCIDevice *dev)
 
     dev->irq_state = 0;
     pci_update_irq_status(dev);
-    dev->config[PCI_COMMAND] &= ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
-                                  PCI_COMMAND_MASTER);
+    /* Clear all writeable bits */
+    pci_set_word(dev->config + PCI_COMMAND,
+                 pci_get_word(dev->config + PCI_COMMAND) &
+                 ~pci_get_word(dev->wmask + PCI_COMMAND));
     dev->config[PCI_CACHE_LINE_SIZE] = 0x0;
     dev->config[PCI_INTERRUPT_LINE] = 0x0;
     for (r = 0; r < PCI_NUM_REGIONS; ++r) {
-        if (!dev->io_regions[r].size) {
+        PCIIORegion *region = &dev->io_regions[r];
+        if (!region->size) {
             continue;
         }
-        pci_set_long(dev->config + pci_bar(dev, r), dev->io_regions[r].type);
+
+        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);
 }