[SLOF] [PATCH v1 07/27] virtio: add virtio 1.0 capability parsing

Nikunj A Dadhania nikunj at linux.vnet.ibm.com
Mon Jan 18 15:31:42 AEDT 2016


Alexey Kardashevskiy <aik at ozlabs.ru> writes:

> On 01/14/2016 09:45 PM, Nikunj A Dadhania wrote:
>> Alexey Kardashevskiy <aik at ozlabs.ru> writes:
>>
>>> On 01/13/2016 10:16 PM, Nikunj A Dadhania wrote:
>>>> Signed-off-by: Nikunj A Dadhania <nikunj at linux.vnet.ibm.com>
>>>> ---
>>>>    lib/libvirtio/virtio.c | 135 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    lib/libvirtio/virtio.h |   3 +-
>>>>    2 files changed, 137 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/libvirtio/virtio.c b/lib/libvirtio/virtio.c
>>>> index 7e0d6a3..bcf050b 100644
>>>> --- a/lib/libvirtio/virtio.c
>>>> +++ b/lib/libvirtio/virtio.c
>>>> @@ -14,6 +14,7 @@
>>>>    #include <stdbool.h>
>>>>    #include <stdlib.h>
>>>>    #include <string.h>
>>>> +#include <stddef.h>
>>>>    #include <cpu.h>
>>>>    #include <cache.h>
>>>>    #include <byteorder.h>
>>>> @@ -31,6 +32,140 @@
>>>>    #define VIRTIOHDR_ISR_STATUS		19
>>>>    #define VIRTIOHDR_DEVICE_CONFIG 	20
>>>>
>>>> +/* PCI defines */
>>>> +#define PCI_BASE_ADDR_SPACE_IO	0x01
>>>> +#define PCI_BASE_ADDR_SPACE_MEM	0x00
>>>> +#define PCI_BASE_ADDR_MEM_MASK	(~0x0fUL)
>>>> +#define PCI_BASE_ADDR_IO_MASK	(~0x03UL)
>>>> +
>>>> +#define PCI_BASE_ADDR_REG_0	0x10
>>>> +#define PCI_CONFIG_CAP_REG	0x34
>>>> +
>>>> +#define PCI_CAP_ID_VNDR		0x9
>>>> +
>>>> +#define VIRTIO_PCI_CAP_COMMON 1
>>>> +#define VIRTIO_PCI_CAP_NOTIFY 2
>>>> +#define VIRTIO_PCI_CAP_ISR    3
>>>> +#define VIRTIO_PCI_CAP_DEVICE 4
>>>> +#define VIRTIO_PCI_CAP_PCI    5
>>>> +
>>>> +#define VIRTIO_CAP_VNDR     0
>>>> +#define VIRTIO_CAP_NEXT     1
>>>> +#define VIRTIO_CAP_LEN      2
>>>> +#define VIRTIO_CAP_CFG_TYPE 3
>>>> +#define VIRTIO_CAP_BAR      4
>>>> +#define VIRTIO_CAP_OFFSET   8
>>>> +#define VIRTIO_CAP_LENGTH  12
>>>
>>>
>>> VIRTIO_CAP_LEN and VIRTIO_CAP_LENGTH - how are they different? I could not
>>> figure out from the code as they are not used at all.
>>
>> VIRTIO_CAP_LEN -    Capability length
>> VIRTIO_CAP_LENGTH - Length of the virtio structure pointed by
>> "addr[bar]+ offset"
>
>
> Are these the same as VIRTIO_PCI_CAP_xxx from the kernel's 
> include/uapi/linux/virtio_pci.h ? Why change their names?
>
> Same comment about the entire "PCI defines" - use the same macro names, 
> easier to grep in the kernel tree.

I will check this had adjust accordingly in my next revision.

