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

[Peter Xu <peterx@redhat.com>]

On Thu, Dec 19, 2019 at 03:38:34PM +0100, Auger Eric wrote:
> 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 <eric.auger@redhat.com>
> >>>>
> >>>> ---
> >>>>
> >>>> 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?

OK, so there's no global enablement knob for virtio-iommu? Hmm... Then:

  - This flag is a must for all virtio-iommu emulation, right?
    (otherwise I can't see how system bootstraps..)

  - Should this flag be gone right after OS starts (otherwise I think
    we still have the issue that any malicious device can be seen as
    in PT mode as default)?  How is that done?

Thanks,

-- 
Peter Xu