[19/41] virtio: use the right types for VirtQueue elements

Submitted by Juan Quintela on Dec. 2, 2009, 12:04 p.m.

Details

Message ID 186e0bde7077cd3384c2e872c008b0c494e0cdf0.1259754427.git.quintela@redhat.com
State New
Headers show

Commit Message

Juan Quintela Dec. 2, 2009, 12:04 p.m.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/virtio.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

Comments

Michael S. Tsirkin Dec. 2, 2009, 1:47 p.m.
On Wed, Dec 02, 2009 at 01:04:17PM +0100, Juan Quintela wrote:
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/virtio.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/virtio.c b/hw/virtio.c
> index fd617ff..2b36cad 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -646,8 +646,8 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
>      qemu_put_sbe32s(f, &vdev->num_pci_queues);
> 
>      for (i = 0; i < vdev->num_pci_queues; i++) {
> -        qemu_put_be32(f, vdev->vq[i].vring.num);
> -        qemu_put_be64(f, vdev->vq[i].pa);
> +        qemu_put_be32s(f, &vdev->vq[i].vring.num);
> +        qemu_put_be64s(f, &vdev->vq[i].pa);
>          qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
>          if (vdev->type == VIRTIO_PCI &&
>              virtio_pci_msix_present(vdev->binding_opaque)) {
> @@ -703,8 +703,8 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
>      qemu_get_sbe32s(f, &vdev->num_pci_queues);
> 
>      for (i = 0; i < vdev->num_pci_queues; i++) {
> -        vdev->vq[i].vring.num = qemu_get_be32(f);
> -        vdev->vq[i].pa = qemu_get_be64(f);
> +        qemu_get_be32s(f, &vdev->vq[i].vring.num);
> +        qemu_get_be64s(f, &vdev->vq[i].pa);
>          qemu_get_be16s(f, &vdev->vq[i].last_avail_idx);
> 
>          if (vdev->type == VIRTIO_PCI &&

Why are these the right types?
I see:
static inline void qemu_put_be64s(QEMUFile *f, const uint64_t *pv)
{
    qemu_put_be64(f, *pv);
}

so passing a pointer to qemu_get_be64s seems exactly equivalent to
qemu_put_be64 on value.

What am I missing?

> -- 
> 1.6.5.2
Michael S. Tsirkin Dec. 2, 2009, 6:24 p.m.
On Wed, Dec 02, 2009 at 07:24:12PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Dec 02, 2009 at 01:04:17PM +0100, Juan Quintela wrote:
> >> 
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  hw/virtio.c |    8 ++++----
> >>  1 files changed, 4 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/hw/virtio.c b/hw/virtio.c
> >> index fd617ff..2b36cad 100644
> >> --- a/hw/virtio.c
> >> +++ b/hw/virtio.c
> >> @@ -646,8 +646,8 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> >>      qemu_put_sbe32s(f, &vdev->num_pci_queues);
> >> 
> >>      for (i = 0; i < vdev->num_pci_queues; i++) {
> >> -        qemu_put_be32(f, vdev->vq[i].vring.num);
> >> -        qemu_put_be64(f, vdev->vq[i].pa);
> >> +        qemu_put_be32s(f, &vdev->vq[i].vring.num);
> >> +        qemu_put_be64s(f, &vdev->vq[i].pa);
> >>          qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
> >>          if (vdev->type == VIRTIO_PCI &&
> >>              virtio_pci_msix_present(vdev->binding_opaque)) {
> >> @@ -703,8 +703,8 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
> >>      qemu_get_sbe32s(f, &vdev->num_pci_queues);
> >> 
> >>      for (i = 0; i < vdev->num_pci_queues; i++) {
> >> -        vdev->vq[i].vring.num = qemu_get_be32(f);
> >> -        vdev->vq[i].pa = qemu_get_be64(f);
> >> +        qemu_get_be32s(f, &vdev->vq[i].vring.num);
> >> +        qemu_get_be64s(f, &vdev->vq[i].pa);
> >>          qemu_get_be16s(f, &vdev->vq[i].last_avail_idx);
> >> 
> >>          if (vdev->type == VIRTIO_PCI &&
> >
> > Why are these the right types?
> > I see:
> > static inline void qemu_put_be64s(QEMUFile *f, const uint64_t *pv)
> > {
> >     qemu_put_be64(f, *pv);
> > }
> >
> > so passing a pointer to qemu_get_be64s seems exactly equivalent to
> > qemu_put_be64 on value.
> >
> > What am I missing?
> 
> While I was porting this to vmstate, it was difficult.  vmstate always
> use the pointer functions (the other ones shouldn't have been defined at
> all, but that is another war).
> 
> It just made the change to vmstate obvious.  I had to redo it several
> times until it worked :(
> 
> Later, Juan.

Maybe try explaining this in commit log.
Juan Quintela Dec. 2, 2009, 6:24 p.m.
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Dec 02, 2009 at 01:04:17PM +0100, Juan Quintela wrote:
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  hw/virtio.c |    8 ++++----
>>  1 files changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/hw/virtio.c b/hw/virtio.c
>> index fd617ff..2b36cad 100644
>> --- a/hw/virtio.c
>> +++ b/hw/virtio.c
>> @@ -646,8 +646,8 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
>>      qemu_put_sbe32s(f, &vdev->num_pci_queues);
>> 
>>      for (i = 0; i < vdev->num_pci_queues; i++) {
>> -        qemu_put_be32(f, vdev->vq[i].vring.num);
>> -        qemu_put_be64(f, vdev->vq[i].pa);
>> +        qemu_put_be32s(f, &vdev->vq[i].vring.num);
>> +        qemu_put_be64s(f, &vdev->vq[i].pa);
>>          qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
>>          if (vdev->type == VIRTIO_PCI &&
>>              virtio_pci_msix_present(vdev->binding_opaque)) {
>> @@ -703,8 +703,8 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
>>      qemu_get_sbe32s(f, &vdev->num_pci_queues);
>> 
>>      for (i = 0; i < vdev->num_pci_queues; i++) {
>> -        vdev->vq[i].vring.num = qemu_get_be32(f);
>> -        vdev->vq[i].pa = qemu_get_be64(f);
>> +        qemu_get_be32s(f, &vdev->vq[i].vring.num);
>> +        qemu_get_be64s(f, &vdev->vq[i].pa);
>>          qemu_get_be16s(f, &vdev->vq[i].last_avail_idx);
>> 
>>          if (vdev->type == VIRTIO_PCI &&
>
> Why are these the right types?
> I see:
> static inline void qemu_put_be64s(QEMUFile *f, const uint64_t *pv)
> {
>     qemu_put_be64(f, *pv);
> }
>
> so passing a pointer to qemu_get_be64s seems exactly equivalent to
> qemu_put_be64 on value.
>
> What am I missing?

While I was porting this to vmstate, it was difficult.  vmstate always
use the pointer functions (the other ones shouldn't have been defined at
all, but that is another war).

It just made the change to vmstate obvious.  I had to redo it several
times until it worked :(

Later, Juan.

Patch hide | download patch | download mbox

diff --git a/hw/virtio.c b/hw/virtio.c
index fd617ff..2b36cad 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -646,8 +646,8 @@  void virtio_save(VirtIODevice *vdev, QEMUFile *f)
     qemu_put_sbe32s(f, &vdev->num_pci_queues);

     for (i = 0; i < vdev->num_pci_queues; i++) {
-        qemu_put_be32(f, vdev->vq[i].vring.num);
-        qemu_put_be64(f, vdev->vq[i].pa);
+        qemu_put_be32s(f, &vdev->vq[i].vring.num);
+        qemu_put_be64s(f, &vdev->vq[i].pa);
         qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
         if (vdev->type == VIRTIO_PCI &&
             virtio_pci_msix_present(vdev->binding_opaque)) {
@@ -703,8 +703,8 @@  int virtio_load(VirtIODevice *vdev, QEMUFile *f)
     qemu_get_sbe32s(f, &vdev->num_pci_queues);

     for (i = 0; i < vdev->num_pci_queues; i++) {
-        vdev->vq[i].vring.num = qemu_get_be32(f);
-        vdev->vq[i].pa = qemu_get_be64(f);
+        qemu_get_be32s(f, &vdev->vq[i].vring.num);
+        qemu_get_be64s(f, &vdev->vq[i].pa);
         qemu_get_be16s(f, &vdev->vq[i].last_avail_idx);

         if (vdev->type == VIRTIO_PCI &&