[Qemu-devel] [PATCH 1/2] pcie: Use same Link Capabilities defines as Linux kernel

[Qemu-devel] [PATCH 1/2] pcie: Use same Link Capabilities defines as Linux kernel

[Bjorn Helgaas]

Linux actually doesn't have definitions for the individual L0s and L1 bits
in Link Capabilities, but if/when it does, it will use these names, which
follow the Link Control naming pattern.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 hw/pci/pcie.c              |    2 +-
 include/hw/pci/pcie_regs.h |    3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 62bd0b8..54fcac8 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -73,7 +73,7 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port)
 
     pci_set_long(exp_cap + PCI_EXP_LNKCAP,
                  (port << PCI_EXP_LNKCAP_PN_SHIFT) |
-                 PCI_EXP_LNKCAP_ASPMS_0S |
+                 PCI_EXP_LNKCAP_ASPM_L0S |
                  PCI_EXP_LNK_MLW_1 |
                  PCI_EXP_LNK_LS_25);
 
diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
index 4d123d9..109f2f4 100644
--- a/include/hw/pci/pcie_regs.h
+++ b/include/hw/pci/pcie_regs.h
@@ -39,8 +39,7 @@
 #define PCI_EXP_LNK_MLW_1               (1 << PCI_EXP_LNK_MLW_SHIFT)
 
 /* PCI_EXP_LINKCAP */
-#define PCI_EXP_LNKCAP_ASPMS_SHIFT      (ffs(PCI_EXP_LNKCAP_ASPMS) - 1)
-#define PCI_EXP_LNKCAP_ASPMS_0S         (1 << PCI_EXP_LNKCAP_ASPMS_SHIFT)
+#define PCI_EXP_LNKCAP_ASPM_L0S		0x00000400 /* L0s supported */
 
 #define PCI_EXP_LNKCAP_PN_SHIFT         (ffs(PCI_EXP_LNKCAP_PN) - 1)
 

[Qemu-devel] [PATCH 2/2] pcie: Add more ASPM support

[Bjorn Helgaas]

Indicate ASPM L0s and L1 support in Link Capabilities and make the ASPM
bits in Link Control writable.  These Link Control bits don't do anything
in qemu, but having them writable means the BIOS or OS can write them as
on real hardware.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 hw/pci/pcie.c              |    4 +++-
 include/hw/pci/pcie_regs.h |    5 ++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 54fcac8..f194445 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -73,13 +73,15 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port)
 
     pci_set_long(exp_cap + PCI_EXP_LNKCAP,
                  (port << PCI_EXP_LNKCAP_PN_SHIFT) |
-                 PCI_EXP_LNKCAP_ASPM_L0S |
+                 PCI_EXP_LNKCAP_ASPM_L0S | PCI_EXP_LNKCAP_ASPM_L1 |
                  PCI_EXP_LNK_MLW_1 |
                  PCI_EXP_LNK_LS_25);
 
     pci_set_word(exp_cap + PCI_EXP_LNKSTA,
                  PCI_EXP_LNK_MLW_1 | PCI_EXP_LNK_LS_25);
 
+    pci_set_word(dev->wmask + pos + PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_ASPMC);
+
     pci_set_long(exp_cap + PCI_EXP_DEVCAP2,
                  PCI_EXP_DEVCAP2_EFF | PCI_EXP_DEVCAP2_EETLPP);
 
diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
index 109f2f4..5b81b36 100644
--- a/include/hw/pci/pcie_regs.h
+++ b/include/hw/pci/pcie_regs.h
@@ -31,7 +31,7 @@
 #define PCI_EXP_FLAGS_TYPE_SHIFT        (ffs(PCI_EXP_FLAGS_TYPE) - 1)
 
 
-/* PCI_EXP_LINK{CAP, STA} */
+/* PCI_EXP_LINK{CAP, STA, CTL} */
 /* link speed */
 #define PCI_EXP_LNK_LS_25               1
 
@@ -40,9 +40,12 @@
 
 /* PCI_EXP_LINKCAP */
 #define PCI_EXP_LNKCAP_ASPM_L0S		0x00000400 /* L0s supported */
