[PATCH 0/4] pcie: cleanup code and add trace point

[PATCH 0/4] pcie: cleanup code and add trace point

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here is tiny code cleanup + on trace point to track power indicator
changes (which may help to analyze
"Hot-unplug failed: guest is busy (power indicator blinking)" error
message).

Vladimir Sementsov-Ogievskiy (4):
  pcie: pcie_cap_slot_write_config(): use correct macro
  pcie_regs: drop duplicated indicator value macros
  pcie: drop unused PCIExpressIndicator
  pcie: add trace-point for power indicator transitions

 include/hw/pci/pcie.h      |  8 --------
 include/hw/pci/pcie_regs.h | 14 --------------
 hw/pci/pcie.c              | 33 +++++++++++++++++++++++++++------
 hw/pci/trace-events        |  3 +++
 4 files changed, 30 insertions(+), 28 deletions(-)

-- 
2.34.1

[PATCH 3/4] pcie: drop unused PCIExpressIndicator

[Vladimir Sementsov-Ogievskiy]

The structure type is unused. Also, it's the only user of corresponding
macros, so drop them too.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 include/hw/pci/pcie.h      | 8 --------
 include/hw/pci/pcie_regs.h | 5 -----
 2 files changed, 13 deletions(-)

diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 798a262a0a..3cc2b15957 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -27,14 +27,6 @@
 #include "hw/pci/pcie_sriov.h"
 #include "hw/hotplug.h"
 
