[Qemu-devel] [PATCH 0/6] pcie: fixes for pci branch

[Qemu-devel] [PATCH 0/6] pcie: fixes for pci branch

[Isaku Yamahata]

This patch series fixes isseus which introduced by
5704608988972359473ce609f9e4884808115b58
cd870502e228f8f028f93fb585ae3b208fcb46c7
4b78560503a802eae3663ace9f9cf080319e7265

Isaku Yamahata (6):
  pcie: fix pcie_cap_slot_write_config().
  pcie/slot: fix hotplug_event_notify().
  pcie/slot: clean up of hotplug_event_notify().
  pcie/slot: factor out the logic to update hotplug event status.
  pcie/slot: fix save/load
  msix: remove range checks.

 hw/ioh3420.c            |    1 +
 hw/msix.c               |    5 -----
 hw/pcie.c               |   46 ++++++++++++++++++++++++++++++----------------
 hw/pcie.h               |    1 +
 hw/xio3130_downstream.c |    1 +
 5 files changed, 33 insertions(+), 21 deletions(-)

[Qemu-devel] [PATCH 1/6] pcie: fix pcie_cap_slot_write_config().

[Isaku Yamahata]

This patch fixes pcie_cap_slot_write_config() which is broken by
5704608988972359473ce609f9e4884808115b58
The command completion event should generated only when the command
is issued (and the command is completed).

>From 6.7.3.2
> Software issues a command to a hot-plug capable Downstream Port by
> issuing a write transaction that targets any portion of the
> Port's Slot Control register. A single write to the Slot Control
> register is considered to be a single command
...
> The Port must process the command normally even if the status field is
> already set when the command is issued.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pcie.c |   25 ++++++++++++++-----------
 1 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/hw/pcie.c b/hw/pcie.c
index 0385f53..64cc003 100644
--- a/hw/pcie.c
+++ b/hw/pcie.c
@@ -19,6 +19,7 @@
  */
 
 #include "sysemu.h"
+#include "range.h"
 #include "pci_bridge.h"
 #include "pcie.h"
 #include "msix.h"
@@ -323,17 +324,19 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
 
     hotplug_event_notify(dev);
 
-    /* command completion.
-     * Real hardware might take a while to complete
-     * requested command because physical movement would be involved
-     * like locking the electromechanical lock.
-     * However in our case, command is completed instantaneously above,
-     * so send a command completion event right now.
-     *
-     * 6.7.3.2 Command Completed Events
-     */
-    /* set command completed bit */
-    pcie_cap_slot_event(dev, PCI_EXP_HP_EV_CCI);
+    if (ranges_overlap(addr, len, pos + PCI_EXP_SLTCTL, 2)) {
+        /* command completion.
+         * Real hardware might take a while to complete
+         * requested command because physical movement would be involved
+         * like locking the electromechanical lock.
+         * However in our case, command is completed instantaneously above,
+         * so send a command completion event right now.
+         *
+         * 6.7.3.2 Command Completed Events
+         */
+        /* set command completed bit */
+        pcie_cap_slot_event(dev, PCI_EXP_HP_EV_CCI);
+    }
 }
 
 void pcie_cap_slot_push_attention_button(PCIDevice *dev)
-- 
1.7.1.1

[Qemu-devel] [PATCH 2/6] pcie/slot: fix hotplug_event_notify().

[Isaku Yamahata]

This patch fixes the miscondition to trigger slot event
which is introduced by cd870502e228f8f028f93fb585ae3b208fcb46c7.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pcie.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/pcie.c b/hw/pcie.c
index 64cc003..3c52849 100644
--- a/hw/pcie.c
+++ b/hw/pcie.c
@@ -148,7 +148,7 @@ static void hotplug_event_notify(PCIDevice *dev)
     uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA);
 
     dev->exp.hpev_notified = (sltctl & PCI_EXP_SLTCTL_HPIE) &&
