From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759477AbcLANgt (ORCPT ); Thu, 1 Dec 2016 08:36:49 -0500 Received: from cn.fujitsu.com ([59.151.112.132]:38500 "EHLO heian.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1758214AbcLANgq (ORCPT ); Thu, 1 Dec 2016 08:36:46 -0500 X-IronPort-AV: E=Sophos;i="5.22,518,1449504000"; d="scan'208";a="13511504" Subject: Re: [PATCH] vfio/pci: Support error recovery To: "Michael S. Tsirkin" , Alex Williamson References: <1480246457-10368-1-git-send-email-caoj.fnst@cn.fujitsu.com> <20161130210413.5161aab1@t450s.home> <20161201064708-mutt-send-email-mst@kernel.org> CC: , , From: Cao jin Message-ID: <58402847.2000003@cn.fujitsu.com> Date: Thu, 1 Dec 2016 21:40:23 +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: <20161201064708-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: F021E40056FC.A8241 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/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. 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. -- Sincerely, Cao jin