+#define PCI_EXP_LNKCAP_ASPM_L1		0x00000800 /* L1 supported */
 
 #define PCI_EXP_LNKCAP_PN_SHIFT         (ffs(PCI_EXP_LNKCAP_PN) - 1)
 
+#define PCI_EXP_LNKCTL_ASPMC		0x0003	/* ASPM Control */
+
 #define PCI_EXP_SLTCAP_PSN_SHIFT        (ffs(PCI_EXP_SLTCAP_PSN) - 1)
 
 #define PCI_EXP_SLTCTL_IND_RESERVED     0x0

Re: [Qemu-devel] [PATCH 2/2] pcie: Add more ASPM support

[Michael S. Tsirkin]

On Mon, May 20, 2013 at 11:09:56AM -0600, Bjorn Helgaas wrote:
> Indicate ASPM L0s and L1 support in Link Capabilities and make the ASPM
> bits in Link Control writable.  These Link Control bits don't do anything
> in qemu, but having them writable means the BIOS or OS can write them as
> on real hardware.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  hw/pci/pcie.c              |    4 +++-
>  include/hw/pci/pcie_regs.h |    5 ++++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 54fcac8..f194445 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -73,13 +73,15 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port)
>  
>      pci_set_long(exp_cap + PCI_EXP_LNKCAP,
>                   (port << PCI_EXP_LNKCAP_PN_SHIFT) |
> -                 PCI_EXP_LNKCAP_ASPM_L0S |
> +                 PCI_EXP_LNKCAP_ASPM_L0S | PCI_EXP_LNKCAP_ASPM_L1 |
>                   PCI_EXP_LNK_MLW_1 |
>                   PCI_EXP_LNK_LS_25);
>  
>      pci_set_word(exp_cap + PCI_EXP_LNKSTA,
>                   PCI_EXP_LNK_MLW_1 | PCI_EXP_LNK_LS_25);
>  
> +    pci_set_word(dev->wmask + pos + PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_ASPMC);
> +
>      pci_set_long(exp_cap + PCI_EXP_DEVCAP2,
>                   PCI_EXP_DEVCAP2_EFF | PCI_EXP_DEVCAP2_EETLPP);
>

OK this is making some new bits writeable so it will break
cross-version migration. Need to add a property and
disable for -M 1.5 or older.
  
> diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
> index 109f2f4..5b81b36 100644
> --- a/include/hw/pci/pcie_regs.h
> +++ b/include/hw/pci/pcie_regs.h
> @@ -31,7 +31,7 @@
>  #define PCI_EXP_FLAGS_TYPE_SHIFT        (ffs(PCI_EXP_FLAGS_TYPE) - 1)
>  
>  
> -/* PCI_EXP_LINK{CAP, STA} */
> +/* PCI_EXP_LINK{CAP, STA, CTL} */
>  /* link speed */
>  #define PCI_EXP_LNK_LS_25               1
>  
> @@ -40,9 +40,12 @@
>  
>  /* PCI_EXP_LINKCAP */
>  #define PCI_EXP_LNKCAP_ASPM_L0S		0x00000400 /* L0s supported */
> +#define PCI_EXP_LNKCAP_ASPM_L1		0x00000800 /* L1 supported */
>  
>  #define PCI_EXP_LNKCAP_PN_SHIFT         (ffs(PCI_EXP_LNKCAP_PN) - 1)
>  
> +#define PCI_EXP_LNKCTL_ASPMC		0x0003	/* ASPM Control */
> +
>  #define PCI_EXP_SLTCAP_PSN_SHIFT        (ffs(PCI_EXP_SLTCAP_PSN) - 1)
>  
>  #define PCI_EXP_SLTCTL_IND_RESERVED     0x0

Re: [Qemu-devel] [PATCH 2/2] pcie: Add more ASPM support

[Bjorn Helgaas]

