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.au...@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).
Thanks,
--
Peter Xu
