| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <44298d5c-5720-a382-07d1-a90a072ff24b@linux.intel.com>
Date: Thu, 6 Sep 2018 10:46:03 +0800
From: Lu Baolu <baolu.lu@...ux.intel.com>
To: "Tian, Kevin" <kevin.tian@...el.com>,
Joerg Roedel <joro@...tes.org>,
David Woodhouse <dwmw2@...radead.org>
Cc: baolu.lu@...ux.intel.com, "Raj, Ashok" <ashok.raj@...el.com>,
"Kumar, Sanjay K" <sanjay.k.kumar@...el.com>,
"Pan, Jacob jun" <jacob.jun.pan@...el.com>,
"Liu, Yi L" <yi.l.liu@...el.com>, "Sun, Yi Y" <yi.y.sun@...el.com>,
"peterx@...hat.com" <peterx@...hat.com>,
Jean-Philippe Brucker <jean-philippe.brucker@....com>,
"iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Jacob Pan <jacob.jun.pan@...ux.intel.com>
Subject: Re: [PATCH v2 02/12] iommu/vt-d: Manage scalalble mode PASID tables
Hi,
On 09/06/2018 10:14 AM, Tian, Kevin wrote:
>> From: Lu Baolu [mailto:baolu.lu@...ux.intel.com]
>> Sent: Thursday, August 30, 2018 9:35 AM
>>
>> In scalable mode, pasid structure is a two level table with
>> a pasid directory table and a pasid table. Any pasid entry
>> can be identified by a pasid value in below way.
>>
>> 1
>> 9 6 5 0
>> .-----------------------.-------.
>> | PASID | |
>> '-----------------------'-------' .-------------.
>> | | | |
>> | | | |
>> | | | |
>> | .-----------. | .-------------.
>> | | | |----->| PASID Entry |
>> | | | | '-------------'
>> | | | |Plus | |
>> | .-----------. | | |
>> |---->| DIR Entry |-------->| |
>> | '-----------' '-------------'
>> .---------. |Plus | |
>> | Context | | | |
>> | Entry |------->| |
>> '---------' '-----------'
>>
>> This changes the pasid table APIs to support scalable mode
>> PASID directory and PASID table. It also adds a helper to
>> get the PASID table entry according to the pasid value.
>>
>> Cc: Ashok Raj <ashok.raj@...el.com>
>> Cc: Jacob Pan <jacob.jun.pan@...ux.intel.com>
>> Cc: Kevin Tian <kevin.tian@...el.com>
>> Cc: Liu Yi L <yi.l.liu@...el.com>
>> Signed-off-by: Sanjay Kumar <sanjay.k.kumar@...el.com>
>> Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com>
>> Reviewed-by: Ashok Raj <ashok.raj@...el.com>
>> ---
>> drivers/iommu/intel-iommu.c | 2 +-
>> drivers/iommu/intel-pasid.c | 72 ++++++++++++++++++++++++++++++++----
>> -
>> drivers/iommu/intel-pasid.h | 10 +++++-
>> drivers/iommu/intel-svm.c | 6 +---
>> 4 files changed, 74 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>> index 5845edf4dcf9..b0da4f765274 100644
>> --- a/drivers/iommu/intel-iommu.c
>> +++ b/drivers/iommu/intel-iommu.c
>> @@ -2507,7 +2507,7 @@ static struct dmar_domain
>> *dmar_insert_one_dev_info(struct intel_iommu *iommu,
>> if (dev)
>> dev->archdata.iommu = info;
>>
>> - if (dev && dev_is_pci(dev) && info->pasid_supported) {
>> + if (dev && dev_is_pci(dev) && sm_supported(iommu)) {
>
> worthy of a comment here that PASID table now is mandatory in
> scalable mode, instead of optional for 1st level usage before.
Fair enough. Will add in the next version.
>
>> ret = intel_pasid_alloc_table(dev);
>> if (ret) {
>> __dmar_remove_one_dev_info(info);
>> diff --git a/drivers/iommu/intel-pasid.c b/drivers/iommu/intel-pasid.c
>> index fe95c9bd4d33..d6e90cd5b062 100644
>> --- a/drivers/iommu/intel-pasid.c
>> +++ b/drivers/iommu/intel-pasid.c
>> @@ -127,8 +127,7 @@ int intel_pasid_alloc_table(struct device *dev)
>> int ret, order;
>>
>> info = dev->archdata.iommu;
>> - if (WARN_ON(!info || !dev_is_pci(dev) ||
>> - !info->pasid_supported || info->pasid_table))
>> + if (WARN_ON(!info || !dev_is_pci(dev) || info->pasid_table))
>> return -EINVAL;
>
> following same logic should you check sm_supported here?
If not sm_supported, info->pasid_table should be NULL. Checking
info->pasid_table is better since even sm_supported, the pasid
table pointer could still possible to be empty.
>
>>
>> /* DMA alias device already has a pasid table, use it: */
>> @@ -143,8 +142,9 @@ int intel_pasid_alloc_table(struct device *dev)
>> return -ENOMEM;
>> INIT_LIST_HEAD(&pasid_table->dev);
>>
>> - size = sizeof(struct pasid_entry);
>> + size = sizeof(struct pasid_dir_entry);
>> count = min_t(int, pci_max_pasids(to_pci_dev(dev)),
>> intel_pasid_max_id);
>> + count >>= PASID_PDE_SHIFT;
>> order = get_order(size * count);
>> pages = alloc_pages_node(info->iommu->node,
>> GFP_ATOMIC | __GFP_ZERO,
>> @@ -154,7 +154,7 @@ int intel_pasid_alloc_table(struct device *dev)
>>
>> pasid_table->table = page_address(pages);
>> pasid_table->order = order;
>> - pasid_table->max_pasid = count;
>> + pasid_table->max_pasid = count << PASID_PDE_SHIFT;
>
> are you sure of that count is PDE_SHIFT aligned? otherwise >>
> then << would lose some bits. If sure, then better add some check.
I am making the max_pasid PDE_SHIFT aligned as the result of shift
operations.
>
>>
>> attach_out:
>> device_attach_pasid_table(info, pasid_table);
>> @@ -162,14 +162,33 @@ int intel_pasid_alloc_table(struct device *dev)
>> return 0;
>> }
>>
>> +/* Get PRESENT bit of a PASID directory entry. */
>> +static inline bool
>> +pasid_pde_is_present(struct pasid_dir_entry *pde)
>> +{
>> + return READ_ONCE(pde->val) & PASID_PTE_PRESENT;
>
> curious why adding READ_ONCE specifically for PASID structure,
> but not used for any other existing vtd structures? Is it to address
> some specific requirement on PASID structure as defined in spec?
READ/WRITE_ONCE are used in pasid entry read/write to prevent the
compiler from merging, refetching or reordering successive instances of
read/write.
>
>> +}
>> +
>> +/* Get PASID table from a PASID directory entry. */
>> +static inline struct pasid_entry *
>> +get_pasid_table_from_pde(struct pasid_dir_entry *pde)
>> +{
>> + if (!pasid_pde_is_present(pde))
>> + return NULL;
>> +
>> + return phys_to_virt(READ_ONCE(pde->val) & PDE_PFN_MASK);
>> +}
>> +
>> void intel_pasid_free_table(struct device *dev)
>> {
>> struct device_domain_info *info;
>> struct pasid_table *pasid_table;
>> + struct pasid_dir_entry *dir;
>> + struct pasid_entry *table;
>> + int i, max_pde;
>>
>> info = dev->archdata.iommu;
>> - if (!info || !dev_is_pci(dev) ||
>> - !info->pasid_supported || !info->pasid_table)
>> + if (!info || !dev_is_pci(dev) || !info->pasid_table)
>> return;
>>
>> pasid_table = info->pasid_table;
>> @@ -178,6 +197,14 @@ void intel_pasid_free_table(struct device *dev)
>> if (!list_empty(&pasid_table->dev))
>> return;
>>
>> + /* Free scalable mode PASID directory tables: */
>> + dir = pasid_table->table;
>> + max_pde = pasid_table->max_pasid >> PASID_PDE_SHIFT;
>> + for (i = 0; i < max_pde; i++) {
>> + table = get_pasid_table_from_pde(&dir[i]);
>> + free_pgtable_page(table);
>> + }
>> +
>> free_pages((unsigned long)pasid_table->table, pasid_table->order);
>> kfree(pasid_table);
>> }
>> @@ -206,17 +233,37 @@ int intel_pasid_get_dev_max_id(struct device
>> *dev)
>>
>> struct pasid_entry *intel_pasid_get_entry(struct device *dev, int pasid)
>> {
>> + struct device_domain_info *info;
>> struct pasid_table *pasid_table;
>> + struct pasid_dir_entry *dir;
>> struct pasid_entry *entries;
>> + int dir_index, index;
>>
>> pasid_table = intel_pasid_get_table(dev);
>> if (WARN_ON(!pasid_table || pasid < 0 ||
>> pasid >= intel_pasid_get_dev_max_id(dev)))
>> return NULL;
>>
>> - entries = pasid_table->table;
>> + dir = pasid_table->table;
>> + info = dev->archdata.iommu;
>> + dir_index = pasid >> PASID_PDE_SHIFT;
>> + index = pasid & PASID_PTE_MASK;
>> +
>> + spin_lock(&pasid_lock);
>> + entries = get_pasid_table_from_pde(&dir[dir_index]);
>> + if (!entries) {
>> + entries = alloc_pgtable_page(info->iommu->node);
>> + if (!entries) {
>> + spin_unlock(&pasid_lock);
>> + return NULL;
>> + }
>> +
>> + WRITE_ONCE(dir[dir_index].val,
>> + (u64)virt_to_phys(entries) | PASID_PTE_PRESENT);
>> + }
>> + spin_unlock(&pasid_lock);
>>
>> - return &entries[pasid];
>> + return &entries[index];
>> }
>>
>> /*
>> @@ -224,7 +271,14 @@ struct pasid_entry *intel_pasid_get_entry(struct
>> device *dev, int pasid)
>> */
>> static inline void pasid_clear_entry(struct pasid_entry *pe)
>> {
>> - WRITE_ONCE(pe->val, 0);
>> + WRITE_ONCE(pe->val[0], 0);
>> + WRITE_ONCE(pe->val[1], 0);
>> + WRITE_ONCE(pe->val[2], 0);
>> + WRITE_ONCE(pe->val[3], 0);
>> + WRITE_ONCE(pe->val[4], 0);
>> + WRITE_ONCE(pe->val[5], 0);
>> + WRITE_ONCE(pe->val[6], 0);
>> + WRITE_ONCE(pe->val[7], 0);
>
> memset?
The order is important here. Otherwise, the PRESENT bit of this pasid
entry might still set while other fields contains invalid values.
>
>> }
>>
>> void intel_pasid_clear_entry(struct device *dev, int pasid)
>> diff --git a/drivers/iommu/intel-pasid.h b/drivers/iommu/intel-pasid.h
>> index 1c05ed6fc5a5..12f480c2bb8b 100644
>> --- a/drivers/iommu/intel-pasid.h
>> +++ b/drivers/iommu/intel-pasid.h
>> @@ -12,11 +12,19 @@
>>
>> #define PASID_MIN 0x1
>> #define PASID_MAX 0x100000
>> +#define PASID_PTE_MASK 0x3F
>> +#define PASID_PTE_PRESENT 1
>> +#define PDE_PFN_MASK PAGE_MASK
>> +#define PASID_PDE_SHIFT 6
>>
>> -struct pasid_entry {
>> +struct pasid_dir_entry {
>> u64 val;
>> };
>>
>> +struct pasid_entry {
>> + u64 val[8];
>> +};
>> +
>> /* The representative of a PASID table */
>> struct pasid_table {
>> void *table; /* pasid table pointer */
>> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
>> index 4a03e5090952..6c0bd9ee9602 100644
>> --- a/drivers/iommu/intel-svm.c
>> +++ b/drivers/iommu/intel-svm.c
>> @@ -65,8 +65,6 @@ int intel_svm_init(struct intel_iommu *iommu)
>>
>> order = get_order(sizeof(struct pasid_entry) * iommu->pasid_max);
>> if (ecap_dis(iommu->ecap)) {
>> - /* Just making it explicit... */
>> - BUILD_BUG_ON(sizeof(struct pasid_entry) != sizeof(struct
>> pasid_state_entry));
>> pages = alloc_pages(GFP_KERNEL | __GFP_ZERO, order);
>> if (pages)
>> iommu->pasid_state_table = page_address(pages);
>> @@ -406,9 +404,7 @@ int intel_svm_bind_mm(struct device *dev, int
>> *pasid, int flags, struct svm_dev_
>> pasid_entry_val |= PASID_ENTRY_FLPM_5LP;
>>
>> entry = intel_pasid_get_entry(dev, svm->pasid);
>> - entry->val = pasid_entry_val;
>> -
>> - wmb();
>> + WRITE_ONCE(entry->val[0], pasid_entry_val);
>>
>> /*
>> * Flush PASID cache when a PASID table entry becomes
>> --
>> 2.17.1
>
>
Best regards,
Lu Baolu
Powered by blists - more mailing lists