On Mon, May 20, 2013 at 12:32 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, May 20, 2013 at 11:09:56AM -0600, Bjorn Helgaas wrote:
>> Indicate ASPM L0s and L1 support in Link Capabilities and make the ASPM
>> bits in Link Control writable.  These Link Control bits don't do anything
>> in qemu, but having them writable means the BIOS or OS can write them as
>> on real hardware.
>>
>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> ---
>>  hw/pci/pcie.c              |    4 +++-
>>  include/hw/pci/pcie_regs.h |    5 ++++-
>>  2 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>> index 54fcac8..f194445 100644
>> --- a/hw/pci/pcie.c
>> +++ b/hw/pci/pcie.c
>> @@ -73,13 +73,15 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port)
>>
>>      pci_set_long(exp_cap + PCI_EXP_LNKCAP,
>>                   (port << PCI_EXP_LNKCAP_PN_SHIFT) |
>> -                 PCI_EXP_LNKCAP_ASPM_L0S |
>> +                 PCI_EXP_LNKCAP_ASPM_L0S | PCI_EXP_LNKCAP_ASPM_L1 |
>>                   PCI_EXP_LNK_MLW_1 |
>>                   PCI_EXP_LNK_LS_25);
>>
>>      pci_set_word(exp_cap + PCI_EXP_LNKSTA,
>>                   PCI_EXP_LNK_MLW_1 | PCI_EXP_LNK_LS_25);
>>
>> +    pci_set_word(dev->wmask + pos + PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_ASPMC);
>> +
>>      pci_set_long(exp_cap + PCI_EXP_DEVCAP2,
>>                   PCI_EXP_DEVCAP2_EFF | PCI_EXP_DEVCAP2_EETLPP);
>>
>
> OK this is making some new bits writeable so it will break
> cross-version migration. Need to add a property and
> disable for -M 1.5 or older.

I'm not a qemu hacker.  Can you point me at an example?

Maybe this isn't really worth it.  I only added this so I could do
some experimentation with how Windows handles these bits, and I'm not
sure it's of general interest.  I guess it is a case of gratuitous
difference compared to real hardware, though -- we already advertise
the L0s capability but the enable bit isn't writable.  The OS will
enable L0s, and lspci will show it enabled on hardware but disabled on
qemu.

Bjorn

>> diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
>> index 109f2f4..5b81b36 100644
>> --- a/include/hw/pci/pcie_regs.h
>> +++ b/include/hw/pci/pcie_regs.h
>> @@ -31,7 +31,7 @@
>>  #define PCI_EXP_FLAGS_TYPE_SHIFT        (ffs(PCI_EXP_FLAGS_TYPE) - 1)
>>
>>
>> -/* PCI_EXP_LINK{CAP, STA} */
>> +/* PCI_EXP_LINK{CAP, STA, CTL} */
>>  /* link speed */
>>  #define PCI_EXP_LNK_LS_25               1
>>
>> @@ -40,9 +40,12 @@
>>
>>  /* PCI_EXP_LINKCAP */
>>  #define PCI_EXP_LNKCAP_ASPM_L0S              0x00000400 /* L0s supported */
>> +#define PCI_EXP_LNKCAP_ASPM_L1               0x00000800 /* L1 supported */
>>
>>  #define PCI_EXP_LNKCAP_PN_SHIFT         (ffs(PCI_EXP_LNKCAP_PN) - 1)
>>
>> +#define PCI_EXP_LNKCTL_ASPMC         0x0003  /* ASPM Control */
>> +
>>  #define PCI_EXP_SLTCAP_PSN_SHIFT        (ffs(PCI_EXP_SLTCAP_PSN) - 1)
>>
>>  #define PCI_EXP_SLTCTL_IND_RESERVED     0x0

Re: [Qemu-devel] [PATCH 2/2] pcie: Add more ASPM support

[Michael S. Tsirkin]

