Re: [Qemu-devel] [RFC v2 00/28] vSMMUv3/pSMMUv3 2 stage VFIO integration

[Auger Eric <eric.auger@redhat.com>]

Hi Yi,

On 10/18/18 12:30 PM, Liu, Yi L wrote:
> Hi Eric,
> 
>> From: Eric Auger [mailto:eric.auger@redhat.com]
>> Sent: Friday, September 21, 2018 4:18 PM
>> Subject: [RFC v2 00/28] vSMMUv3/pSMMUv3 2 stage VFIO integration
>>
>> Up to now vSMMUv3 has not been integrated with VFIO. VFIO
>> integration requires to program the physical IOMMU consistently
>> with the guest mappings. However, as opposed to VTD, SMMUv3 has
>> no "Caching Mode" which allows easy trapping of guest mappings.
>> This means the vSMMUV3 cannot use the same VFIO integration as VTD.
>>
>> However SMMUv3 has 2 translation stages. This was devised with
>> virtualization use case in mind where stage 1 is "owned" by the
>> guest whereas the host uses stage 2 for VM isolation.
>>
>> This series sets up this nested translation stage. It only works
>> if there is one physical SMMUv3 used along with QEMU vSMMUv3 (in
>> other words, it does not work if there is a physical SMMUv2).
>>
>> The series uses a new kernel user API [1], still under definition.
>>
>> - We force the host to use stage 2 instead of stage 1, when we
>>   detect a vSMMUV3 is behind a VFIO device. For a VFIO device
>>   without any virtual IOMMU, we still use stage 1 as many existing
>>   SMMUs expect this behavior.
>> - We introduce new IOTLB "config" notifiers, requested to notify
>>   changes in the config of a given iommu memory region. So now
>>   we have notifiers for IOTLB changes and config changes.
>> - vSMMUv3 calls config notifiers when STE (Stream Table Entries)
>>   are updated by the guest.
>> - We implement a specific UNMAP notifier that conveys guest
>>   IOTLB invalidations to the host
>> - We implement a new MAP notifiers only used for MSI IOVAs so
>>   that the host can build a nested stage translation for MSI IOVAs
>> - As the legacy MAP notifier is not called anymore, we must make
>>   sure stage 2 mappings are set. This is achieved through another
>>   memory listener.
>> - Physical SMMUs faults are reported to the guest via en eventfd
>>   mechanism and reinjected into this latter.
>>
>> Note: some iommu memory notifier rework related patches are close
>> to those previously published by Peter and Liu. I will be pleased to
>> add their Signed-off-by if they agree/wish.
> 
> Yeah, feel free to add mine if it's originated from our previous work.
OK
> 
> BTW. Curiously, are you planning to implement all vIOMMU related
> operations through MemoryRegion notifiers? Honestly, I did it in such
> way in early RFC for vSVA work. However, we encountered two issues
> at that time. First, how to check whether the notifier should be registered.
On my side I think I resolved this by querying the iommu mr about the
new IOMMU_ATTR_VFIO_NESTED IOMMU attribute  in vfio_connect_container

See patches 5 and 6, 8

This tells me whether the nested mode must be setup and choose the right
container->iommu_type which is then used in vfio_listener_region_add()
to decide whether the specific notifiers mustd to be registered.


> Second, there are cases in which the vfio_listener_region_add is not
> triggered but vIOMMU exists. Details can be got by the link below:
> (http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg00078.html)

Yes I remember this, due to the PT=1 case. On ARM we don't have this
specificity hence this integration.
> 
> As thus, we had some discussions in community. Last time, PCIPASIDOps
> was proposed. It is to add callbacks in PCIDevice. VFIO would register its
> implementations in vfio_realize(). Supposedly, pasid_table_bind,
> page_table_bind, sva_tlb_invalidation_passdown and other vIOMMU
> related operations can be done in such way. The sample patch below
> may show how it looks like. (the full patch is in my sandbox, planned to
> send out with Scalable Mode emulation patch).

To be honest, I lost track of the series and did not see this
PCIPASIDOps proposal. I will study whether this can fit my needs.

Thank you for the link!

Best Regards

Eric
> 
> Sample patch:
> include/hw/pci/pci.h:
>  +typedef struct PCIPASIDOps PCIPASIDOps;
>  +struct PCIPASIDOps {
>  +    void (*pasid_bind_table)(PCIBus *bus, int32_t devfn,
>  +                            struct pasid_table_config *ptbl_cfg);
>  +    void (*pasid_invalidate_extend_iotlb)(PCIBus *bus, int32_t devfn,
>  +                            struct tlb_invalidate_info *inv_info);
>  +};
> 
>  @@ -350,6 +359,7 @@ struct PCIDevice {
>       MSIVectorUseNotifier msix_vector_use_notifier;
>       MSIVectorReleaseNotifier msix_vector_release_notifier;
>       MSIVectorPollNotifier msix_vector_poll_notifier;
>  +    PCIPASIDOps *pasid_ops;
>   };
> 
> hw/pci/pci.c:
> +void pci_setup_pasid_ops(PCIDevice *dev, PCIPASIDOps *ops)
>  +{
>  +    assert(ops && !dev->pasid_ops);
>  +    dev->pasid_ops = ops;
>  +}
>  +
>  +void pci_device_pasid_bind_table(PCIBus *bus, int32_t devfn,
>  +                                 struct pasid_table_config *ptbl_cfg)
>  +{
>  +    PCIDevice *dev;
>  +
>  +    if (!bus) {
>  +        return;
>  +    }
>  +
>  +    dev = bus->devices[devfn];
>  +    if (dev && dev->pasid_ops) {
>  +        dev->pasid_ops->pasid_bind_table(bus, devfn, ptbl_cfg);
>  +    }
>  +}
> 
> hw/vfio/pci.c:
> +static PCIPASIDOps vfio_pci_pasid_ops = {
>  +    .pasid_bind_table = vfio_pci_device_pasid_bind_table,
>  +    .pasid_invalidate_extend_iotlb = vfio_pci_device_pasid_invalidate_eiotlb,
>  +};
>  +
>   static void vfio_realize(PCIDevice *pdev, Error **errp)
>   {
>       VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>  @@ -3048,6 +3068,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>       vfio_register_req_notifier(vdev);
>       vfio_setup_resetfn_quirk(vdev);
> 
>  +    pci_setup_pasid_ops(pdev, &vfio_pci_pasid_ops);
>  +
>       return;
> 
> Regards,
> Yi Liu
> 
>