From mboxrd@z Thu Jan 1 00:00:00 1970 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932948AbeAKVJW (ORCPT + 1 other); Thu, 11 Jan 2018 16:09:22 -0500 Received: from mga02.intel.com ([134.134.136.20]:64396 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932290AbeAKVJV (ORCPT ); Thu, 11 Jan 2018 16:09:21 -0500 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,346,1511856000"; d="scan'208";a="10077245" Date: Thu, 11 Jan 2018 13:10:55 -0800 From: Jacob Pan To: Jean-Philippe Brucker Cc: "iommu@lists.linux-foundation.org" , LKML , Joerg Roedel , David Woodhouse , Greg Kroah-Hartman , Rafael Wysocki , Alex Williamson , Lan Tianyu , Yi L , "Liu@mail.linuxfoundation.org" , jacob.jun.pan@linux.intel.com Subject: Re: [PATCH v3 08/16] iommu: introduce device fault data Message-ID: <20180111131055.436b7ccc@jacob-builder> In-Reply-To: References: <1510944914-54430-1-git-send-email-jacob.jun.pan@linux.intel.com> <1510944914-54430-9-git-send-email-jacob.jun.pan@linux.intel.com> Organization: OTC X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On Wed, 10 Jan 2018 11:41:58 +0000 Jean-Philippe Brucker wrote: > Hi Jacob, > > On 17/11/17 18:55, Jacob Pan wrote: > [...] > > +/** > > + * struct iommu_fault_event - Generic per device fault data > > + * > > + * - PCI and non-PCI devices > > + * - Recoverable faults (e.g. page request), information based on > > PCI ATS > > + * and PASID spec. > > + * - Un-recoverable faults of device interest > > + * - DMA remapping and IRQ remapping faults > > + > > + * @type contains fault type. > > + * @reason fault reasons if relevant outside IOMMU driver, IOMMU > > driver internal > > + * faults are not reported > > + * @addr: tells the offending page address > > + * @pasid: contains process address space ID, used in shared > > virtual memory(SVM) > > + * @rid: requestor ID > > + * @page_req_group_id: page request group index > > + * @last_req: last request in a page request group > > + * @pasid_valid: indicates if the PRQ has a valid PASID > > + * @prot: page access protection flag, e.g. IOMMU_FAULT_READ, > > IOMMU_FAULT_WRITE > > + * @device_private: if present, uniquely identify device-specific > > + * private data for an individual page request. > > + * @iommu_private: used by the IOMMU driver for storing > > fault-specific > > + * data. Users should not modify this field before > > + * sending the fault response. > > + */ > > +struct iommu_fault_event { > > + enum iommu_fault_type type; > > + enum iommu_fault_reason reason; > > + u64 addr; > > + u32 pasid; > > + u32 page_req_group_id : 9; > > As I've been rebasing my work onto your series, I have a few more > comments about this structure. Is there any advantage in limiting the > PRGI as a bitfield? PCI uses 9 bits, but others might need more. For > instance ARM Stall uses 16-bit IDs to identify a fault event. > > Could you please make it a u32 (as well as in page_response_msg), and > could page_req_group_id be renamed to simply "id"? > sure, I will make it u32 in v4 version of the patchset. I was using PCI standard as a base with no specific advantage. I am running into little bit problem with testing, so perhaps next week. > > + u32 last_req : 1; > > + u32 pasid_valid : 1; > I noticed that page_response_msg in patch 15/16 calls this bit > "pasid_present". Could you rename it to "pasid_valid" for consistency? > make sense. > Thanks, > Jean [Jacob Pan] From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jacob Pan Subject: Re: [PATCH v3 08/16] iommu: introduce device fault data Date: Thu, 11 Jan 2018 13:10:55 -0800 Message-ID: <20180111131055.436b7ccc@jacob-builder> References: <1510944914-54430-1-git-send-email-jacob.jun.pan@linux.intel.com> <1510944914-54430-9-git-send-email-jacob.jun.pan@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Jean-Philippe Brucker Cc: Lan Tianyu , Yi L , "Liu-i9wRM+HIrmnmtl4Z8vJ8Kg761KYD1DLY@public.gmane.org" , Greg Kroah-Hartman , Rafael Wysocki , LKML , "iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org" , David Woodhouse List-Id: iommu@lists.linux-foundation.org On Wed, 10 Jan 2018 11:41:58 +0000 Jean-Philippe Brucker wrote: > Hi Jacob, > > On 17/11/17 18:55, Jacob Pan wrote: > [...] > > +/** > > + * struct iommu_fault_event - Generic per device fault data > > + * > > + * - PCI and non-PCI devices > > + * - Recoverable faults (e.g. page request), information based on > > PCI ATS > > + * and PASID spec. > > + * - Un-recoverable faults of device interest > > + * - DMA remapping and IRQ remapping faults > > + > > + * @type contains fault type. > > + * @reason fault reasons if relevant outside IOMMU driver, IOMMU > > driver internal > > + * faults are not reported > > + * @addr: tells the offending page address > > + * @pasid: contains process address space ID, used in shared > > virtual memory(SVM) > > + * @rid: requestor ID > > + * @page_req_group_id: page request group index > > + * @last_req: last request in a page request group > > + * @pasid_valid: indicates if the PRQ has a valid PASID > > + * @prot: page access protection flag, e.g. IOMMU_FAULT_READ, > > IOMMU_FAULT_WRITE > > + * @device_private: if present, uniquely identify device-specific > > + * private data for an individual page request. > > + * @iommu_private: used by the IOMMU driver for storing > > fault-specific > > + * data. Users should not modify this field before > > + * sending the fault response. > > + */ > > +struct iommu_fault_event { > > + enum iommu_fault_type type; > > + enum iommu_fault_reason reason; > > + u64 addr; > > + u32 pasid; > > + u32 page_req_group_id : 9; > > As I've been rebasing my work onto your series, I have a few more > comments about this structure. Is there any advantage in limiting the > PRGI as a bitfield? PCI uses 9 bits, but others might need more. For > instance ARM Stall uses 16-bit IDs to identify a fault event. > > Could you please make it a u32 (as well as in page_response_msg), and > could page_req_group_id be renamed to simply "id"? > sure, I will make it u32 in v4 version of the patchset. I was using PCI standard as a base with no specific advantage. I am running into little bit problem with testing, so perhaps next week. > > + u32 last_req : 1; > > + u32 pasid_valid : 1; > I noticed that page_response_msg in patch 15/16 calls this bit > "pasid_present". Could you rename it to "pasid_valid" for consistency? > make sense. > Thanks, > Jean [Jacob Pan]