linux-kernel - Re: [Xen-devel] [PATCH V2] Xen/pciback: Implement PCI slot or bus reset with 'do

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: <c97d2b6e-55ee-fb9a-5a86-8fad87448a67@oracle.com>
Date:   Wed, 29 Nov 2017 09:37:13 -0600
From:   Govinda Tatti <govinda.tatti@...cle.com>
To:     Jan Beulich <JBeulich@...e.com>
Cc:     Juergen Gross <jgross@...e.com>, konrad.wilk@...cle.com,
        linux-kernel@...r.kernel.org, xen-devel@...ts.xenproject.org,
        boris.ostrovsky@...cle.com, roger.pau@...rix.com
Subject: Re: [Xen-devel] [PATCH V2] Xen/pciback: Implement PCI slot or bus
 reset with 'do_flr' SysFS attribute

Jan,

Please see below for my comments.

On 11/9/2017 2:49 AM, Jan Beulich wrote:
>>>> On 09.11.17 at 00:06, <Govinda.Tatti@...cle.COM> wrote:
>> --- a/drivers/xen/xen-pciback/pci_stub.c
>> +++ b/drivers/xen/xen-pciback/pci_stub.c
>> @@ -244,6 +244,91 @@ struct pci_dev *pcistub_get_pci_dev(struct xen_pcibk_device *pdev,
>>   	return found_dev;
>>   }
>>   
>> +struct pcistub_args {
>> +	struct pci_dev *dev;
> Please don't ignore prior review comments: Either carry out what
> was requested, or explain why the request can't be fulfilled. You
> saying "This field will point to first device that is not owned by
> pcistub" to Roger's request to make this a pointer to const is not a
> valid reason to not do the adjustment; in fact your reply is entirely
> unrelated to the request.
We can use "const" with above field since we will set this field only once.
I will change it.
>
>> +static int pcistub_search_dev(struct pci_dev *dev, void *data)
>> +{
>> +	struct pcistub_device *psdev;
>> +	struct pcistub_args *arg = data;
>> +	bool found_dev = false;
> Purely cosmetical, but anyway: Why not just "found"? What else
> could be (not) found here other than the device in question?
Sure, I will change it to "found".

>
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&pcistub_devices_lock, flags);
>> +
>> +	list_for_each_entry(psdev, &pcistub_devices, dev_list) {
>> +		if (psdev->dev == dev) {
>> +			found_dev = true;
>> +			arg->dcount++;
>> +			break;
>> +		}
>> +	}
>> +
>> +	spin_unlock_irqrestore(&pcistub_devices_lock, flags);
>> +
>> +	/* Device not owned by pcistub, someone owns it. Abort the walk */
>> +	if (!found_dev)
>> +		arg->dev = dev;
>> +
>> +	return found_dev ? 0 : 1;
> Despite the function needing to return int, this can be simplified to
> "return !found_dev". I'd also like to note that the part of the
> earlier comment related to this is sort of disconnected. How about
>
> 	/* Device not owned by pcistub, someone owns it. Abort the walk */
> 	if (!found_dev) {
> 		arg->dev = dev;
> 		return 1;
> 	}
>
> 	return 0;
Fine with me.
>
> And finally - I don't think the comment is entirely correct - the
> device not being owned by pciback doesn't necessarily mean it's
> owned by another driver. It could as well be unowned.
OK. I will modify this comment as " Device not owned by pcistub. Abort 
the walk".
>
>> +static int pcistub_reset_dev(struct pci_dev *dev)
>> +{
>> +	struct xen_pcibk_dev_data *dev_data;
>> +	bool slot = false, bus = false;
>> +	struct pcistub_args arg = {};
>> +
>> +	if (!dev)
>> +		return -EINVAL;
>> +
>> +	dev_dbg(&dev->dev, "[%s]\n", __func__);
>> +
>> +	if (!pci_probe_reset_slot(dev->slot))
>> +		slot = true;
>> +	else if ((!pci_probe_reset_bus(dev->bus)) &&
>> +		 (!pci_is_root_bus(dev->bus)))
>> +		bus = true;
>> +
>> +	if (!bus && !slot)
>> +		return -EOPNOTSUPP;
>> +
>> +	/*
>> +	 * Make sure all devices on this bus are owned by the
>> +	 * PCI backend so that we can safely reset the whole bus.
>> +	 */
> Is that really the case when you mean to do a slot reset? It was for
> a reason that I had asked about a missing "else" in v1 review,
> rather than questioning the conditional around the logic.

In the case of bus or slot reset, our goal is to reset connected PCIe 
fabric/card/endpoint.
The connected card/endpoint can be multi-function device. So, same 
walk-through and checking
is needed irrespective of type of reset being used.

>
>> +	pci_walk_bus(dev->bus, pcistub_search_dev, &arg);
>> +
>> +	/* All devices under the bus should be part of pcistub! */
>> +	if (arg.dev) {
>> +		dev_err(&dev->dev, "%s device on bus 0x%x is not owned by pcistub\n",
> %#x
>
> Yet then, thinking about what would be useful information should the
> situation really arise, I'm not convinced printing a bare bus number
> here is useful either. Especially for the case of multiple parallel
> requests you want to make it possible to match each message to the
> original request (guest start or whatever). Hence I think you want
> something like
>
> "%s on the same bus as %s is not owned by " DRV_NAME "\n"
Sure, I will make this change.
>
>> +			pci_name(arg.dev), dev->bus->number);
>> +
>> +		return -EBUSY;
>> +	}
>> +
>> +	dev_dbg(&dev->dev, "pcistub owns %d devices on bus 0x%x\n",
>> +		arg.dcount, dev->bus->number);
> While here the original device is perhaps not necessary to print,
> the bare bus number doesn't carry enough information: You'll
> want to prefix it by the segment number. Plus you'll want to use
> canonical formatting (ssss:bb), so one can get matches when
> suitably grep-ing the log. Perhaps bus->name is what you're
> after.
Sure, I can change it to

dev_dbg(&dev->dev, "pcistub owns %d devices on PCI Bus %04x:%02x", pci_domain_nr(bus), bus->number);

Cheers
GOVINDA