From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39701) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UeiPM-0002yP-Gk for qemu-devel@nongnu.org; Tue, 21 May 2013 04:59:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UeiPJ-0004pV-NJ for qemu-devel@nongnu.org; Tue, 21 May 2013 04:59:00 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42293) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UeiCP-0000HH-P2 for qemu-devel@nongnu.org; Tue, 21 May 2013 04:45:37 -0400 Date: Tue, 21 May 2013 11:45:56 +0300 From: "Michael S. Tsirkin" Message-ID: <20130521084555.GB27391@redhat.com> References: <20130520170949.29259.99889.stgit@bhelgaas-glaptop> <20130520170956.29259.87841.stgit@bhelgaas-glaptop> <20130520183256.GA16569@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH 2/2] pcie: Add more ASPM support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Bjorn Helgaas Cc: "qemu-devel@nongnu.org" 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 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 > >> --- > >> 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