From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jean-Philippe Brucker Subject: Re: [PATCH v4 04/22] iommu/vt-d: add bind_pasid_table function Date: Thu, 31 May 2018 10:09:46 +0100 Message-ID: <1c1094e8-ecbb-7731-910c-59e4de1e5c70@arm.com> References: <1523915351-54415-1-git-send-email-jacob.jun.pan@linux.intel.com> <1523915351-54415-5-git-send-email-jacob.jun.pan@linux.intel.com> <20180417131047.0a9c310f@w520.home> <20180420164251.5245f822@jacob-builder> <20180529140915.1f174689@w520.home> <20180529211746.74f1dd23@w520.home> <20180530125240.34e0e80c@jacob-builder> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20180530125240.34e0e80c@jacob-builder> Content-Language: en-US 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: Jacob Pan Cc: Yi L , "Raj, Ashok" , Greg Kroah-Hartman , "Wysocki, Rafael J" , "iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org" , LKML , David Woodhouse List-Id: iommu@lists.linux-foundation.org On 30/05/18 20:52, Jacob Pan wrote: >> However I think the model number should be added to >> pasid_table_config. For one thing it gives us a simple sanity-check, >> but it also tells which other fields are valid in pasid_table_config. >> Arm-smmu-v3 needs at least two additional 8-bit fields describing the >> PASID table format (number of levels and PASID0 behaviour), which are >> written to device context tables when installing the PASID table >> pointer. >> > We had model number field in v2 of this patchset. My thought was that > since the config info is meant to be generic, we shouldn't include > model info. But I also think a simple sanity check can be useful, > would that be sufficient to address Alex's concern? Of course we still > need sysfs for more specific IOMMU features. > > Would this work? > enum pasid_table_model { > PASID_TABLE_FORMAT_HOST, > PASID_TABLE_FORMAT_ARM_1LVL, > PASID_TABLE_FORMAT_ARM_2LVL, I'd rather use a single PASID_TABLE_FORMAT_ARM, because "2LVL" may be further split into 2LVL_4k or 2LVL_64k leaf tables... I think it's best if I add an arch-specific field in pasid_table_config for that, and for the PASID0 configuration, when adding FORMAT_ARM in a future patch > PASID_TABLE_FORMAT_AMD, > PASID_TABLE_FORMAT_INTEL, > }; > > /** > * PASID table data used to bind guest PASID table to the host IOMMU. This will > * enable guest managed first level page tables. > * @version: for future extensions and identification of the data format > * @bytes: size of this structure > * @model: PASID table format for different IOMMU models > * @base_ptr: PASID table pointer > * @pasid_bits: number of bits supported in the guest PASID table, must be less > * or equal than the host supported PASID size. > */ > struct pasid_table_config { > __u32 version; > #define PASID_TABLE_CFG_VERSION_1 1 > __u32 bytes; "bytes" could be passed by VFIO as argument to bind_pasid_table, since it can deduce it from argsz Thanks, Jean > enum pasid_table_model model; > __u64 base_ptr; > __u8 pasid_bits; > }; > > > >> Compatibility: new optional features are easy to add to a given model, >> just add a new sysfs file. If in the future, the host describes a new >> feature that is mandatory, or implements a different PASID table >> format, how does it ensure that user understands it? Perhaps use a >> new model number for this, e.g. "arm-smmu-v3-a=3", with similar >> features. I think it would be the same if the host stops supporting a >> feature for a given model, because they are ABI. But we can also >> define default values from the start, for example "if ssid_bits file >> isn't present, default value is 0 - PASID not supported" >> >> Thanks, >> Jean > > [Jacob Pan] >