Re: [PATCH v6 05/20] vfio: mdev: common lib code for setting up Interrupt Message Store

[Thomas Gleixner]

On Tue, Jun 08 2021 at 08:57, Dave Jiang wrote:
> On 5/31/2021 6:48 AM, Thomas Gleixner wrote:
>> What's unclear to me is under which circumstances does the IMS interrupt
>> require a PASID.
>>
>>     1) Always
>>     2) Use case dependent
>>
> Thomas, thank you for the review. I'll try to provide a summary below
> with what's going on with IMS after taking in yours and Jason's
> comments.

<snip>

No need to paste the manuals into mail.

</snip>

> DSA provides a way to skip PASID validation for IMS handles. This can
> be used if host kernel is the *only* agent generating work. Host
> usages without IOMMU scalable mode are not currently implemented.

So the IMS irq chip driver can do:

ims_array_alloc_msi_store(domain, dev)
{
        struct msi_domain_info *info = domain->host_data;
        struct ims_array_data *ims = info->data;

        if (ims->flags & VALIDATE_PASID) {
        	if (!valid_pasid(dev))
                	return -EINVAL;
        }

or something like that.

> The following is the call flow for mdev without vSVM support:
> 1.    idxd host driver sets PASID from iommu_aux_get_pasid() to ‘struct device’

Why needs every driver to implement that?

That should be part of the iommu management to store that.

> 2.    idxd guest driver calls request_irq()
> 3.    VFIO calls VFIO_DEVICE_SET_IRQS ioctl

How does the guest driver request_irq() end up in the VFIO ioctl on the
host?

> 4.    idxd host driver calls vfio_set_ims_trigger() (newly created common helper function)
> 	a.    VFIO calls msi_domain_alloc_irqs() and programs valid 'struct device' PASID as auxdata to IMS entry

VFIO does not program anything into the IMS entry.

The IMS irq chip driver retrieves PASID from struct device and does
that. That can be part of the domain allocation function, but there is
no requirement to do so. It can be done later, e.g. when the interrupt
is started up.

> 	b.    Host driver calls request_irq() for IMS interrupts
>
> With a default pasid programmed to 'struct device', for this use case
> above we shouldn't have the need of programming pasid outside of
> irqchip.

s/shouldn't/do not/

> The following is the call flow for mdev with vSVM support:
> 1. idxd host driver sets PASID to mdev ‘struct device’ via iommu_aux_get_PASID()
> 2. idxd guest driver binds supervisor pasid
> 3. idxd guest driver calls request_irq()
> 4. VFIO calls VFIO_DEVICE_SET_IRQS ioctl
> 5. idxd host driver calls vfio_set_ims_trigger()
>    a. VFIO calls msi_domain_alloc_irqs() and programs PASID as auxdata to IMS entry
>    b. Host driver calls request_irq() for IMS interrupts
> 6. idxd guest driver programs virtual device MSIX permission table with guest PASID.
> 7. Host driver mdev MMIO emulation retrieves guest PASID from vdev
>    MSIXPERM table and matches to host PASID via ioasid_find_by_spid().
>    a. Host driver calls irq_set_auxdata() to change to the new PASID
>       for IMS entry.

What enforces this ordering? Certainly not the hardware.

The guest driver knows the guest PASID _before_ interrupts are allocated
or requested for the device. So it can store the guest PASID _before_ it
triggers the mechanism which makes vfio/host initialize the interrupts.

So no. It's not needed at all. It's pretty much the same as the host
side driver except for the that MSIXPERM stuff.

And just for the record. Setting MSIXPERM _after_ request_irq()
completed is just wrong because if an interrupt is raised _before_ that
MSIXPERM muck is set up, then it will fire with the host PASID and not
with the guest's.

This whole IDXD stuff has been a monstrous layering violation from the
very beginning and unfortunately this hasn't changed much since then.

Thanks,

        tglx