linux-kernel - Re: [PATCH 1/2] PCI: Add support for VF Resizable Bar extended cap

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: <6e98eced-9882-2397-663e-94470c88ec48@intel.com>
Date:   Fri, 17 Dec 2021 13:38:06 +0100
From:   Michał Winiarski <michal.winiarski@...el.com>
To:     Krzysztof Wilczyński <kw@...ux.com>
CC:     <linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Christian König <christian.koenig@....com>,
        Ard Biesheuvel <ardb@...nel.org>,
        "Michael J . Ruhl" <michael.j.ruhl@...el.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>
Subject: Re: [PATCH 1/2] PCI: Add support for VF Resizable Bar extended cap

On 16.12.2021 01:21, Krzysztof Wilczyński wrote:
> Hi Michał,
> 
> [...]
>> +static int pci_memory_decoding(struct pci_dev *dev)
>> +{
>> +	u16 cmd;
>> +
>> +	pci_read_config_word(dev, PCI_COMMAND, &cmd);
>> +	if (cmd & PCI_COMMAND_MEMORY)
>> +		return -EBUSY;
>> +
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_PCI_IOV
>> +static int pci_vf_memory_decoding(struct pci_dev *dev)
>> +{
>> +	u16 cmd;
>> +
>> +	pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_CTRL, &cmd);
>> +	if (cmd & PCI_SRIOV_CTRL_MSE)
>> +		return -EBUSY;
>> +
>> +	return 0;
>> +}
>> +#endif
>> +
>>   int pci_resize_resource(struct pci_dev *dev, int resno, int size)
>>   {
>>   	struct resource *res = dev->resource + resno;
>>   	struct pci_host_bridge *host;
>>   	int old, ret;
>>   	u32 sizes;
>> -	u16 cmd;
>>   
>>   	/* Check if we must preserve the firmware's resource assignment */
>>   	host = pci_find_host_bridge(dev->bus);
>> @@ -424,9 +447,14 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size)
>>   	if (!(res->flags & IORESOURCE_UNSET))
>>   		return -EBUSY;
>>   
>> -	pci_read_config_word(dev, PCI_COMMAND, &cmd);
>> -	if (cmd & PCI_COMMAND_MEMORY)
>> -		return -EBUSY;
>> +#ifdef CONFIG_PCI_IOV
>> +	if (resno >= PCI_IOV_RESOURCES)
>> +		ret = pci_vf_memory_decoding(dev);
>> +	else
>> +#endif
>> +	ret = pci_memory_decoding(dev);
>> +	if (ret)
>> +		return ret;
> 
> The above else works, however, it does trip our static analysis tools, and
> lack of the indentation makes it slightly confusing to read, at least at
> the first glance.  Thus, I wonder whether it would be possible to combine
> the pci_vf_memory_decoding() and pci_memory_decoding() somehow neatly into
> a private helper that takes a boolean to indicate whether it's a VF or not.
> Then, we could drop the if-statement, since some of the logic could live
> within the helper.
> 
> What do you think?

Sure - implementing a helper in a way that Christian suggested (the 
helper still takes resno though) should also take care of that.

Thanks
-Michał

> 
> 	Krzysztof