-typedef enum {
-    /* for attention and power indicator */
-    PCI_EXP_HP_IND_RESERVED     = PCI_EXP_SLTCTL_IND_RESERVED,
-    PCI_EXP_HP_IND_ON           = PCI_EXP_SLTCTL_IND_ON,
-    PCI_EXP_HP_IND_BLINK        = PCI_EXP_SLTCTL_IND_BLINK,
-    PCI_EXP_HP_IND_OFF          = PCI_EXP_SLTCTL_IND_OFF,
-} PCIExpressIndicator;
-
 typedef enum {
     /* these bits must match the bits in Slot Control/Status registers.
      * PCI_EXP_HP_EV_xxx = PCI_EXP_SLTCTL_xxxE = PCI_EXP_SLTSTA_xxx
diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
index 00b595a82e..1fe0bdd25b 100644
--- a/include/hw/pci/pcie_regs.h
+++ b/include/hw/pci/pcie_regs.h
@@ -66,11 +66,6 @@ typedef enum PCIExpLinkWidth {
 
 #define PCI_EXP_SLTCAP_PSN_SHIFT        ctz32(PCI_EXP_SLTCAP_PSN)
 
-#define PCI_EXP_SLTCTL_IND_RESERVED     0x0
-#define PCI_EXP_SLTCTL_IND_ON           0x1
-#define PCI_EXP_SLTCTL_IND_BLINK        0x2
-#define PCI_EXP_SLTCTL_IND_OFF          0x3
-
 #define PCI_EXP_SLTCTL_SUPPORTED        \
             (PCI_EXP_SLTCTL_ABPE |      \
              PCI_EXP_SLTCTL_PDCE |      \
-- 
2.34.1

Re: [PATCH 3/4] pcie: drop unused PCIExpressIndicator

[Philippe Mathieu-Daudé]

On 4/2/23 18:47, Vladimir Sementsov-Ogievskiy wrote:
> The structure type is unused. Also, it's the only user of corresponding
> macros, so drop them too.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>   include/hw/pci/pcie.h      | 8 --------
>   include/hw/pci/pcie_regs.h | 5 -----
>   2 files changed, 13 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 4/4] pcie: add trace-poing for power indicator transitions

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/pci/pcie.c       | 20 ++++++++++++++++++++
 hw/pci/trace-events |  3 +++
 2 files changed, 23 insertions(+)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index ccdb2377e1..1a19368994 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -28,6 +28,7 @@
 #include "hw/pci/pcie_regs.h"
 #include "hw/pci/pcie_port.h"
 #include "qemu/range.h"
+#include "trace.h"
 
 //#define DEBUG_PCIE
 #ifdef DEBUG_PCIE
@@ -718,6 +719,20 @@ void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slt_ctl, uint16_t *slt_sta)
     *slt_sta = pci_get_word(exp_cap + PCI_EXP_SLTSTA);
 }
 
+static const char *pcie_sltctl_pic_str(uint16_t sltctl)
+{
+    switch (sltctl & PCI_EXP_SLTCTL_PIC) {
+    case PCI_EXP_SLTCTL_PWR_IND_ON:
+        return "on";
+    case PCI_EXP_SLTCTL_PWR_IND_BLINK:
+        return "blink";
+    case PCI_EXP_SLTCTL_PWR_IND_OFF:
+        return "off";
+    default:
+        return "?";
+    }
+}
+
 void pcie_cap_slot_write_config(PCIDevice *dev,
                                 uint16_t old_slt_ctl, uint16_t old_slt_sta,
                                 uint32_t addr, uint32_t val, int len)
@@ -762,6 +777,11 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
                         sltsta);
     }
 
+    if ((val & PCI_EXP_SLTCTL_PIC) != (old_slt_ctl & PCI_EXP_SLTCTL_PIC)) {
+        trace_pcie_power_indicator(pcie_sltctl_pic_str(old_slt_ctl),
+                                   pcie_sltctl_pic_str(val));
+    }
+
     /*
      * If the slot is populated, power indicator is off and power
      * controller is off, it is safe to detach the devices.
diff --git a/hw/pci/trace-events b/hw/pci/trace-events
index aaf46bc92d..ec4a5ff43d 100644
--- a/hw/pci/trace-events
+++ b/hw/pci/trace-events
@@ -15,3 +15,6 @@ msix_write_config(char *name, bool enabled, bool masked) "dev %s enabled %d mask
 sriov_register_vfs(const char *name, int slot, int function, int num_vfs) "%s %02x:%x: creating %d vf devs"
 sriov_unregister_vfs(const char *name, int slot, int function, int num_vfs) "%s %02x:%x: Unregistering %d vf devs"
 sriov_config_write(const char *name, int slot, int fun, uint32_t offset, uint32_t val, uint32_t len) "%s %02x:%x: sriov offset 0x%x val 0x%x len %d"
+
+# pcie.c
+pcie_power_indicator(const char *old, const char *new) "%s -> %s"
-- 
2.34.1

Re: [PATCH 2/4] pcie_regs: drop duplicated indicator value macros

[Philippe Mathieu-Daudé]

On 4/2/23 18:47, Vladimir Sementsov-Ogievskiy wrote:
> We already have indicator values in
> include/standard-headers/linux/pci_regs.h , no reason to reinvent them
> in include/hw/pci/pcie_regs.h. (and we already have usage of
> PCI_EXP_SLTCTL_PWR_IND_BLINK and PCI_EXP_SLTCTL_PWR_IND_OFF in
> hw/pci/pcie.c, so let's be consistent)
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>   include/hw/pci/pcie_regs.h |  9 ---------
>   hw/pci/pcie.c              | 13 +++++++------
>   2 files changed, 7 insertions(+), 15 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 1/4] pcie: pcie_cap_slot_write_config(): use correct macro

[Philippe Mathieu-Daudé]

On 4/2/23 18:47, Vladimir Sementsov-Ogievskiy wrote:
> PCI_EXP_SLTCTL_PIC_OFF is a value, and PCI_EXP_SLTCTL_PIC is a mask.
> Happily PCI_EXP_SLTCTL_PIC_OFF is a maximum value for this mask and is
> equal to the mask itself. Still the code looks like a bug. Let's make
> it more reader-friendly.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>   hw/pci/pcie.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 4/4] pcie: add trace-point for power indicator transitions

[Philippe Mathieu-Daudé]

On 4/2/23 18:47, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>   hw/pci/pcie.c       | 20 ++++++++++++++++++++
>   hw/pci/trace-events |  3 +++
>   2 files changed, 23 insertions(+)

> +static const char *pcie_sltctl_pic_str(uint16_t sltctl)
> +{
> +    switch (sltctl & PCI_EXP_SLTCTL_PIC) {
> +    case PCI_EXP_SLTCTL_PWR_IND_ON:
> +        return "on";
> +    case PCI_EXP_SLTCTL_PWR_IND_BLINK:
> +        return "blink";
> +    case PCI_EXP_SLTCTL_PWR_IND_OFF:
> +        return "off";
> +    default:
> +        return "?";

Maybe "illegal"?

Otherwise:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +    }
> +}

Re: [PATCH 4/4] pcie: add trace-point for power indicator transitions

[Vladimir Sementsov-Ogievskiy]

Thanks for reviewing!

On 05.02.23 13:56, Philippe Mathieu-Daudé wrote:
> On 4/2/23 18:47, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   hw/pci/pcie.c       | 20 ++++++++++++++++++++
>>   hw/pci/trace-events |  3 +++
>>   2 files changed, 23 insertions(+)
> 
>> +static const char *pcie_sltctl_pic_str(uint16_t sltctl)
>> +{
>> +    switch (sltctl & PCI_EXP_SLTCTL_PIC) {
>> +    case PCI_EXP_SLTCTL_PWR_IND_ON:
>> +        return "on";
>> +    case PCI_EXP_SLTCTL_PWR_IND_BLINK:
>> +        return "blink";
>> +    case PCI_EXP_SLTCTL_PWR_IND_OFF:
>> +        return "off";
>> +    default:
>> +        return "?";
> 
> Maybe "illegal"?

I just was unsure about it.

For SHPC, 0 is correct, and means that this command don't change the led state.

But with PCI-e hotplug we don't have such commands but change the led directly, so it must be one of "on"/"blink"/"off", and zero is really wrong, right?


Also, I'm now looking at /* TODO: send event to monitor */ in shpc code, and working on it. So, I think, I'll soon send patches with such event for both SHPC and PCI-e, and probably that trace point becomes not needed.

> 
> Otherwise:
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
>> +    }
>> +}
> 

