VirtIO-net driver cannot send packets

[mariuszp]

I am trying to write a VirtIO-net driver (testing under VirtualBox). I found different versions of the specification and they were a bit unclear about the constants (especially in bit fields), sometimes indicating their values at the bit number, and sometimes as the value of the mask, without indicating which one it is. The specification inside the Linux kernel seems to always use bit numbers though. Just in case, let me drop my defines here:

Code: Select all

#define VIONET_DESC_F_NEXT       	1
#define VIONET_DESC_F_WRITE      	2

#define	VIONET_REG_QADDR		0x08
#define	VIONET_REG_QSIZE		0x0C
#define	VIONET_REG_QSEL			0x0E
#define	VIONET_REG_DEVST		0x12
#define	VIONET_REG_QNOTF		0x10

#define VIONET_ALIGN(x) (((x) + 4095) & ~4095)

#define	VIONET_DEVST_ACK		(1 << 1)
#define	VIONET_DEVST_DRV		(1 << 2)
#define	VIONET_DEVST_OK			(1 << 3)
#define	VIONET_DEVST_FAIL		(1 << 8)

typedef struct
{
	uint64_t			phaddr;
	uint32_t			len;
	uint16_t			flags;
	uint16_t			next;
} VionetBufferDesc;

typedef struct
{
	uint32_t			index;
	uint32_t			len;
} VionetUsedElem;

typedef struct
{
	uint16_t			flags;
	uint16_t			index;
	uint16_t			ring[];
	/* after the ring: uint16_t intIndex */
} VionetAvail;

typedef struct
{
	uint16_t			flags;
	uint16_t			index;
	VionetUsedElem			ring[];
	/* after the ring: uint16_t intIndex */
} VionetUsed;

typedef struct
{
	uint16_t			count;
	VionetBufferDesc*		bufDesc;
	VionetAvail*			avail;
	VionetUsed*			used;
	uint16_t*			availIntIndex;
	uint16_t*			usedIntIndex;
} VionetQueue;

typedef struct
{
	uint8_t				flags;
	uint8_t				seg;
	uint16_t			headlen;
	uint16_t			seglen;
	uint16_t			cstart;
	uint16_t			coff;
	uint16_t			bufcount;
} VionetPacketHeader;

static void vionet_qinit(VionetQueue *q, uint16_t count, void *p)
{
	uint64_t addr = (uint64_t) p;
	q->count = count;
	q->bufDesc = (VionetBufferDesc*) addr;
	q->avail = (VionetAvail*) (addr + sizeof(VionetBufferDesc) * count);
	q->used = (VionetUsed*) VIONET_ALIGN(((uint64_t) &q->avail->ring[count]));
};

static inline size_t vionet_qdsize(uint16_t qsz)
{ 
	return VIONET_ALIGN(sizeof(VionetBufferDesc)*qsz + sizeof(uint16_t)*(2 + qsz)) + VIONET_ALIGN(sizeof(VionetUsedElem)*qsz);
}

To the best of my knowledge, the #defines, the structs and the vionet_qdsize() and vionet_qinit() functions work correctly (they match with the examples in the specification). I initialize the device as follows:

Code: Select all

