From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53655) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVWiS-0004cX-6m for qemu-devel@nongnu.org; Tue, 10 Mar 2015 22:49:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YVWiL-0000eC-QU for qemu-devel@nongnu.org; Tue, 10 Mar 2015 22:49:48 -0400 Received: from [59.151.112.132] (port=56166 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVWiL-0000e2-8K for qemu-devel@nongnu.org; Tue, 10 Mar 2015 22:49:41 -0400 Message-ID: <54FFABAC.5020201@cn.fujitsu.com> Date: Wed, 11 Mar 2015 10:42:52 +0800 From: Chen Fan MIME-Version: 1.0 References: <2f705a83edae9e2bfbff6d0bdc5d2e28ca16f927.1425280224.git.chen.fan.fnst@cn.fujitsu.com> <1425932939.4675.210.camel@redhat.com> In-Reply-To: <1425932939.4675.210.camel@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC v4 3/9] 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 03/10/2015 04:28 AM, Alex Williamson wrote: > On Mon, 2015-03-02 at 15:16 +0800, Chen Fan wrote: >> For vfio pcie device, we could expose the extanded capability on > s/extanded/extended/ > >> PCIE bus. in order to avoid config space broken, we introduce >> a copy config for parsing extended caps. and rebuild the pcie >> extended config space. >> >> Signed-off-by: Chen Fan >> --- >> hw/vfio/pci.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 82 insertions(+), 1 deletion(-) >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index 84e9d99..96cb52b 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -2482,6 +2482,21 @@ static uint8_t vfio_std_cap_max_size(PCIDevice *pdev, uint8_t pos) >> return next - pos; >> } >> >> + >> +static uint16_t vfio_ext_cap_max_size(const uint8_t *config, 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(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); >> @@ -2705,16 +2720,82 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos) >> return 0; >> } >> >> +static int vfio_add_ext_cap(VFIOPCIDevice *vdev, const uint8_t *config, >> + 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(config + pos); >> + cap_id = PCI_EXT_CAP_ID(header); >> + cap_ver = PCI_EXT_CAP_VER(header); >> + next = PCI_EXT_CAP_NEXT(header); >> + >> + /* >> + * If it becomes important to configure extended capabilities to their >> + * actual size, use this as the default when it's something we don't >> + * recognize. Since QEMU doesn't actually handle many of the config >> + * accesses, exact size doesn't seem worthwhile. >> + */ >> + size = vfio_ext_cap_max_size(config, pos); >> + >> + pcie_add_capability(pdev, cap_id, cap_ver, pos, size); >> + if (pos == PCI_CONFIG_SPACE_SIZE) { >> + /* Begin the rebuild, we should set the next offset zero. */ >> + pci_set_long(pdev->config + pos, PCI_EXT_CAP(cap_id, cap_ver, 0)); >> + } >> + >> + /* Use emulated header pointer to allow dropping extended caps */ >> + pci_set_long(vdev->emulated_config_bits + pos, 0xffffffff); >> + >> + if (next) { >> + ret = vfio_add_ext_cap(vdev, config, next); >> + if (ret) { >> + return ret; >> + } >> + } > This recursion seems pointless. We use it for the standard capabilities > to make the capability list ordering correct and to avoid the config > space copy that is necessary for this version, but I don't see the > advantage to using it here vs a for-loop in the below function. > Recursion is more complicated, so let's only use it if it provides some > benefit. indeed. Thanks, Chen > >> + >> + return 0; >> +} >> + >> static int vfio_add_capabilities(VFIOPCIDevice *vdev) >> { >> PCIDevice *pdev = &vdev->pdev; >> + int ret; >> + uint8_t *config; >> >> 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) { >> + return ret; >> + } >> + >> + /* on PCI bus, it doesn't make sense to expose extended capabilities. */ >> + if (!pci_bus_is_express(vdev->pdev.bus) || >> + !pci_get_long(pdev->config + PCI_CONFIG_SPACE_SIZE)) { > > Don't we need a pci_is_express(pdev) here too? Also, we already have > pdev->bus to use as an argument, no need to start with the vdev. > >> + return 0; >> + } >> + >> + /* >> + * In order to avoid config space broken, here using a copy config to >> + * parse extended capabilitiess. > spelling > >> + */ >> + config = g_malloc0(vdev->config_size); >> + if (!config) { >> + return -ENOMEM; >> + } > g_malloc can't fail, no need for this error out condition. It's > strange, but it's the QEMU way. > >> + memcpy(config, pdev->config, vdev->config_size); > There's a g_memdup() function > >> + ret = vfio_add_ext_cap(vdev, config, PCI_CONFIG_SPACE_SIZE); >> + >> + g_free(config); >> + return ret; >> } >> >> static void vfio_pci_pre_reset(VFIOPCIDevice *vdev) > > > . >