Re: [Qemu-devel] winXP "Standard PC" HAL and qemu-kvm >= 0.15

[Michael S. Tsirkin]

On Tue, Dec 06, 2011 at 03:02:49PM +0400, Michael Tokarev wrote:
> On 06.12.2011 14:32, Avi Kivity wrote:
> > On 12/05/2011 10:19 PM, Michael Tokarev wrote:
> >> On 05.12.2011 17:28, Avi Kivity wrote:
> >> []
> >>>> I haven't debugged further yet, -- because it were
> >>>> not easy to find out what was causing the regression
> >>>> and how to reproduce it, and also because I don't think
> >>>> it is the right HAL for qemu-kvm guest anyway.
> >>>
> >>> It's not, but the regression indicates we broke something.  It would be
> >>> good to know what that is.
> >>
> >> So today I gave it a chance with git bisect, and here's what it found:
> 
> >> First bad commit ef390067a72fe09977bb4ac8211313e1503302ea
> >> Merge: c7b3e90 0fd542f
> >> Author: Avi Kivity <address@hidden>
> >> Date:   Sun May 15 04:48:05 2011 -0400
> >>
> >>     Merge commit '0fd542fb7d13ddf12f897bb27c5950f31638b1df' into 
> >> upstream-merge
> 
> And after applying Avi's instructions here's the real bisect
> result:
> 
> ab431c283e7055bcd6fb622f212bb29e84a6a134 is the first bad commit
> commit ab431c283e7055bcd6fb622f212bb29e84a6a134
> Author: Isaku Yamahata <address@hidden>
> Date:   Fri Apr 1 20:43:23 2011 +0900
> 
>     piix_pci: optimize set irq path

Could you try with this commit reverted please?
Reverting patch below. Warning: compiled only.

commit 8f40db3918a0618a3beb9a771a569d20fe9c1bb3
Author: Michael S. Tsirkin <address@hidden>
Date:   Tue Dec 6 14:24:32 2011 +0200

    Revert "piix_pci: optimize set irq path"
    
    This reverts commit ab431c283e7055bcd6fb622f212bb29e84a6a134.
    
    Conflicts:
    
        hw/piix_pci.c

diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index ee11ff2..5b35c01 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -38,28 +38,12 @@
 
 typedef PCIHostState I440FXState;
 
-#define PIIX_NUM_PIC_IRQS       16      /* i8259 * 2 */
 #define PIIX_NUM_PIRQS          4ULL    /* PIRQ[A-D] */
 #define XEN_PIIX_NUM_PIRQS      128ULL
 #define PIIX_PIRQC              0x60
 
 typedef struct PIIX3State {
     PCIDevice dev;
-
-    /*
-     * bitmap to track pic levels.
-     * The pic level is the logical OR of all the PCI irqs mapped to it
-     * So one PIC level is tracked by PIIX_NUM_PIRQS bits.
-     *
-     * PIRQ is mapped to PIC pins, we track it by
-     * PIIX_NUM_PIRQS * PIIX_NUM_PIC_IRQS = 64 bits with
-     * pic_irq * PIIX_NUM_PIRQS + pirq
-     */
-#if PIIX_NUM_PIC_IRQS * PIIX_NUM_PIRQS > 64
-#error "unable to encode pic state in 64bit in pic_levels."
-#endif
-    uint64_t pic_levels;
-
     qemu_irq *pic;
 
     /* This member isn't used. Just for save/load compatibility */
@@ -365,61 +349,24 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int 
*piix3_devfn,
     return b;
 }
 
