Patchwork [PATCH-RFC,01/13] virtio: export virtqueue structure

login
register
mail settings
Submitter Michael S. Tsirkin
Date Jan. 11, 2010, 5:16 p.m.
Message ID <20100111171649.GB11936@redhat.com>
Download mbox | patch
Permalink /patch/42636/
State New
Headers show

Comments

Michael S. Tsirkin - Jan. 11, 2010, 5:16 p.m.
vhost needs physical addresses for
ring so expose that structure.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio.c |   18 ------------------
 hw/virtio.h |   17 +++++++++++++++++
 2 files changed, 17 insertions(+), 18 deletions(-)
Anthony Liguori - Jan. 12, 2010, 10:32 p.m.
On 01/11/2010 11:16 AM, Michael S. Tsirkin wrote:
> vhost needs physical addresses for
> ring so expose that structure.
>
> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>    

I think accessor functions might make more sense.

Regards,

Anthony Liguori
Michael S. Tsirkin - Jan. 25, 2010, 7:10 p.m.
On Tue, Jan 12, 2010 at 04:32:44PM -0600, Anthony Liguori wrote:
> On 01/11/2010 11:16 AM, Michael S. Tsirkin wrote:
>> vhost needs physical addresses for
>> ring so expose that structure.
>>
>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>>    
>
> I think accessor functions might make more sense.
>
> Regards,
>
> Anthony Liguori

Well, take a look:

typedef struct VRing
{
    unsigned int num;
    target_phys_addr_t desc;
    target_phys_addr_t avail;
    target_phys_addr_t used;
} VRing;

struct VirtQueue
{
    VRing vring;
    target_phys_addr_t pa;
    uint16_t last_avail_idx;
    int inuse;
    uint16_t vector;
    void (*handle_output)(VirtIODevice *vdev, VirtQueue *vq);
    VirtIODevice *vdev;
    EventNotifier guest_notifier;
    EventNotifier host_notifier;
}

(Notifiers are added by patches I will shortly post).
We need at least:

    unsigned int num;
    target_phys_addr_t desc;
    target_phys_addr_t avail;
    target_phys_addr_t used;
    VRing vring;
    target_phys_addr_t pa;
    uint16_t vector;
    VirtIODevice *vdev;
    EventNotifier guest_notifier;
    EventNotifier host_notifier;

We do not need:
    uint16_t last_avail_idx;
    int inuse;
    void (*handle_output)(VirtIODevice *vdev, VirtQueue *vq);

(last_avail_idx will be needed if we ever want to
 move a running guest from kernel to userspace
 or back).
IOW, most of VirtQueue needs to be exposed.
So - do we really want accessors?
Anthony Liguori - Jan. 25, 2010, 7:23 p.m.
On 01/25/2010 01:10 PM, Michael S. Tsirkin wrote:
> On Tue, Jan 12, 2010 at 04:32:44PM -0600, Anthony Liguori wrote:
>    
>> On 01/11/2010 11:16 AM, Michael S. Tsirkin wrote:
>>      
>>> vhost needs physical addresses for
>>> ring so expose that structure.
>>>
>>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>>>
>>>        
>> I think accessor functions might make more sense.
>>
>> Regards,
>>
>> Anthony Liguori
>>      
> Well, take a look:
>
> typedef struct VRing
> {
>      unsigned int num;
>      target_phys_addr_t desc;
>      target_phys_addr_t avail;
>      target_phys_addr_t used;
> } VRing;
>
> struct VirtQueue
> {
>      VRing vring;
>      target_phys_addr_t pa;
>      uint16_t last_avail_idx;
>      int inuse;
>      uint16_t vector;
>      void (*handle_output)(VirtIODevice *vdev, VirtQueue *vq);
>      VirtIODevice *vdev;
>      EventNotifier guest_notifier;
>      EventNotifier host_notifier;
> }
>
> (Notifiers are added by patches I will shortly post).
> We need at least:
>
>      unsigned int num;
>      target_phys_addr_t desc;
>      target_phys_addr_t avail;
>      target_phys_addr_t used;
>      VRing vring;
>      target_phys_addr_t pa;
>    

You need num and the physical address of the ring.  The location of 
desc/available/used are all fixed offsets rom the start of the ring (by 
spec).

>      uint16_t vector;
>      VirtIODevice *vdev;
>      EventNotifier guest_notifier;
>      EventNotifier host_notifier;
>    

guest_notifier and host_notifier appear to be new.  I don't see them in 
your previous patchset.  I can't see why you need vdev.  The backlink is 
there only for handle_output because handle_output is passed a 
VirtQueue.  I also don't see you using vector in this patch series.

So at least from what you posted in the RFC, you just need to know the 
ring pa and num which would be totally reasonable to have an accessor for.

Regards,

Anthony Liguori

Patch

diff --git a/hw/virtio.c b/hw/virtio.c
index 3c609ce..8e3c9ad 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -49,24 +49,6 @@  typedef struct VRingUsed
     VRingUsedElem ring[0];
 } VRingUsed;
 
-typedef struct VRing
-{
-    unsigned int num;
-    target_phys_addr_t desc;
-    target_phys_addr_t avail;
-    target_phys_addr_t used;
-} VRing;
-
-struct VirtQueue
-{
-    VRing vring;
-    target_phys_addr_t pa;
-    uint16_t last_avail_idx;
-    int inuse;
-    uint16_t vector;
-    void (*handle_output)(VirtIODevice *vdev, VirtQueue *vq);
-};
-
 #define VIRTIO_PCI_QUEUE_MAX        16
 
 /* virt queue functions */
diff --git a/hw/virtio.h b/hw/virtio.h
index 3994cc9..ca840e1 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -180,5 +180,22 @@  void virtio_net_exit(VirtIODevice *vdev);
 	DEFINE_PROP_BIT("indirect_desc", _state, _field, \
 			VIRTIO_RING_F_INDIRECT_DESC, true)
 
+typedef struct VRing
+{
+    unsigned int num;
+    target_phys_addr_t desc;
+    target_phys_addr_t avail;
+    target_phys_addr_t used;
+} VRing;
+
+struct VirtQueue
+{
+    VRing vring;
+    target_phys_addr_t pa;
+    uint16_t last_avail_idx;
+    int inuse;
+    uint16_t vector;
+    void (*handle_output)(VirtIODevice *vdev, VirtQueue *vq);
+};
 
 #endif