MODULE_INIT(const char *opt)
{
	kprintf("vionet: enumerating virtio-net devices\n");
	pciEnumDevices(THIS_MODULE, vionet_enumerator, NULL);
	
	kprintf("vionet: creating network interfaces\n");
	VionetInterface *nif;
	for (nif=interfaces; nif!=NULL; nif=nif->next)
	{
		NetIfConfig ifconfig;
		memset(&ifconfig, 0, sizeof(NetIfConfig));
		ifconfig.ethernet.type = IF_ETHERNET;
		ifconfig.ethernet.send = vionet_send;
		
		int i;
		for (i=0; i<6; i++)
		{
			ifconfig.ethernet.mac.addr[i] = inb(nif->iobase + 0x14 + i);
		};
		
		pciSetBusMastering(nif->pcidev, 1);
		
		outw(nif->iobase + VIONET_REG_DEVST, 0);
		outw(nif->iobase + VIONET_REG_DEVST, VIONET_DEVST_ACK);
		outw(nif->iobase + VIONET_REG_DEVST, VIONET_DEVST_ACK | VIONET_DEVST_DRV);
		
		// find out the size of RX and TX queues.
		outw(nif->iobase + VIONET_REG_QSEL, 0);
		uint16_t rxqsize = inw(nif->iobase + VIONET_REG_QSIZE);
		outw(nif->iobase + VIONET_REG_QSEL, 1);
		uint16_t txqsize = inw(nif->iobase + VIONET_REG_QSIZE);
		
		if ((rxqsize < 4) || (txqsize < 4))
		{
			panic("vionet: at least 4 RX and 4 TX buffers must be available!");
		};
		
		size_t rxqdsize = vionet_qdsize(rxqsize);
		size_t txqdsize = vionet_qdsize(txqsize);
		
		// allocate the queues as DMA buffers
		int errnum;
		if ((errnum = dmaCreateBuffer(&nif->dmaRX, rxqdsize, DMA_32BIT)) != 0)
		{
			panic("vionet: failed to allocate RX queue! (%d)", errnum);
		};
		
		if ((errnum = dmaCreateBuffer(&nif->dmaTX, txqdsize, DMA_32BIT)) != 0)
		{
			panic("vionet: failed to allocate TX queue! (%d)", errnum);
		};
		
		// zero out the queue descriptors
		memset(dmaGetPtr(&nif->dmaRX), 0, rxqdsize);
		memset(dmaGetPtr(&nif->dmaTX), 0, txqdsize);
		
		// load the queue handles
		vionet_qinit(&nif->rxq, rxqsize, dmaGetPtr(&nif->dmaRX));
		vionet_qinit(&nif->txq, txqsize, dmaGetPtr(&nif->dmaTX));
		
		// allocate the RX/TX space. It starts with 8 packet headers
		// (4 for RX, then 4 for TX), followed by 1518-byte frame
		// buffers (4 for RX and another 4 for TX). The packet headers
		// shall be zeroed out and left like that.
		if ((errnum = dmaCreateBuffer(&nif->dmaIO, 8*sizeof(VionetPacketHeader) + 8*1518, 0)) != 0)
		{
			panic("vionet: failed to allocate RX/TX space! (%d)", errnum);
		};
		
		// zero out the RX/TX space then post the 4 RX buffers to the queue.
		memset(dmaGetPtr(&nif->dmaIO), 0, 8*sizeof(VionetPacketHeader) + 8*1518);
		
		uint64_t spaceBase = dmaGetPhys(&nif->dmaIO);
		for (i=0; i<4; i++)
		{
			// packet header buffer
			nif->rxq.bufDesc[2*i].phaddr = spaceBase + i * sizeof(VionetPacketHeader);
			nif->rxq.bufDesc[2*i].len = sizeof(VionetPacketHeader);
			nif->rxq.bufDesc[2*i].flags = VIONET_DESC_F_NEXT;
			nif->rxq.bufDesc[2*i].next = 2*i+1;
			
			// frame buffer
			nif->rxq.bufDesc[2*i+1].phaddr = spaceBase + 8*sizeof(VionetPacketHeader) + i*1518;
			nif->rxq.bufDesc[2*i+1].len = 1518;
			nif->rxq.bufDesc[2*i+1].flags = VIONET_DESC_F_WRITE;
			nif->rxq.bufDesc[2*i+1].next = 0;
			
			// post as available
			nif->rxq.avail->ring[i] = 2*i;
		};
		
		// update the RX ring index
		nif->rxq.avail->index = 4;
		
		// make sure all memory is flushed!
		__sync_synchronize();
		
		// program in the queue addresses
		uint32_t rxphys = (uint32_t) (dmaGetPhys(&nif->dmaRX) >> 12);
		uint32_t txphys = (uint32_t) (dmaGetPhys(&nif->dmaTX) >> 12);
		outw(nif->iobase + VIONET_REG_QSEL, 0);
		outd(nif->iobase + VIONET_REG_QADDR, rxphys);
		outw(nif->iobase + VIONET_REG_QSEL, 1);
		outd(nif->iobase + VIONET_REG_QADDR, txphys);
		
		nif->netif = CreateNetworkInterface(nif, &ifconfig);
		if (nif->netif == NULL)
		{
			kprintf("vionet: CreateNetworkInterface() failed\n");
		}
		else
		{
			kprintf("vionet: created interface: %s\n", nif->netif->name);
		};

		nif->running = 1;
		spinlockRelease(&nif->lock);
		nif->txBitmap = 0;
		
		// tell the host that we are ready to drive the device, and that we support MAC.
		outw(nif->iobase + 0x04, (1 << 5));
		outw(nif->iobase + VIONET_REG_DEVST, VIONET_DEVST_ACK | VIONET_DEVST_DRV | VIONET_DEVST_OK);
		
		KernelThreadParams pars;
		memset(&pars, 0, sizeof(KernelThreadParams));
		pars.name = "VirtIO-net Interrupt Handler";
		pars.stackSize = DEFAULT_STACK_SIZE;
		nif->thread = CreateKernelThread(vionet_thread, &pars, nif);
	};
	
	if (interfaces == NULL)
	{
		return MODINIT_CANCEL;
	};
	
	return MODINIT_OK;
};

