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 <address@hidden> 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 <address@hidden>
> >> ---
> >>  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