lkml.org 
[lkml]   [2019]   [Mar]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 3/7] iommu/vt-d: Expose ISA direct mapping region via iommu_get_resv_regions
From
Date
Hi,

On 3/25/19 8:57 PM, James Sewart wrote:
> Hey Lu,
>
>> On 25 Mar 2019, at 02:03, Lu Baolu <baolu.lu@linux.intel.com> wrote:
>>
>> Hi James,
>>
>> On 3/22/19 5:57 PM, James Sewart wrote:
>>> Hey Lu,
>>>> On 15 Mar 2019, at 02:19, Lu Baolu <baolu.lu@linux.intel.com> wrote:
>>>>
>>>> Hi James,
>>>>
>>>> On 3/14/19 7:58 PM, James Sewart wrote:
>>>>> To support mapping ISA region via iommu_group_create_direct_mappings,
>>>>> make sure its exposed by iommu_get_resv_regions. This allows
>>>>> deduplication of reserved region mappings
>>>>> Signed-off-by: James Sewart <jamessewart@arista.com>
>>>>> ---
>>>>> drivers/iommu/intel-iommu.c | 42 +++++++++++++++++++++++++++++--------
>>>>> 1 file changed, 33 insertions(+), 9 deletions(-)
>>>>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>>>>> index 8e0a4e2ff77f..2e00e8708f06 100644
>>>>> --- a/drivers/iommu/intel-iommu.c
>>>>> +++ b/drivers/iommu/intel-iommu.c
>>>>> @@ -337,6 +337,8 @@ static LIST_HEAD(dmar_rmrr_units);
>>>>> #define for_each_rmrr_units(rmrr) \
>>>>> list_for_each_entry(rmrr, &dmar_rmrr_units, list)
>>>>> +static struct iommu_resv_region *isa_resv_region;
>>>>> +
>>>>> /* bitmap for indexing intel_iommus */
>>>>> static int g_num_of_iommus;
>>>>> @@ -2780,26 +2782,34 @@ static inline int iommu_prepare_rmrr_dev(struct dmar_rmrr_unit *rmrr,
>>>>> rmrr->end_address);
>>>>> }
>>>>> +static inline struct iommu_resv_region *iommu_get_isa_resv_region(void)
>>>>> +{
>>>>> + if (!isa_resv_region)
>>>>> + isa_resv_region = iommu_alloc_resv_region(0,
>>>>> + 16*1024*1024,
>>>>> + 0, IOMMU_RESV_DIRECT);
>>>>> +
>>>>> + return isa_resv_region;
>>>>> +}
>>>>> +
>>>>> #ifdef CONFIG_INTEL_IOMMU_FLOPPY_WA
>>>>> -static inline void iommu_prepare_isa(void)
>>>>> +static inline void iommu_prepare_isa(struct pci_dev *pdev)
>>>>> {
>>>>> - struct pci_dev *pdev;
>>>>> int ret;
>>>>> + struct iommu_resv_region *reg = iommu_get_isa_resv_region();
>>>>> - pdev = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
>>>>> - if (!pdev)
>>>>> + if (!reg)
>>>>> return;
>>>>> pr_info("Prepare 0-16MiB unity mapping for LPC\n");
>>>>> - ret = iommu_prepare_identity_map(&pdev->dev, 0, 16*1024*1024 - 1);
>>>>> + ret = iommu_prepare_identity_map(&pdev->dev, reg->start,
>>>>> + reg->start + reg->length - 1);
>>>>> if (ret)
>>>>> pr_err("Failed to create 0-16MiB identity map - floppy might not work\n");
>>>>> -
>>>>> - pci_dev_put(pdev);
>>>>> }
>>>>> #else
>>>>> -static inline void iommu_prepare_isa(void)
>>>>> +static inline void iommu_prepare_isa(struct pci_dev *pdev)
>>>>> {
>>>>> return;
>>>>> }
>>>>> @@ -3289,6 +3299,7 @@ static int __init init_dmars(void)
>>>>> struct dmar_rmrr_unit *rmrr;
>>>>> bool copied_tables = false;
>>>>> struct device *dev;
>>>>> + struct pci_dev *pdev;
>>>>> struct intel_iommu *iommu;
>>>>> int i, ret;
>>>>> @@ -3469,7 +3480,11 @@ static int __init init_dmars(void)
>>>>> }
>>>>> }
>>>>> - iommu_prepare_isa();
>>>>> + pdev = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
>>>>> + if (pdev) {
>>>>> + iommu_prepare_isa(pdev);
>>>>> + pci_dev_put(pdev);
>>>>> + }
>>>>> domains_done:
>>>>> @@ -5266,6 +5281,7 @@ static void intel_iommu_get_resv_regions(struct device *device,
>>>>> struct iommu_resv_region *reg;
>>>>> struct dmar_rmrr_unit *rmrr;
>>>>> struct device *i_dev;
>>>>> + struct pci_dev *pdev;
>>>>> int i;
>>>>> rcu_read_lock();
>>>>> @@ -5280,6 +5296,14 @@ static void intel_iommu_get_resv_regions(struct device *device,
>>>>> }
>>>>> rcu_read_unlock();
>>>>> + if (dev_is_pci(device)) {
>>>>> + pdev = to_pci_dev(device);
>>>>> + if ((pdev->class >> 8) == PCI_CLASS_BRIDGE_ISA) {
>>>>> + reg = iommu_get_isa_resv_region();
>>>>> + list_add_tail(&reg->list, head);
>>>>> + }
>>>>> + }
>>>>> +
>>>>
>>>> Just wondering why not just
>>>>
>>>> +#ifdef CONFIG_INTEL_IOMMU_FLOPPY_WA
>>>> + if (dev_is_pci(device)) {
>>>> + pdev = to_pci_dev(device);
>>>> + if ((pdev->class >> 8) == PCI_CLASS_BRIDGE_ISA) {
>>>> + reg = iommu_alloc_resv_region(0,
>>>> + 16*1024*1024,
>>>> + 0, IOMMU_RESV_DIRECT);
>>>> + if (reg)
>>>> + list_add_tail(&reg->list, head);
>>>> + }
>>>> + }
>>>> +#endif
>>>>
>>>> and, remove all other related code?
>>> At this point in the patchset if we remove iommu_prepare_isa then the ISA
>>> region won’t be mapped to the device. Only once the dma domain is allocable
>>> will the reserved regions be mapped by iommu_group_create_direct_mappings.
>>
>> Yes. So if we put the allocation code here, it won't impact anything and
>> will take effect as soon as the dma domain is allocatable.
>>
>>> Theres an issue that if we choose to alloc a new resv_region with type
>>> IOMMU_RESV_DIRECT, we will need to refactor intel_iommu_put_resv_regions
>>> to free this entry type which means refactoring the rmrr regions in
>>> get_resv_regions. Should this work be in this patchset?
>>
>> Do you mean the rmrr regions are not allocated in get_resv_regions, but
>> are freed in put_resv_regions? I think we should fix this in this patch
>> set since this might impact the device passthrough if we don't do it.
>
> They’re not allocated and not freed currently, only type IOMMU_RESV_MSI is
> freed in put_resv_regions. If we allocate a new resv_region with type
> IOMMU_RESV_DIRECT for the isa region, then it won’t be freed. If we modify
> put_resv_regions to free type IOMMU_RESV_DIRECT, then we will try to free
> the static RMRR regions.

Ah yes! Thanks for the explanation.

>
> Either the ISA region is static and not freed as with my implementation,
> or the RMRR regions are converted to be allocated on each call to
> get_resv_regions and freed in put_resv_regions.

Okay, I second your implementation now.

>
>>
>> Best regards,
>> Lu Baolu
>
> Cheers,
> James.
>

Best regards,
Lu Baolu

\
 
 \ /
  Last update: 2019-03-26 02:16    [W:0.118 / U:0.544 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site