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

[Auger Eric]

Hi Jean,

On 1/9/20 9:47 AM, Jean-Philippe Brucker wrote:
> 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.
Yes I could easily add a device option to disable the default bypass.

Shall we change the meaning of the F_BYPASS feature then? If exposed by
the device, the device does bypass by default, otherwise it doesn't.
This would be controlled by the device option.

The driver then could have means to overwrite this behavior once loaded?

Thanks

Eric
> 
> Thanks,
> Jean
>