Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Tian, Kevin" <kevin.tian@intel.com>
To:     Jean-Philippe Brucker <jean-philippe@linaro.org>,
        Baolu Lu <baolu.lu@linux.intel.com>
CC:     Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
        "Robin Murphy" <robin.murphy@arm.com>,
        Jason Gunthorpe <jgg@ziepe.ca>,
        Nicolin Chen <nicolinc@nvidia.com>,
        "Liu, Yi L" <yi.l.liu@intel.com>,
        Jacob Pan <jacob.jun.pan@linux.intel.com>,
        "iommu@lists.linux.dev" <iommu@lists.linux.dev>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH 1/9] iommu: Move iommu fault data to linux/iommu.h
Thread-Topic: [PATCH 1/9] iommu: Move iommu fault data to linux/iommu.h
Thread-Index: AQHZs5RFjSKO9r3Ol0q1EtvhMNZbRK+0EKaQgAFTYQCAAHy3AIABJTsA
Date:   Thu, 13 Jul 2023 03:22:20 +0000
Message-ID: <BN9PR11MB5276C09E743E99D92BB5C1B28C37A@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20230711010642.19707-1-baolu.lu@linux.intel.com>
 <20230711010642.19707-2-baolu.lu@linux.intel.com>
 <BN9PR11MB5276859ED6825C0A496C9C5E8C31A@BN9PR11MB5276.namprd11.prod.outlook.com>
 <38c31fb0-1068-4855-c896-27d6a2bca747@linux.intel.com>
 <20230712093344.GA507884@myrica>
In-Reply-To: <20230712093344.GA507884@myrica>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?A5nJaSGnCxt8AvFjzHjA3QGN0yMbG6ahYoKr19KAkBOZ4XUhx9nRVhstlCuY?=
 =?us-ascii?Q?lv4oC+DlyJQI+0Jd395BX/A1ESD6VZgruJigGSDYqsxSjmgk8wUHcPqDp/Ak?=
 =?us-ascii?Q?qbn/IOszhi1rB5NBHuKLBLSUEa6JgW3llehojlwOhblyLT8pXv3arBdi9nH+?=
 =?us-ascii?Q?0MVa2EytCFhSovWOWmMOtSIO/A3XPMrPSqbuHRYn4dHMSq6hVtgG3vVv8JSz?=
 =?us-ascii?Q?DzfdP/H7b50F2UDpotmc/XdRB9F6iLug3gbU8F+rO95IDOX4Nlv0LM/zf4g+?=
 =?us-ascii?Q?0qL5iFr+0bz+wqQPcuRRKYkdIr6tGVHlbESsxVm4tJbumVUHb6ppwupzO9kc?=
 =?us-ascii?Q?x1ktheSQYr/ZLDKLNbE4JPD+aVpX3Q2FirYfadMD1x75vjBE0vNrr48veEX8?=
 =?us-ascii?Q?sKnrBzCWmZFZ//HObsbVG8XzcIyzyZXg1oIKkZTzJzRw2gwta6Bhmsbwf1+M?=
 =?us-ascii?Q?Gp/uh9bwAKsiHxdEUDWfJxIGZGmAFiz352I6Te31YrYsqvNqX4S0/FIMYXgz?=
 =?us-ascii?Q?cJPAyBkobbqzAXHXfd+IcnT6fIVjXb8hcjtMkx3yB/RsbvuAul5y5jcv5g6H?=
 =?us-ascii?Q?/mvfVXZWaRgttKs/rL3mQ7V7zLevmk9dZi6MDfNbw8JuYBnNJuiwJSumXspM?=
 =?us-ascii?Q?LAq2uLIHauSV658OzJx7WP0CMFR4ncT0H7RPJunari2KrylhI3fLZqCJcQu2?=
 =?us-ascii?Q?0Ftc/NhsuqRtZveaxxZsB2usZM1ecBMGzfzu/sTm6AGQM0c/6QLxJLR2UNT8?=
 =?us-ascii?Q?95dtPmN3RXZnujnmSeqMhy9MMlMvBqmVlBDusb/3xL2Y8zfAXvj7c7Yewkc3?=
 =?us-ascii?Q?uYEkWVGzESjLDR1mN/Q1u8UKWDmO1Yj+1RhQdE+tdDp4F8j0W5bkj7mw6iIu?=
 =?us-ascii?Q?HTOQ6dO+6EmO8KHsdmwUzYw64aB91zeXHYtuIzUah/ELS0bKuxhIJ9ir9Nov?=
 =?us-ascii?Q?Dj0NonW/X5l/iOCkACsIrMrD7iqrZoM+Fqjcz5BZaN29+ImGvV6Fg/4stB77?=
 =?us-ascii?Q?sEuiNv6/hrvCwvw7uT9mQfHmEVLlSjS7rx9UYBFvIzZz/imdNdXWYoFnIcKw?=
 =?us-ascii?Q?6pYKmxfr4F6WNEJTEIwE4j8nmripdua7bl2Dl0I4c10TYf8FRCbpHSiQfq4X?=
 =?us-ascii?Q?990Ipi/5sdL4S3aeMuvIH1L/HGSWMYZ3SXvlqe2GMDgQs8GARzDpDcN4IeCU?=
 =?us-ascii?Q?cwXeSU5nADZffUnCejAjAapaNH8cNkmpMVNUS4b7HTLrl+3er5KWfaNX/QML?=
 =?us-ascii?Q?47SfQc14ickTAc+zNkQ43n58GaVSk9fttoYlsW5suToU2Wf5C/KRme3CYAKj?=
 =?us-ascii?Q?7VQjKOloeOXCNzxCfWNefM1ja/b+vfBew0tQ5y8AMsGhfuLNAfmw2cFlvWOu?=
 =?us-ascii?Q?o8dYW6sAV5Y3Xh9X0d5T/Qf/IDmPX5QZcSDPWzK1Yf2eESkhD/az0aW3aKuD?=
 =?us-ascii?Q?GKS5lUuEJhThqmQ7DE634Mu2i3p/xV2lC39KvriQOqZPnLFEypw2mlT6/bhL?=
 =?us-ascii?Q?faJGO2wTnbMY5idl1yajcfoS706Qai8rHBEpAHSTa/eN/2HhoWalONdKDYK9?=
 =?us-ascii?Q?K+Caea1gX3UBg9v1XrYwGG/8eIlHVi0T5XxYTByt?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN9PR11MB5276.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 318f327f-b062-46a3-600f-08db83505e0f
