From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933279AbcECM2f (ORCPT ); Tue, 3 May 2016 08:28:35 -0400 Received: from mail-wm0-f53.google.com ([74.125.82.53]:37145 "EHLO mail-wm0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932762AbcECM2d (ORCPT ); Tue, 3 May 2016 08:28:33 -0400 Subject: Re: [PATCH v8 6/8] iommu/msi-iommu: iommu_msi_domain To: Alex Williamson References: <1461831323-5480-1-git-send-email-eric.auger@linaro.org> <1461831323-5480-7-git-send-email-eric.auger@linaro.org> <20160428162740.28db4b8d@t450s.home> <572776BD.2080503@linaro.org> <20160502142328.2d97f7a4@t450s.home> Cc: eric.auger@st.com, robin.murphy@arm.com, will.deacon@arm.com, joro@8bytes.org, tglx@linutronix.de, jason@lakedaemon.net, marc.zyngier@arm.com, christoffer.dall@linaro.org, linux-arm-kernel@lists.infradead.org, patches@linaro.org, linux-kernel@vger.kernel.org, Bharat.Bhushan@freescale.com, pranav.sawargaonkar@gmail.com, p.fedin@samsung.com, iommu@lists.linux-foundation.org, Jean-Philippe.Brucker@arm.com, julien.grall@arm.com From: Eric Auger Message-ID: <5728991C.5060807@linaro.org> Date: Tue, 3 May 2016 14:27:08 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <20160502142328.2d97f7a4@t450s.home> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Alex, On 05/02/2016 10:23 PM, Alex Williamson wrote: > Hi Eric, > > On Mon, 2 May 2016 17:48:13 +0200 > Eric Auger wrote: > >> Hi Alex, >> On 04/29/2016 12:27 AM, Alex Williamson wrote: >>> On Thu, 28 Apr 2016 08:15:21 +0000 >>> Eric Auger wrote: >>> >>>> This function checks whether >>>> - the device belongs to a non default iommu domain >>>> - this iommu domain requires the MSI address to be mapped. >>>> >>>> If those conditions are met, the function returns the iommu domain >>>> to be used for mapping the MSI doorbell; else it returns NULL. >>>> >>>> Signed-off-by: Eric Auger >>>> >>>> --- >>>> >>>> v7 -> v8: >>>> - renamed iommu_msi_mapping_desc_to_domain to iommu_msi_domain >>>> - the function now takes a struct device * >>>> - use DOMAIN_ATTR_MSI_GEOMETRY attribute >>>> --- >>>> drivers/iommu/msi-iommu.c | 17 +++++++++++++++++ >>>> include/linux/msi-iommu.h | 14 ++++++++++++++ >>>> 2 files changed, 31 insertions(+) >>>> >>>> diff --git a/drivers/iommu/msi-iommu.c b/drivers/iommu/msi-iommu.c >>>> index 203e86e..023ff17 100644 >>>> --- a/drivers/iommu/msi-iommu.c >>>> +++ b/drivers/iommu/msi-iommu.c >>>> @@ -243,3 +243,20 @@ unlock: >>>> } >>>> } >>>> EXPORT_SYMBOL_GPL(iommu_msi_put_doorbell_iova); >>>> + >>>> +struct iommu_domain *iommu_msi_domain(struct device *dev) >>>> +{ >>>> + struct iommu_domain *d = iommu_get_domain_for_dev(dev); >>>> + struct iommu_domain_msi_geometry msi_geometry; >>>> + >>>> + if (!d || (d->type == IOMMU_DOMAIN_DMA)) >>>> + return NULL; >>>> + >>>> + iommu_domain_get_attr(d, DOMAIN_ATTR_MSI_GEOMETRY, &msi_geometry); >>>> + if (!msi_geometry.programmable) >>> >>> It feels like we're conflating ideas with using the "programmable" flag >>> in this way. AIUI the IOMMU API consumer is supposed to see the >>> invalid MSI geometry with the programmable feature set and know to call >>> iommu_msi_set_aperture(). Looking for the msi_cookie would tell us if >>> that had been done, but that doesn't tell us if it should have been >>> done. iommu_msi_msg_pa_to_va() handles this later, if we return >>> NULL here that function returns success otherwise it goes on to fail if >>> the iova or msi cookie is not set. So really what we're trying to flag >>> is that the MSI geometry participates in the IOMMU-MSI API you've >>> created and we should pick a feature name that says that rather than >>> something as vague a "programmable". Perhaps simply iommu_msi_api >>> rather than programmable. >> Yes I had the same questioning too when I wrote this code. So if my >> understanding is correct we would have >> - programmable: tells whether the MSI range is fixed or pogrammable and, >> - mapping_required (new boolean field): indicating whether MSIs need to >> be mapped in the IOMMU > > Let's say we had a flag "iommu_msi_supported", doesn't that already > tell us whether the MSI range is programmable and the API to use to do > it? Can't we figure out if mapping is required based on whether > iommu_msi_supported is set and aperture_start and aperture_end indicate > a valid range? It seems like what we want on this code path is to > simply know whether the IOMMU domain is relevant to the IOMMU MSI API, > which would be abundantly clear with such a flag. OK I now get your point. Thanks for the clarification. I renamed programmable into iommu_msi_supported. > >>> >>> BTW, I don't see that you ever set aperture_start/end once >>> iommu_msi_set_aperture() has been called. It seems like doing so would >>> add some consistency to that MSI geometry attribute. >> Indeed currently I mentionned: >> /* In case the aperture is programmable, start/end are set to 0 */ > > Which is confusing, if a user sets an aperture, I would think they'd > like to see the MSI geometry updated to reflect that. > >> If I set start/end in iommu_msi_set_aperture then I think I should also >> return the actual values in iommu_domain_get_attr. I thought the extra >> care to handle the possible race between the set_aperture (msi_iommu) >> and the get_attr (iommu) was maybe not worth the benefits: >> is_aperture_set is not visible to get_attr so I would be forced to >> introduce a mutex at iommu_domain level or call an msi-iommu function >> from iommu implementation. So I preferred to keep start/end as read-only >> info initialized by the iommu driver. > > Seems like a race between iommu_msi_set_aperture() and > iommu_domain_get_attr() is the caller's problem. We can always define > that start >= end is invalid and set them in the right order that > iommu_domain_get_attr() may get different versions of invalid, but it > will always be invalid or valid. A helper function could tell us if we > have a valid range rather than using is_aperture_set. Thanks, Agreed; I added an iommu_domain_msi_aperture_valid helper function. Thanks! Eric > > Alex >