Hi Michael, On Tue, Nov 3, 2020 at 8:05 PM Michael S. Tsirkin <m...@redhat.com> wrote: > > 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. >
Correct. > > 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. > Yes. > 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? > The obvious fix can be made to the guest which exposed this issue, but I was wondering whether we should enforce all virtio devices' config space size to be a multiple of 4 bytes which sounds more natural. Regards, Bin