On 12/10/2018 17:35, Michael S. Tsirkin wrote:
>> + list_del(&req->list);
>> + kfree(req);
>
> So with DEBUG set, this will actually free memory that device still
> DMA's into. Hardly pretty. I think you want to mark device broken,
> queue the request and then wait for device to be reset.
Ok, let's remove DEBUG for the moment
>> +static int viommu_probe(struct virtio_device *vdev)
>> +{
>> + struct device *parent_dev = vdev->dev.parent;
>> + struct viommu_dev *viommu = NULL;
>> + struct device *dev = &vdev->dev;
>> + u64 input_start = 0;
>> + u64 input_end = -1UL;
>> + int ret;
>> +
>> + if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
>> + return -ENODEV;
>
> I'm a bit confused about what will happen if this device
> happens to be behind an iommu itself.
>
> If we can't handle that, should we clear PLATFORM_IOMMU
> e.g. like the balloon does?
I think the DMA API can handle this device doing DMA through another
IOMMU. I haven't tested this case because it is very unusual (IOMMUs
themselves generally access the physical address space) but I don't see
anything preventing it.
What we can't handle is a device performing DMA through two
daisy-chained IOMMUs, but clearing PLATFORM_IOMMU on the first one
wouldn't make things work in that case, we'd need some core changes.
>> +struct virtio_iommu_config {
>> + /* Supported page sizes */
>> + __u64 page_size_mask;
>> + /* Supported IOVA range */
>> + struct virtio_iommu_range {
>
> I'd rather we moved the definition outside even though gcc allows it -
> some old userspace compilers might not.
>
>> + __u64 start;
>> + __u64 end;
>> + } input_range;
>> + /* Max domain ID size */
>> + __u8 domain_bits;
>
> Let's add explicit padding here as well?
Ok
Thanks,
Jean
