| 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
| ||
|
Message-ID: <08305e99-022a-7e24-ee17-10d93b876908@amd.com>
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