X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Jul 2023 03:22:20.1258
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 2mNzpDHl8wLHuKAWk47ArkiSX+9CYsERf3UAKqYT2Vc9o016UuRxx6HFisO89c2NoLFuwfCrmt5VLhhFpQeebQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB5876
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,
        RCVD_IN_DNSWL_BLOCKED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Sent: Wednesday, July 12, 2023 5:34 PM
>=20
> On Wed, Jul 12, 2023 at 10:07:22AM +0800, Baolu Lu wrote:
> > > > +/**
> > > > + * struct iommu_fault_unrecoverable - Unrecoverable fault data
> > > > + * @reason: reason of the fault, from &enum iommu_fault_reason
> > > > + * @flags: parameters of this fault (IOMMU_FAULT_UNRECOV_*
> values)
> > > > + * @pasid: Process Address Space ID
> > > > + * @perm: requested permission access using by the incoming
> transaction
> > > > + *        (IOMMU_FAULT_PERM_* values)
> > > > + * @addr: offending page address
> > > > + * @fetch_addr: address that caused a fetch abort, if any
> > > > + */
> > > > +struct iommu_fault_unrecoverable {
> > > > +	__u32	reason;
> > > > +#define IOMMU_FAULT_UNRECOV_PASID_VALID		(1 <<
> 0)
> > > > +#define IOMMU_FAULT_UNRECOV_ADDR_VALID		(1 <<
> 1)
> > > > +#define IOMMU_FAULT_UNRECOV_FETCH_ADDR_VALID	(1 <<
> 2)
> > > > +	__u32	flags;
> > > > +	__u32	pasid;
> > > > +	__u32	perm;
> > > > +	__u64	addr;
> > > > +	__u64	fetch_addr;
> > > > +};
> > >
> > > Currently there is no handler for unrecoverable faults.
>=20
> Yes those were meant for guest injection. Another goal was to replace
> report_iommu_fault(), which also passes unrecoverable faults to host
> drivers. Three drivers use that API:
> * usnic just prints the error, which could be done by the IOMMU driver,
> * remoteproc attempts to recover from the crash,
> * msm attempts to handle the fault, or at least recover from the crash.

I was not aware of them. Thanks for pointing out.

>=20
> So the first one can be removed, and the others could move over to IOPF
> (which may need to indicate that the fault is not actually recoverable by
> the IOMMU) and return IOMMU_PAGE_RESP_INVALID.

Yep, presumably we should have just one interface to handle fault.

>=20
> > >
> > > Both Intel/ARM register iommu_queue_iopf() as the device fault handle=
r.
> > > It returns -EOPNOTSUPP for unrecoverable faults.
> > >
> > > In your series the common iommu_handle_io_pgfault() also only works
> > > for PRQ.
> > >
> > > It kinds of suggest above definitions are dead code, though arm-smmu-=
v3
> > > does attempt to set them.
> > >
> > > Probably it's right time to remove them.
> > >
> > > In the future even if there might be a need of forwarding unrecoverab=
le
> > > faults to the user via iommufd, fault reasons reported by the physica=
l
> > > IOMMU doesn't make any sense to the guest.
>=20
> I guess it depends on the architecture?  The SMMU driver can report only
> stage-1 faults through iommu_report_device_fault(), which are faults due
> to a guest misconfiguring the tables assigned to it. At the moment
> arm_smmu_handle_evt() only passes down stage-1 page table errors, the
> rest
> is printed by the host.

In that case the kernel just needs to notify the vIOMMU an error happened
along with access permissions (r/w/e/p). vIOMMU can figure out the reason
itself by walking the stage-1 page table. Likely it will find the same reas=
on
as host reports, but that sounds a clearer path in concept.

>=20
> > > Presumably the vIOMMU
> > > should walk guest configurations to set a fault reason which makes se=
nse
> > > from guest p.o.v.
> >
> > I am fine to remove unrecoverable faults data. But it was added by Jean=
,
> > so I'd like to know his opinion on this.
>=20
> Passing errors to the guest could be a useful diagnostics tool for
> debugging, once the guest gets more controls over the IOMMU hardware,
> but
> it doesn't have a purpose beyond that. It could be the only tool
> available, though: to avoid a guest voluntarily flooding the host logs by
> misconfiguring its tables, we may have to disable printing in the host
> errors that come from guest misconfiguration, in which case there won't b=
e
> any diagnostics available for guest bugs.
>=20
> For now I don't mind if they're removed, if there is an easy way to
> reintroduce them later.
>=20

We can keep whatever is required to satisfy the kernel drivers which
want to know the fault.

But for anything invented for old uAPI (e.g. fault_reason) let's remove
them and redefine later when introducing the support to the user.