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.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).
>
> 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
