Re: [PATCH for-5.0 v11 08/20] virtio-iommu: Implement translate

[Auger Eric]

Hi Peter,

On 12/19/19 2:33 PM, Peter Xu wrote:
> On Thu, Dec 19, 2019 at 11:30:40AM +0100, Auger Eric wrote:
>> Hi Peter,
>> On 12/10/19 8:33 PM, Peter Xu wrote:
>>> On Fri, Nov 22, 2019 at 07:29:31PM +0100, Eric Auger wrote:
>>>> This patch implements the translate callback
>>>>
>>>> Signed-off-by: Eric Auger <address@hidden>
>>>>
>>>> ---
>>>>
>>>> v10 -> v11:
>>>> - take into account the new value struct and use
>>>>   g_tree_lookup_extended
>>>> - switched to error_report_once
>>>>
>>>> v6 -> v7:
>>>> - implemented bypass-mode
>>>>
>>>> v5 -> v6:
>>>> - replace error_report by qemu_log_mask
>>>>
>>>> v4 -> v5:
>>>> - check the device domain is not NULL
>>>> - s/printf/error_report
>>>> - set flags to IOMMU_NONE in case of all translation faults
>>>> ---
>>>>  hw/virtio/trace-events   |  1 +
>>>>  hw/virtio/virtio-iommu.c | 63 +++++++++++++++++++++++++++++++++++++++-
>>>>  2 files changed, 63 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
>>>> index f25359cee2..de7cbb3c8f 100644
>>>> --- a/hw/virtio/trace-events
>>>> +++ b/hw/virtio/trace-events
>>>> @@ -72,3 +72,4 @@ virtio_iommu_get_endpoint(uint32_t ep_id) "Alloc 
>>>> endpoint=%d"
>>>>  virtio_iommu_put_endpoint(uint32_t ep_id) "Free endpoint=%d"
>>>>  virtio_iommu_get_domain(uint32_t domain_id) "Alloc domain=%d"
>>>>  virtio_iommu_put_domain(uint32_t domain_id) "Free domain=%d"
>>>> +virtio_iommu_translate_out(uint64_t virt_addr, uint64_t phys_addr, 
>>>> uint32_t sid) "0x%"PRIx64" -> 0x%"PRIx64 " for sid=%d"
>>>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>>>> index f0a56833a2..a83666557b 100644
>>>> --- a/hw/virtio/virtio-iommu.c
>>>> +++ b/hw/virtio/virtio-iommu.c
>>>> @@ -412,19 +412,80 @@ static IOMMUTLBEntry 
>>>> virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>>>>                                              int iommu_idx)
>>>>  {
>>>>      IOMMUDevice *sdev = container_of(mr, IOMMUDevice, iommu_mr);
>>>> +    viommu_interval interval, *mapping_key;
>>>> +    viommu_mapping *mapping_value;
>>>> +    VirtIOIOMMU *s = sdev->viommu;
>>>> +    viommu_endpoint *ep;
>>>> +    bool bypass_allowed;
>>>>      uint32_t sid;
>>>> +    bool found;
>>>> +
>>>> +    interval.low = addr;
>>>> +    interval.high = addr + 1;
>>>>  
>>>>      IOMMUTLBEntry entry = {
>>>>          .target_as = &address_space_memory,
>>>>          .iova = addr,
>>>>          .translated_addr = addr,
>>>> -        .addr_mask = ~(hwaddr)0,
>>>> +        .addr_mask = (1 << ctz32(s->config.page_size_mask)) - 1,
>>>>          .perm = IOMMU_NONE,
>>>>      };
>>>>  
>>>> +    bypass_allowed = virtio_has_feature(s->acked_features,
>>>> +                                        VIRTIO_IOMMU_F_BYPASS);
>>>> +
>>>
>>> Would it be easier to check bypass_allowed here once and then drop the
>>> latter [1] and [2] check?
>> bypass_allowed does not mean you systematically bypass. You bypass if
>> the SID is unknown or if the device is not attached to any domain.
>> Otherwise you translate. But maybe I miss your point.
> 
> Ah ok, then could I ask how will this VIRTIO_IOMMU_F_BYPASS be used?
> For example, I think VT-d defines passthrough in a totally different
> way in that the PT mark will be stored in the per-device context
> entries, then we can allow a specific device to be pass-through when
> doing DMA.  That information is explicit (e.g., unknown SID will
> always fail the DMA), and per-device.
> 
> Here do you mean that you just don't put a device into any domain to
> show it wants to use PT?  Then I'm not sure how do you identify
> whether this is a legal PT or a malicious device (e.g., an unknown
> device that even does not have any driver bound to it, which will also
> satisfy "unknown SID" and "not attached to any domain", iiuc).

The virtio-iommu spec currently says:

"If the VIRTIO_IOMMU_F_BYPASS feature is negotiated, all accesses from
unattached endpoints are
allowed and translated by the IOMMU using the identity function. If the
feature is not negotiated, any
memory access from an unattached endpoint fails. Upon attaching an
endpoint in bypass mode to a new
domain, any memory access from the endpoint fails, since the domain does
not contain any mapping.
"

I guess this can serve the purpose of devices doing early accesses,
before the guest OS gets the hand and maps them?

Thanks

Eric
> 
> Thanks,
>