From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751977AbcLFDqf (ORCPT ); Mon, 5 Dec 2016 22:46:35 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39142 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751500AbcLFDqd (ORCPT ); Mon, 5 Dec 2016 22:46:33 -0500 Date: Tue, 6 Dec 2016 05:46:15 +0200 From: "Michael S. Tsirkin" To: Cao jin Cc: Alex Williamson , linux-kernel@vger.kernel.org, kvm@vger.kernel.org, izumi.taku@jp.fujitsu.com Subject: Re: [PATCH] vfio/pci: Support error recovery Message-ID: <20161206053519-mutt-send-email-mst@kernel.org> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <58402847.2000003@cn.fujitsu.com> X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Tue, 06 Dec 2016 03:46:20 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > 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? > -- > Sincerely, > Cao jin >