[ipxe-devel] [PATCH v2 3/4] [virtio] Add virtio 1.0 PCI support
Ladi Prosek
lprosek at redhat.com
Tue Apr 5 10:43:44 UTC 2016
On Sun, Apr 3, 2016 at 1:04 PM, Michael S. Tsirkin <mst at redhat.com> wrote:
> On Mon, Mar 14, 2016 at 03:48:24PM +0100, Ladi Prosek wrote:
>> This commit adds support for driving virtio 1.0 PCI devices.
>> In addition to various helpers, a number of vpm_ functions are
>> introduced to be used instead of their legacy vp_ counterparts
>> when accessing virtio 1.0 (aka modern) devices.
>>
>> Signed-off-by: Ladi Prosek <lprosek at redhat.com>
>> ---
>> src/drivers/bus/virtio-pci.c | 324 ++++++++++++++++++++++++++++++++++++++++-
>> src/drivers/bus/virtio-ring.c | 15 +-
>> src/drivers/net/virtio-net.c | 4 +-
>> src/include/ipxe/errfile.h | 1 +
>> src/include/ipxe/virtio-pci.h | 147 +++++++++++++++++++
>> src/include/ipxe/virtio-ring.h | 4 +-
>> 6 files changed, 483 insertions(+), 12 deletions(-)
>>
>> diff --git a/src/drivers/bus/virtio-pci.c b/src/drivers/bus/virtio-pci.c
>> index fbef067..d4eb7c0 100644
>> --- a/src/drivers/bus/virtio-pci.c
>> +++ b/src/drivers/bus/virtio-pci.c
>> @@ -11,10 +11,14 @@
>> *
>> */
>>
>> +#include "errno.h"
>> +#include "byteswap.h"
>> #include "etherboot.h"
>> #include "ipxe/io.h"
>> -#include "ipxe/virtio-ring.h"
>> +#include "ipxe/iomap.h"
>> +#include "ipxe/pci.h"
>> #include "ipxe/virtio-pci.h"
>> +#include "ipxe/virtio-ring.h"
>>
>> int vp_find_vq(unsigned int ioaddr, int queue_index,
>> struct vring_virtqueue *vq)
>> @@ -30,19 +34,19 @@ int vp_find_vq(unsigned int ioaddr, int queue_index,
>>
>> num = inw(ioaddr + VIRTIO_PCI_QUEUE_NUM);
>> if (!num) {
>> - printf("ERROR: queue size is 0\n");
>> + DBG("VIRTIO-PCI ERROR: queue size is 0\n");
>> return -1;
>> }
>>
>> if (num > MAX_QUEUE_NUM) {
>> - printf("ERROR: queue size %d > %d\n", num, MAX_QUEUE_NUM);
>> + DBG("VIRTIO-PCI ERROR: queue size %d > %d\n", num, MAX_QUEUE_NUM);
>> return -1;
>> }
>>
>> /* check if the queue is already active */
>>
>> if (inl(ioaddr + VIRTIO_PCI_QUEUE_PFN)) {
>> - printf("ERROR: queue already active\n");
>> + DBG("VIRTIO-PCI ERROR: queue already active\n");
>> return -1;
>> }
>>
>> @@ -62,3 +66,315 @@ int vp_find_vq(unsigned int ioaddr, int queue_index,
>>
>> return num;
>> }
>> +
>> +#define CFG_POS(vdev, field) \
>> + (vdev->cfg_cap_pos + offsetof(struct virtio_pci_cfg_cap, field))
>> +
>> +static void prep_pci_cfg_cap(struct virtio_pci_modern_device *vdev,
>> + struct virtio_pci_region *region,
>> + size_t offset, u32 length)
>> +{
>> + pci_write_config_byte(vdev->pci, CFG_POS(vdev, cap.bar), region->bar);
>> + pci_write_config_dword(vdev->pci, CFG_POS(vdev, cap.length), length);
>> + pci_write_config_dword(vdev->pci, CFG_POS(vdev, cap.offset),
>> + (uint32_t)(region->base + offset));
>> +}
>> +
>> +void vpm_iowrite8(struct virtio_pci_modern_device *vdev,
>> + struct virtio_pci_region *region, u8 data, size_t offset)
>> +{
>> + switch (region->flags & VIRTIO_PCI_REGION_TYPE_MASK) {
>> + case VIRTIO_PCI_REGION_MEMORY:
>> + writeb(data, region->base + offset);
>> + break;
>> + case VIRTIO_PCI_REGION_PORT:
>> + outb(data, region->base + offset);
>> + break;
>> + default: /* VIRTIO_PCI_REGION_PCI_CONFIG */
>
> how about checking the type here, and crashing
> if the type is unexpected?
Sure, will do.
>> + prep_pci_cfg_cap(vdev, region, offset, 1);
>> + pci_write_config_byte(vdev->pci, CFG_POS(vdev, pci_cfg_data), data);
>> + break;
>> + }
>> +}
>> +
>> +void vpm_iowrite16(struct virtio_pci_modern_device *vdev,
>> + struct virtio_pci_region *region, u16 data, size_t offset)
>> +{
>> + data = cpu_to_le16(data);
>> + switch (region->flags & VIRTIO_PCI_REGION_TYPE_MASK) {
>> + case VIRTIO_PCI_REGION_MEMORY:
>> + writew(data, region->base + offset);
>> + break;
>> + case VIRTIO_PCI_REGION_PORT:
>> + outw(data, region->base + offset);
>> + break;
>> + default: /* VIRTIO_PCI_REGION_PCI_CONFIG */
>> + prep_pci_cfg_cap(vdev, region, offset, 2);
>> + pci_write_config_word(vdev->pci, CFG_POS(vdev, pci_cfg_data), data);
>> + break;
>> + }
>> +}
>> +
>> +void vpm_iowrite32(struct virtio_pci_modern_device *vdev,
>> + struct virtio_pci_region *region, u32 data, size_t offset)
>> +{
>> + data = cpu_to_le32(data);
>> + switch (region->flags & VIRTIO_PCI_REGION_TYPE_MASK) {
>> + case VIRTIO_PCI_REGION_MEMORY:
>> + writel(data, region->base + offset);
>> + break;
>> + case VIRTIO_PCI_REGION_PORT:
>> + outl(data, region->base + offset);
>> + break;
>> + default: /* VIRTIO_PCI_REGION_PCI_CONFIG */
>> + prep_pci_cfg_cap(vdev, region, offset, 4);
>> + pci_write_config_dword(vdev->pci, CFG_POS(vdev, pci_cfg_data), data);
>> + break;
>> + }
>> +}
>> +
>> +u8 vpm_ioread8(struct virtio_pci_modern_device *vdev,
>> + struct virtio_pci_region *region, size_t offset)
>> +{
>> + uint8_t data;
>> + switch (region->flags & VIRTIO_PCI_REGION_TYPE_MASK) {
>> + case VIRTIO_PCI_REGION_MEMORY:
>> + data = readb(region->base + offset);
>> + break;
>> + case VIRTIO_PCI_REGION_PORT:
>> + data = inb(region->base + offset);
>> + break;
>> + default: /* VIRTIO_PCI_REGION_PCI_CONFIG */
>> + prep_pci_cfg_cap(vdev, region, offset, 1);
>> + pci_read_config_byte(vdev->pci, CFG_POS(vdev, pci_cfg_data), &data);
>> + break;
>> + }
>> + return data;
>> +}
>> +
>> +u16 vpm_ioread16(struct virtio_pci_modern_device *vdev,
>> + struct virtio_pci_region *region, size_t offset)
>> +{
>> + uint16_t data;
>> + switch (region->flags & VIRTIO_PCI_REGION_TYPE_MASK) {
>> + case VIRTIO_PCI_REGION_MEMORY:
>> + data = readw(region->base + offset);
>> + break;
>> + case VIRTIO_PCI_REGION_PORT:
>> + data = inw(region->base + offset);
>> + break;
>> + default: /* VIRTIO_PCI_REGION_PCI_CONFIG */
>> + prep_pci_cfg_cap(vdev, region, offset, 2);
>> + pci_read_config_word(vdev->pci, CFG_POS(vdev, pci_cfg_data), &data);
>> + break;
>> + }
>> + return le16_to_cpu(data);
>> +}
>> +
>> +u32 vpm_ioread32(struct virtio_pci_modern_device *vdev,
>> + struct virtio_pci_region *region, size_t offset)
>> +{
>> + uint32_t data;
>> + switch (region->flags & VIRTIO_PCI_REGION_TYPE_MASK) {
>> + case VIRTIO_PCI_REGION_MEMORY:
>> + data = readw(region->base + offset);
>> + break;
>> + case VIRTIO_PCI_REGION_PORT:
>> + data = inw(region->base + offset);
>> + break;
>> + default: /* VIRTIO_PCI_REGION_PCI_CONFIG */
>> + prep_pci_cfg_cap(vdev, region, offset, 4);
>> + pci_read_config_dword(vdev->pci, CFG_POS(vdev, pci_cfg_data), &data);
>> + break;
>> + }
>> + return le32_to_cpu(data);
>> +}
>> +
>> +int virtio_pci_find_capability(struct pci_device *pci, uint8_t cfg_type)
>> +{
>> + int pos;
>> + uint8_t type, bar;
>> +
>> + for (pos = pci_find_capability(pci, PCI_CAP_ID_VNDR);
>> + pos > 0;
>> + pos = pci_find_next_capability(pci, pos, PCI_CAP_ID_VNDR)) {
>> +
>> + pci_read_config_byte(pci, pos + offsetof(struct virtio_pci_cap,
>> + cfg_type), &type);
>> + pci_read_config_byte(pci, pos + offsetof(struct virtio_pci_cap,
>> + bar), &bar);
>> +
>> + /* Ignore structures with reserved BAR values */
>> + if (bar > 0x5) {
>> + continue;
>> + }
>> +
>> + if (type == cfg_type) {
>> + return pos;
>> + }
>> + }
>> + return 0;
>> +}
>> +
>> +int virtio_pci_map_capability(struct pci_device *pci, int cap, size_t minlen,
>> + u32 align, u32 start, u32 size,
>> + struct virtio_pci_region *region)
>> +{
>> + u8 bar;
>> + u32 offset, length, base_raw;
>> + unsigned long base;
>> +
>> + pci_read_config_byte(pci, cap + offsetof(struct virtio_pci_cap, bar), &bar);
>> + pci_read_config_dword(pci, cap + offsetof(struct virtio_pci_cap, offset),
>> + &offset);
>> + pci_read_config_dword(pci, cap + offsetof(struct virtio_pci_cap, length),
>> + &length);
>> +
>> + if (length <= start) {
>> + DBG("VIRTIO-PCI bad capability len %u (>%u expected)\n", length, start);
>> + return -EINVAL;
>> + }
>> + if (length - start < minlen) {
>> + DBG("VIRTIO-PCI bad capability len %u (>=%zu expected)\n", length, minlen);
>> + return -EINVAL;
>> + }
>> + length -= start;
>> + if (start + offset < offset) {
>> + DBG("VIRTIO-PCI map wrap-around %u+%u\n", start, offset);
>> + return -EINVAL;
>> + }
>> + offset += start;
>> + if (offset & (align - 1)) {
>> + DBG("VIRTIO-PCI offset %u not aligned to %u\n", offset, align);
>> + return -EINVAL;
>> + }
>> + if (length > size) {
>> + length = size;
>> + }
>> +
>> + if (minlen + offset < minlen ||
>> + minlen + offset > pci_bar_size(pci, PCI_BASE_ADDRESS(bar))) {
>> + DBG("VIRTIO-PCI map virtio %zu@%u out of range on bar %i length %lu\n",
>> + minlen, offset,
>> + bar, (unsigned long)pci_bar_size(pci, PCI_BASE_ADDRESS(bar)));
>> + return -EINVAL;
>> + }
>> +
>> + region->base = NULL;
>> + region->length = length;
>> + region->bar = bar;
>> +
>> + base = pci_bar_start(pci, PCI_BASE_ADDRESS(bar));
>> + if (base) {
>> + pci_read_config_dword(pci, PCI_BASE_ADDRESS(bar), &base_raw);
>> +
>> + if (base_raw & PCI_BASE_ADDRESS_SPACE_IO) {
>> + /* Region accessed using port I/O */
>> + region->base = (void *)(base + offset);
>> + region->flags = VIRTIO_PCI_REGION_PORT;
>> + } else {
>> + /* Region mapped into memory space */
>> + region->base = ioremap(base + offset, length);
>> + region->flags = VIRTIO_PCI_REGION_MEMORY;
>> + }
>> + }
>
> So there's implicit logic here that handles BAR = 0 as disabled
> by BIOS. I think it would be cleaner not to enable IO or memory
> in that case: set a flag and then use it later in adjust_pci.
Will do.
>> + if (!region->base) {
>
> Probably "else" would be cleaner here.
I'm intentionally retesting here because region->base is written to
under the if and I want to handle the case where ioremap failed.
>> + /* Region accessed via PCI config space window */
>> + region->base = (void *)offset;
>> + region->flags = VIRTIO_PCI_REGION_PCI_CONFIG;
>> + }
>> + return 0;
>> +}
More information about the ipxe-devel
mailing list