From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx0b-00082601.pphosted.com (mx0b-00082601.pphosted.com [67.231.153.30]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 790B82D738F for ; Fri, 8 May 2026 15:30:49 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=67.231.153.30 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778254252; cv=none; b=fqKj/QLc1NT1/93yAqoHvtamcL0etj5xzYIj1t0Brfq8ilbNMckNKm1jJH+eBctPz+ZyfEb96mo5j4UtK4fulb983vpSlMtjQ101Du4vJXl7BjMpU3d6DUw7LqNsZhSW1ZOKHD1s52TLb2uhp2Cjt0OhgVIp3j3ooH977cZWzgg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778254252; c=relaxed/simple; bh=EtgD1VNRsK8d4lAzXro19T0Vx6V8jLE24r087m6Qgqk=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=nXWmX8lKuqZsJGMQ+bQb3NOKZFrCBUTVFgELuuvfO/i6kHebIHM+ekKNVT1d4MCDq6Jd0qeISl/d8Ob/1wCOWwBDlp5he/F1s42scmOEDBMad5V/orv15rd+TGsXzyb9UVjpxbsVRSvqSCBElTiUl2/is/xjHYRevmbd7LoGqzg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=meta.com; spf=pass smtp.mailfrom=meta.com; dkim=pass (2048-bit key) header.d=meta.com header.i=@meta.com header.b=BQwi8Bh8; arc=none smtp.client-ip=67.231.153.30 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=meta.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=meta.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=meta.com header.i=@meta.com header.b="BQwi8Bh8" Received: from pps.filterd (m0528006.ppops.net [127.0.0.1]) by mx0a-00082601.pphosted.com (8.18.1.11/8.18.1.11) with ESMTP id 6489ohcr414276 for ; Fri, 8 May 2026 08:30:48 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=meta.com; h=cc :content-transfer-encoding:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to; s=s2048-2025-q2; bh=vpkNWOxv2MKVZdyw2YC4ieRyxiQ+NH8A65VfzBl7rQU=; b=BQwi8Bh8NYox b7LJuIr3GvE6TtLB8vNaoKOlW8rcipR+gOoYdRAN1z32fYQhHOo5eO1HD/NEXF3S zxXqZ1n6O1zuSR25f/KVQ8Gx77g8GinFvbAGVh7X945EiNFmeNMtP8ifc567Fy4+ Nm+PAjuvCiPE/zYOcAzWdJbpnqPzhDDCSp0dEsaDToXDXNDXgauWSQUDxTJlD6xx /BujEr0VQGxiG1IqiuYPMpQ1CIMH1rWkNco0hTvaONyRLBz9ShO6JLc7uyF3htOq FDzLTBOz7w/D4U/jIAE7xrGQnpM8lStBJQSPiT5VPCoDLX286+TC9pmjo2KKkmIa mRx292hXqg== Received: from mail-dy1-f199.google.com (mail-dy1-f199.google.com [74.125.82.199]) by mx0a-00082601.pphosted.com (PPS) with ESMTPS id 4e1dj75wwm-1 (version=TLSv1.3 cipher=TLS_AES_128_GCM_SHA256 bits=128 verify=NOT) for ; Fri, 08 May 2026 08:30:47 -0700 (PDT) Received: by mail-dy1-f199.google.com with SMTP id 5a478bee46e88-2ee1da7a13fso3011821eec.1 for ; Fri, 08 May 2026 08:30:47 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1778254247; x=1778859047; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=vpkNWOxv2MKVZdyw2YC4ieRyxiQ+NH8A65VfzBl7rQU=; b=GXaXW141vpfmywEpHVhA4BRa4IjAsxIdSshrlTllecBCYHZD9e25DcTe9BuNd1mBdf mxRyIXtuU5t9uzMd+U1QNwzQ1qqgND/k4Av2iOuvTsiU2yArYMT48YLMUZWl8fY2q4oO +QsapUTAG+vyFDVpv12cyrvOF/jyavUhqiQO2RjmH2xu7hMx8TQutK68uA+7FmcDmM0F tvHP6eHmWttBFvF13l1QLebIIFxWq1tQ8xrYkcfq9ui0dlFiFk5FibyHDhJnhv8h6kFS 1n57WfYLttu58sYb0Tga7ini8oaJ9SEoqlkOyXhrRz0k/PENHo3Ro/Mu2D9WABzL/VT4 TBvw== X-Forwarded-Encrypted: i=1; AFNElJ9UxUoK3uUK6gfHmNr55MJ+F0OO2wLxrDoV5QPfC9ypa0TimkEqkhRuOUO9OujjU+gbSGsiX8xJZ+xUEjs=@vger.kernel.org X-Gm-Message-State: AOJu0Yxv7oGTJ8L8FZeFFDiYZjevmcUTn3UcrLj4MoTYvcU92F52+7yh KYCpRFIookQQgU++/8qzcTeZJ9Tt9glPYMV2b3CLK35gr3ekjssyVX3lLS/AAlkUrlHa6Ut/tk/ ZiXkm2oFzgfPlOpM5+7rqO+3s0mYIb/RXqD7t8uFzq1ElbgfcOcbAbsd6tr0ag+zv X-Gm-Gg: Acq92OG7NroBQvZF+k3dGHL/FhtuKCl7XE3gGjsaHO6VbLIbvzlivRsogPwV8l4YqCS H0qzGNpX037cmzzbkDJNC+HWMHsYMJhcWjKTLYeX7rj4kgJDv2/ydC90j9yC0YbhaQd1d1u2QnR MjM3Hj9lW0LIMCZ4ukBihLRl3l/zfbKRyOpTvBjV6U+34zQ6jYwsFQBXLBoDuBOj4k3cnQ0Xpp5 O5+dVfxyT3qO3zr/FQrOYy2fXtsoW0XM8jsewjsQwAcc5Dy+WmVeosxRSIfQ6AYT5aBGbt+DeZv SZvuIdASjhWRbduOEI0q9LQKcqF50DL5liMVOLRIsotCPg8fcaf9P/5/XJUwsHUmM4VTswG7EyG dTZEnArd8nGm0QyXi4qDTIE4H X-Received: by 2002:a05:7300:cc0e:b0:2f5:285c:4374 with SMTP id 5a478bee46e88-2f55114357bmr6248702eec.35.1778254246646; Fri, 08 May 2026 08:30:46 -0700 (PDT) X-Received: by 2002:a05:7300:cc0e:b0:2f5:285c:4374 with SMTP id 5a478bee46e88-2f55114357bmr6248656eec.35.1778254245892; Fri, 08 May 2026 08:30:45 -0700 (PDT) Received: from [10.0.40.30] ([51.52.155.79]) by smtp.gmail.com with ESMTPSA id 5a478bee46e88-2f91004b6a2sm1188733eec.0.2026.05.08.08.30.42 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 08 May 2026 08:30:45 -0700 (PDT) Message-ID: <015b1e9c-0a2d-472a-b750-9154800832ee@meta.com> Date: Fri, 8 May 2026 16:30:40 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4 3/3] vfio/pci: Replace vfio_pci_core_setup_barmap() with vfio_pci_core_get_iomap() Content-Language: en-GB To: Alex Williamson Cc: Kevin Tian , Jason Gunthorpe , Ankit Agrawal , Alistair Popple , Leon Romanovsky , Kees Cook , Shameer Kolothum , Yishai Hadas , Alexey Kardashevskiy , Eric Auger , Peter Xu , Vivek Kasireddy , Zhi Wang , kvm@vger.kernel.org, linux-kernel@vger.kernel.org, virtualization@lists.linux.dev References: <20260505173835.2324179-1-mattev@meta.com> <20260505173835.2324179-4-mattev@meta.com> <20260507162141.072483ce@shazbot.org> From: Matt Evans In-Reply-To: <20260507162141.072483ce@shazbot.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Authority-Analysis: v=2.4 cv=H9rrBeYi c=1 sm=1 tr=0 ts=69fe01a7 cx=c_pps a=cFYjgdjTJScbgFmBucgdfQ==:117 a=2UbFsIa4v//lIgRL4kGwwA==:17 a=IkcTkHD0fZMA:10 a=NGcC8JguVDcA:10 a=VkNPw1HP01LnGYTKEx00:22 a=7x6HtfJdh03M6CCDgxCd:22 a=kkcUborcUVj0H7zxAXTl:22 a=VabnemYjAAAA:8 a=CeaZP3YjJKAO-qlWyq0A:9 a=QEXdDO2ut3YA:10 a=scEy_gLbYbu1JhEsrz4S:22 a=gKebqoRLp9LExxC7YDUY:22 X-Proofpoint-Spam-Details-Enc: AW1haW4tMjYwNTA4MDE1NSBTYWx0ZWRfX062+LSQa25FC f74W/RkHtQm2Lquo2NE0vms21tZtt9NPiEdDr0zpVW4K1lTjo64TSz7TEailza1clifHhzNPZ/z hYwkDRObNKHXqd/YVWCenLVpKVozF+r0KI9yxMwGod+mLAmDdkuiAFdtZVQehkM3n7gp+c69tsS 8vWG1nXG2aiB/XmnmPp0prpik9Cjg1Cz1Q48DsQzTTUQ0+BiJ7Y3VSIczhc65ueFrY/oSv+HO7e /gK9HIwAxwLCUIK+4fbA7p7TYa6ULdEHRXTq40bbHtqcd3wBl6xbxItqvWekSKFpeCB34zCZQ7S D1AvlVJg1GcA1b4tIsbDeAQ9jzZOuO7i6tEe1QWQxovMV5QRrvCumQapRDtNeQ3PYtKADqrS0zI Q2iir4rWANC6xCSJ+hsJdGorCo+cUc02JzHkLNSHE1edTHiA8Dx4js3Y07vT3PdudpSU5DCOKof w2dNMF2+D2Qy8tflziQ== X-Proofpoint-ORIG-GUID: kMOAI2au17Glcp5TtjjZVLbssDpndAfj X-Proofpoint-GUID: kMOAI2au17Glcp5TtjjZVLbssDpndAfj X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.293,Aquarius:18.0.1143,Hydra:6.1.51,FMLib:17.12.100.49 definitions=2026-05-07_02,2026-05-08_02,2025-10-01_01 Hi Alex, On 07/05/2026 23:21, Alex Williamson wrote: > > On Tue, 5 May 2026 10:38:31 -0700 > Matt Evans wrote: > >> Since "vfio/pci: Set up barmap in vfio_pci_core_enable()", the >> resource request and iomap for the BARs was performed early, and >> vfio_pci_core_setup_barmap() just checks those actions succeeded. >> >> Move this logic to a new helper that checks success and returns the >> iomap address, replacing the various bare vdev->barmap[] lookups. >> This maintains the error behaviour of the previous on-demand >> vfio_pci_core_setup_barmap() scheme. >> >> Signed-off-by: Matt Evans >> --- >> drivers/vfio/pci/nvgrace-gpu/main.c | 11 ++++------- >> drivers/vfio/pci/vfio_pci_core.c | 11 +++++------ >> drivers/vfio/pci/vfio_pci_dmabuf.c | 2 +- >> drivers/vfio/pci/vfio_pci_rdwr.c | 30 ++++++++--------------------- >> drivers/vfio/pci/virtio/legacy_io.c | 13 ++++++------- >> include/linux/vfio_pci_core.h | 20 ++++++++++++++++++- >> 6 files changed, 43 insertions(+), 44 deletions(-) >> >> diff --git a/drivers/vfio/pci/nvgrace-gpu/main.c b/drivers/vfio/pci/nvgrace-gpu/main.c >> index fa056b69f899..e153002258ce 100644 >> --- a/drivers/vfio/pci/nvgrace-gpu/main.c >> +++ b/drivers/vfio/pci/nvgrace-gpu/main.c >> @@ -184,13 +184,10 @@ static int nvgrace_gpu_open_device(struct vfio_device *core_vdev) >> >> /* >> * GPU readiness is checked by reading the BAR0 registers. >> - * >> - * ioremap BAR0 to ensure that the BAR0 mapping is present before >> - * register reads on first fault before establishing any GPU >> - * memory mapping. >> + * The BAR map was just set up by vfio_pci_core_enable(), so >> + * check that was successful and bail early if not: >> */ >> - ret = vfio_pci_core_setup_barmap(vdev, 0); >> - if (ret) >> + if (IS_ERR(vfio_pci_core_get_iomap(vdev, 0))) >> goto error_exit; > > Sashiko notes we're not setting ret here. The bots are also paranoid > about the unreachable condition that the get_iomap below could return an > ERR_PTR. Maybe head off both by adding an __iomem pointer to the > nvgrace_gpu_pci_core_device struct and a temporary one here. Store the > iomap in the temporary variable, use it to test for IS_ERR() and > PTR_ERR(), then set the pointer in the structure after the last error > condition here. Add one line in the close_device to set it NULL. Then > just use nvdev->bar0_io below. Right about ret. On the 2nd, the bots could benefit from a comment on the ...get_iomap() below saying that it "cannot fail" if this one passes, but hey. I can add a struct member to track it (bots can then worry that it might be NULL, if they don't notice that nvgrace_gpu_check_device_ready() can't happen if nvgrace_gpu_open_device() didn't succeed, etc. etc.). >> >> if (nvdev->resmem.memlength) { >> @@ -275,7 +272,7 @@ nvgrace_gpu_check_device_ready(struct nvgrace_gpu_pci_core_device *nvdev) >> if (!__vfio_pci_memory_enabled(vdev)) >> return -EIO; >> >> - ret = nvgrace_gpu_wait_device_ready(vdev->barmap[0]); >> + ret = nvgrace_gpu_wait_device_ready(vfio_pci_core_get_iomap(vdev, 0)); >> if (ret) >> return ret; >> >> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c >> index 62931dc381d8..5c8bd13f10d0 100644 >> --- a/drivers/vfio/pci/vfio_pci_core.c >> +++ b/drivers/vfio/pci/vfio_pci_core.c >> @@ -1761,7 +1761,7 @@ int vfio_pci_core_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma >> struct pci_dev *pdev = vdev->pdev; >> unsigned int index; >> u64 phys_len, req_len, pgoff, req_start; >> - int ret; >> + void __iomem *bar_io; >> >> index = vma->vm_pgoff >> (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT); >> >> @@ -1795,12 +1795,11 @@ int vfio_pci_core_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma >> return -EINVAL; >> >> /* >> - * Even though we don't make use of the barmap for the mmap, >> - * we need to request the region and the barmap tracks that. >> + * Ensure the BAR resource region is reserved for use. >> */ >> - ret = vfio_pci_core_setup_barmap(vdev, index); >> - if (ret) >> - return ret; >> + bar_io = vfio_pci_core_get_iomap(vdev, index); >> + if (IS_ERR(bar_io)) >> + return PTR_ERR(bar_io); >> >> vma->vm_private_data = vdev; >> vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); >> diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c b/drivers/vfio/pci/vfio_pci_dmabuf.c >> index 69a5c2d511e6..46cd44b22c9c 100644 >> --- a/drivers/vfio/pci/vfio_pci_dmabuf.c >> +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c >> @@ -248,7 +248,7 @@ int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags, >> * else. Check that PCI resources have been claimed for it. >> */ >> if (get_dma_buf.region_index >= VFIO_PCI_ROM_REGION_INDEX || >> - vfio_pci_core_setup_barmap(vdev, get_dma_buf.region_index)) >> + IS_ERR(vfio_pci_core_get_iomap(vdev, get_dma_buf.region_index))) >> return -ENODEV; >> >> dma_ranges = memdup_array_user(&arg->dma_ranges, get_dma_buf.nr_ranges, >> diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c >> index 3bfbb879a005..7f14dd46de17 100644 >> --- a/drivers/vfio/pci/vfio_pci_rdwr.c >> +++ b/drivers/vfio/pci/vfio_pci_rdwr.c >> @@ -198,19 +198,6 @@ ssize_t vfio_pci_core_do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem, >> } >> EXPORT_SYMBOL_GPL(vfio_pci_core_do_io_rw); >> >> -/* >> - * The barmap is set up in vfio_pci_core_enable(). Callers use this >> - * function to check that the BAR resources are requested or that the >> - * pci_iomap() was done. >> - */ >> -int vfio_pci_core_setup_barmap(struct vfio_pci_core_device *vdev, int bar) >> -{ >> - if (IS_ERR(vdev->barmap[bar])) >> - return PTR_ERR(vdev->barmap[bar]); >> - return 0; >> -} >> -EXPORT_SYMBOL_GPL(vfio_pci_core_setup_barmap); >> - >> ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf, >> size_t count, loff_t *ppos, bool iswrite) >> { >> @@ -262,13 +249,11 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf, >> */ >> max_width = VFIO_PCI_IO_WIDTH_4; >> } else { >> - int ret = vfio_pci_core_setup_barmap(vdev, bar); >> - if (ret) { >> - done = ret; >> + io = vfio_pci_core_get_iomap(vdev, bar); >> + if (IS_ERR(io)) { >> + done = PTR_ERR(io); >> goto out; >> } >> - >> - io = vdev->barmap[bar]; >> } >> >> if (bar == vdev->msix_bar) { >> @@ -423,6 +408,7 @@ int vfio_pci_ioeventfd(struct vfio_pci_core_device *vdev, loff_t offset, >> loff_t pos = offset & VFIO_PCI_OFFSET_MASK; >> int ret, bar = VFIO_PCI_OFFSET_TO_INDEX(offset); >> struct vfio_pci_ioeventfd *ioeventfd; >> + void __iomem *io; >> >> /* Only support ioeventfds into BARs */ >> if (bar > VFIO_PCI_BAR5_REGION_INDEX) >> @@ -440,9 +426,9 @@ int vfio_pci_ioeventfd(struct vfio_pci_core_device *vdev, loff_t offset, >> if (count == 8) >> return -EINVAL; >> >> - ret = vfio_pci_core_setup_barmap(vdev, bar); >> - if (ret) >> - return ret; >> + io = vfio_pci_core_get_iomap(vdev, bar); >> + if (IS_ERR(io)) >> + return PTR_ERR(io); > > Sashiko seems to note a real existing error here that should also be > pulled out to a separate fix. Given the right offset, this could > generate a negative BAR value. Yuck, loff_t signed, yep. Isn't the real root of this that it never makes sense for VFIO_PCI_OFFSET_TO_INDEX() to return a negative index here or anywhere else? I suggest instead, to also avoid this elsewhere in future, something like: #define VFIO_PCI_OFFSET_TO_INDEX(off) ((u64)(off) >> VFIO_PCI_OFFSET_SHIFT) > The test at the end of the previous > chunk should should be expanded to `if (bar < 0 || bar > ...BAR5...)`. Not necessary if VFIO_PCI_OFFSET_TO_INDEX() can't return < 0 (the magnitude would be 24b so can't overflow the `int bar` it's assigned into). > Do you want to pick that up in this series? I think it's the only case > that lets that slip through. Thanks, Sure, I'll post a fix. I don't think it needs to be part of this series though if it's just the macro, do you agree? Do you know why drivers/gpu/drm/i915/gvt/kvmgt.c has copied VFIO_PCI_OFFSET_TO_INDEX() and friends? Perhaps the shift was different (the reason drivers/vfio/pci/ism/main.c has its own versions). The same loff_t issue seems to exist in both of those places, unfortunately. Matt PS: with minor question: Relatedly, I'd made `bar` an int following existing convention in vfio_pci_core_get_iomap(struct vfio_pci_core_device *vdev, int bar) But I'll make this `unsigned int`, please flag if this violates taste and decency. IMO any BAR/index parameter should be unsigned; most are, some signed remain.