From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751729AbcLFGoE (ORCPT ); Tue, 6 Dec 2016 01:44:04 -0500 Received: from cn.fujitsu.com ([59.151.112.132]:59984 "EHLO heian.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1751085AbcLFGoA (ORCPT ); Tue, 6 Dec 2016 01:44:00 -0500 X-IronPort-AV: E=Sophos;i="5.22,518,1449504000"; d="scan'208";a="13643396" Subject: Re: [PATCH] vfio/pci: Support error recovery To: "Michael S. Tsirkin" References: <1480246457-10368-1-git-send-email-caoj.fnst@cn.fujitsu.com> <20161130210413.5161aab1@t450s.home> <20161201064708-mutt-send-email-mst@kernel.org> <58402847.2000003@cn.fujitsu.com> <20161206053519-mutt-send-email-mst@kernel.org> CC: Alex Williamson , , , From: Cao jin Message-ID: <58465F0B.5040401@cn.fujitsu.com> Date: Tue, 6 Dec 2016 14:47:39 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <20161206053519-mutt-send-email-mst@kernel.org> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.167.226.69] X-yoursite-MailScanner-ID: E21334818570.A22E3 X-yoursite-MailScanner: Found to be clean X-yoursite-MailScanner-From: caoj.fnst@cn.fujitsu.com Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/06/2016 11:46 AM, Michael S. Tsirkin wrote: > On Thu, Dec 01, 2016 at 09:40:23PM +0800, Cao jin wrote: >> >> >> On 12/01/2016 12:51 PM, Michael S. Tsirkin wrote: >>> On Wed, Nov 30, 2016 at 09:04:13PM -0700, Alex Williamson wrote: >>>> On Sun, 27 Nov 2016 19:34:17 +0800 >>>> Cao jin wrote: >>>> >> >>>>> @@ -1187,10 +1200,30 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev, >>>>> return PCI_ERS_RESULT_DISCONNECT; >>>>> } >>>>> >>>>> + /* get device's uncorrectable error status as soon as possible, >>>>> + * and signal it to user space. The later we read it, the possibility >>>>> + * the register value is mangled grows. */ >>>> >>>> Bad comment style (throughout). >>>> >>>>> + aer_cap_offset = pci_find_ext_capability(vdev->pdev, PCI_EXT_CAP_ID_ERR); >>>>> + ret = pci_read_config_dword(vdev->pdev, aer_cap_offset + >>>>> + PCI_ERR_UNCOR_STATUS, &uncor_status); >>>>> + if (ret) >>>>> + return PCI_ERS_RESULT_DISCONNECT; >>>>> + >>>>> + pr_err("device %d got AER detect notification. uncorrectable error status = 0x%x\n", pdev->devfn, uncor_status);//to be removed >>>>> mutex_lock(&vdev->igate); >>>>> + >>>>> + vdev->aer_recovering = true; >>>>> + reinit_completion(&vdev->aer_error_completion); >>>>> + >>>>> + /* suspend config space access from user space, >>>>> + * when vfio-pci's error recovery process is on */ >>>>> + pci_cfg_access_trylock(vdev->pdev); >>>>> >>>>> - if (vdev->err_trigger) >>>>> - eventfd_signal(vdev->err_trigger, 1); >>>>> + if (vdev->err_trigger && uncor_status) { >>>>> + pr_err("device %d signal uncor status to user space", pdev->devfn);//may be removed >>>>> + /* signal uncorrectable error status to user space */ >>>>> + eventfd_signal(vdev->err_trigger, uncor_status); >>> >>> What does this mean that we trigger with uncor_status as opposed to 1? >>> >> >> I guess you missed the changelog in qemu patchset's cover letter, see if >> it helps(copy from cover letter): >> >> 5. Change what eventfd signals(check vfio driver patch). Before, >> vfio-pci driver add 1 to the counter, which doesn't have meaning, just >> for notification. Now, vfio-pci's error detected handler read the >> uncorrectable error status and signal it to qemu. > > I don't think you can use an eventfd to send values like this. > > eventfd does a sum of these values, so sending e.g. > value 2 will look the same as sending value 1 twice. > Yes, eventfd has a uint_64 counter, and eventfd_signal does a sum. manpage of "eventfd" says: The semantics of read(2) depend on whether the eventfd counter currently has a nonzero value and whether the EFD_SEMAPHORE flag was specified when creating the eventfd file descriptor: * If EFD_SEMAPHORE was not specified and the eventfd counter has a nonzero value, then a read(2) returns 8 bytes containing that value, and the counter's value is reset to zero. ...... * If the eventfd counter is zero at the time of the call to read(2), then the call either blocks until the counter becomes nonzero (at which time, the read(2) proceeds as described above) or fails with the error EAGAIN if the file descriptor has been made nonblocking. And in actual tests, the debug line I add in vfio_err_notifier_handler shows[*]: qemu read this value right every time, the same as in vfio-pci driver. quick reference: [*]https://lists.gnu.org/archive/html/qemu-devel/2016-11/msg04831.html > >> Why? When testing previous version(host aer driver still reset link), >> found that there is quite possibility that register reading returns the >> invalid value 0xFFFFFFFF, which results in all(2 in my environment) >> qemu's vfio-pci function send AER to root port while I only inject error >> into one function. This is unreasonable, and I think it is the result of >> reading during device reset. >> >> Previous patch does considered to find the >> real function who has error happened, but won't work under the situation >> I found. So move the register reading as early as possible would be the >> nature thoughts, and it is moved into vfio-pci driver. Although now >> reset_link is disabled in aer driver, get the uncor error status as >> early as possible still make sense, for example: if user space driver >> does device reset once receiving the error notification, and then read >> register. > > I had trouble understanding the above. Let me ask you this: > should we try to avoid triggering uncorrectable errors? > Aren't any such errors likely to crash guests? > Sorry I don't quite understand why your questions are raised, it seems just describing the current situation without my AER patch: when assign device via vfio, we don't handle uncorrectable error, just vm_stop()(this is what you mean "crash guests", right?), this is the contents of current vfio_err_notifier_handler(). Without our AER patches, the answer to your questions are YES. But the purpose of our patch is to fix your questions. Not sure my answer helps you, but feel free to ask if still any questions. -- Sincerely, Cao jin