From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39317) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YQuzk-0002jl-So for qemu-devel@nongnu.org; Thu, 26 Feb 2015 04:44:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YQuzh-0003q3-I1 for qemu-devel@nongnu.org; Thu, 26 Feb 2015 04:44:36 -0500 Received: from [59.151.112.132] (port=29849 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YQuzg-0003ph-Tt for qemu-devel@nongnu.org; Thu, 26 Feb 2015 04:44:33 -0500 Message-ID: <54EEE96A.6050705@cn.fujitsu.com> Date: Thu, 26 Feb 2015 17:37:46 +0800 From: Chen Fan MIME-Version: 1.0 References: <814aad038816b71c4c01d12a16a327c864a66fcc.1423551266.git.chen.fan.fnst@cn.fujitsu.com> <1423586378.22865.813.camel@redhat.com> In-Reply-To: <1423586378.22865.813.camel@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC v3 04/10] vfio: add pcie extanded capability support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: izumi.taku@jp.fujitsu.com, qemu-devel@nongnu.org On 02/11/2015 12:39 AM, Alex Williamson wrote: > On Tue, 2015-02-10 at 15:03 +0800, Chen Fan wrote: >> when we detect extanded capability in vfio device, then >> we should initialize the vfio device corresponding feature >> register bits. >> so guest OS can find it and set those bits as needed. >> and initialize aer capability. >> >> Signed-off-by: Chen Fan >> --- >> hw/vfio/pci.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 84 insertions(+), 1 deletion(-) >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index 014a92c..75c932b 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -2435,6 +2435,20 @@ static uint8_t vfio_std_cap_max_size(PCIDevice *pdev, uint8_t pos) >> return next - pos; >> } >> >> +static uint16_t vfio_ext_cap_max_size(PCIDevice *pdev, uint16_t pos) >> +{ >> + uint16_t tmp, next = PCIE_CONFIG_SPACE_SIZE - 1; >> + >> + for (tmp = PCI_CONFIG_SPACE_SIZE; tmp; >> + tmp = PCI_EXT_CAP_NEXT(pci_get_long(pdev->config + tmp))) { >> + if (tmp > pos && tmp < next) { >> + next = tmp; >> + } >> + } >> + >> + return next - pos; >> +} >> + >> static void vfio_set_word_bits(uint8_t *buf, uint16_t val, uint16_t mask) >> { >> pci_set_word(buf, (pci_get_word(buf) & ~mask) | val); >> @@ -2658,16 +2672,85 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos) >> return 0; >> } >> >> +static void vfio_setup_pcie_aer(VFIOPCIDevice *vdev, uint16_t pos) >> +{ >> + PCIDevice *pdev = &vdev->pdev; >> + uint32_t err_cap; >> + >> + err_cap = pci_get_long(pdev->config + pos + PCI_ERR_CAP); >> + >> + if (err_cap & PCI_ERR_CAP_MHRC) { >> + pdev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT; >> + } else { >> + pdev->exp.aer_log.log_max = 1; >> + } >> + >> + pdev->exp.aer_log.log = g_malloc0(sizeof pdev->exp.aer_log.log[0] * >> + pdev->exp.aer_log.log_max); >> + >> + pcie_aer_setup(pdev, pos, pdev->exp.aer_log.log_max); >> +} >> + >> +static int vfio_add_ext_cap(VFIOPCIDevice *vdev, uint16_t pos) >> +{ >> + PCIDevice *pdev = &vdev->pdev; >> + uint32_t header; >> + uint16_t cap_id, next, size; >> + uint8_t cap_ver; >> + int ret; >> + >> + header = pci_get_long(pdev->config + pos); >> + cap_id = PCI_EXT_CAP_ID(header); >> + cap_ver = PCI_EXT_CAP_VER(header); >> + next = PCI_EXT_CAP_NEXT(header); >> + >> + size = vfio_ext_cap_max_size(pdev, pos); >> + > There's a big comment in the standard capability version of this that > indicates that pci_add_capability() always adds capabilities to the head > of the chain and therefore we use this recursion to keep the same > ordering as hardware. pcie_add_capability() seems to add at the tail of > the list, so I think this actually reverses the list versus hardware, > which is undesirable. Code comments would be nice too. IIUC, Compare pcie_add_capability() with pci_add_capability(), pcie_add_capability() not only add caps at the tail of linked list, but also always add a new cap at the tail of linked list. but for vfio device, we don't need to add a new cap. we only need to parse the existed capability in config space. so I think we should modify pcie_add_capability() as pci_add_capability2(). Thanks, Chen > >> + if (next) { >> + ret = vfio_add_ext_cap(vdev, next); >> + if (ret) { >> + return ret; >> + } >> + } >> + >> + pci_set_long(vdev->emulated_config_bits + pos, 0xffff); >> + >> + switch (cap_id) { >> + case PCI_EXT_CAP_ID_ERR: >> + pcie_add_capability(pdev, cap_id, cap_ver, pos, size); >> + vfio_setup_pcie_aer(vdev, pos); >> + break; >> + default: >> + pcie_add_capability(pdev, cap_id, cap_ver, pos, size); >> + break; >> + } >> + >> + return 0; >> +} >> + >> static int vfio_add_capabilities(VFIOPCIDevice *vdev) >> { >> PCIDevice *pdev = &vdev->pdev; >> + int ret; >> >> if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST) || >> !pdev->config[PCI_CAPABILITY_LIST]) { >> return 0; /* Nothing to add */ >> } >> >> - return vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST]); >> + ret = vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST]); >> + if (ret) { >> + goto out; >> + } >> + >> + if (!pci_get_long(pdev->config + PCI_CONFIG_SPACE_SIZE)) { >> + goto out; >> + } >> + >> + ret = vfio_add_ext_cap(vdev, PCI_CONFIG_SPACE_SIZE); >> + >> +out: >> + return ret; >> } >> >> static void vfio_pci_pre_reset(VFIOPCIDevice *vdev) > > > . >