El Wed, 13 Aug 2014 18:27:40 +0100
Stefan Hajnoczi <stefa...@redhat.com> escribió:
> > +void qvring_init(const QGuestAllocator *alloc, QVirtQueue *vq,
> > uint64_t addr) +{
> > + int i;
> > +
> > + vq->desc = addr;
> > + vq->avail = vq->desc + vq->size*sizeof(QVRingDesc);
> > + vq->used = (uint64_t)((vq->avail + sizeof(uint16_t) * (3 +
> > vq->size)
> > + +vq->align-1) & ~(vq->align - 1));
> > +
> > + for (i = 0; i < vq->size-1; i++) {
> > + /* vq->desc[i].addr */
> > + writew(vq->desc+(16*i), 0);
> > + /* vq->desc[i].next */
> > + writew(vq->desc+(16*i)+14, i+1);
> > + }
>
> Please use space between operators:
> writew(vq->desc + (16 * i) + 14, i + 1)
>
> This applies to all patches.
>
> > +void qvirtqueue_kick(const QVirtioBus *bus, QVirtioDevice *d,
> > QVirtQueue *vq,
> > +
> > uint32_t free_head) +{
> > + /* vq->avail->idx */
> > + uint16_t idx = readl(vq->avail+2);
> > +
> > + /* vq->avail->ring[idx % vq->size] */
> > + writel(vq->avail+4+(2*(idx % vq->size)), free_head);
>
> If you want to use typed pointers you can. It will make the code
> nicer to read, just be careful never to dereference them and only to
> access through writel()/readl():
>
> typedef struct {
> uint16_t foo; /* Forgot what these fields are called and didn't check
> */ uint16_t bar;
> uint16_t ring[];
> } VirtioAvail;
>
> writel(&vq->avail.ring[idx % vq->size], free_head);
>
> Now it looks more like normal C and the compiler is doing the address
> calculations for us.
If it is not dereferenced, it can work. But all references will have to
be casted from (void *) to (uint64_t) always which is what readl and
writel expect. I think this is better than calculating the addresses,
but is still a bit ugly.