-        (sltsta & PCI_EXP_HP_EV_SUPPORTED);
+        (sltsta & sltctl & PCI_EXP_HP_EV_SUPPORTED);
 
     if (prev == dev->exp.hpev_notified) {
         return;
-- 
1.7.1.1

[Qemu-devel] [PATCH 3/6] pcie/slot: clean up of hotplug_event_notify().

[Isaku Yamahata]

remove redundant return and blank line from hotplug_event_notify().

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pcie.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/hw/pcie.c b/hw/pcie.c
index 3c52849..37d7457 100644
--- a/hw/pcie.c
+++ b/hw/pcie.c
@@ -164,9 +164,7 @@ static void hotplug_event_notify(PCIDevice *dev)
         qemu_set_irq(dev->irq[dev->exp.hpev_intx], dev->exp.hpev_notified);
     } else if (dev->exp.hpev_notified) {
         pci_msi_notify(dev, pcie_cap_flags_get_vector(dev));
-        return;
     }
-
 }
 
 /*
-- 
1.7.1.1

[Qemu-devel] [PATCH 4/6] pcie/slot: factor out the logic to update hotplug event status.

[Isaku Yamahata]

This patch factors out the hot plug event logic.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pcie.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/pcie.c b/hw/pcie.c
index 37d7457..97d17fa 100644
--- a/hw/pcie.c
+++ b/hw/pcie.c
@@ -139,9 +139,8 @@ void pcie_cap_deverr_reset(PCIDevice *dev)
                                  PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE);
 }
 
-static void hotplug_event_notify(PCIDevice *dev)
+static void hotplug_event_update_event_status(PCIDevice *dev)
 {
-    bool prev = dev->exp.hpev_notified;
     uint32_t pos = dev->exp.exp_cap;
     uint8_t *exp_cap = dev->config + pos;
     uint16_t sltctl = pci_get_word(exp_cap + PCI_EXP_SLTCTL);
@@ -149,6 +148,13 @@ static void hotplug_event_notify(PCIDevice *dev)
 
     dev->exp.hpev_notified = (sltctl & PCI_EXP_SLTCTL_HPIE) &&
         (sltsta & sltctl & PCI_EXP_HP_EV_SUPPORTED);
+}
+
+static void hotplug_event_notify(PCIDevice *dev)
+{
+    bool prev = dev->exp.hpev_notified;
+
+    hotplug_event_update_event_status(dev);
 
     if (prev == dev->exp.hpev_notified) {
         return;
-- 
1.7.1.1

[Qemu-devel] [PATCH 5/6] pcie/slot: fix save/load

[Isaku Yamahata]

This patch fixes save/load of pcie slot which was broken by
cd870502e228f8f028f93fb585ae3b208fcb46c7.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/ioh3420.c            |    1 +
 hw/pcie.c               |    7 +++++++
 hw/pcie.h               |    1 +
 hw/xio3130_downstream.c |    1 +
 4 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/hw/ioh3420.c b/hw/ioh3420.c
index 23aecbf..3cc129f 100644
--- a/hw/ioh3420.c
+++ b/hw/ioh3420.c
@@ -139,6 +139,7 @@ static const VMStateDescription vmstate_ioh3420 = {
     .version_id = 1,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
+    .post_load = pcie_cap_slot_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_PCIE_DEVICE(port.br.dev, PCIESlot),
         /* TODO: AER */
diff --git a/hw/pcie.c b/hw/pcie.c
index 97d17fa..23e35dc 100644
--- a/hw/pcie.c
+++ b/hw/pcie.c
@@ -343,6 +343,13 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
     }
 }
 
+int pcie_cap_slot_post_load(void *opaque, int version_id)
+{
+    PCIDevice *dev = opaque;
+    hotplug_event_update_event_status(dev);
+    return 0;
+}
+
 void pcie_cap_slot_push_attention_button(PCIDevice *dev)
 {
     pcie_cap_slot_event(dev, PCI_EXP_HP_EV_ABP);
diff --git a/hw/pcie.h b/hw/pcie.h
index 39c6e47..8708504 100644
--- a/hw/pcie.h
+++ b/hw/pcie.h
@@ -95,6 +95,7 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot);
 void pcie_cap_slot_reset(PCIDevice *dev);
 void pcie_cap_slot_write_config(PCIDevice *dev,
                                 uint32_t addr, uint32_t val, int len);
+int pcie_cap_slot_post_load(void *opaque, int version_id);
 void pcie_cap_slot_push_attention_button(PCIDevice *dev);
 
 void pcie_cap_root_init(PCIDevice *dev);
