Re: [PATCH] hw/9pfs: virtio-9p: Ensure config space is a multiple of 4 bytes

[Bin Meng]

Hi Christian,

On Thu, Oct 29, 2020 at 8:52 PM Christian Schoenebeck
<qemu_oss@crudebyte.com> wrote:
>
> On Donnerstag, 29. Oktober 2020 09:25:41 CET Bin Meng wrote:
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > At present the virtio device config space access is handled by the
> > virtio_config_readX() and virtio_config_writeX() APIs. They perform
> > a sanity check on the result of address plus size against the config
> > space size before the access occurs.
>
> Since I am not very familiar with the virtio implementation side, I hope
> Michael would have a look at this patch.
>
> But some comments from my side ...

Thanks for the review.

>
> >
> > For unaligned access, the last converted naturally aligned access
> > will fail the sanity check on 9pfs. For example, with a mount_tag
> > `p9fs`, if guest software tries to read the mount_tag via a 4 byte
> > read at the mount_tag offset which is not 4 byte aligned, the read
> > result will be `p9\377\377`, which is wrong.
>
> Why 4? Shouldn't this rather consider worst case alignment?
>

Both pci and mmio transports only support 1/2/4 bytes access
granularity in the config space, hence the worst case alignment is 4.

> >
> > This changes the size of device config space to be a multiple of 4
> > bytes so that correct result can be returned in all circumstances.
> >
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > ---
> >
> >  hw/9pfs/virtio-9p-device.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> > index 14371a7..e6a1432 100644
> > --- a/hw/9pfs/virtio-9p-device.c
> > +++ b/hw/9pfs/virtio-9p-device.c
> > @@ -201,6 +201,7 @@ static void virtio_9p_device_realize(DeviceState *dev, 
> > Error **errp)
> >      V9fsVirtioState *v = VIRTIO_9P(dev);
> >      V9fsState *s = &v->state;
> >      FsDriverEntry *fse = get_fsdev_fsentry(s->fsconf.fsdev_id);
> > +    size_t config_size;
> >
> >      if (qtest_enabled() && fse) {
> >          fse->export_flags |= V9FS_NO_PERF_WARN;
> > @@ -211,7 +212,8 @@ static void virtio_9p_device_realize(DeviceState *dev, 
> > Error **errp)
> >      }
> >
> >      v->config_size = sizeof(struct virtio_9p_config) + 
> > strlen(s->fsconf.tag);
> > -    virtio_init(vdev, "virtio-9p", VIRTIO_ID_9P, v->config_size);
> > +    config_size = ROUND_UP(v->config_size, 4);
> > +    virtio_init(vdev, "virtio-9p", VIRTIO_ID_9P, config_size);
> >      v->vq = virtio_add_queue(vdev, MAX_REQ, handle_9p_output);
> >  }
>
> Shouldn't this config_size correction rather be handled on virtio.c side
> instead, i.e. in virtio_init()?

I checked other existing virtio devices, and their config space sizes
seem to be already multiple of 4 bytes. If we fix it in virtio_init()
that sounds to be future-proof. Michael?

Regards,
Bin