linux-kernel - Re: [PATCH] vfio/pci: Support error recovery

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <58402847.2000003@cn.fujitsu.com>
Date:   Thu, 1 Dec 2016 21:40:23 +0800
From:   Cao jin <caoj.fnst@...fujitsu.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>,
        Alex Williamson <alex.williamson@...hat.com>
CC:     <linux-kernel@...r.kernel.org>, <kvm@...r.kernel.org>,
        <izumi.taku@...fujitsu.com>
Subject: Re: [PATCH] vfio/pci: Support error recovery

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 <caoj.fnst@...fujitsu.com> 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