From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 0C942C6FA82 for ; Wed, 14 Sep 2022 19:56:27 +0000 (UTC) Received: from localhost ([::1]:56044 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oYYUk-0002eB-Mh for qemu-devel@archiver.kernel.org; Wed, 14 Sep 2022 15:56:26 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:58244) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oYYSO-0001FL-Ki for qemu-devel@nongnu.org; Wed, 14 Sep 2022 15:54:00 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.129.124]:58664) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oYYSI-0001b3-Gu for qemu-devel@nongnu.org; Wed, 14 Sep 2022 15:53:57 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1663185231; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=0EQgiJKXGpPNKplJVER7eGQGq01L7LdxU29W2IHf6TQ=; b=afaUXd+ubQ8uLqbsXEBx/RTquyvdKeQq5/Xmk9wwpmlvlGdTaELgRA6uLH5v7FiMvc6N6p BVFIWEEQzVXDxgFLK0X6+mRJVCEjb0rmuE6BTJmGhaIEP/Ok7oG0vt/9ATI6fYb/w8CL+T lHZPzccjfdstnX10yfmfPQm4LRRz9zg= Received: from mail-il1-f200.google.com (mail-il1-f200.google.com [209.85.166.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-307-rhev3L9ZMmieMg0upwywBQ-1; Wed, 14 Sep 2022 15:53:42 -0400 X-MC-Unique: rhev3L9ZMmieMg0upwywBQ-1 Received: by mail-il1-f200.google.com with SMTP id z9-20020a921a49000000b002f0f7fb57e3so10949219ill.2 for ; Wed, 14 Sep 2022 12:53:42 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date; bh=0EQgiJKXGpPNKplJVER7eGQGq01L7LdxU29W2IHf6TQ=; b=a6d5UL7iENNH1KbcCNSm1Prg5QPQIw1kWZVdR0kdLQNnxedDB55VoY3iLQKYCHUsUc ZMPVyECgp7ehW11AVg+dEx6mxbug2W/6Hxo+utLk/MKrJuaKMtdBKZLjorgQNJQMhyqb OXSzVPALAitV/CU2jy9k4CRU6ehIWg2HBfOarkCMzyzPl4VYjZxn5w+5AZB0TGGuVLXV D47anSm84LN6RhfnHeJfZq7TDu0VAK3b2cqY+VwLMF/Q7/Zp7Fo4RYzl87kLtcQLtXxX M3rLlUrStQ6c4qkMNuR1A/7So3YnZP29+IQwMVKiJDCkvN8qeclIQIVu5QJEAchBTOD/ WhSA== X-Gm-Message-State: ACgBeo0eZwLYwre/o4oCIXNZ2ACYMgKQ5tO5cK+J5RbFg/BQm5o6kDb5 yzO6GWxUIC1un2jeKpr0OQslaoV1Vc5wikVx1tb+3GJWE9n4ptKjvH9+nZEAmt61EXFryBcKcOE Y8BwGvTGVlNnezuU= X-Received: by 2002:a02:c55a:0:b0:358:317b:7472 with SMTP id g26-20020a02c55a000000b00358317b7472mr18028212jaj.248.1663185222111; Wed, 14 Sep 2022 12:53:42 -0700 (PDT) X-Google-Smtp-Source: AA6agR57xOYdo0oOWz9hch54JdQGu7k4DKCzMloHcqAv82FDcoTxyKBI51i7Z1EV4uSPqcbhU1/ARQ== X-Received: by 2002:a02:c55a:0:b0:358:317b:7472 with SMTP id g26-20020a02c55a000000b00358317b7472mr18028200jaj.248.1663185221866; Wed, 14 Sep 2022 12:53:41 -0700 (PDT) Received: from redhat.com ([38.15.36.239]) by smtp.gmail.com with ESMTPSA id k16-20020a0566022d9000b006a1fed36549sm732555iow.10.2022.09.14.12.53.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 14 Sep 2022 12:53:41 -0700 (PDT) Date: Wed, 14 Sep 2022 13:53:39 -0600 From: Alex Williamson To: Nicolin Chen Cc: Cornelia Huck , , , , , Subject: Re: [PATCH] vfio/common: Do not g_free in vfio_get_iommu_info Message-ID: <20220914135339.665b90b1.alex.williamson@redhat.com> In-Reply-To: References: <20220910004245.2878-1-nicolinc@nvidia.com> <8735cwu5r7.fsf@redhat.com> <20220914121029.1a693e5d.alex.williamson@redhat.com> Organization: Red Hat MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Received-SPF: pass client-ip=170.10.129.124; envelope-from=alex.williamson@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -27 X-Spam_score: -2.8 X-Spam_bar: -- X-Spam_report: (-2.8 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" On Wed, 14 Sep 2022 12:02:56 -0700 Nicolin Chen wrote: > Hi Alex, > > On Wed, Sep 14, 2022 at 12:10:29PM -0600, Alex Williamson wrote: > > > > > > Its caller vfio_connect_container() assigns a default value > > > > > to info->iova_pgsizes, even if vfio_get_iommu_info() fails. > > > > > This would result in a "Segmentation fault" error, when the > > > > > VFIO_IOMMU_GET_INFO ioctl errors out. > > > > > > > > > > Since the caller has g_free already, drop the g_free in its > > > > > rollback routine and add a line of comments to highlight it. > > > > > > > > There's basically two ways to fix this: > > > > > > > > - return *info in any case, even on error > > > > - free *info on error, and make sure that the caller doesn't try to > > > > access *info if the function returned !0 > > > > > > > > The problem with the first option is that the caller will access invalid > > > > information if it neglects to check the return code, and that might lead > > > > to not-that-obvious errors; in the second case, a broken caller would at > > > > least fail quickly with a segfault. The current code is easier to fix > > > > with the first option. > > > > > > > > I think I'd prefer the second option; but obviously maintainer's choice. > > > > > > The caller does check rc all the time. So I made a smaller fix > > > (the first option). Attaching the git-diff for the second one. > > > > > > Alex, please let me know which one you prefer. Thanks! > > > I think we can do better than that, I don't think we need to maintain > > the existing grouping, and that FIXME comment is outdated and has > > drifted from the relevant line of code. What about: > > Your patch looks good and clean to me (some nits inline). > > Would you please send and merge it, replacing mine? > > > + /* > > + * Setup defaults for container pgsizes and dma_max_mappings if not > > + * provided by kernel below. > > */ > > Indentation is misaligned at the first line. Oops, will run through checkpatch. > > > + container->pgsizes = 4096; > > This might be a separate question/issue: I wonder if we should use > "sysconf(_SC_PAGE_SIZE)" here instead of 4096. > > With a kernel using a larger page size, e.g. CONFIG_ARM64_64K_PAGES, > the IO page size is likely to be 64K too. If the ioctl fails, this > default 4K setup won't work. Perhaps, but IIRC this solution came about because we originally forgot to expose the IOMMU_INFO flag to indicate the pgsize field was valid. At the time we only supported 4K systems, so it made sense to provide this default, though it is indeed dated. TBH, I don't really see why we should try to continue if the ioctl itself fails, so maybe this should be: diff --git a/hw/vfio/common.c b/hw/vfio/common.c index ace9562a9ba1..ad188b7649e6 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -2111,29 +2111,31 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as, { struct vfio_iommu_type1_info *info; - /* - * FIXME: This assumes that a Type1 IOMMU can map any 64-bit - * IOVA whatsoever. That's not actually true, but the current - * kernel interface doesn't tell us what it can map, and the - * existing Type1 IOMMUs generally support any IOVA we're - * going to actually try in practice. - */ ret = vfio_get_iommu_info(container, &info); + if (ret) { + error_setg_errno(errp, -ret, "Failed to get VFIO IOMMU info"); + goto enable_discards_exit:; + } - if (ret || !(info->flags & VFIO_IOMMU_INFO_PGSIZES)) { - /* Assume 4k IOVA page size */ - info->iova_pgsizes = 4096; + if (info->flags & VFIO_IOMMU_INFO_PGSIZES) { + container->pgsizes = info->iova_pgsizes; + } else { + container->pgsizes = qemu_real_host_page_size(); } - vfio_host_win_add(container, 0, (hwaddr)-1, info->iova_pgsizes); - container->pgsizes = info->iova_pgsizes; - /* The default in the kernel ("dma_entry_limit") is 65535. */ - container->dma_max_mappings = 65535; - if (!ret) { - vfio_get_info_dma_avail(info, &container->dma_max_mappings); - vfio_get_iommu_info_migration(container, info); + if (!vfio_get_info_dma_avail(info, &container->dma_max_mappings)) { + container->dma_max_mappings = 65535; } + vfio_get_iommu_info_migration(container, info); g_free(info); + + /* + * FIXME: We should parse VFIO_IOMMU_TYPE1_INFO_CAP_IOVA_RANGE + * information to get the actual window extent rather than assume + * a 64-bit IOVA address space. + */ + vfio_host_win_add(container, 0, (hwaddr)-1, container->pgsizes); + break; } case VFIO_SPAPR_TCE_v2_IOMMU: