Re: [PATCH v5 0/4] uio: add MSI/MSI-X support to uio_pci_generic driver

[Vlad Zolotarov <vladz@cloudius-systems.com>]

On 10/07/15 01:51, Michael S. Tsirkin wrote:
> On Tue, Oct 06, 2015 at 11:35:27PM +0300, Vlad Zolotarov wrote:
>>
>> On 10/06/15 21:27, Michael S. Tsirkin wrote:
>>> On Tue, Oct 06, 2015 at 08:17:35PM +0300, Vlad Zolotarov wrote:
>>>> This series add support for MSI and MSI-X interrupts to uio_pci_generic driver.
>>>>
>>>> Currently uio_pci_generic demands INT#x interrupts source be available. However
>>>> there are devices that simply don't have INT#x capability, for instance SR-IOV
>>>> VF devices that simply don't have INT#x capability. For such devices
>>>> uio_pci_generic will simply fail (more specifically its probe() will fail).
>>>>
>>>> When IOMMU is either not available (e.g. Amazon EC2) or not acceptable due to
>>>> performance overhead and thus VFIO is not an option users that develop
>>>> user-space drivers are left without any option but to develop some proprietary
>>>> UIO drivers (e.g. igb_uio driver in Intel's DPDK) just to be able to use UIO
>>>> infrastructure.
>>>>
>>>> This series provides a generic solution for this problem while preserving the
>>>> original behaviour for devices for which the original uio_pci_generic had worked
>>>> before (i.e. INT#x will be used by default).
>>>>
>>>> New in v5:
>>>>     - Expanded the commitlog on PATCH1.
>>> Looks like you didn't attempt to address any of my review comments.
>>> I don't intend to review this until you do.
>> So far there hasn't been any comments related to the code in these patches
>> from your side but rather comments about the general flaws of the current
>> uio_pci_generic in particular and UIO in general that have nothing to do
>> with this series. Therefore obviously there was nothing to address.
>> If u have any comments related to _THIS_ series I'd be glad to address. So
>> far I was under the strong impression that u develop an obviously
>> theoretical discussion about "nice to have fixed" stuff in UIO, which was
>> obvious to everybody on this thread had nothing to do with this patch
>> series.
>>
>> Could it be that I've got u wrong? If so, could u, pls., clarify what u'd
>> like me to fix in these patches exactly and why?
>>
>> thanks,
>> vlad
> The issues I pointed out are not "nice to have fixed" at all.
> The patchset isn't acceptable if you don't address them.
> Sorry, I don't have the time to go over them again.
> Please just dig them out of the archive.

Let's summarize "the issues u've pointed out" and the explanations why 
they are not relevant to this series for those that haven't been addressed.

 1. "This patch enables bus mastering when MSI or MSI-X interrupts mode
    is used and this would allow bogus user space application trash
    kernel memory".
     1. It's been explained that simple bug in the application that uses
        the newly added interface may not do this. Only specifically
        written user space code that explicitly programs MSI table from
        the user space may cause such harm and this may be done even
        without these patches with the current uio_pci_generic
        implementation.
     2. It's been described how bus mastering and MSI may be enabled
        from the user space right now.
     3. It's been explained how root may trash the kernel memory right
        now even without UIO.
 2. "This patch allows insecure DMA operations since MSI actually
    performs them"
     1. It's been explained that this series adds no additional threat
        to the current driver state since (as described above) this may
        be done without these patches right now.
 3. "Users should not be able to program MSI table from the user space
    therefore it should be zero-mapped".
     1. This may be true but this has nothing to do with this patches
        since they have nothing to do with this ability.
 4. "Why to mimic VFIO SET_IRQ ioctl instead of patching VFIO to be able
    to work in iommu-less mode?"
     1. Because this would break the basic VFIO design assumption and
        would require a lot of non-trivial codding with little-to-none
        added value. See the relevant thread for more detail.
 5. "Separate bars mapping code into a separate patch"
     1. Addressed.

Hope I didn't miss anything.
So, as u may see everything that could be addressed have been addressed 
and the rest are as I've already stated - irrelevant for this patch.

thanks,
vlad

>
>>>> New in v4:
>>>>     - Use portable __u32 and __s32 types from asm/types.h for
>>>>       defining uio_pci_generic_irq_set fields.
>>>>     - Use proper _IO macros for defining read and write ioctl()
>>>>       commands.
>>>>     - Moved bars mapping setting into a separate patch.
>>>>     - Update uio_pci_generic example in uio-howto.tmpl.
>>>>
>>>> New in v3:
>>>>     - Add __iomem qualifier to temp buffer receiving ioremap value.
>>>> New in v2:
>>>>     - Added #include <linux/uaccess.h> to uio_pci_generic.c
>>>>
>>>>
>>>> Vlad Zolotarov (4):
>>>>    uio: add ioctl support
>>>>    uio_pci_generic: properly initialize PCI bars mappings towards UIO
>>>>    uio_pci_generic: add MSI/MSI-X support
>>>>    Documentation: update uio-howto
>>>>
>>>>   Documentation/DocBook/uio-howto.tmpl | 139 ++++++++++--
>>>>   drivers/uio/uio.c                    |  15 ++
>>>>   drivers/uio/uio_pci_generic.c        | 409 +++++++++++++++++++++++++++++++++--
>>>>   include/linux/uio_driver.h           |   3 +
>>>>   include/uapi/linux/uio_pci_generic.h |  51 +++++
>>>>   5 files changed, 574 insertions(+), 43 deletions(-)
>>>>   create mode 100644 include/uapi/linux/uio_pci_generic.h
>>>>
>>>> -- 
>>>> 2.1.0