Hi Robin,
On 27/07/2017 16:54, Robin Murphy wrote:
> If the IOMMU driver advertises 'real' reserved regions for MSIs, but
> still includes the software-managed region as well, we are currently
> blind to the former and will configure the IOMMU domain to map MSIs into
> the latter, which is unlikely to work as expected.
>
> Since it would take a ridiculous hardware topology for both regions to
> be valid (which would be rather difficult to support in general), we
> should be safe to assume that the presence of any hardware regions makes
> the software region irrelevant. However, the IOMMU driver might still
> advertise the software region by default, particularly if the hardware
> regions are filled in elsewhere by generic code, so it might not be fair
> for VFIO to be super-strict about not mixing them. To that end, make
> vfio_iommu_has_sw_msi() robust against the presence of both region types
> at once, so that we end up doing what is almost certainly right, rather
> than what is almost certainly wrong.
>
> Signed-off-by: Robin Murphy <robin.mur...@arm.com>
Reviewed-by: Eric Auger <eric.au...@redhat.com>
Thanks
Eric
> ---
> drivers/vfio/vfio_iommu_type1.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 2328be628f21..92155cce926d 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -1169,13 +1169,21 @@ static bool vfio_iommu_has_sw_msi(struct iommu_group
> *group, phys_addr_t *base)
> INIT_LIST_HEAD(&group_resv_regions);
> iommu_get_group_resv_regions(group, &group_resv_regions);
> list_for_each_entry(region, &group_resv_regions, list) {
> + /*
> + * The presence of any 'real' MSI regions should take
> + * precedence over the software-managed one if the
> + * IOMMU driver happens to advertise both types.
> + */
> + if (region->type == IOMMU_RESV_MSI) {
> + ret = false;
> + break;
> + }
> +
> if (region->type == IOMMU_RESV_SW_MSI) {
> *base = region->start;
> ret = true;
> - goto out;
> }
> }
> -out:
> list_for_each_entry_safe(region, next, &group_resv_regions, list)
> kfree(region);
> return ret;
>
