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

login
register
mail settings
Submitter Juan Quintela
Date Dec. 2, 2009, 12:04 p.m.
Message ID <186e0bde7077cd3384c2e872c008b0c494e0cdf0.1259754427.git.quintela@redhat.com>
Download mbox | patch
Permalink /patch/40025/
State New
Headers show

Comments

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(-)
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

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 &&