The "vionet_thread()" currently does nothing other than wait for an interrupt and print "VIONET INTERRUPT!!!" when it arrive (spoiler alert: it never does).

I transmit the frame as follows:

Code: Select all

static void vionet_send(NetIf *netif, const void *frame, size_t framelen)
{
	VionetInterface *nif = (VionetInterface*) netif->drvdata;
	
	spinlockAcquire(&nif->lock);
	int txbuf;
	if ((txbuf = vionet_findAvailTXBuffer(nif)) == 4)
	{
		// there are no available buffers, so 
		spinlockRelease(&nif->lock);
	};
	
	nif->txBitmap |= (1 << txbuf);
	
	uint64_t spaceBase = dmaGetPhys(&nif->dmaIO);
	
	// load the frame into the buffer
	uint64_t fbufAddr = (uint64_t) dmaGetPtr(&nif->dmaIO) + 8*sizeof(VionetPacketHeader) + (txbuf+4)*1518;
	memset((void*)fbufAddr, 0, 1518);		// never send uninitialized data!
	memcpy((void*)fbufAddr, frame, framelen);
	
	// fill in the descriptors
	nif->txq.bufDesc[2*txbuf].phaddr = spaceBase + (txbuf+4)*sizeof(VionetPacketHeader);
	nif->txq.bufDesc[2*txbuf].len = sizeof(VionetPacketHeader);
	nif->txq.bufDesc[2*txbuf].flags = VIONET_DESC_F_NEXT;
	nif->txq.bufDesc[2*txbuf].next = 2*txbuf+1;
	
	// frame buffer
	nif->txq.bufDesc[2*txbuf+1].phaddr = spaceBase + 8*sizeof(VionetPacketHeader) + (txbuf+4)*1518;
	nif->txq.bufDesc[2*txbuf+1].len = framelen;
	nif->txq.bufDesc[2*txbuf+1].flags = 0;
	nif->txq.bufDesc[2*txbuf+1].next = 0;
	
	// flush memory
	__sync_synchronize();
	
	// post the buffer to the available ring
	nif->txq.avail->ring[nif->txq.avail->index] = 2*txbuf;
	__sync_synchronize();
	nif->txq.avail->index++;
	__sync_synchronize();
	
	// notify the device that we have updated the TX queue.
	outw(nif->iobase + VIONET_REG_QNOTF, 1);
	
	spinlockRelease(&nif->lock);
};

After vionet_send() is called, no interrupt arrives, and nothing goes into the "used" ring of the TX queue (I previously put a loop at the end of vionet_send() to keep checking the used index (as uint16_t *volatile). The index was always 0).

Where is the problem? Am I not initializing correctly?
NOTE: The MAC address is correctly identified.

[SpyderTL]

I'm not sure exactly what the problem is, but here are some suggestions...

1) Put your headers and your packets together in memory. It would make the math a lot easier.
2) When you set the client feature flags register, you are supposed to check the host features first, and then AND that with your features.
3) Your device status constants appear to all be off by one... They should be:

Code: Select all

#define   VIONET_DEVST_ACK      (1 << 0)
#define   VIONET_DEVST_DRV      (1 << 1)
#define   VIONET_DEVST_OK         (1 << 2)
#define   VIONET_DEVST_FAIL      (1 << 7)

Easy mistake...

Let us know if this fixes the problem or not.

EDIT: Just out of curiosity, why are your network packets 1518 bytes long, instead of 1514? You may want to try changing this to see if it helps.

Also, you could probably just use a single network "header" that contains 12 bytes of zeros, and use that for every packet header that you send, rather than creating multiple packet headers...

