From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756640AbdJJV3j (ORCPT ); Tue, 10 Oct 2017 17:29:39 -0400 Received: from mga01.intel.com ([192.55.52.88]:62076 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754947AbdJJV3h (ORCPT ); Tue, 10 Oct 2017 17:29:37 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,359,1503385200"; d="scan'208";a="137205760" Date: Tue, 10 Oct 2017 14:32:14 -0700 From: Jacob Pan To: Joerg Roedel Cc: iommu@lists.linux-foundation.org, LKML , David Woodhouse , Greg Kroah-Hartman , Rafael Wysocki , Jean-Philippe Brucker , "Liu, Yi L" , Lan Tianyu , "Tian, Kevin" , Raj Ashok , Alex Williamson , Liu@8bytes.org, Yi L , jacob.jun.pan@linux.intel.com Subject: Re: [PATCH v2 01/16] iommu: introduce bind_pasid_table API function Message-ID: <20171010143214.05fe8ecd@jacob-builder> In-Reply-To: <20171010131433.fgo5tnwidzywfnx4@8bytes.org> References: <1507244624-39189-1-git-send-email-jacob.jun.pan@linux.intel.com> <1507244624-39189-2-git-send-email-jacob.jun.pan@linux.intel.com> <20171010131433.fgo5tnwidzywfnx4@8bytes.org> 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 On Tue, 10 Oct 2017 15:14:33 +0200 Joerg Roedel wrote: > Hi Jacob, > > On Thu, Oct 05, 2017 at 04:03:29PM -0700, Jacob Pan wrote: > > +int iommu_unbind_pasid_table(struct iommu_domain *domain, struct > > device *dev) +{ > > + if (unlikely(!domain->ops->unbind_pasid_table)) > > + return -EINVAL; > > + > > + return domain->ops->unbind_pasid_table(domain, dev); > > +} > > +EXPORT_SYMBOL_GPL(iommu_unbind_pasid_table); > > Are there other reasons to let the unbind fail? Otherwise I'd suggest > to just make this a void function. Also not sure what the user of this > function should do when the unbind really fails. > Agreed, void should do. There is no action for the callers. > > +enum pasid_table_model { > > + PASID_TABLE_FORMAT_HOST, > > What is this FORMAT_HOST for? > I was thinking it indicates whatever the host format is, if the guest does not care. > > + PASID_TABLE_FORMAT_ARM_1LVL, > > + PASID_TABLE_FORMAT_ARM_2LVL, > > + 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 > > + * @base_ptr: PASID table pointer > > + * @pasid_bits: number of bits supported in the guest PASID > > table, must be less > > + * or equal than the host table size. > > + * @model: PASID table format for different IOMMU models > > + */ > > +struct pasid_table_config { > > + __u32 version; > > Can you also add a define for the version number? Userspace needs it > to initialize the struct and the kernel to check against it. > Good point. > > + __u32 bytes; > > + __u64 base_ptr; > > + __u8 pasid_bits; > > + enum pasid_table_model model; > > + union { > > + struct { > > + /* Intel specific fields */ > > + } intel; > > + > > + struct { > > + /* ARM specific fields */ > > + bool pasid0_dma_no_pasid; > > + } arm; > > + > > + struct { > > + /* AMD specific fields */ > > + } amd; > > Thinking more about this, we can omit the sub-structs for models that > don't need them. For the amd-model for example the base_ptr and > pasid_bits fields are sufficient. > > Sounds good, we can always add later and bump up the version. Intel does not need model data for now. > Regards, > > Joerg [Jacob Pan]