[PATCH 06/11] virtio: pci: Read entire capability into memory
Bin Meng
bmeng.cn at gmail.com
Fri Mar 25 05:31:43 CET 2022
On Sun, Mar 20, 2022 at 7:42 PM Andrew Scull <ascull at google.com> wrote:
>
> Read the virtio PCI capability out of the device configuration space to
> a struct rather than accessing fields directly from the configuration
> space as they are needed. This both makes access to the fields easier
> and avoids re-reading fields.
>
> Re-reading fields could result in time-of-check to time-of-use problems,
> should the value in the configuration space change. The range check of
> the `bar` field and the later call to `dm_pci_read_bar32()` is an
> example of where this could happen.
I don't see the need to avoid the time-of-check to time-of-use
problems, as it can only happen with the PCI configuration access
capability, which U-Boot driver does not touch.
Am I missing something?
>
> Signed-off-by: Andrew Scull <ascull at google.com>
> ---
> drivers/virtio/virtio_pci_modern.c | 74 ++++++++++++++++--------------
> 1 file changed, 40 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index 4b346be257..38a0da0a84 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -393,15 +393,16 @@ static int virtio_pci_notify(struct udevice *udev, struct virtqueue *vq)
> * @udev: the transport device
> * @cfg_type: the VIRTIO_PCI_CAP_* value we seek
> * @cap_size: expected size of the capability
> + * @cap: capability read from the config space
> *
> * Return: offset of the configuration structure
> */
> static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
> - size_t cap_size)
> + size_t cap_size,
> + struct virtio_pci_cap *cap)
> {
> int pos;
> int offset;
> - u8 type, bar;
>
> if (cap_size < sizeof(struct virtio_pci_cap))
> return 0;
> @@ -409,6 +410,9 @@ static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
> if (cap_size > PCI_CFG_SPACE_SIZE)
> return 0;
>
> + if (!cap)
> + return 0;
> +
> for (pos = dm_pci_find_capability(udev, PCI_CAP_ID_VNDR);
> pos > 0;
> pos = dm_pci_find_next_capability(udev, pos, PCI_CAP_ID_VNDR)) {
> @@ -416,16 +420,26 @@ static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
> if (PCI_CFG_SPACE_SIZE - cap_size < pos)
> return 0;
>
> + offset = pos + offsetof(struct virtio_pci_cap, cap_vndr);
> + dm_pci_read_config8(udev, offset, &cap->cap_vndr);
> + offset = pos + offsetof(struct virtio_pci_cap, cap_next);
> + dm_pci_read_config8(udev, offset, &cap->cap_next);
> + offset = pos + offsetof(struct virtio_pci_cap, cap_len);
> + dm_pci_read_config8(udev, offset, &cap->cap_len);
> offset = pos + offsetof(struct virtio_pci_cap, cfg_type);
> - dm_pci_read_config8(udev, offset, &type);
> + dm_pci_read_config8(udev, offset, &cap->cfg_type);
> offset = pos + offsetof(struct virtio_pci_cap, bar);
> - dm_pci_read_config8(udev, offset, &bar);
> + dm_pci_read_config8(udev, offset, &cap->bar);
> + offset = pos + offsetof(struct virtio_pci_cap, offset);
> + dm_pci_read_config32(udev, offset, &cap->offset);
> + offset = pos + offsetof(struct virtio_pci_cap, length);
> + dm_pci_read_config32(udev, offset, &cap->length);
>
> /* Ignore structures with reserved BAR values */
> - if (bar > 0x5)
> + if (cap->bar > 0x5)
> continue;
>
> - if (type == cfg_type)
> + if (cap->cfg_type == cfg_type)
> return pos;
> }
>
> @@ -436,33 +450,27 @@ static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
> * virtio_pci_map_capability - map base address of the capability
> *
> * @udev: the transport device
> - * @off: offset of the configuration structure
> + * @cap: capability to map
> *
> * Return: base address of the capability
> */
> -static void __iomem *virtio_pci_map_capability(struct udevice *udev, int off)
> +static void __iomem *virtio_pci_map_capability(struct udevice *udev,
> + const struct virtio_pci_cap *cap)
> {
> - u8 bar;
> - u32 offset;
> ulong base;
> void __iomem *p;
>
> - if (!off)
> + if (!cap)
> return NULL;
>
> - offset = off + offsetof(struct virtio_pci_cap, bar);
> - dm_pci_read_config8(udev, offset, &bar);
> - offset = off + offsetof(struct virtio_pci_cap, offset);
> - dm_pci_read_config32(udev, offset, &offset);
> -
> /*
> * TODO: adding 64-bit BAR support
> *
> * Per spec, the BAR is permitted to be either 32-bit or 64-bit.
> * For simplicity, only read the BAR address as 32-bit.
> */
> - base = dm_pci_read_bar32(udev, bar);
> - p = (void __iomem *)base + offset;
> + base = dm_pci_read_bar32(udev, cap->bar);
> + p = (void __iomem *)base + cap->offset;
>
> return p;
> }
> @@ -487,7 +495,7 @@ static int virtio_pci_probe(struct udevice *udev)
> u16 subvendor;
> u8 revision;
> int common, notify, device;
> - u32 common_length;
> + struct virtio_pci_cap common_cap, notify_cap, device_cap;
> int offset;
>
> /* We only own devices >= 0x1040 and <= 0x107f: leave the rest. */
> @@ -504,46 +512,44 @@ static int virtio_pci_probe(struct udevice *udev)
>
> /* Check for a common config: if not, use legacy mode (bar 0) */
> common = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_COMMON_CFG,
> - sizeof(struct virtio_pci_cap));
> + sizeof(struct virtio_pci_cap),
> + &common_cap);
> if (!common) {
> printf("(%s): leaving for legacy driver\n", udev->name);
> return -ENODEV;
> }
>
> - offset = common + offsetof(struct virtio_pci_cap, length);
> - dm_pci_read_config32(udev, offset, &common_length);
> - if (common_length < sizeof(struct virtio_pci_common_cfg)) {
> + if (common_cap.length < sizeof(struct virtio_pci_common_cfg)) {
> printf("(%s): virtio common config too small\n", udev->name);
> return -EINVAL;
> }
>
> /* If common is there, notify should be too */
> notify = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_NOTIFY_CFG,
> - sizeof(struct virtio_pci_notify_cap));
> + sizeof(struct virtio_pci_notify_cap),
> + ¬ify_cap);
> if (!notify) {
> printf("(%s): missing capabilities %i/%i\n", udev->name,
> common, notify);
> return -EINVAL;
> }
>
> - offset = notify + offsetof(struct virtio_pci_cap, length);
> - dm_pci_read_config32(udev, offset, &priv->notify_len);
> + priv->notify_len = notify_cap.length;
>
> /*
> * Device capability is only mandatory for devices that have
> * device-specific configuration.
> */
> device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG,
> - sizeof(struct virtio_pci_cap));
> - if (device) {
> - offset = device + offsetof(struct virtio_pci_cap, length);
> - dm_pci_read_config32(udev, offset, &priv->device_len);
> - }
> + sizeof(struct virtio_pci_cap),
> + &device_cap);
> + if (device)
> + priv->device_len = device_cap.length;
>
> /* Map configuration structures */
> - priv->common = virtio_pci_map_capability(udev, common);
> - priv->notify_base = virtio_pci_map_capability(udev, notify);
> - priv->device = virtio_pci_map_capability(udev, device);
> + priv->common = virtio_pci_map_capability(udev, &common_cap);
> + priv->notify_base = virtio_pci_map_capability(udev, ¬ify_cap);
> + priv->device = virtio_pci_map_capability(udev, &device_cap);
> debug("(%p): common @ %p, notify base @ %p, device @ %p\n",
> udev, priv->common, priv->notify_base, priv->device);
>
> --
Regards,
Bin
More information about the U-Boot
mailing list