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

[Bin Meng]

Hi Michael,

On Fri, Oct 30, 2020 at 5:29 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Oct 29, 2020 at 04:25:41PM +0800, 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.
> >
> > 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.
> >
> > 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>
>
>
>
> The patch is ok, but I'd like to clarify the commit log.

Thanks for the review.

>
> If I understand correctly, what happens is:
> - tag is set to a value that is not a multiple of 4 bytes

It's not about the mount_tag value, but the length of the mount_tag is 4.

> - guest attempts to read the last 4 bytes of the tag

Yep. So the config space of a 9pfs looks like the following:

offset: 0x14, size: 2 bytes indicating the length of the following mount_tag
offset: 0x16, size: value of (offset 0x14).

When a 4-byte mount_tag is given, guest software is subject to read 4
bytes (value read from offset 0x14) at offset 0x16.

> - access returns -1
>

The access will be split into 2 accesses, either by hardware or
software. On RISC-V such unaligned access is emulated by M-mode
firmware. On ARM I believe it's supported by the CPU. So the first
converted aligned access is to read 4 byte at 0x14 and the second
converted aligned access is to read 4 byte at 0x16, and drop the bytes
that are not needed, assemble the remaining bytes and return the
result to the guest software. The second aligned access will fail the
sanity check and return -1, but not the first access, hence the result
will be `p9\377\377`.

>
> What I find confusing in the above description:
> - reference to unaligned access - I don't think these
>   are legal or allowed by QEMU
> - reference to `p9\377\377` - I think returned value will be -1
>

Regards,
Bin