-- 
Best regards,
Vladimir

Re: [PATCH 4/4] pcie: add trace-point for power indicator transitions

[Michael S. Tsirkin]

On Tue, Feb 07, 2023 at 01:39:03PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Thanks for reviewing!
> 
> On 05.02.23 13:56, Philippe Mathieu-Daudé wrote:
> > On 4/2/23 18:47, Vladimir Sementsov-Ogievskiy wrote:
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > > ---
> > >   hw/pci/pcie.c       | 20 ++++++++++++++++++++
> > >   hw/pci/trace-events |  3 +++
> > >   2 files changed, 23 insertions(+)
> > 
> > > +static const char *pcie_sltctl_pic_str(uint16_t sltctl)
> > > +{
> > > +    switch (sltctl & PCI_EXP_SLTCTL_PIC) {
> > > +    case PCI_EXP_SLTCTL_PWR_IND_ON:
> > > +        return "on";
> > > +    case PCI_EXP_SLTCTL_PWR_IND_BLINK:
> > > +        return "blink";
> > > +    case PCI_EXP_SLTCTL_PWR_IND_OFF:
> > > +        return "off";
> > > +    default:
> > > +        return "?";
> > 
> > Maybe "illegal"?
> 
> I just was unsure about it.
> 
> For SHPC, 0 is correct, and means that this command don't change the led state.
> 
> But with PCI-e hotplug we don't have such commands but change the led directly, so it must be one of "on"/"blink"/"off", and zero is really wrong, right?
> 
> 
> Also, I'm now looking at /* TODO: send event to monitor */ in shpc code, and working on it. So, I think, I'll soon send patches with such event for both SHPC and PCI-e, and probably that trace point becomes not needed.


I think it's ok to queue as is, change it with a patch on top.

> > 
> > Otherwise:
> > 
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > 
> > > +    }
> > > +}
> > 
> 
> -- 
> Best regards,
> Vladimir

[PATCH 0/4] pcie: cleanup code and add trace point

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here is tiny code cleanup + on trace point to track power indicator
changes (which may help to analyze
"Hot-unplug failed: guest is busy (power indicator blinking)" error
message).

Vladimir Sementsov-Ogievskiy (4):
  pcie: pcie_cap_slot_write_config(): use correct macro
  pcie_regs: drop duplicated indicator value macros
  pcie: drop unused PCIExpressIndicator
  pcie: add trace-point for power indicator transitions

 include/hw/pci/pcie.h      |  8 --------
 include/hw/pci/pcie_regs.h | 14 --------------
 hw/pci/pcie.c              | 33 +++++++++++++++++++++++++++------
 hw/pci/trace-events        |  3 +++
 4 files changed, 30 insertions(+), 28 deletions(-)

-- 
2.34.1

[PATCH 2/4] pcie_regs: drop duplicated indicator value macros

[Vladimir Sementsov-Ogievskiy]

