From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id E679AA04BC; Thu, 8 Oct 2020 11:39:41 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 4266F1BC29; Thu, 8 Oct 2020 11:39:40 +0200 (CEST) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id 30B891BBFD for ; Thu, 8 Oct 2020 11:39:38 +0200 (CEST) IronPort-SDR: 7eR6PbeO7u/ZIeGlRhaIB581CsHp8EPEDeNRHyTZVgzfy+lA8BTdCfcWj3ShaObTJtX/rETaHY fRUzy03RH6PQ== X-IronPort-AV: E=McAfee;i="6000,8403,9767"; a="153142437" X-IronPort-AV: E=Sophos;i="5.77,350,1596524400"; d="scan'208";a="153142437" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Oct 2020 02:39:36 -0700 IronPort-SDR: yinm3NoiYjQc7Z7GNnqvSPlg8bS7Xep59nYFhuMM0NvHONrttj6QpLnFdRuwoUo2cd3qsJo1+o K2/PcgEn3mHg== X-IronPort-AV: E=Sophos;i="5.77,350,1596524400"; d="scan'208";a="528426209" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.213.226.43]) ([10.213.226.43]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Oct 2020 02:39:35 -0700 To: David Christensen , david.marchand@redhat.com Cc: dev@dpdk.org References: <20200630213823.1583764-1-drc@linux.vnet.ibm.com> <20200810210707.745083-1-drc@linux.vnet.ibm.com> <20200810210707.745083-2-drc@linux.vnet.ibm.com> <2c830988-c4db-7bdc-50f3-3fa445a81673@intel.com> From: "Burakov, Anatoly" Message-ID: <03bace21-0a55-e88b-7f13-2a8764125547@intel.com> Date: Thu, 8 Oct 2020 10:39:33 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v3 1/1] vfio: modify spapr iommu support to use static window sizing X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list 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 07-Oct-20 6:44 PM, David Christensen wrote: > > > On 9/17/20 4:13 AM, Burakov, Anatoly wrote: >> On 10-Aug-20 10:07 PM, David Christensen wrote: >>> The SPAPR IOMMU requires that a DMA window size be defined before memory >>> can be mapped for DMA. Current code dynamically modifies the DMA window >>> size in response to every new memory allocation which is potentially >>> dangerous because all existing mappings need to be unmapped/remapped in >>> order to resize the DMA window, leaving hardware holding IOVA addresses >>> that are temporarily unmapped.  The new SPAPR code statically assigns >>> the DMA window size on first use, using the largest physical memory >>> memory address when IOVA=PA and the highest existing memseg virtual >>> address when IOVA=VA. >>> >>> Signed-off-by: David Christensen >>> --- >> >> >> >>> +struct spapr_size_walk_param { >>> +    uint64_t max_va; >>> +    uint64_t page_sz; >>> +    int external; >>> +}; >>> + >>> +/* >>> + * In order to set the DMA window size required for the SPAPR IOMMU >>> + * we need to walk the existing virtual memory allocations as well as >>> + * find the hugepage size used. >>> + */ >>>   static int >>> -vfio_spapr_unmap_walk(const struct rte_memseg_list *msl, >>> -        const struct rte_memseg *ms, void *arg) >>> +vfio_spapr_size_walk(const struct rte_memseg_list *msl, void *arg) >>>   { >>> -    int *vfio_container_fd = arg; >>> +    struct spapr_size_walk_param *param = arg; >>> +    uint64_t max = (uint64_t) msl->base_va + (uint64_t) msl->len; >>> -    /* skip external memory that isn't a heap */ >>> -    if (msl->external && !msl->heap) >>> -        return 0; >>> +    if (msl->external) { >>> +        param->external++; >>> +        if (!msl->heap) >>> +            return 0; >>> +    } >> >> It would be nice to have some comments in the code explaining what >> we're skipping and why. > > Reviewing this again, my inclination is to skip ALL external memory, > which by definition would seem to be outside of IOMMU control, so the > code would read: > >    if (msl->external) { >        param->external++; >        return 0; >    } The external memory can still be mapped for DMA with rte_dev_dma_map() API. The heap memory is meant to be mapped automatically by DPDK, while the non-heap memory (created with rte_extmem_register() API) is meant to be managed by the user and will be mapped using the user_mem_map functions in this file. > > Not sure why existing code such as vfio_spapr_map_walk() distinguishes > between heap and non-heap in this situation.  Are there instances in x86 > where it would matter? > >> Also, seems that you're using param->external as bool? This is a >> non-public API so using stdbool is not an issue here, perhaps replace >> it with bool param->has_external? > > Why do you think the distinction is necessary? > It's not *necessary*, i just don't like the ancient C style where ints are used as booleans :D Not a serious issue though, your choice. > Dave -- Thanks, Anatoly