Hi Andre,
On 3/13/20 3:08 PM, Robin Murphy wrote:
> On 2020-03-12 6:19 pm, Andre Przywara wrote:
>> When we try to get an MSI cookie for a VFIO device, that can fail if
>> CONFIG_IOMMU_DMA is not set. In this case iommu_get_msi_cookie() returns
>> -ENODEV, and that should not be fatal.
>>
>> Ignore that case and proceed with the initialisation.
>>
>> This fixes VFIO with a platform device on the Calxeda Midway (no MSIs).
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>> ---
>> Hi,
>>
>> not sure this is the right fix, or we should rather check if the
>> platform doesn't support MSIs at all (which doesn't seem to be easy
>> to do).
>> Or is this because arm-smmu.c always reserves an IOMMU_RESV_SW_MSI
>> region?
>
> Both, really - ideally VFIO should be able to skip all MSI_related setup
> if the system doesn't support MSIs, but equally the SMMU drivers would
> also ideally not expose a pointless SW_MSI region in the same situation.
>
> In lieu of a 'nice' way of acheiving that, I think this patch seems
> reasonable - ENODEV doesn't clash with any real error that can occur
> when iommu-dma is present, and carrying on without a cookie should be
> fine since the MSI hooks that would otherwise dereference it will also
> be no-ops.
Looks OK to me as well.
About looking at whether MSI is in use I wonder whether we could do like
irq_domain_check_msi_remap() in kernel/irq/irqdomain.c without checking
IRQ_DOMAIN_FLAG_MSI_REMAP.
But if this simple patch fixes this marginal Midway vfio-platform use
case, that should be good enough.
Reviewed-by: Eric Auger <eric.auger@xxxxxxxxxx>
Thanks
Eric
>
> Perhaps it might be worth a comment to clarify that this is specifically
> to allow vfio-platform to work with iommu-dma disabled, but either way,
>
> Acked-by: Robin Murphy <robin.murphy@xxxxxxx>
>
>> Also this seems to be long broken, actually since Eric introduced MSI
>> support in 4.10-rc3, but at least since the initialisation order was
>> fixed with f6810c15cf9.
>
> I'm sure the entire Midway userbase have been up-in-arms the whole
> time... :P
>
> Robin.
>
>>
>> Grateful for any insight.
>>
>> Cheers,
>> Andre
>>
>> drivers/vfio/vfio_iommu_type1.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c
>> b/drivers/vfio/vfio_iommu_type1.c
>> index a177bf2c6683..467e217ef09a 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -1786,7 +1786,7 @@ static int vfio_iommu_type1_attach_group(void
>> *iommu_data,
>> if (resv_msi) {
>> ret = iommu_get_msi_cookie(domain->domain, resv_msi_base);
>> - if (ret)
>> + if (ret && ret != -ENODEV)
>> goto out_detach;
>> }
>>
>