On Mon, May 20, 2013 at 02:44:25PM -0600, Bjorn Helgaas wrote:
> On Mon, May 20, 2013 at 12:32 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, May 20, 2013 at 11:09:56AM -0600, Bjorn Helgaas wrote:
> >> Indicate ASPM L0s and L1 support in Link Capabilities and make the ASPM
> >> bits in Link Control writable.  These Link Control bits don't do anything
> >> in qemu, but having them writable means the BIOS or OS can write them as
> >> on real hardware.
> >>
> >> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >> ---
> >>  hw/pci/pcie.c              |    4 +++-
> >>  include/hw/pci/pcie_regs.h |    5 ++++-
> >>  2 files changed, 7 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> >> index 54fcac8..f194445 100644
> >> --- a/hw/pci/pcie.c
> >> +++ b/hw/pci/pcie.c
> >> @@ -73,13 +73,15 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port)
> >>
> >>      pci_set_long(exp_cap + PCI_EXP_LNKCAP,
> >>                   (port << PCI_EXP_LNKCAP_PN_SHIFT) |
> >> -                 PCI_EXP_LNKCAP_ASPM_L0S |
> >> +                 PCI_EXP_LNKCAP_ASPM_L0S | PCI_EXP_LNKCAP_ASPM_L1 |
> >>                   PCI_EXP_LNK_MLW_1 |
> >>                   PCI_EXP_LNK_LS_25);
> >>
> >>      pci_set_word(exp_cap + PCI_EXP_LNKSTA,
> >>                   PCI_EXP_LNK_MLW_1 | PCI_EXP_LNK_LS_25);
> >>
> >> +    pci_set_word(dev->wmask + pos + PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_ASPMC);
> >> +
> >>      pci_set_long(exp_cap + PCI_EXP_DEVCAP2,
> >>                   PCI_EXP_DEVCAP2_EFF | PCI_EXP_DEVCAP2_EETLPP);
> >>
> >
> > OK this is making some new bits writeable so it will break
> > cross-version migration. Need to add a property and
> > disable for -M 1.5 or older.
> 
> I'm not a qemu hacker.  Can you point me at an example?

Okay. The closest thing is when we made SERR enable writeable.
You can grep for command_serr_enable and QEMU_PCI_CAP_SERR_BITNR.

> Maybe this isn't really worth it.  I only added this so I could do
> some experimentation with how Windows handles these bits, and I'm not
> sure it's of general interest.  I guess it is a case of gratuitous
> difference compared to real hardware, though -- we already advertise
> the L0s capability but the enable bit isn't writable.  The OS will
> enable L0s, and lspci will show it enabled on hardware but disabled on
> qemu.
> 
> Bjorn

I agree and at least for L0s, I think it's worth fixing.
It looks like we could advertize L1 or ignore it,
so I don't mind what happens there, it's up to you to decide.

> >> diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
> >> index 109f2f4..5b81b36 100644
> >> --- a/include/hw/pci/pcie_regs.h
> >> +++ b/include/hw/pci/pcie_regs.h
> >> @@ -31,7 +31,7 @@
> >>  #define PCI_EXP_FLAGS_TYPE_SHIFT        (ffs(PCI_EXP_FLAGS_TYPE) - 1)
> >>
> >>
> >> -/* PCI_EXP_LINK{CAP, STA} */
> >> +/* PCI_EXP_LINK{CAP, STA, CTL} */
> >>  /* link speed */
> >>  #define PCI_EXP_LNK_LS_25               1
> >>
> >> @@ -40,9 +40,12 @@
> >>
> >>  /* PCI_EXP_LINKCAP */
> >>  #define PCI_EXP_LNKCAP_ASPM_L0S              0x00000400 /* L0s supported */
> >> +#define PCI_EXP_LNKCAP_ASPM_L1               0x00000800 /* L1 supported */
> >>
> >>  #define PCI_EXP_LNKCAP_PN_SHIFT         (ffs(PCI_EXP_LNKCAP_PN) - 1)
> >>
> >> +#define PCI_EXP_LNKCTL_ASPMC         0x0003  /* ASPM Control */
> >> +
> >>  #define PCI_EXP_SLTCAP_PSN_SHIFT        (ffs(PCI_EXP_SLTCAP_PSN) - 1)
> >>
> >>  #define PCI_EXP_SLTCTL_IND_RESERVED     0x0