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

[Eric Auger <eric.auger@redhat.com>]

On 6/23/22 10:41, Jean-Philippe Brucker wrote:
> On Thu, Jun 23, 2022 at 01:40:58AM +0000, Duan, Zhenzhong wrote:
>>
>>> -----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?
> Yes, we can't change the probe struct anymore since it's API, but we could
> use the no_tail prefix on future structs

agreed

Eric
>
> Thanks,
> Jean
>