From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754230AbeEaJKB (ORCPT ); Thu, 31 May 2018 05:10:01 -0400 Received: from foss.arm.com ([217.140.101.70]:38326 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754026AbeEaJJ5 (ORCPT ); Thu, 31 May 2018 05:09:57 -0400 Subject: Re: [PATCH v4 04/22] iommu/vt-d: add bind_pasid_table function To: Jacob Pan Cc: "Tian, Kevin" , Alex Williamson , "iommu@lists.linux-foundation.org" , LKML , Joerg Roedel , David Woodhouse , Greg Kroah-Hartman , "Wysocki, Rafael J" , "Liu, Yi L" , "Raj, Ashok" , Christoph Hellwig , Lu Baolu , Yi L , Auger Eric 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> From: Jean-Philippe Brucker Message-ID: <1c1094e8-ecbb-7731-910c-59e4de1e5c70@arm.com> Date: Thu, 31 May 2018 10:09:46 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180530125240.34e0e80c@jacob-builder> Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.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] >