From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36443) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLSxb-0008BM-Vn for qemu-devel@nongnu.org; Tue, 19 Jan 2016 04:52:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aLSxX-0003P2-TR for qemu-devel@nongnu.org; Tue, 19 Jan 2016 04:52:23 -0500 Received: from [59.151.112.132] (port=37038 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLSxW-0003Og-Tt for qemu-devel@nongnu.org; Tue, 19 Jan 2016 04:52:19 -0500 Message-ID: <569E0636.5060406@cn.fujitsu.com> Date: Tue, 19 Jan 2016 17:47:34 +0800 From: Chen Fan MIME-Version: 1.0 References: <569CAC6E.8080802@gmail.com> In-Reply-To: <569CAC6E.8080802@gmail.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v16 07/14] vfio: add aer support for vfio device List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: marcel@redhat.com, Cao jin , qemu-devel@nongnu.org Cc: izumi.taku@jp.fujitsu.com, alex.williamson@redhat.com, mst@redhat.com On 01/18/2016 05:12 PM, Marcel Apfelbaum wrote: > On 01/12/2016 04:43 AM, Cao jin wrote: >> From: Chen Fan >> >> Calling pcie_aer_init to initilize aer related registers for >> vfio device, then reload physical related registers to expose >> device capability. >> >> Signed-off-by: Chen Fan >> --- >> hw/vfio/pci.c | 81 >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--- >> hw/vfio/pci.h | 3 +++ >> 2 files changed, 81 insertions(+), 3 deletions(-) >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index 64b0867..38b0aa5 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -1832,6 +1832,62 @@ static int vfio_add_std_cap(VFIOPCIDevice >> *vdev, uint8_t pos) >> return 0; >> } >> >> +static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver, >> + int pos, uint16_t size) >> +{ >> + PCIDevice *pdev = &vdev->pdev; >> + PCIDevice *dev_iter; >> + uint8_t type; >> + uint32_t errcap; >> + >> + if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) { >> + pcie_add_capability(pdev, PCI_EXT_CAP_ID_ERR, >> + cap_ver, pos, size); >> + return 0; >> + } >> + >> + dev_iter = pci_bridge_get_device(pdev->bus); >> + if (!dev_iter) { >> + goto error; >> + } >> + >> + while (dev_iter) { >> + type = pcie_cap_get_type(dev_iter); >> + if ((type != PCI_EXP_TYPE_ROOT_PORT && >> + type != PCI_EXP_TYPE_UPSTREAM && >> + type != PCI_EXP_TYPE_DOWNSTREAM)) { >> + goto error; >> + } >> + >> + if (!dev_iter->exp.aer_cap) { >> + goto error; >> + } >> + >> + dev_iter = pci_bridge_get_device(dev_iter->bus); >> + } >> + >> + errcap = vfio_pci_read_config(pdev, pos + PCI_ERR_CAP, 4); >> + /* >> + * The ability to record multiple headers is depending on >> + * the state of the Multiple Header Recording Capable bit and >> + * enabled by the Multiple Header Recording Enable bit. >> + */ >> + if ((errcap & PCI_ERR_CAP_MHRC) && >> + (errcap & PCI_ERR_CAP_MHRE)) { >> + pdev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT; >> + } else { >> + pdev->exp.aer_log.log_max = 0; >> + } >> + >> + pcie_cap_deverr_init(pdev); >> + return pcie_aer_init(pdev, pos, size); >> + >> +error: >> + error_report("vfio: Unable to enable AER for device %s, parent >> bus " >> + "does not support AER signaling", >> vdev->vbasedev.name); >> + return -1; >> +} >> + >> static int vfio_add_ext_cap(VFIOPCIDevice *vdev) >> { >> PCIDevice *pdev = &vdev->pdev; >> @@ -1839,6 +1895,7 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev) >> uint16_t cap_id, next, size; >> uint8_t cap_ver; >> uint8_t *config; >> + int ret = 0; >> >> /* >> * In order to avoid breaking config space, create a copy to >> @@ -1860,16 +1917,29 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev) >> */ >> size = vfio_ext_cap_max_size(config, next); >> >> - pcie_add_capability(pdev, cap_id, cap_ver, next, size); >> - pci_set_long(dev->config + next, PCI_EXT_CAP(cap_id, >> cap_ver, 0)); >> + switch (cap_id) { >> + case PCI_EXT_CAP_ID_ERR: >> + ret = vfio_setup_aer(vdev, cap_ver, next, size); >> + break; >> + default: >> + pcie_add_capability(pdev, cap_id, cap_ver, next, size); >> + break; >> + } >> + >> + if (ret) { >> + goto out; >> + } >> + >> + pci_set_long(pdev->config + next, PCI_EXT_CAP(cap_id, >> cap_ver, 0)); >> >> /* Use emulated next pointer to allow dropping extended >> caps */ >> pci_long_test_and_set_mask(vdev->emulated_config_bits + next, >> PCI_EXT_CAP_NEXT_MASK); >> } >> >> +out: >> g_free(config); >> - return 0; >> + return ret; >> } >> >> static int vfio_add_capabilities(VFIOPCIDevice *vdev) >> @@ -2624,6 +2694,11 @@ static int vfio_initfn(PCIDevice *pdev) >> goto out_teardown; >> } >> >> + if ((vdev->features & VFIO_FEATURE_ENABLE_AER) && >> + !pdev->exp.aer_cap) { > > Hi, > > I think we need an error_report here, otherwise the init > will fail without knowing the reason. maybe we need to exclude this case. if the device hasn't the aer cap, the ENABLE_AER would be not affected. Thanks, Chen > > > Thanks, > Marcel > > >> + goto out_teardown; >> + } >> + >> /* QEMU emulates all of MSI & MSIX */ >> if (pdev->cap_present & QEMU_PCI_CAP_MSIX) { >> memset(vdev->emulated_config_bits + pdev->msix_cap, 0xff, >> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h >> index f004d52..48c1f69 100644 >> --- a/hw/vfio/pci.h >> +++ b/hw/vfio/pci.h >> @@ -15,6 +15,7 @@ >> #include "qemu-common.h" >> #include "exec/memory.h" >> #include "hw/pci/pci.h" >> +#include "hw/pci/pci_bridge.h" >> #include "hw/vfio/vfio-common.h" >> #include "qemu/event_notifier.h" >> #include "qemu/queue.h" >> @@ -127,6 +128,8 @@ typedef struct VFIOPCIDevice { >> #define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT) >> #define VFIO_FEATURE_ENABLE_REQ_BIT 1 >> #define VFIO_FEATURE_ENABLE_REQ (1 << VFIO_FEATURE_ENABLE_REQ_BIT) >> +#define VFIO_FEATURE_ENABLE_AER_BIT 2 >> +#define VFIO_FEATURE_ENABLE_AER (1 << VFIO_FEATURE_ENABLE_AER_BIT) >> int32_t bootindex; >> uint8_t pm_cap; >> bool has_vga; >> > > > > . >