RE: [PATCH v2] virtio-iommu: Fix the partial copy of probe request

[Duan, Zhenzhong]

>-----Original Message-----
>From: Jean-Philippe Brucker <jean-philippe@linaro.org>
>Sent: Wednesday, June 22, 2022 9:58 PM
>To: Eric Auger <eric.auger@redhat.com>
>Cc: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-
>devel@nongnu.org; mst@redhat.com
>Subject: Re: [PATCH v2] virtio-iommu: Fix the partial copy of probe request
>
>On Wed, Jun 22, 2022 at 02:22:18PM +0200, Eric Auger wrote:
>> >> 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
>> the fact "struct virtio_iommu_req_tail tail;" was part of the
>>
>> virtio_iommu_req_probe struct
>
>Right, it would have been better to use a different name than
>virtio_iommu_req_probe in virtio_iommu.h, to make the pitfall clear.
>
Maybe virtio_iommu_req_probe_no_tail?

>The larger problem is using C structs across the virtio spec instead of an
>abstract format. Someone implementing the device in another language
>would already not encounter this problem since they would read the spec as
>an abstract format. For documentation purposes I do prefer displaying the
>whole struct like this rather than working around limitations of C, which may
>be more confusing.
>
>
>> >>> 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
>>
>> In virtio_iommu_iov_to_req(), payload_sz is computed as
>>
>> payload_sz = req_sz - sizeof(struct virtio_iommu_req_tail);
>>
>> sz = iov_to_buf(iov, iov_cnt, 0, req, payload_sz);
>>
>> This works for other command structs but not for probe one.
>
>Aah right sorry. The resulting code may be less confusing if we moved
>"- sizeof(struct virtio_iommu_req_tail)" to virtio_iommu_handle_req()
>
Looks better, thanks. Will send v3.

Zhenzhong