>
>
>
>>  From the virtio 1.0 spec
>>
>> struct virtio_pci_cap {
>> u8 cap_vndr; /* Generic PCI field: PCI_CAP_ID_VNDR */
>> u8 cap_next; /* Generic PCI field: next ptr. */
>> u8 cap_len;  /* Generic PCI field: capability length */
>> u8 cfg_type; /* Identifies the structure. */
>> u8 bar;      /* Where to find it. */
>> u8 padding[3]; /* Pad to full dword. */
>> le32 offset;   /* Offset within the bar. */
>> le32 length; /* Length of the structure, in bytes. */
>> };
>>
>>>
>>>
>>>> +
>>>> +#define le64 uint64_t
>>>> +#define le32 uint32_t
>>>> +#define le16 uint16_t
>>>
>>>
>>> I'd use:
>>>
>>> typedef __u16 __bitwise __le16;
>>> typedef __u16 __bitwise __be16;
>>> typedef __u32 __bitwise __le32;
>>> typedef __u32 __bitwise __be32;
>>> typedef __u64 __bitwise __le64;
>>> typedef __u64 __bitwise __be64;
>>>
>>> as include/uapi/linux/types.h does in the kernel.
>>
>> Sure. I can take that.
>>
>>>
>>>> +
>>>> +struct virtio_dev_common {
>>>> +	le32 dev_features_sel;
>>>> +	le32 dev_features;
>>>> +	le32 drv_features_sel;
>>>> +	le32 drv_features;
>>>> +	le16 msix_config;
>>>> +	le16 num_queues;
>>>> +	uint8_t dev_status;
>>>> +	uint8_t cfg_generation;
>>>> +
>>>> +	le16 q_select;
>>>> +	le16 q_size;
>>>> +	le16 q_msix_vec;
>>>> +	le16 q_enable;
>>>> +	le16 q_notify_off;
>>>> +	le64 q_desc;
>>>> +	le64 q_avail;
>>>> +	le64 q_used;
>>>> +} __attribute__ ((packed));
>>>> +
>>>> +static void virtio_process_cap(struct virtio_device *dev, uint8_t cap_ptr)
>>>> +{
>>>> +	struct virtio_cap *cap;
>>>> +	uint8_t cfg_type, bar;
>>>> +	uint32_t offset;
>>>> +	uint64_t addr = 0;
>>>> +
>>>> +	cfg_type = SLOF_pci_config_read8(cap_ptr + VIRTIO_CAP_CFG_TYPE);
>>>> +	bar = SLOF_pci_config_read8(cap_ptr + VIRTIO_CAP_BAR);
>>>> +	offset = SLOF_pci_config_read32(cap_ptr + VIRTIO_CAP_OFFSET);
>>>> +
>>>> +	switch(cfg_type) {
>>>> +	case VIRTIO_PCI_CAP_COMMON:
>>>> +		cap = &dev->common;
>>>> +		break;
>>>> +	case VIRTIO_PCI_CAP_NOTIFY:
>>>> +		cap = &dev->notify;
>>>> +		dev->notify_off_mul = SLOF_pci_config_read32(cap_ptr + sizeof(struct virtio_cap));
>>>> +		break;
>>>> +	case VIRTIO_PCI_CAP_ISR:
>>>> +		cap = &dev->isr;
>>>> +		break;
>>>> +	case VIRTIO_PCI_CAP_DEVICE:
>>>> +		cap = &dev->device;
>>>> +		break;
>>>> +	default:
>>>> +		cap = NULL;
>>>> +	}
>>>> +
>>>> +	if (cap && cfg_type) {
>>>> +		cap->bar = bar;
>>>> +		addr = SLOF_pci_config_read32(PCI_BASE_ADDR_REG_0 + 4 * cap->bar);
>>>> +		if (addr & PCI_BASE_ADDR_SPACE_IO) {
>>>> +			cap->is_io = 1;
>>>> +			addr = addr & PCI_BASE_ADDR_IO_MASK;
>>>> +		} else if (addr & 4) {
>>>> +			addr = SLOF_pci_config_read32(PCI_BASE_ADDR_REG_0 + 4 * cap->bar) |
>>>> +				(SLOF_pci_config_read32(PCI_BASE_ADDR_REG_0 + 4 * (cap->bar + 1)) << 32);
>>>> +			addr = (uint64_t)SLOF_translate_my_address((void *)addr);
>>>> +			addr = addr & PCI_BASE_ADDR_MEM_MASK;
>>>> +		}
>>>> +		else {
>>>> +			cap->is_io = 0;
>>>> +			addr = addr & PCI_BASE_ADDR_MEM_MASK;
>>>> +		}
>>>> +		cap->addr = (void *)addr + offset;
>>>> +		cap->cap_id = cfg_type;
>>>> +	}
>>>> +	return;
>>>> +}
>>>> +/**
>>>> + * Setup virtio device details, gets called from SLOF routines
>>>> + * Determines legacy or modern device.
>>>> + */
>>>> +void virtio_setup_device(struct virtio_device *dev)
>>>
>>>
>>> s/virtio_setup_device/virtio_parse_capabilities/ ? This function does not
>>> set up anything...w
>>
>> Sure.
>>
>>>
>>>
>>>> +{
>>>> +	uint8_t cap_ptr, cap_vndr;
>>>> +	uint64_t addr = 0;
>>>> +
>>>> +	cap_ptr = SLOF_pci_config_read8(PCI_CONFIG_CAP_REG);
>>>> +	while (cap_ptr != 0)
>>>> +	{
>>>> +		cap_vndr = SLOF_pci_config_read8(cap_ptr + VIRTIO_CAP_VNDR);
>>>> +		if (cap_vndr == PCI_CAP_ID_VNDR)
>>>> +			virtio_process_cap(dev, cap_ptr);
>>>> +		cap_ptr = SLOF_pci_config_read8(cap_ptr+VIRTIO_CAP_NEXT);
>>>> +	}
>>>> +	if (dev->common.cap_id && dev->notify.cap_id && dev->isr.cap_id && dev->device.cap_id)
>>>> +		dev->is_modern = 1;
>>>> +	else {
>>>> +		dev->is_modern = 0;
>>>> +		dev->legacy.cap_id = 0;
>>>> +		dev->legacy.bar = 0;
>>>> +		addr = SLOF_pci_config_read32(PCI_BASE_ADDR_REG_0);
>>>> +		if (addr & PCI_BASE_ADDR_SPACE_IO) {
>>>> +			dev->legacy.is_io = 1;
>>>> +			addr = addr & PCI_BASE_ADDR_IO_MASK;
>>>> +		} else {
>>>> +			dev->legacy.is_io = 0;
>>>> +			addr = addr & PCI_BASE_ADDR_MEM_MASK;
>>>> +		}
>>>> +		dev->legacy.addr = (void *)addr;
>>>> +	}
>>>> +	return;
>>>> +}
>>>>
>>>>    /**
>>>>     * Calculate ring size according to queue size number
>>>> diff --git a/lib/libvirtio/virtio.h b/lib/libvirtio/virtio.h
>>>> index 56e044b..eee3144 100644
>>>> --- a/lib/libvirtio/virtio.h
>>>> +++ b/lib/libvirtio/virtio.h
>>>> @@ -74,7 +74,7 @@ struct vring_used {
>>>>
>>>>    /* Structure shared with SLOF and is 16bytes */
>>>>    struct virtio_cap {
>>>> -	uint64_t addr;
>>>> +	void *addr;
>>>>    	uint8_t bar;
>>>>    	uint8_t is_io;
>>>>    	uint8_t cap_id;
>>>> @@ -116,6 +116,7 @@ extern void virtio_fill_desc(struct vring_desc *desc,
>>>>                                 uint16_t flags, uint16_t next);
>>>>    extern int virtio_queue_init_vq(struct virtio_device *dev, struct vqs *vq, unsigned int id);
>>>>
>>>> +extern void virtio_setup_device(struct virtio_device *dev);
>>>>    extern void virtio_reset_device(struct virtio_device *dev);
>>>>    extern void virtio_queue_notify(struct virtio_device *dev, int queue);
>>>>    extern void virtio_set_status(struct virtio_device *dev, int status);
>>>>
>>>
>>>
>>> The whole patch seems to belong to 18/27.
>>
>> why ?
>
>
> Because this is where this new code gets used. Before 18/27, this one is 
> not needed, used. When I was reading 18/27, I had to search for this one in 
> order to understand what this new virtio_setup_device() actually does.

I am changing all the virtio apis after this patch, which refers
is_modern and the capability structures. How can i move them to 18/27.
All the 13 patches after this one are making changes to the api and
immediately after that is the 18/27 which is using all the added api. I
cannot change one api and use it immediately.

>
>
>
> -- 
> Alexey



More information about the SLOF mailing list