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
---
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 {
in
Re: [PATCH v4 virtio 01/10] virtio: allow caller to override device id and DMA mask
On 4/25/23 7:09 PM, Xuan Zhuo wrote:
On Tue, 25 Apr 2023 14:25:53 -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.
Signed-off-by: Shannon Nelson
---
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;
I would want to know is there any other reason to return the errno?
How about return -ENODEV directly?
Because if device_id_check() is returning an errno, it is trying to
communicate some information about what went wrong, and I really get
annoyed when an intermediate layer stomps on the value and makes that
potentially useful information disappear.
sln
Thanks.
+ 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;
@@ -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));
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
@@ -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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 virtio 01/10] virtio: allow caller to override device id and DMA mask
On Tue, 25 Apr 2023 14:25:53 -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.
>
> Signed-off-by: Shannon Nelson
> ---
> 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;
I would want to know is there any other reason to return the errno?
How about return -ENODEV directly?
Thanks.
> + 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;
>
> @@ -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));
> 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
> @@ -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
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