diff --git a/hw/xio3130_downstream.c b/hw/xio3130_downstream.c
index d46f911..854eba8 100644
--- a/hw/xio3130_downstream.c
+++ b/hw/xio3130_downstream.c
@@ -141,6 +141,7 @@ static const VMStateDescription vmstate_xio3130_downstream = {
     .version_id = 1,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
+    .post_load = pcie_cap_slot_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_PCIE_DEVICE(port.br.dev, PCIESlot),
         /* TODO: AER */
-- 
1.7.1.1

[Qemu-devel] [PATCH 6/6] msix: remove range checks.

[Isaku Yamahata]

Same as 4b78560503a802eae3663ace9f9cf080319e7265
> config write handlers should be idempotent.
> So no need for range checks.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/msix.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index f66d255..20caa5e 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -157,14 +157,9 @@ static void msix_handle_mask_update(PCIDevice *dev, int vector)
 void msix_write_config(PCIDevice *dev, uint32_t addr,
                        uint32_t val, int len)
 {
-    unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
     int vector;
     int i;
 
-    if (!range_covers_byte(addr, len, enable_pos)) {
-        return;
-    }
-
     if (!msix_enabled(dev)) {
         return;
     }
-- 
1.7.1.1

[Qemu-devel] Re: [PATCH 6/6] msix: remove range checks.

[Michael S. Tsirkin]

On Wed, Oct 27, 2010 at 07:21:21PM +0900, Isaku Yamahata wrote:
> Same as 4b78560503a802eae3663ace9f9cf080319e7265
> > config write handlers should be idempotent.
> > So no need for range checks.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Well I changed my mind on 4b78560503a802eae3663ace9f9cf080319e7265 :) As
long as the check is simple, it does not hurt.

> ---
>  hw/msix.c |    5 -----
>  1 files changed, 0 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index f66d255..20caa5e 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -157,14 +157,9 @@ static void msix_handle_mask_update(PCIDevice *dev, int vector)
>  void msix_write_config(PCIDevice *dev, uint32_t addr,
>                         uint32_t val, int len)
>  {
> -    unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
>      int vector;
>      int i;
>  
> -    if (!range_covers_byte(addr, len, enable_pos)) {
> -        return;
> -    }
> -
>      if (!msix_enabled(dev)) {
>          return;
>      }
> -- 
> 1.7.1.1

[Qemu-devel] Re: [PATCH 0/6] pcie: fixes for pci branch

[Michael S. Tsirkin]

On Wed, Oct 27, 2010 at 07:21:15PM +0900, Isaku Yamahata wrote:
> This patch series fixes isseus which introduced by
> 5704608988972359473ce609f9e4884808115b58
> cd870502e228f8f028f93fb585ae3b208fcb46c7
> 4b78560503a802eae3663ace9f9cf080319e7265

OK, I applied this, except I basically backed out range check changes.
Pls take a look at the branch, and test if possible -
if OK with you I think we can merge this part.

> Isaku Yamahata (6):
>   pcie: fix pcie_cap_slot_write_config().
>   pcie/slot: fix hotplug_event_notify().
>   pcie/slot: clean up of hotplug_event_notify().
>   pcie/slot: factor out the logic to update hotplug event status.
>   pcie/slot: fix save/load
>   msix: remove range checks.
> 
>  hw/ioh3420.c            |    1 +
>  hw/msix.c               |    5 -----
>  hw/pcie.c               |   46 ++++++++++++++++++++++++++++++----------------
>  hw/pcie.h               |    1 +
>  hw/xio3130_downstream.c |    1 +
>  5 files changed, 33 insertions(+), 21 deletions(-)

[Qemu-devel] Re: [PATCH 0/6] pcie: fixes for pci branch

[Isaku Yamahata]

On Wed, Oct 27, 2010 at 06:40:48PM +0200, Michael S. Tsirkin wrote:
> On Wed, Oct 27, 2010 at 07:21:15PM +0900, Isaku Yamahata wrote:
> > This patch series fixes isseus which introduced by
> > 5704608988972359473ce609f9e4884808115b58
> > cd870502e228f8f028f93fb585ae3b208fcb46c7
> > 4b78560503a802eae3663ace9f9cf080319e7265
> 
> OK, I applied this, except I basically backed out range check changes.
> Pls take a look at the branch, and test if possible -
> if OK with you I think we can merge this part.

I took a look at it. It looks good.
-- 
yamahata