Hi,
On Wed, Jun 22, 2022 at 12:20:45PM +0200, Eric Auger wrote:
> Hi,
>
> On 6/17/22 08:20, Zhenzhong Duan wrote:
> > The structure of probe request doesn't include the tail, this leads
> > to a few field missed to be copied. Currently this isn't an issue as
> > those missed field belong to reserved field, just in case reserved
> > field will be used in the future.
> >
> > Fixes: 1733eebb9e75b ("virtio-iommu: Implement RESV_MEM probe request")
> > Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
> nice catch.
>
> Reviewed-by: Eric Auger <eric.au...@redhat.com>
>
> the spec is pretty confusing here though (virtio-v1.2-csd01.pdf) as it
> presents the struct as follows:
>
> struct virtio_iommu_req_probe {
> struct virtio_iommu_req_head head;
> /* Device-readable */
> le32 endpoint;
> u8 reserved[64];
>
> /* Device-writable */
> u8 properties[probe_size];
> struct virtio_iommu_req_tail tail;
> };
Hm, which part is confusing? Yes it's not valid C since probe_size is
defined dynamically ('probe_size' in the device config), but I thought it
would be nicer to show the whole request layout this way. Besides, at
least virtio-blk and virtio-scsi have similar variable-sized arrays in
their definitions
>
> Adding Jean in the loop ...
>
> Thanks!
>
> Eric
>
>
>
>
> > ---
> > v2: keep bugfix change and drop cleanup change
> >
> > hw/virtio/virtio-iommu.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> > index 7c122ab95780..195f909620ab 100644
> > --- a/hw/virtio/virtio-iommu.c
> > +++ b/hw/virtio/virtio-iommu.c
> > @@ -708,7 +708,8 @@ static int virtio_iommu_handle_probe(VirtIOIOMMU *s,
> > uint8_t *buf)
> > {
> > struct virtio_iommu_req_probe req;
> > - int ret = virtio_iommu_iov_to_req(iov, iov_cnt, &req, sizeof(req));
> > + int ret = virtio_iommu_iov_to_req(iov, iov_cnt, &req,
> > + sizeof(req) + sizeof(struct virtio_iommu_req_tail));
Not sure this is correct, because what we are doing here is reading the
device-readable part of the property from the request. That part is only
composed of fields 'head', 'endpoint' and 'reserved[64]' and its size is
indeed sizeof(struct virtio_iommu_req_probe).
The 'properties' and 'tail' fields shouldn't be read by the device here,
they are instead written later. It is virtio_iommu_handle_command() that
copies both of them into the request:
output_size = s->config.probe_size + sizeof(tail);
buf = g_malloc0(output_size);
ptail = (struct virtio_iommu_req_tail *)
(buf + s->config.probe_size);
ptail->status = virtio_iommu_handle_probe(s, iov, iov_cnt, buf);
// and virtio_iommu_probe() fills 'properties' as needed
...
// then copy the lot
sz = iov_from_buf(elem->in_sg, elem->in_num, 0,
buf ? buf : &tail, output_size);
So I think the current code is correct, as all fields are accounted for
Thanks,
Jean
> >
> > return ret ? ret : virtio_iommu_probe(s, &req, buf);
> > }
>
