| 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: <20240904083936.ftqz4sbuykk6mbjy@joelS2.panther.com>
Date: Wed, 4 Sep 2024 10:39:36 +0200
From: Joel Granados <j.granados@...sung.com>
To: Baolu Lu <baolu.lu@...ux.intel.com>
CC: Klaus Jensen <its@...elevant.dk>, David Woodhouse <dwmw2@...radead.org>,
Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>, Robin Murphy
<robin.murphy@....com>, Jason Gunthorpe <jgg@...pe.ca>, Kevin Tian
<kevin.tian@...el.com>, Minwoo Im <minwoo.im@...sung.com>,
<linux-kernel@...r.kernel.org>, <iommu@...ts.linux.dev>, Klaus Jensen
<k.jensen@...sung.com>
Subject: Re: [PATCH RFC PREVIEW 1/6] iommu/vt-d: Separate page request queue
from SVM
On Sun, Sep 01, 2024 at 01:16:43PM +0800, Baolu Lu wrote:
> On 2024/8/26 19:40, Klaus Jensen wrote:
> > From: Joel Granados<j.granados@...sung.com>
> >
> > IO page faults are no longer dependent on CONFIG_INTEL_IOMMU_SVM. Move
> > all Page Request Queue (PRQ) functions that handle prq events to a new
> > file in drivers/iommu/intel/prq.c. The page_req_des struct is made
> > available in drivers/iommu/intel/iommu.h.
> >
> > No functional changes are intended. This is a preparation patch to
> > enable the use of IO page faults outside the SVM and nested use cases.
> >
> > Signed-off-by: Joel Granados<j.granados@...sung.com>
> > ---
> > drivers/iommu/intel/Makefile | 2 +-
> > drivers/iommu/intel/iommu.c | 18 +--
> > drivers/iommu/intel/iommu.h | 40 +++++-
> > drivers/iommu/intel/prq.c | 290 ++++++++++++++++++++++++++++++++++++++++
> > drivers/iommu/intel/svm.c | 308 -------------------------------------------
> > 5 files changed, 331 insertions(+), 327 deletions(-)
...
> > diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
> > index b67c14da1240..b3d98e706ed8 100644
> > --- a/drivers/iommu/intel/iommu.h
> > +++ b/drivers/iommu/intel/iommu.h
> > @@ -694,6 +694,35 @@ struct iommu_pmu {
> > #define IOMMU_IRQ_ID_OFFSET_PRQ (DMAR_UNITS_SUPPORTED)
> > #define IOMMU_IRQ_ID_OFFSET_PERF (2 * DMAR_UNITS_SUPPORTED)
> >
> > +/* Page request queue descriptor */
> > +struct page_req_dsc {
> > + union {
> > + struct {
> > + u64 type:8;
> > + u64 pasid_present:1;
> > + u64 rsvd:7;
> > + u64 rid:16;
> > + u64 pasid:20;
> > + u64 exe_req:1;
> > + u64 pm_req:1;
> > + u64 rsvd2:10;
> > + };
> > + u64 qw_0;
> > + };
> > + union {
> > + struct {
> > + u64 rd_req:1;
> > + u64 wr_req:1;
> > + u64 lpig:1;
> > + u64 prg_index:9;
> > + u64 addr:52;
> > + };
> > + u64 qw_1;
> > + };
> > + u64 qw_2;
> > + u64 qw_3;
> > +};
>
> Why not move this structure to prq.c? It is specific to that file. Or
> not?
I had left it here because it was included in `struct intel_iommu` but
since it is just a pointer, I think it can be put there if it makes more
sense.
>
> > +
> > struct intel_iommu {
> > void __iomem *reg; /* Pointer to hardware regs, virtual addr */
> > u64 reg_phys; /* physical address of hw register set */
> > @@ -719,12 +748,10 @@ struct intel_iommu {
> >
> > struct iommu_flush flush;
> > #endif
> > -#ifdef CONFIG_INTEL_IOMMU_SVM
> > struct page_req_dsc *prq;
> > unsigned char prq_name[16]; /* Name for PRQ interrupt */
> > unsigned long prq_seq_number;
> > struct completion prq_complete;
> > -#endif
> > struct iopf_queue *iopf_queue;
> > unsigned char iopfq_name[16];
> > /* Synchronization between fault report and iommu device release. */
> > @@ -1156,12 +1183,13 @@ void intel_context_flush_present(struct device_domain_info *info,
> > struct context_entry *context,
> > bool affect_domains);
> >
> > +int intel_enable_prq(struct intel_iommu *iommu);
> > +int intel_finish_prq(struct intel_iommu *iommu);
> > +void intel_page_response(struct device *dev, struct iopf_fault *evt,
> > + struct iommu_page_response *msg);
> > +
> > #ifdef CONFIG_INTEL_IOMMU_SVM
> > void intel_svm_check(struct intel_iommu *iommu);
> > -int intel_svm_enable_prq(struct intel_iommu *iommu);
> > -int intel_svm_finish_prq(struct intel_iommu *iommu);
> > -void intel_svm_page_response(struct device *dev, struct iopf_fault *evt,
> > - struct iommu_page_response *msg);
> > struct iommu_domain *intel_svm_domain_alloc(struct device *dev,
> > struct mm_struct *mm);
> > void intel_drain_pasid_prq(struct device *dev, u32 pasid);
> > diff --git a/drivers/iommu/intel/prq.c b/drivers/iommu/intel/prq.c
> > new file mode 100644
> > index 000000000000..2814373e95d8
> > --- /dev/null
> > +++ b/drivers/iommu/intel/prq.c
> > @@ -0,0 +1,290 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright © 2015 Intel Corporation.
> > + *
> > + * Authors: David Woodhouse<dwmw2@...radead.org>
>
> Many contributors have worked on the code moved in this change. The
> original authorship is no longer relevant.
>
> Consider adding a comment like 'Split from svm.c' to document the
> origin.
Good point. Will do that.
> > + */
> > +
> > +#include <linux/pci.h>
> > +
> > +#include "iommu.h"
> > +#include "../iommu-pages.h"
> > +#include "trace.h"
> > +
> > +static bool is_canonical_address(u64 addr)
> > +{
> > + int shift = 64 - (__VIRTUAL_MASK_SHIFT + 1);
> > + long saddr = (long) addr;
> > +
> > + return (((saddr << shift) >> shift) == saddr);
> > +}
...
> > + QI_PGRP_RESP_CODE(msg->code) |
> > + QI_PGRP_RESP_TYPE;
> > + desc.qw1 = QI_PGRP_IDX(prm->grpid) | QI_PGRP_LPIG(last_page);
> > + desc.qw2 = 0;
> > + desc.qw3 = 0;
> > +
> > + qi_submit_sync(iommu, &desc, 1, 0);
> > +}
>
> The intel_drain_pasid_prq() helper should be moved to prq.c. It's no
> longer specific to SVM.
Oops. missed that one. Will do.
Will address these and the rest of the comments in my V1
Thx for the review
Best
--
Joel Granados
Powered by blists - more mailing lists