Hi Robin,
On 13/03/2017 15:24, Robin Murphy wrote:
> On 13/03/17 13:08, Auger Eric wrote:
>> Hi Robin,
>>
>> On 09/03/2017 20:50, Robin Murphy wrote:
>>> Whilst it doesn't matter much to VFIO at the moment, when parsing
>>> reserved regions on the host side we really needs to be able to tell
>> s/needs/need
>
> Oops!
>
>>> the difference between the software-reserved region used to map MSIs
>>> translated by an IOMMU, and hardware regions for which the write might
>>> never even reach the IOMMU. In particular, ARM systems assume the former
>>> topology, but may need to cope with the latter as well, which will
>>> require rather different handling in the iommu-dma layer.
>>>
>>> For clarity, rename the software-managed type to IOMMU_RESV_SW_MSI, use
>>> IOMMU_RESV_MSI to describe the hardware type, and document everything a
>>> little bit. Since the x86 MSI remapping hardware falls squarely under
>>> this meaning of IOMMU_RESV_MSI, apply that type to their regions as well,
>>> so that we tell a consistent story to userspace across platforms (and
>>> have future consistency if those drivers start migrating to iommu-dma).
>>>
>>> Fixes: d30ddcaa7b02 ("iommu: Add a new type field in iommu_resv_region")
>> does it really fall under the category of fix here?
>
> I was somewhat on the fence about that, as the rationale above does tend
> towards future new functionality, but the primary effect of this patch
> alone is an ABI-visible change for x86 in terms of "expose the MSI
> region as an MSI region". IMO it would be better to get that in before
> said ABI gets baked into a kernel release, hence leaning towards the
> "fix" side of things. I'm happy to rewrite the commit message in reverse
> order (i.e. "clean up this ABI inconsistency, with these additional
> benefits") if it would be clearer.
OK no worries. I understand what you meant.
>
>>> CC: Eric Auger <eric.auger@xxxxxxxxxx>
>>> CC: Alex Williamson <alex.williamson@xxxxxxxxxx>
>>> CC: David Woodhouse <dwmw2@xxxxxxxxxxxxx>
>>> CC: kvm@xxxxxxxxxxxxxxx
>>> Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
>>> ---
>>> drivers/iommu/amd_iommu.c | 2 +-
>>> drivers/iommu/arm-smmu-v3.c | 2 +-
>>> drivers/iommu/arm-smmu.c | 2 +-
>>> drivers/iommu/intel-iommu.c | 2 +-
>>> drivers/iommu/iommu.c | 1 +
>>> drivers/vfio/vfio_iommu_type1.c | 2 +-
>>> include/linux/iommu.h | 5 +++++
>>> 7 files changed, 11 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
>>> index 98940d1392cb..b17536d6e69b 100644
>>> --- a/drivers/iommu/amd_iommu.c
>>> +++ b/drivers/iommu/amd_iommu.c
>>> @@ -3202,7 +3202,7 @@ static void amd_iommu_get_resv_regions(struct device *dev,
>>>
>>> region = iommu_alloc_resv_region(MSI_RANGE_START,
>>> MSI_RANGE_END - MSI_RANGE_START + 1,
>>> - 0, IOMMU_RESV_RESERVED);
>>> + 0, IOMMU_RESV_MSI);
>>> if (!region)
>>> return;
>>> list_add_tail(®ion->list, head);
>>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>>> index 5806a6acc94e..591bb96047c9 100644
>>> --- a/drivers/iommu/arm-smmu-v3.c
>>> +++ b/drivers/iommu/arm-smmu-v3.c
>>> @@ -1888,7 +1888,7 @@ static void arm_smmu_get_resv_regions(struct device *dev,
>>> int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
>>>
>>> region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
>>> - prot, IOMMU_RESV_MSI);
>>> + prot, IOMMU_RESV_SW_MSI);
>>> if (!region)
>>> return;
>>>
>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>> index abf6496843a6..b493c99e17f7 100644
>>> --- a/drivers/iommu/arm-smmu.c
>>> +++ b/drivers/iommu/arm-smmu.c
>>> @@ -1608,7 +1608,7 @@ static void arm_smmu_get_resv_regions(struct device *dev,
>>> int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
>>>
>>> region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
>>> - prot, IOMMU_RESV_MSI);
>>> + prot, IOMMU_RESV_SW_MSI);
>>> if (!region)
>>> return;
>>>
>>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>>> index 238ad3447712..f1611fd6f5b0 100644
>>> --- a/drivers/iommu/intel-iommu.c
>>> +++ b/drivers/iommu/intel-iommu.c
>>> @@ -5249,7 +5249,7 @@ static void intel_iommu_get_resv_regions(struct device *device,
>>>
>>> reg = iommu_alloc_resv_region(IOAPIC_RANGE_START,
>>> IOAPIC_RANGE_END - IOAPIC_RANGE_START + 1,
>>> - 0, IOMMU_RESV_RESERVED);
>>> + 0, IOMMU_RESV_MSI);
>>> if (!reg)
>>> return;
>>> list_add_tail(®->list, head);
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>> index 8ea14f41a979..7dbc05f10d5a 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -72,6 +72,7 @@ static const char * const iommu_group_resv_type_string[] = {
>>> [IOMMU_RESV_DIRECT] = "direct",
>>> [IOMMU_RESV_RESERVED] = "reserved",
>>> [IOMMU_RESV_MSI] = "msi",
>>> + [IOMMU_RESV_SW_MSI] = "msi",
>
> As a side note, is it intentional that the values are bitfields, in
> other words, is it ever going to make sense to combine them? The fact
> that we use them directly to index an array here suggests not, and that
> we might be better off with a plain enum. Otherwise, another possibility
> would be to keep a single "MSI" type flag and add a separate
> "software-managed" flag alongside it, with a corresponding tweak to
> iommu_group_show_resv_regions(). Does anyone have a strong opinion
> either way?
Originally this stems from "IOMMU_NOMAP" flavored define but I think
this should be transformed into an enum instead.
Thanks
Eric
>
>>> };
>>>
>>> #define IOMMU_GROUP_ATTR(_name, _mode, _show, _store) \
>>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>>> index c26fa1f3ed86..e32abdebd2df 100644
>>> --- a/drivers/vfio/vfio_iommu_type1.c
>>> +++ b/drivers/vfio/vfio_iommu_type1.c
>>> @@ -1192,7 +1192,7 @@ static bool vfio_iommu_has_resv_msi(struct iommu_group *group,
>> Maybe we should change the name of the function into
>> vfio_iommu_has_resv_sw_msi?
>
> Good idea, will do.
>
>> Besides
>> Reviewed-by: Eric Auger <eric.auger@xxxxxxxxxx>
>
> Thanks!
>
> Robin.
>
>>
>> Thanks
>>
>> Eric
>>
>>
>>> INIT_LIST_HEAD(&group_resv_regions);
>>> iommu_get_group_resv_regions(group, &group_resv_regions);
>>> list_for_each_entry(region, &group_resv_regions, list) {
>>> - if (region->type & IOMMU_RESV_MSI) {
>>> + if (region->type & IOMMU_RESV_SW_MSI) {
>>> *base = region->start;
>>> ret = true;
>>> goto out;
>>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>>> index 6a6de187ddc0..fad2c4913be4 100644
>>> --- a/include/linux/iommu.h
>>> +++ b/include/linux/iommu.h
>>> @@ -125,9 +125,14 @@ enum iommu_attr {
>>> };
>>>
>>> /* These are the possible reserved region types */
>>> +/* Memory regions which must have 1:1 translations present at all times */
>>> #define IOMMU_RESV_DIRECT (1 << 0)
>>> +/* Arbitrary "never map this or give it to a device" address ranges */
>>> #define IOMMU_RESV_RESERVED (1 << 1)
>>> +/* Hardware MSI region (untranslated) */
>>> #define IOMMU_RESV_MSI (1 << 2)
>>> +/* Software-managed MSI translation window */
>>> +#define IOMMU_RESV_SW_MSI (1 << 3)
>>>
>>> /**
>>> * struct iommu_resv_region - descriptor for a reserved memory region
>>>
>