We already have indicator values in
include/standard-headers/linux/pci_regs.h , no reason to reinvent them
in include/hw/pci/pcie_regs.h. (and we already have usage of
PCI_EXP_SLTCTL_PWR_IND_BLINK and PCI_EXP_SLTCTL_PWR_IND_OFF in
hw/pci/pcie.c, so let's be consistent)

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 include/hw/pci/pcie_regs.h |  9 ---------
 hw/pci/pcie.c              | 13 +++++++------
 2 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
index 963dc2e170..00b595a82e 100644
--- a/include/hw/pci/pcie_regs.h
+++ b/include/hw/pci/pcie_regs.h
@@ -70,15 +70,6 @@ typedef enum PCIExpLinkWidth {
 #define PCI_EXP_SLTCTL_IND_ON           0x1
 #define PCI_EXP_SLTCTL_IND_BLINK        0x2
 #define PCI_EXP_SLTCTL_IND_OFF          0x3
-#define PCI_EXP_SLTCTL_AIC_SHIFT        ctz32(PCI_EXP_SLTCTL_AIC)
-#define PCI_EXP_SLTCTL_AIC_OFF                          \
-    (PCI_EXP_SLTCTL_IND_OFF << PCI_EXP_SLTCTL_AIC_SHIFT)
-
-#define PCI_EXP_SLTCTL_PIC_SHIFT        ctz32(PCI_EXP_SLTCTL_PIC)
-#define PCI_EXP_SLTCTL_PIC_OFF                          \
-    (PCI_EXP_SLTCTL_IND_OFF << PCI_EXP_SLTCTL_PIC_SHIFT)
-#define PCI_EXP_SLTCTL_PIC_ON                          \
-    (PCI_EXP_SLTCTL_IND_ON << PCI_EXP_SLTCTL_PIC_SHIFT)
 
 #define PCI_EXP_SLTCTL_SUPPORTED        \
             (PCI_EXP_SLTCTL_ABPE |      \
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 82ef723983..ccdb2377e1 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -634,8 +634,8 @@ void pcie_cap_slot_init(PCIDevice *dev, PCIESlot *s)
                                  PCI_EXP_SLTCTL_PIC |
                                  PCI_EXP_SLTCTL_AIC);
     pci_word_test_and_set_mask(dev->config + pos + PCI_EXP_SLTCTL,
-                               PCI_EXP_SLTCTL_PIC_OFF |
-                               PCI_EXP_SLTCTL_AIC_OFF);
+                               PCI_EXP_SLTCTL_PWR_IND_OFF |
+                               PCI_EXP_SLTCTL_ATTN_IND_OFF);
     pci_word_test_and_set_mask(dev->wmask + pos + PCI_EXP_SLTCTL,
                                PCI_EXP_SLTCTL_PIC |
                                PCI_EXP_SLTCTL_AIC |
@@ -679,7 +679,7 @@ void pcie_cap_slot_reset(PCIDevice *dev)
                                  PCI_EXP_SLTCTL_PDCE |
                                  PCI_EXP_SLTCTL_ABPE);
     pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL,
-                               PCI_EXP_SLTCTL_AIC_OFF);
+                               PCI_EXP_SLTCTL_ATTN_IND_OFF);
 
     if (dev->cap_present & QEMU_PCIE_SLTCAP_PCP) {
         /* Downstream ports enforce device number 0. */
@@ -694,7 +694,8 @@ void pcie_cap_slot_reset(PCIDevice *dev)
                                        PCI_EXP_SLTCTL_PCC);
         }
 
-        pic = populated ? PCI_EXP_SLTCTL_PIC_ON : PCI_EXP_SLTCTL_PIC_OFF;
+        pic = populated ?
+                PCI_EXP_SLTCTL_PWR_IND_ON : PCI_EXP_SLTCTL_PWR_IND_OFF;
         pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL, pic);
     }
 
@@ -770,9 +771,9 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
      * control of powered off slots before powering them on.
      */
     if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) &&
-        (val & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PIC_OFF &&
+        (val & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF &&
         (!(old_slt_ctl & PCI_EXP_SLTCTL_PCC) ||
-        (old_slt_ctl & PCI_EXP_SLTCTL_PIC) != PCI_EXP_SLTCTL_PIC_OFF)) {
+        (old_slt_ctl & PCI_EXP_SLTCTL_PIC) != PCI_EXP_SLTCTL_PWR_IND_OFF)) {
         pcie_cap_slot_do_unplug(dev);
     }
     pcie_cap_update_power(dev);
-- 
2.34.1