Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752903AbcJUDBL (ORCPT ); Thu, 20 Oct 2016 23:01:11 -0400 Received: from hqemgate14.nvidia.com ([216.228.121.143]:17348 "EHLO hqemgate14.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752263AbcJUDBI (ORCPT ); Thu, 20 Oct 2016 23:01:08 -0400 X-PGP-Universal: processed; by hqpgpgate101.nvidia.com on Thu, 20 Oct 2016 20:01:06 -0700 Subject: Re: [PATCH v9 10/12] vfio: Add function to get device_api string from vfio_device_info.flags To: Alex Williamson References: <1476739332-4911-1-git-send-email-kwankhede@nvidia.com> <1476739332-4911-11-git-send-email-kwankhede@nvidia.com> <20161020133403.08828f47@t450s.home> <20161020150501.39f28c7e@t450s.home> <6687c068-33f0-c437-3106-bbd1d22f4d81@nvidia.com> <20161020152220.57f6ee06@t450s.home> CC: , , , , , , , , X-Nvconfidentiality: public From: Kirti Wankhede Message-ID: <3a51ce1c-1396-676f-3a9b-9faa8390c632@nvidia.com> Date: Fri, 21 Oct 2016 08:30:53 +0530 MIME-Version: 1.0 In-Reply-To: <20161020152220.57f6ee06@t450s.home> X-Originating-IP: [10.24.71.144] X-ClientProxiedBy: DRBGMAIL102.nvidia.com (10.18.16.21) To bgmail102.nvidia.com (10.25.59.11) Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2510 Lines: 72 On 10/21/2016 2:52 AM, Alex Williamson wrote: > On Fri, 21 Oct 2016 02:44:37 +0530 > Kirti Wankhede 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