linux-kernel - Re: [PATCH v9 10/12] vfio: Add function to get device_api string from vfio_device

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: <3a51ce1c-1396-676f-3a9b-9faa8390c632@nvidia.com>
Date:   Fri, 21 Oct 2016 08:30:53 +0530
From:   Kirti Wankhede <kwankhede@...dia.com>
To:     Alex Williamson <alex.williamson@...hat.com>
CC:     <pbonzini@...hat.com>, <kraxel@...hat.com>, <cjia@...dia.com>,
        <qemu-devel@...gnu.org>, <kvm@...r.kernel.org>,
        <kevin.tian@...el.com>, <jike.song@...el.com>,
        <bjsdjshi@...ux.vnet.ibm.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v9 10/12] vfio: Add function to get device_api string from
 vfio_device_info.flags



On 10/21/2016 2:52 AM, Alex Williamson wrote:
> On Fri, 21 Oct 2016 02:44:37 +0530
> Kirti Wankhede <kwankhede@...dia.com> wrote:
> 
...

>>>>>>  
>>>>>> +extern const char *vfio_device_api_string(u32 flags);
>>>>>> +
>>>>>>  struct pci_dev;
>>>>>>  #ifdef CONFIG_EEH
>>>>>>  extern void vfio_spapr_pci_eeh_open(struct pci_dev *pdev);    
>>>>>
>>>>> Couldn't this simply be a #define in the uapi header?
>>>>>
>>>>> #define VFIO_DEVICE_PCI_API_STRING "vfio-pci"
>>>>>
>>>>> I don't really see why we need a lookup function.
>>>>>     
>>>>
>>>> String is tightly coupled with the FLAG, right?
>>>> Instead user need to take care of making sure to return proper string,
>>>> and don't mis-match the string, I think having function is easier.  
>>>
>>> That's exactly why I proposed putting the #define string in the uapi,
>>> by that I mean the vfio uapi header.  That keeps the tight coupling to
>>> the flag, they're both defined in the same place, plus it gives
>>> userspace a reference so they're not just inventing a string to compare
>>> against.  IOW, the vendor driver simply does an sprintf of
>>> VFIO_DEVICE_PCI_API_STRING and userspace (ie. libvirt) can do a strcmp
>>> with VFIO_DEVICE_PCI_API_STRING from the same header and everybody
>>> arrives at the same result.
>>>   
>>>> Vendor driver should decide the type of device they want to expose and
>>>> set the flag, using this function vendor driver would return string
>>>> which is based on flag they set.  
>>>
>>> Being a function adds no intrinsic value and being in a uapi header does
>>> add value to userspace.  Thanks,
>>>   
>>
>> Ok. The strings should be in uapi, but having function (like below) to
>> return proper string based on flag would be good to have for vendor driver.
>>
>>  +const char *vfio_device_api_string(u32 flags)
>>  +{
>>  +	if (flags & VFIO_DEVICE_FLAGS_PCI)
>>  +		return VFIO_DEVICE_API_PCI_STRING;
>>  +
>>  +	if (flags & VFIO_DEVICE_FLAGS_PLATFORM)
>>  +		return VFIO_DEVICE_API_PLATFORM_STRING;
>>  +
>>  +	if (flags & VFIO_DEVICE_FLAGS_AMBA)
>>  +		return VFIO_DEVICE_API_AMBA_STRING;
>>  +
>>  +	return "";
>>  +}
>>  +EXPORT_SYMBOL(vfio_device_api_string);
> 
> I disagree, it's pointless maintenance overhead.  It's yet another
> function that we need to care about for kABI and it offers almost no
> value.  Thanks,
> 

If any vendor driver sets VFIO_DEVICE_FLAGS_PLATFORM flag but sets
VFIO_DEVICE_API_PCI_STRING, we don't have a way to verify this in kernel
driver. Is that acceptable?


Kirti