Date: Wed, 3 May 2023 07:21:47 -0700
From: Shannon Nelson <shannon.nelson@....com>
To: Simon Horman <simon.horman@...igine.com>
Cc: jasowang@...hat.com, mst@...hat.com,
 virtualization@...ts.linux-foundation.org, brett.creeley@....com,
 netdev@...r.kernel.org, drivers@...sando.io
Subject: Re: [PATCH v4 virtio 01/10] virtio: allow caller to override device
 id and DMA mask

On 5/1/23 7:44 AM, Simon Horman wrote:
> 
> 
> On Tue, Apr 25, 2023 at 02:25:53PM -0700, Shannon Nelson wrote:
>> To add a bit of flexibility with various virtio based devices, allow
>> the caller to specify a different device id and DMA mask.  This adds
>> fields to struct virtio_pci_modern_device to specify an override device
>> id check and a DMA mask.
>>
>> int (*device_id_check)(struct pci_dev *pdev);
>>        If defined by the driver, this function will be called to check
>>        that the PCI device is the vendor's expected device, and will
>>        return the found device id to be stored in mdev->id.device.
>>        This allows vendors with alternative vendor device ids to use
>>        this library on their own device BAR.
>>
>> u64 dma_mask;
>>        If defined by the driver, this mask will be used in a call to
>>        dma_set_mask_and_coherent() instead of the traditional
>>        DMA_BIT_MASK(64).  This allows limiting the DMA space on
>>        vendor devices with address limitations.
> 
> Hi Shannon,
> 
> I don't feel strongly about this, but as there are two new features,
> perhaps it would appropriate to have two patches.

Sure, I can respin and split these out to separate patches, and I'll 
keep your verbosity notes below in mind :-).  Yes, the kdoc would be a 
good thing, but I'd like to keep the mission-creep to a minimum and come 
back to that one separately.

sln

> 
>> Signed-off-by: Shannon Nelson <shannon.nelson@....com>
>> ---
>>   drivers/virtio/virtio_pci_modern_dev.c | 37 +++++++++++++++++---------
>>   include/linux/virtio_pci_modern.h      |  6 +++++
>>   2 files changed, 31 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
>> index 869cb46bef96..1f2db76e8f91 100644
>> --- a/drivers/virtio/virtio_pci_modern_dev.c
>> +++ b/drivers/virtio/virtio_pci_modern_dev.c
>> @@ -218,21 +218,29 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
>>        int err, common, isr, notify, device;
>>        u32 notify_length;
>>        u32 notify_offset;
>> +     int devid;
>>
>>        check_offsets();
>>
>> -     /* We only own devices >= 0x1000 and <= 0x107f: leave the rest. */
>> -     if (pci_dev->device < 0x1000 || pci_dev->device > 0x107f)
>> -             return -ENODEV;
>> -
>> -     if (pci_dev->device < 0x1040) {
>> -             /* Transitional devices: use the PCI subsystem device id as
>> -              * virtio device id, same as legacy driver always did.
>> -              */
>> -             mdev->id.device = pci_dev->subsystem_device;
>> +     if (mdev->device_id_check) {
>> +             devid = mdev->device_id_check(pci_dev);
>> +             if (devid < 0)
>> +                     return devid;
>> +             mdev->id.device = devid;
>>        } else {
>> -             /* Modern devices: simply use PCI device id, but start from 0x1040. */
>> -             mdev->id.device = pci_dev->device - 0x1040;
>> +             /* We only own devices >= 0x1000 and <= 0x107f: leave the rest. */
>> +             if (pci_dev->device < 0x1000 || pci_dev->device > 0x107f)
>> +                     return -ENODEV;
>> +
>> +             if (pci_dev->device < 0x1040) {
>> +                     /* Transitional devices: use the PCI subsystem device id as
>> +                      * virtio device id, same as legacy driver always did.
>> +                      */
>> +                     mdev->id.device = pci_dev->subsystem_device;
>> +             } else {
>> +                     /* Modern devices: simply use PCI device id, but start from 0x1040. */
>> +                     mdev->id.device = pci_dev->device - 0x1040;
>> +             }
>>        }
>>        mdev->id.vendor = pci_dev->subsystem_vendor;
>>
> 
> The diff above is verbose, but looks good to me :)
> 
>> @@ -260,7 +268,12 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
>>                return -EINVAL;
>>        }
>>
>> -     err = dma_set_mask_and_coherent(&pci_dev->dev, DMA_BIT_MASK(64));
>> +     if (mdev->dma_mask)
>> +             err = dma_set_mask_and_coherent(&pci_dev->dev,
>> +                                             mdev->dma_mask);
>> +     else
>> +             err = dma_set_mask_and_coherent(&pci_dev->dev,
>> +                                             DMA_BIT_MASK(64));
> 
> Maybe it is nicer to avoid duplicating the function call, something like
> this:
> 
>          u64 dma_mask;
>          ...
> 
>          dma_mask = mdev->dma_mask ? : DMA_BIT_MASK(64);
>          err = dma_set_mask_and_coherent(&pci_dev->dev, dma_mask);
> 
> or, without a local variable.
> 
>          err = dma_set_mask_and_coherent(&pci_dev->dev,
>                                          mdev->dma_mask ? : DMA_BIT_MASK(64));
> 
>>        if (err)
>>                err = dma_set_mask_and_coherent(&pci_dev->dev,
>>                                                DMA_BIT_MASK(32));
>> diff --git a/include/linux/virtio_pci_modern.h b/include/linux/virtio_pci_modern.h
>> index c4eeb79b0139..067ac1d789bc 100644
>> --- a/include/linux/virtio_pci_modern.h
>> +++ b/include/linux/virtio_pci_modern.h
> 
> Maybe it would be good to add kdoc for struct virtio_pci_modern_device
> at some point.
> 
>> @@ -38,6 +38,12 @@ struct virtio_pci_modern_device {
>>        int modern_bars;
>>
>>        struct virtio_device_id id;
>> +
>> +     /* optional check for vendor virtio device, returns dev_id or -ERRNO */
>> +     int (*device_id_check)(struct pci_dev *pdev);
>> +
>> +     /* optional mask for devices with limited DMA space */
>> +     u64 dma_mask;
>>   };
>>
>>   /*
>> --
>> 2.17.1
>>

Powered by blists - more mailing lists