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

[Jean-Philippe Brucker]

On Wed, Jan 08, 2020 at 05:55:52PM +0100, Auger Eric wrote:
> Hi Jean-Philippe, Peter,
> 
> On 1/7/20 11:10 AM, Jean-Philippe Brucker wrote:
> > On Mon, Jan 06, 2020 at 12:58:50PM -0500, Peter Xu wrote:
> >> 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?
> > 
> > Yes, it looks like EDK II uses IOMMU operations in order to access those
> > devices on platforms where the IOMMU isn't default-bypass (AMD SEV support
> > is provided by edk2, and a VT-d driver seems provided by edk2-platforms).
> > However for OVMF we could just set the bypass feature bit in virtio-iommu
> > device, which doesn't even requires setting up the virtqueue.
> > 
> > I'm missing a piece of the puzzle for Arm platforms though, because it
> > looks like Trusted Firmware-A sets up the default-deny policy on reset
> > even when it wasn't hardwired, but doesn't provide a service to create
> > SMMUv3 mappings for the bootloader.
> > 
> > Thanks,
> > Jean
> > 
> 
> I think we have a concrete example for the above discussion. The AHCI.
> When running the virtio-iommu on x86 I get messages like:
> 
> virtio_iommu_translate sid=250 is not known!!
> no buffer available in event queue to report event
> 
> and a bunch of "AHCI: Failed to start FIS receive engine: bad FIS
> receive buffer address" messages (For each port)
> 
> This was reported in my cover letter (*). This happens very early in the
> boot process, before the OS get the hand and before the virtio-iommu
> driver creates any mapping. It does not prevent the guest from booting
> though.
> 
> Currently the virtio-iommu device checks the VIRTIO_IOMMU_F_BYPASS. If I
> overwrite it to true in the device, then, the guest boots without those
> messages.

Oh that's good, I was afraid it was an issue in Linux.

> I share Peter's concern about having a different default policy than x86.

Yes I'd say just align with whatever policy is already in place. Do you
think we could add a command-line option to let people disable
default-bypass, though?  That would be a convenient way to make the IOMMU
protection airtight for those who need it.

Thanks,
Jean