And, just in case you didn't already know, the latest documentation for the VirtIO specifications can be found here: http://docs.oasis-open.org/virtio/virti ... -v1.0.html

These documents explicitly define several details that were left out (or not well defined) in the 0.95 draft version that I used.

[mariuszp]

I'm not sure exactly what the problem is, but here are some suggestions...

1) Put your headers and your packets together in memory. It would make the math a lot easier.
2) When you set the client feature flags register, you are supposed to check the host features first, and then AND that with your features.
3) Your device status constants appear to all be off by one... They should be:

Code: Select all

#define   VIONET_DEVST_ACK      (1 << 0)
#define   VIONET_DEVST_DRV      (1 << 1)
#define   VIONET_DEVST_OK         (1 << 2)
#define   VIONET_DEVST_FAIL      (1 << 7)

Easy mistake...

Let us know if this fixes the problem or not.

EDIT: Just out of curiosity, why are your network packets 1518 bytes long, instead of 1514? You may want to try changing this to see if it helps.

Also, you could probably just use a single network "header" that contains 12 bytes of zeros, and use that for every packet header that you send, rather than creating multiple packet headers...

And, just in case you didn't already know, the latest documentation for the VirtIO specifications can be found here: http://docs.oasis-open.org/virtio/virti ... -v1.0.html

These documents explicitly define several details that were left out (or not well defined) in the 0.95 draft version that I used.

