From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60297) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YSKo9-0005SY-Og for qemu-devel@nongnu.org; Mon, 02 Mar 2015 02:30:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YSKo6-0000ag-Ib for qemu-devel@nongnu.org; Mon, 02 Mar 2015 02:30:29 -0500 Received: from [59.151.112.132] (port=18709 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YSKo5-0000aS-Qt for qemu-devel@nongnu.org; Mon, 02 Mar 2015 02:30:26 -0500 Message-ID: <54F40FF7.6090200@cn.fujitsu.com> Date: Mon, 2 Mar 2015 15:23:35 +0800 From: Chen Fan MIME-Version: 1.0 References: <119e402b6962fe647f428e8984ae55b112ff7a93.1423551266.git.chen.fan.fnst@cn.fujitsu.com> <1423586352.22865.810.camel@redhat.com> <54EEC151.4070202@cn.fujitsu.com> <1424989139.5200.81.camel@redhat.com> In-Reply-To: <1424989139.5200.81.camel@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC v3 08/10] vfio-pci: add VFIO_FEATURE_ENABLE_AER_CAP feature 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/27/2015 06:18 AM, Alex Williamson wrote: > On Thu, 2015-02-26 at 14:46 +0800, Chen Fan wrote: >> On 02/11/2015 12:39 AM, Alex Williamson wrote: >>> On Tue, 2015-02-10 at 15:03 +0800, Chen Fan wrote: >>>> add a new "aercap" feature in vfio device, for controlling >>>> whether expose aer capability. >>>> >>>> Signed-off-by: Chen Fan >>>> --- >>>> hw/vfio/pci.c | 10 ++++++++-- >>>> 1 file changed, 8 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >>>> index bf314a1..c21b40b 100644 >>>> --- a/hw/vfio/pci.c >>>> +++ b/hw/vfio/pci.c >>>> @@ -138,6 +138,8 @@ typedef struct VFIOMSIXInfo { >>>> enum { >>>> #define VFIO_FEATURE_ENABLE_VGA_BIT 0 >>>> VFIO_FEATURE_ENABLE_VGA = (1 << VFIO_FEATURE_ENABLE_VGA_BIT), >>>> +#define VFIO_FEATURE_ENABLE_AER_CAP_BIT 1 >>>> + VFIO_FEATURE_ENABLE_AER_CAP = (1 << VFIO_FEATURE_ENABLE_AER_CAP_BIT), >>>> }; >>>> >>>> typedef struct VFIOPCIDevice { >>>> @@ -2721,8 +2723,10 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev, uint16_t pos) >>>> >>>> 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); >>>> + if (vdev->features & VFIO_FEATURE_ENABLE_AER_CAP) { >>>> + pcie_add_capability(pdev, cap_id, cap_ver, pos, size); >>>> + vfio_setup_pcie_aer(vdev, pos); >>>> + } >> Hi Alex, >> sorry for replaying so late. I am just back from holiday. :) >> >>> Maybe the question should be why we're adding extended capabilities at >>> all if the chipset doesn't expose them. If we boot on 440fx, all >>> extended capability parsing could be disabled. We could then add an >>> x-aer=off option to vfio-pci to allow the user to disable aer >>> specifically if they want. >> your meaning is adding two option: >> >> 1) one controls exposing extended caps to device. in particular on >> 440fx chipset we must disable it. >> >> 2) the other one controls exposing aer capability to user specifically. >> >> right? > Right, a flag toggled by the chipset type doesn't work because whether > we want to expose AER is dependent on the exposed bus type to the guest. > Even if we're using an express chipset like q35, the device could be > behind a PCIe-to-PCI bridge in the guest and the guest would not have > access to extended config space, including AER. Therefore it seems like > we simply need to test pci_bus_is_express() for a device and skip all > extended capability parsing when false. > > Then we've solved the bus topology problem and we only need to be > concerned about whether the user has a need to disable AER by choice. > We probably do need to allow that for compatibility with existing PCIe > guest configurations. I'd probably name it something like "disable_aer" > because we don't want to give the user any ideas that it can be used to > toggle AER on, only off. Thanks, Hi Alex, I has made a v4 patch which added a 'x-aer' to enable/disable aer capability and remake the parsing extended caps method. pls help to review it. :) Thanks, Chen > > Alex > >>>> break; >>>> default: >>>> pcie_add_capability(pdev, cap_id, cap_ver, pos, size); >>>> @@ -3487,6 +3491,8 @@ static Property vfio_pci_dev_properties[] = { >>>> DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features, >>>> VFIO_FEATURE_ENABLE_VGA_BIT, false), >>>> DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1), >>>> + DEFINE_PROP_BIT("aercap", VFIOPCIDevice, features, >>>> + VFIO_FEATURE_ENABLE_AER_CAP_BIT, true), >>>> /* >>>> * TODO - support passed fds... is this necessary? >>>> * DEFINE_PROP_STRING("vfiofd", VFIOPCIDevice, vfiofd_name), >>> >>> . >>> > > > . >