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] > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu