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

[Peter Xu]

On Mon, Jan 06, 2020 at 06:06:34PM +0100, Jean-Philippe Brucker wrote:
> On Fri, Dec 20, 2019 at 11:51:00AM -0500, Peter Xu wrote:
> > On Fri, Dec 20, 2019 at 05:26:42PM +0100, Jean-Philippe Brucker wrote:
> > > There is at the virtio transport level: the driver sets status to
> > > FEATURES_OK once it accepted the feature bits, and to DRIVER_OK once its
> > > fully operational. The virtio-iommu spec says:
> > > 
> > >   If the driver does not accept the VIRTIO_IOMMU_F_BYPASS feature, the
> > >   device SHOULD NOT let endpoints access the guest-physical address space.
> > > 
> > > So before features negotiation, there is no access. Afterwards it depends
> > > if the VIRTIO_IOMMU_F_BYPASS has been accepted by the driver.
> > 
> > Before enabling virtio-iommu device, should we still let the devices
> > to access the whole system address space?  I believe that's at least
> > what Intel IOMMUs are doing.  From code-wise, its:
> > 
> >     if (likely(s->dmar_enabled)) {
> >         success = vtd_do_iommu_translate(vtd_as, vtd_as->bus, vtd_as->devfn,
> >                                          addr, flag & IOMMU_WO, &iotlb);
> >     } else {
> >         /* DMAR disabled, passthrough, use 4k-page*/
> >         iotlb.iova = addr & VTD_PAGE_MASK_4K;
> >         iotlb.translated_addr = addr & VTD_PAGE_MASK_4K;
> >         iotlb.addr_mask = ~VTD_PAGE_MASK_4K;
> >         iotlb.perm = IOMMU_RW;
> >         success = true;
> >     }
> > 
> > From hardware-wise, an IOMMU should be close to transparent if you
> > never enable it, imho.
> 
> For hardware that's not necessarily the best choice. As cited in my
> previous reply it has been shown to introduce vulnerabilities since
> malicious devices can DMA during boot, before the OS takes control of the
> IOMMU. The Arm SMMU allows an implementation to adopt a deny policy by
> default.

I see.  But then how to read a sector from the block to at least boot
an OS if we use a default-deny policy?  Does it still need a mapping
that is established somehow by someone before hand?

> 
> > Otherwise I'm confused on how a guest (with virtio-iommu) could boot
> > with a normal BIOS that does not contain a virtio-iommu driver.  For
> > example, what if the BIOS needs to read some block sectors (as you
> > mentioned)?
> 
> Ideally we should aim at supporting the device in both the BIOS and the
> OS. Failing that, there should at least be a way to instantiate a
> virtio-iommu device that is blocking by default, that cannot be bypassed
> unless the controlling software decides to allow it. Could the bypass
> policy be a command-line option to the virtio-iommu device?
> 
> [...]
> > > Yes bypass mode assumes that devices and drivers aren't malicious, and the
> > > IOMMU is only used for things like assigning devices to guest userspace,
> > > or having large contiguous DMA buffers.
> > 
> > Yes I agree.  However again when the BYPASS flag was introduced, have
> > you thought of introducing that flag per-device?  IMHO that could be
> > better because you have a finer granularity on controlling all these,
> > so you'll be able to reject malicious devices but at the meantime
> > grant permission to trusted devices.
> 
> At the moment that per-device behavior can be emulated by sending an
> ATTACH request followed by identity MAP. It could be a little more
> efficient to add a "bypass" flag to the ATTACH request and avoid setting
> up the identity mapping manually, since the device then wouldn't need to
> look up the mapping on translation, but I don't know how much it would
> improve performance. The device could also cache the fact that the address
> space is identity-mapped, for the same result. The domain lookup has to
> happen in any case, so you can never get the full iommu-free performance
> with these mechanisms.

IMHO it's really a matter of whether virtio-iommu wants to have a
device layer besides the domain layer for the initial versions (just
like VT-d has a device context, then it points to a domain, so it has
these two layers).  But I agree for the bypass feature it should work
(though trying to detect "a device is put into an identital domain is
bypassed" is still a bit tricky to me).  And after all virtio-iommu is
always extensible when needs come.

Thanks,

-- 
Peter Xu