Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 8/9] vfio/pci: use x86 naming instead of igd
To:     Cornelia Huck <cohuck@redhat.com>,
        Max Gurtovoy <mgurtovoy@nvidia.com>
Cc:     jgg@nvidia.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        alex.williamson@redhat.com, liranl@nvidia.com, oren@nvidia.com,
        tzahio@nvidia.com, leonro@nvidia.com, yarong@nvidia.com,
        aviadye@nvidia.com, shahafs@nvidia.com, artemp@nvidia.com,
        kwankhede@nvidia.com, ACurrid@nvidia.com, gmataev@nvidia.com,
        cjia@nvidia.com, yishaih@nvidia.com, aik@ozlabs.ru
References: <20210201162828.5938-1-mgurtovoy@nvidia.com>
 <20210201162828.5938-9-mgurtovoy@nvidia.com>
 <20210201181454.22112b57.cohuck@redhat.com>
From:   Matthew Rosato <mjrosato@linux.ibm.com>
Message-ID: <599c6452-8ba6-a00a-65e7-0167f21eac35@linux.ibm.com>
Date:   Mon, 1 Feb 2021 12:49:12 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.1
MIME-Version: 1.0
In-Reply-To: <20210201181454.22112b57.cohuck@redhat.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2/1/21 12:14 PM, Cornelia Huck wrote:
> On Mon, 1 Feb 2021 16:28:27 +0000
> Max Gurtovoy <mgurtovoy@nvidia.com> wrote:
> 
>> This patch doesn't change any logic but only align to the concept of
>> vfio_pci_core extensions. Extensions that are related to a platform
>> and not to a specific vendor of PCI devices should be part of the core
>> driver. Extensions that are specific for PCI device vendor should go
>> to a dedicated vendor vfio-pci driver.
> 
> My understanding is that igd means support for Intel graphics, i.e. a
> strict subset of x86. If there are other future extensions that e.g.
> only make sense for some devices found only on AMD systems, I don't
> think they should all be included under the same x86 umbrella.
> 
> Similar reasoning for nvlink, that only seems to cover support for some
> GPUs under Power, and is not a general platform-specific extension IIUC.
> 
> We can arguably do the zdev -> s390 rename (as zpci appears only on
> s390, and all PCI devices will be zpci on that platform), although I'm
> not sure about the benefit.

As far as I can tell, there isn't any benefit for s390 it's just 
"re-branding" to match the platform name rather than the zdev moniker, 
which admittedly perhaps makes it more clear to someone outside of s390 
that any PCI device on s390 is a zdev/zpci type, and thus will use this 
extension to vfio_pci(_core).  This would still be true even if we added 
something later that builds atop it (e.g. a platform-specific device 
like ism-vfio-pci).  Or for that matter, mlx5 via vfio-pci on s390x uses 
these zdev extensions today and would need to continue using them in a 
world where mlx5-vfio-pci.ko exists.

I guess all that to say: if such a rename matches the 'grand scheme' of 
this design where we treat arch-level extensions to vfio_pci(_core) as 
"vfio_pci_(arch)" then I'm not particularly opposed to the rename.  But 
by itself it's not very exciting :)

> 
>>
>> For now, x86 extensions will include only igd.
>>
>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>> ---
>>   drivers/vfio/pci/Kconfig                            | 13 ++++++-------
>>   drivers/vfio/pci/Makefile                           |  2 +-
>>   drivers/vfio/pci/vfio_pci_core.c                    |  2 +-
>>   drivers/vfio/pci/vfio_pci_private.h                 |  2 +-
>>   drivers/vfio/pci/{vfio_pci_igd.c => vfio_pci_x86.c} |  0
>>   5 files changed, 9 insertions(+), 10 deletions(-)
>>   rename drivers/vfio/pci/{vfio_pci_igd.c => vfio_pci_x86.c} (100%)
> 
> (...)
> 
>> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
>> index c559027def2d..e0e258c37fb5 100644
>> --- a/drivers/vfio/pci/vfio_pci_core.c
>> +++ b/drivers/vfio/pci/vfio_pci_core.c
>> @@ -328,7 +328,7 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
>>   
>>   	if (vfio_pci_is_vga(pdev) &&
>>   	    pdev->vendor == PCI_VENDOR_ID_INTEL &&
>> -	    IS_ENABLED(CONFIG_VFIO_PCI_IGD)) {
>> +	    IS_ENABLED(CONFIG_VFIO_PCI_X86)) {
>>   		ret = vfio_pci_igd_init(vdev);
> 
> This one explicitly checks for Intel devices, so I'm not sure why you
> want to generalize this to x86?
> 
>>   		if (ret && ret != -ENODEV) {
>>   			pci_warn(pdev, "Failed to setup Intel IGD regions\n");
>