| 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
| ||
|
Date: Sun, 26 Sep 2021 03:01:37 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: Lu Baolu <baolu.lu@...ux.intel.com>, Joerg Roedel <joro@...tes.org>
CC: "Raj, Ashok" <ashok.raj@...el.com>,
"Liu, Yi L" <yi.l.liu@...el.com>,
"iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/1] iommu/vt-d: Use second level for GPA->HPA translation
> From: Lu Baolu <baolu.lu@...ux.intel.com>
> Sent: Sunday, September 26, 2021 10:43 AM
>
> Hi Kevin,
>
> Thanks for reviewing my patch.
>
> On 9/24/21 11:16 AM, Tian, Kevin wrote:
> >> From: Lu Baolu <baolu.lu@...ux.intel.com>
> >> Sent: Friday, September 24, 2021 10:30 AM
> >>
> >> The IOMMU VT-d implementation uses the first level for GPA->HPA
> >> translation
> >> by default. Although both the first level and the second level could handle
> >> the DMA translation, they are different in some way. For example, the
> >> second
> >> level translation has separate controls for the Access/Dirty page tracking
> >> and the page-level forcing snoop. With first level translation, there're
> >> no such controls. This uses the second level for GPA->HPA translation so
> >
> > first-level has no page-granular snoop control, but has a global control
> > in pasid entry.
>
> Agreed. Will add this.
>
> >
> >> that it could provide a consistent hardware interface for use cases like
> >> dirty page tracking during the VM live migration.
> >>
> >> Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com>
> >> ---
> >> include/linux/intel-iommu.h | 7 ++-----
> >> drivers/iommu/intel/iommu.c | 21 +++++++++++++--------
> >> 2 files changed, 15 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> >> index 05a65eb155f7..a5fb20702201 100644
> >> --- a/include/linux/intel-iommu.h
> >> +++ b/include/linux/intel-iommu.h
> >> @@ -517,22 +517,19 @@ struct context_entry {
> >> u64 hi;
> >> };
> >>
> >> -/* si_domain contains mulitple devices */
> >> -#define DOMAIN_FLAG_STATIC_IDENTITY BIT(0)
> >
> > this is a separate cleanup. better mention it in the commit msg or
> > put in another patch.
>
> Sure.
>
> >
> >> -
> >> /*
> >> * When VT-d works in the scalable mode, it allows DMA translation to
> >> * happen through either first level or second level page table. This
> >> * bit marks that the DMA translation for the domain goes through the
> >> * first level page table, otherwise, it goes through the second level.
> >> */
> >> -#define DOMAIN_FLAG_USE_FIRST_LEVEL BIT(1)
> >> +#define DOMAIN_FLAG_USE_FIRST_LEVEL BIT(0)
> >>
> >> /*
> >> * Domain represents a virtual machine which demands iommu nested
> >> * translation mode support.
> >> */
> >> -#define DOMAIN_FLAG_NESTING_MODE BIT(2)
> >> +#define DOMAIN_FLAG_NESTING_MODE BIT(1)
> >>
> >> struct dmar_domain {
> >> int nid; /* node id */
> >> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> >> index d75f59ae28e6..c814fea0522e 100644
> >> --- a/drivers/iommu/intel/iommu.c
> >> +++ b/drivers/iommu/intel/iommu.c
> >> @@ -522,7 +522,7 @@ static inline void free_devinfo_mem(void *vaddr)
> >>
> >> static inline int domain_type_is_si(struct dmar_domain *domain)
> >> {
> >> - return domain->flags & DOMAIN_FLAG_STATIC_IDENTITY;
> >> + return domain->domain.type == IOMMU_DOMAIN_IDENTITY;
> >> }
> >>
> >> static inline bool domain_use_first_level(struct dmar_domain *domain)
> >> @@ -1874,12 +1874,18 @@ static void free_dmar_iommu(struct
> >> intel_iommu *iommu)
> >> * Check and return whether first level is used by default for
> >> * DMA translation.
> >> */
> >> -static bool first_level_by_default(void)
> >> +static bool first_level_by_default(unsigned int type)
> >> {
> >> - return scalable_mode_support() && intel_cap_flts_sanity();
> >> + if (type == IOMMU_DOMAIN_UNMANAGED)
> >> + return false;
> >> +
> >
> > I think the order is not correct. what about 2nd level is even not
> > present?
>
> Fair enough. How about
>
> #1) hardware only capable of first level, return true
> #2) hardware only capable of second level, return false
>
> (we fail iommu initialization if neither FL nor SL)
> Then, both FL and SL are supported.
>
> #3) domain is type of UNMANAGED, return false
> #4) otherwise, return true.
>
> Does this make sense?
sounds good.
>
> >
> >
> >> + if (!scalable_mode_support() || !intel_cap_flts_sanity())
> >> + return false;
> >> +
> >> + return true;
> >> }
> >>
> >> -static struct dmar_domain *alloc_domain(int flags)
> >> +static struct dmar_domain *alloc_domain(unsigned int type)
> >> {
> >> struct dmar_domain *domain;
> >>
> >> @@ -1889,8 +1895,7 @@ static struct dmar_domain *alloc_domain(int
> flags)
> >>
> >> memset(domain, 0, sizeof(*domain));
> >> domain->nid = NUMA_NO_NODE;
> >> - domain->flags = flags;
> >> - if (first_level_by_default())
> >> + if (first_level_by_default(type))
> >> domain->flags |= DOMAIN_FLAG_USE_FIRST_LEVEL;
> >> domain->has_iotlb_device = false;
> >> INIT_LIST_HEAD(&domain->devices);
> >> @@ -2708,7 +2713,7 @@ static int __init si_domain_init(int hw)
> >> struct device *dev;
> >> int i, nid, ret;
> >>
> >> - si_domain = alloc_domain(DOMAIN_FLAG_STATIC_IDENTITY);
> >> + si_domain = alloc_domain(IOMMU_DOMAIN_IDENTITY);
> >> if (!si_domain)
> >> return -EFAULT;
> >>
> >> @@ -4517,7 +4522,7 @@ static struct iommu_domain
> >> *intel_iommu_domain_alloc(unsigned type)
> >> case IOMMU_DOMAIN_DMA:
> >> case IOMMU_DOMAIN_DMA_FQ:
> >> case IOMMU_DOMAIN_UNMANAGED:
> >> - dmar_domain = alloc_domain(0);
> >> + dmar_domain = alloc_domain(type);
> >> if (!dmar_domain) {
> >> pr_err("Can't allocate dmar_domain\n");
> >> return NULL;
> >> --
> >> 2.25.1
> >
>
> Best regards,
> baolu
Powered by blists - more mailing lists