linux-kernel - Re: [PATCH V3 08/10] vfio/pci: Probe and store ability to support dynamic MSI-X

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] [day] [month] [year] [list]

Message-ID: <c044a192-ab6a-9ccc-8eb0-46f9fa6821e2@intel.com>
Date:   Tue, 25 Apr 2023 09:52:23 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Jason Gunthorpe <jgg@...dia.com>
CC:     Alex Williamson <alex.williamson@...hat.com>, <yishaih@...dia.com>,
        <shameerali.kolothum.thodi@...wei.com>, <kevin.tian@...el.com>,
        <tglx@...utronix.de>, <darwi@...utronix.de>, <kvm@...r.kernel.org>,
        <dave.jiang@...el.com>, <jing2.liu@...el.com>,
        <ashok.raj@...el.com>, <fenghua.yu@...el.com>,
        <tom.zanussi@...ux.intel.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V3 08/10] vfio/pci: Probe and store ability to support
 dynamic MSI-X

Hi Jason,

On 4/25/2023 7:51 AM, Jason Gunthorpe wrote:
> On Mon, Apr 24, 2023 at 04:52:08PM -0700, Reinette Chatre wrote:
>> On 4/24/2023 10:43 AM, Jason Gunthorpe wrote:
>>> On Wed, Apr 19, 2023 at 11:11:48AM -0700, Reinette Chatre wrote:
>>>> On 4/18/2023 3:38 PM, Alex Williamson wrote:
>>>>> On Tue, 18 Apr 2023 10:29:19 -0700
>>>>> Reinette Chatre <reinette.chatre@...el.com> wrote:
>>
>> ...
>>
>>>> diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
>>>> index 4f070f2d6fde..d730d78754a2 100644
>>>> --- a/include/linux/vfio_pci_core.h
>>>> +++ b/include/linux/vfio_pci_core.h
>>>> @@ -67,8 +67,8 @@ struct vfio_pci_core_device {
>>>>  	u8			msix_bar;
>>>>  	u16			msix_size;
>>>>  	u32			msix_offset;
>>>> -	bool			has_dyn_msix;
>>>>  	u32			rbar[7];
>>>> +	bool			has_dyn_msix;
>>>>  	bool			pci_2_3;
>>>>  	bool			virq_disabled;
>>>>  	bool			reset_works;
>>>
>>> Also, Linus on record as strongly disliking these lists of bools
>>
>> This looks like an example:
>> https://lkml.org/lkml/2017/11/21/384
>>
>>>
>>> If they don't need read_once/etc stuff then use a list of bitfields
>>
>> I do not see any direct usage of read_once in the driver, but it is not
>> clear to me what falls under the "etc" umbrella.
> 
> Anything that might assume atomicity, smp_store_release, set_bit, and others
> 
>>  Do you consider all the bools in struct vfio_pci_core_device to be
>> candidates for transition?
> 
> Yes, group them ito into a bitfield.

Will do.

> 
>> I think a base type of unsigned int since it appears to be the custom
>> and (if I understand correctly) was preferred at the time Linus wrote
>> the message I found.
> 
> It doesn't matter a lot, using "bool" means the compiler adds extra
> code to ensure "foo = 4" stores true, and the underyling size is not
> well defined (but we don't care here)

Looking further outside that email thread I do see using base type of bool
is common. I will use that. Doing so also reduces the churn of this
transition since only the data structure changes, not the code. 
>> Looking ahead there seems be be a bigger task here. A quick search
>> revealed a few other instances of vfio using "bool" in a struct. It
>> does not all qualify for your "lists of bools" comment, but they
>> may need a closer look because of the "please don't use "bool" in
>> structures at all" comment made by Linus in the email I found.
> 
> IMHO bool is helpful for clarity, it says it is a flag. In these cases
> we won't gain anything by using u8 instead
> 
> Lists of bools however start to get a little silly when we use maybe 4
> bytes per bool (though x86-64 is using 1 byte in structs)
> 

Thank you very much for catching this and providing guidance. I plan to
include this change to struct vfio_pci_core_device as a prep
patch within this series.

Reinette