From mboxrd@z Thu Jan 1 00:00:00 1970 From: santosh Subject: Re: [PATCH v9 2/9] eal/pci: get iommu class Date: Fri, 6 Oct 2017 14:43:50 +0530 Message-ID: References: <20170918104234.9149-1-santosh.shukla@caviumnetworks.com> <5989047.qJ3B3g597f@xps> <2281320.59S7KcHRhB@xps> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org, olivier.matz@6wind.com, jerin.jacob@caviumnetworks.com, hemant.agrawal@nxp.com, aconole@redhat.com, stephen@networkplumber.org, anatoly.burakov@intel.com, gaetan.rivet@6wind.com, shreyansh.jain@nxp.com, bruce.richardson@intel.com, sergio.gonzalez.monroy@intel.com, maxime.coquelin@redhat.com To: Thomas Monjalon Return-path: Received: from NAM01-BY2-obe.outbound.protection.outlook.com (mail-by2nam01on0051.outbound.protection.outlook.com [104.47.34.51]) by dpdk.org (Postfix) with ESMTP id AC70E1B296 for ; Fri, 6 Oct 2017 11:14:14 +0200 (CEST) In-Reply-To: <2281320.59S7KcHRhB@xps> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Friday 06 October 2017 12:54 PM, Thomas Monjalon wrote: > 06/10/2017 05:04, santosh: >> Thomas, >> >> You comment is annoying and infuriating both. >> Patch is their for more than 4month, had enough time for you to comment >> and understand the topic. Thorough review and testing has happened both. >> >> NOTE: You have already delayed this series by one release and >> I'm guessing that you intent to push by one more, if you had such >> mundane question then why not ask before? Make me think that you are >> wasting my time and effort both. > You misunderstand me. > My intent is to push this patch. > A lot of people have reviewed it during this cycle. > I was just looking for wording details in order to ease people > when they will see this abstraction in the code base. > >> On Friday 06 October 2017 05:28 AM, Thomas Monjalon wrote: >> >>> This patch is introducing a new abstraction. >>> It is important to explain it for future readers of this code. >> If you don't know - What is iova? How to program iova? >> purpose of iova then should read and educate your know - how first. >> >> Yes, its is introducing new abstraction, because dpdk from >> ancient days does only one programming mode aka iova=pa. >> >> note:You were still using iova mode as _pa (and didn't care to ask yourself about IOVA!) >> which is one of iova mode too!. >> >> However, IOMMU can also generate _va address too called iova=_va mode.. >> which is also correct/viable/applicable/Okiesh programming mode >> for iommu capable HW like dma for example(Note again,.. AGNOSTIC behavior of iommu). >> >> Now Why dpdk needs to understand IOVA programming philosophy: >> >> Though DPDK was _silenty_ using iova as pa mode but then there >> is a need arise to make mapping mode explicit and for that we need >> abstraction since there wasn't one existed. >> >> Reason: >> Because From last few years,.ONA participants like Cavium, nxp >> added ARM arch support in dpdk and included drivers for their HW.. >> and their hw has use-case (example external mempool), such a way that >> programming those HW in iova as va mode would save cycle in fast path >> (this part, we explained so many-1000 time in series and same understood by reviewer) >> thus its vital to introduce iova infra in dpdk. >> >> Same applicable for intel HW blocks too. Its works for intel too! > I know all of that! > I was just thinking that you could add more explanations somewhere > in the code or the doc. > >>> 20/09/2017 13:23, Santosh Shukla: >>>> +/** >>>> + * IOVA mapping mode. >>>> + */ >>> Please explain what IOVA means and what is the purpose of >>> distinguish the different modes. >>> >> IOVA mapping mode is device aka iommu programming mode by which >> HW(iommu) will generate _pa or _va address accordingly. sending v10 with doc changes. > In this doxygen block, it would be the right place to explain how the > IOVA mode will impact the rest of DPDK. > >>>> +enum rte_iova_mode { >>>> + RTE_IOVA_DC = 0, /* Don't care mode */ >>>> + RTE_IOVA_PA = (1 << 0), >>>> + RTE_IOVA_VA = (1 << 1) >>>> +}; >>> You should explain each value of the enum. >> Aren't naming choice for each member of enum is self-explanatory? >> I don't find logic anymore in your question? are you asking about side commenting? >> if not then IFAIU, you question is basically about what is _pa and _va? if so then >> reader should have little know-how before they intent to do fast-path programming. >> Author can't write whole IOMMU spec for reader sake. Those are minute and mundate info >> incase any user want to program device in _pa or _va. I'm at loss with you question, >> I don;t see logic and it is frustrating to me. You had enough time for all this >> in case you had really cared,, we have series for external PMD and drivers waiting >> for iova infra, I see it a your move nothing bu blocking ONA series progress >> Don;t you trust Reviewer in case you have hard time understaing topic and that >> makese me to ask - Are you willing to accept this feature or not? if not then >> I'm wasting my energy on it. > Santosh, I'm sorry if you don't understand that I was just asking for > a bit more doc. > You could just add something like > /* DMA using physical address */ > /* DMA using virtual address */ in v10. Thanks. > Anyway, if you don't want to add any explanation, it won't prevent > pushing this patch.