Well, one of the specifications I found said that (For example) "FAIL" was "(8)" and another that it was "(128)", so I logically assumed that it is bit 8 ie (1 << . Same for the other ones.

After changing to the values that you have given, it still does not appear to be sending the packet - no interrupts arrive. I made the packets 1518 bytes long since I didn't know whether or not I need to send the CRC (now I know that I do not). I shortened the buffers to be 1514 bytes now, it still does not fix the problem.

Note that the packet I am sending is a DHCPDISCOVER, so I am also expecting another interrupt due to a DHCPOFFER coming back.
(I know that my DHCP client works because I already have a NE2000 driver).

[mariuszp]

I also attempted to notify the device of both queues at the end of initialization, that still doesn't fix it.

[SpyderTL]

Is your TX packet buffer getting moved to the Used queue?

[mariuszp]

Is your TX packet buffer getting moved to the Used queue?

If I add this to the end of vionet_send():

Code: Select all

	uint16_t *volatile idx = &nif->txq.used->index;
	while (1)
	{
		kprintf("Index: %d\n", (int) (*idx));
	};

Then it simply print "Index: 0". So the TX packet buffer is not getting moved to the used queue.

[SpyderTL]

In your virtqueue struct, I think your availIntIndex field is in the wrong place. It should be directly after the available array.

Edit: actually the pointers were throwing me off. I think the problem may be in your virtqueue calculations somewhere. After everything is initialized, check all of your pointers and make sure the math works out properly.

Edit2: I'm not seeing where you are forcing your virtqueues to be aligned to a 4k boundary. You are shifting the address properly when writing the addresses back to the device, but you need to make sure they are 4K aligned in the first place...

[mariuszp]

dmaGetPhys() returns 4k-aligned addresses
SIDE QUESTION: How do I know how long a received frame is? Does the host modify the length of the receive buffer? Or do I have to assume it is 1514 bytes long and then imply the length of the data based on the length field of the IP header etc?

[SpyderTL]

The length field in the used buffer array will tell you how many bytes were actually written to the buffer.

[mariuszp]

The length field in the used buffer array will tell you how many bytes were actually written to the buffer.

Have you written a driver for your OS? If so, can I look at the code to see if I'm missing anything?
EDIT: I'm getting the descriptor table, the available ring, and the used ring on consecutive pages (which makes sense since the desciptor table has 256 entries, each 16 bytes long, so a single page in total, followed by the available ring which is less than 1 page long, followed by the page-aligned used ring).

[dschatz]

I didn't look through it all but isn't vionet_qdsize wrong? http://docs.oasis-open.org/virtio/virti ... o-v1.0.pdf page 17 shows the correct calculation.
Very confused by how you manage DMA - are you reusing the same region for tx and rx? Why are you copying to transmit?

[SpyderTL]

Have you written a driver for your OS? If so, can I look at the code to see if I'm missing anything?

Yes, and no. I have "code" that sends and receives "packets" on VirtIO-net devices (i.e. VirtualBox). But, it's entirely written in my own XML based language, so it may or may not be helpful. Also, all of my memory addresses are hard coded, at the moment, so I'm not dynamically setting up any structures at all.

But, if you're interested, just click on the Ozone link below.

[SpyderTL]

I didn't look through it all but isn't vionet_qdsize wrong? http://docs.oasis-open.org/virtio/virti ... o-v1.0.pdf page 17 shows the correct calculation.

1.0 Specs

Code: Select all

#define ALIGN(x) (((x) + qalign) & ~qalign)
static inline unsigned virtq_size(unsigned int qsz)
{
    return ALIGN(sizeof(struct virtq_desc)*qsz + sizeof(u16)*(3 + qsz))
        + ALIGN(sizeof(u16)*3 + sizeof(struct virtq_used_elem)*qsz);
}

0.95 Specs

Code: Select all

#define ALIGN(x)(((x)+4095) & ~4095)
static inline unsigned vring_size(unsigned int qsz)
{
return ALIGN(sizeof(struct vring_desc)*qsz+sizeof(u16)*(2+qsz))
+ ALIGN(sizeof(struct vring_used_elem)*qsz);
}

mariuszp

Code: Select all

#define VIONET_ALIGN(x) (((x) + 4095) & ~4095)
static inline size_t vionet_qdsize(uint16_t qsz)
{
    return VIONET_ALIGN(sizeof(VionetBufferDesc)*qsz + sizeof(uint16_t)*(2 + qsz))
      + VIONET_ALIGN(sizeof(VionetUsedElem)*qsz);
}

This looks pretty close to the 0.95 specs to me, assuming that the structures are correct...

[mariuszp]

I didn't look through it all but isn't vionet_qdsize wrong? http://docs.oasis-open.org/virtio/virti ... o-v1.0.pdf page 17 shows the correct calculation.
Very confused by how you manage DMA - are you reusing the same region for tx and rx? Why are you copying to transmit?

dmaRX contains the RX queue, dmaTX is the TX queue, and dmaIO is the region which I split up into the RX and TX buffers (it uses 4 of each).

I copy to transmit because I'm copying the data passed by the OS into the TX buffer, which I later add to the available ring of the TX queue.

@SpyderTL I have now updated my code to the version in 1.0:

Code: Select all

static inline size_t vionet_qdsize(uint16_t qsz)
{ 
	return VIONET_ALIGN(sizeof(VionetBufferDesc)*qsz + sizeof(uint16_t)*(3 + qsz))
		+ VIONET_ALIGN(sizeof(uint16_t)*3 + sizeof(VionetUsed)*qsz);
}

However, in both cases, the returned size results in 3 pages being allocated anyway. I noticed the 2 turned into a 3 - which leads me to question this part of my code:

Code: Select all

static void vionet_qinit(VionetQueue *q, uint16_t count, void *p)
{
	uint64_t addr = (uint64_t) p;
	q->count = count;
	q->bufDesc = (VionetBufferDesc*) addr;
	q->avail = (VionetAvail*) (addr + sizeof(VionetBufferDesc) * count);
	q->used = (VionetUsed*) VIONET_ALIGN(((uint64_t) &q->avail->ring[count]));
	//q->availIntIndex = &q->avail->ring[count];
	//q->usedIntIndex = (uint16_t*) &q->used->ring[count];
};

Should "used" be VIONET_ALIGN(((uint64_t) &q->avail->ring[count+1])) ? Because the "avail" struct also has an "interrupt index" field at the end (after the flexible array "ring"). Version 0.95 specification gave code which calculates the position of all those parts, but I can't find that in the Version 1.0 specification, so I'm not sure if the field is still there even if not enabled.

(If only the VirtualBox log file gave some information about this)

[SpyderTL]

Yes, I'd say that you need to add one to the count in order to get "over" the interrupt index field. It would be easy to test by just adding 4096 to the value returned by your function.

I watched a YouTube video where one of the designers gave a presentation on the VirtIO specs, and how they just threw it together and published it without putting any real thought into it. Then they later decided to fix all of the issues in version 1.0 while removing features that nobody used. I think the end result is even more confusing than just leaving it alone...

If nothing else, dump all of your pointer addresses and do the math on paper and make sure all of the values match.

Edit: Here is the video if anyone is interested. Pretty funny, actually.
https://m.youtube.com/watch?v=FY-iQnrrOgk