On Tue, Nov 03, 2020 at 02:26:10PM +0800, Bin Meng wrote:
> Hi Michael,
>
> On Fri, Oct 30, 2020 at 5:29 PM Michael S. Tsirkin <m...@redhat.com> wrote:
> >
> > On Thu, Oct 29, 2020 at 04:25:41PM +0800, Bin Meng wrote:
> > > From: Bin Meng <bin.m...@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.m...@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.
Well looking at Linux guest code:
static inline void __virtio_cread_many(struct virtio_device *vdev,
unsigned int offset,
void *buf, size_t count, size_t bytes)
{
u32 old, gen = vdev->config->generation ?
vdev->config->generation(vdev) : 0;
int i;
might_sleep();
do {
old = gen;
for (i = 0; i < count; i++)
vdev->config->get(vdev, offset + bytes * i,
buf + i * bytes, bytes);
gen = vdev->config->generation ?
vdev->config->generation(vdev) : 0;
} while (gen != old);
}
static inline void virtio_cread_bytes(struct virtio_device *vdev,
unsigned int offset,
void *buf, size_t len)
{
__virtio_cread_many(vdev, offset, buf, len, 1);
}
and:
virtio_cread_bytes(vdev, offsetof(struct virtio_9p_config, tag),
tag, tag_len);
So guest is doing multiple 1-byte reads.
Spec actually says:
For device configuration access, the driver MUST use 8-bit wide
accesses for 8-bit wide fields, 16-bit wide
and aligned accesses for 16-bit wide fields and 32-bit wide and aligned
accesses for 32-bit and 64-bit wide
fields. For 64-bit fields, the driver MAY access each of the high and
low 32-bit parts of the field independently.
9p was never standardized, but the linux header at least lists it as
follows:
struct virtio_9p_config {
/* length of the tag name */
__virtio16 tag_len;
/* non-NULL terminated tag name */
__u8 tag[0];
} __attribute__((packed));
In that sense tag is an 8 byte field.
So which guest reads tag using a 32 bit read, and why?
> > - 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