-/* PIIX3 PCI to ISA bridge */
-static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
-{
-    qemu_set_irq(piix3->pic[pic_irq],
-                 !!(piix3->pic_levels &
-                    (((1ULL << PIIX_NUM_PIRQS) - 1) <<
-                     (pic_irq * PIIX_NUM_PIRQS))));
-}
-
-static void piix3_set_irq_level(PIIX3State *piix3, int pirq, int level)
-{
-    int pic_irq;
-    uint64_t mask;
-
-    pic_irq = piix3->dev.config[PIIX_PIRQC + pirq];
-    if (pic_irq >= PIIX_NUM_PIC_IRQS) {
-        return;
-    }
-
-    mask = 1ULL << ((pic_irq * PIIX_NUM_PIRQS) + pirq);
-    piix3->pic_levels &= ~mask;
-    piix3->pic_levels |= mask * !!level;
-
-    piix3_set_irq_pic(piix3, pic_irq);
-}
-
-static void piix3_set_irq(void *opaque, int pirq, int level)
+static void piix3_set_irq(void *opaque, int irq_num, int level)
 {
+    int i, pic_irq, pic_level;
     PIIX3State *piix3 = opaque;
-    piix3_set_irq_level(piix3, pirq, level);
-}
-
-/* irq routing is changed. so rebuild bitmap */
-static void piix3_update_irq_levels(PIIX3State *piix3)
-{
-    int pirq;
-
-    piix3->pic_levels = 0;
-    for (pirq = 0; pirq < PIIX_NUM_PIRQS; pirq++) {
-        piix3_set_irq_level(piix3, pirq,
-                            pci_bus_get_irq_level(piix3->dev.bus, pirq));
-    }
-}
 
-static void piix3_write_config(PCIDevice *dev,
-                               uint32_t address, uint32_t val, int len)
-{
-    pci_default_write_config(dev, address, val, len);
-    if (ranges_overlap(address, len, PIIX_PIRQC, 4)) {
-        PIIX3State *piix3 = DO_UPCAST(PIIX3State, dev, dev);
-        int pic_irq;
-        piix3_update_irq_levels(piix3);
-        for (pic_irq = 0; pic_irq < PIIX_NUM_PIC_IRQS; pic_irq++) {
-            piix3_set_irq_pic(piix3, pic_irq);
+    /* now we change the pic irq level according to the piix irq mappings */
+    /* XXX: optimize */
+    pic_irq = piix3->dev.config[0x60 + irq_num];
+    if (pic_irq < 16) {
+        /* The pic level is the logical OR of all the PCI irqs mapped
+           to it */
+        pic_level = 0;
+        for (i = 0; i < 4; i++) {
+            if (pic_irq == piix3->dev.config[0x60 + i]) {
+                pic_level |= pci_bus_get_irq_level(piix3->dev.bus, i);
+            }
         }
+        qemu_set_irq(piix3->pic[pic_irq], pic_level);
     }
 }
 
@@ -434,7 +381,7 @@ static void piix3_write_config_xen(PCIDevice *dev,
                                uint32_t address, uint32_t val, int len)
 {
     xen_piix_pci_write_config_client(address, val, len);
-    piix3_write_config(dev, address, val, len);
+    pci_default_write_config(dev, address, val, len);
 }
 
 static void piix3_reset(void *opaque)
@@ -473,15 +420,6 @@ static void piix3_reset(void *opaque)
     pci_conf[0xab] = 0x00;
     pci_conf[0xac] = 0x00;
     pci_conf[0xae] = 0x00;
-
-    d->pic_levels = 0;
-}
-
-static int piix3_post_load(void *opaque, int version_id)
-{
-    PIIX3State *piix3 = opaque;
-    piix3_update_irq_levels(piix3);
-    return 0;
 }
 
 static void piix3_pre_save(void *opaque)
@@ -500,7 +438,6 @@ static const VMStateDescription vmstate_piix3 = {
     .version_id = 3,
     .minimum_version_id = 2,
     .minimum_version_id_old = 2,
-    .post_load = piix3_post_load,
     .pre_save = piix3_pre_save,
     .fields      = (VMStateField []) {
         VMSTATE_PCI_DEVICE(dev, PIIX3State),
@@ -541,7 +478,6 @@ static PCIDeviceInfo i440fx_info[] = {
         .qdev.no_user = 1,
         .no_hotplug   = 1,
         .init         = piix3_initfn,
-        .config_write = piix3_write_config,
         .vendor_id    = PCI_VENDOR_ID_INTEL,
         .device_id    = PCI_DEVICE_ID_INTEL_82371SB_0, // 82371SB PIIX3 
PCI-to-ISA bridge (Step A1)
         .class_id     = PCI_CLASS_BRIDGE_ISA,