From owner-dev-commits-src-main@freebsd.org Thu Aug 12 17:01:46 2021 Return-Path: Delivered-To: dev-commits-src-main@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 7ECC166F59A; Thu, 12 Aug 2021 17:01:46 +0000 (UTC) (envelope-from royger@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4GltJV37npz3J7T; Thu, 12 Aug 2021 17:01:46 +0000 (UTC) (envelope-from royger@FreeBSD.org) Received: from localhost (unknown [93.176.191.254]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) (Authenticated sender: royger) by smtp.freebsd.org (Postfix) with ESMTPSA id 0076D25B11; Thu, 12 Aug 2021 17:01:45 +0000 (UTC) (envelope-from royger@FreeBSD.org) Date: Thu, 12 Aug 2021 19:01:43 +0200 From: Roger Pau =?utf-8?B?TW9ubsOp?= To: John Baldwin Cc: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: Re: git: f4c6843ec2b9 - main - xen: use correct cache attributes for Xen specific memory regions Message-ID: References: <202108120724.17C7OJ8h043708@gitrepo.freebsd.org> <32e784a3-9c96-9818-8fc3-a5ad85643f2c@FreeBSD.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <32e784a3-9c96-9818-8fc3-a5ad85643f2c@FreeBSD.org> X-BeenThere: dev-commits-src-main@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for the main branch of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 12 Aug 2021 17:01:46 -0000 On Thu, Aug 12, 2021 at 09:43:57AM -0700, John Baldwin wrote: > On 8/12/21 8:53 AM, Roger Pau Monné wrote: > > On Thu, Aug 12, 2021 at 08:20:51AM -0700, John Baldwin wrote: > > > On 8/12/21 12:24 AM, Roger Pau Monné wrote: > > > > The branch main has been updated by royger: > > > > > > > > URL: https://cgit.FreeBSD.org/src/commit/?id=f4c6843ec2b9aa5eff475778fb000ed6278c5b77 > > > > > > > > commit f4c6843ec2b9aa5eff475778fb000ed6278c5b77 > > > > Author: Roger Pau Monné > > > > AuthorDate: 2021-04-09 09:31:44 +0000 > > > > Commit: Roger Pau Monné > > > > CommitDate: 2021-08-12 07:18:32 +0000 > > > > > > > > xen: use correct cache attributes for Xen specific memory regions > > > > bus_activate_resource maps memory regions as uncacheable on x86, which > > > > is more strict than required for regions allocated using xenmem_alloc, > > > > so don't rely on bus_activate_resource and instead map the region > > > > using pmap_mapdev_attr and VM_MEMATTR_XEN as the cache attribute. > > > > Sponsored by: Citrix Systems R&D > > > > > > It would probably be cleaner to use bus_map_resource() for this instead. It > > > would mean you would have to use a structure that writes to as the argument > > > to bus_read/write_* instead of using the resource directly. > > > > Those regions are usually handled to other subsystems of the kernel. > > They are mostly used to map memory from other domains and then perform > > IO on their behalf (like blkback and netback do), so it's not really > > possible to assert that all users of the regions would use > > bus_read/write_* to access them. > > > > I could however switch to using bus_map_resource if I can pass the > > desired memory cache attribute and get a linear address back. It looks > > like resource_map_request parameter allows to select the cache > > attribute. > > Yes, one of the use case for bus_map_resource is to permit passing a > non-default memory attribute. (It also permits mapping sub-ranges, but > that isn't applicable in this case.) The 'r_vaddr' in the resource_map > is the equivalent of rman_get_addr. It'd be a little cleaner (and > perhaps friendly to future changes in this area) if your clients used > the 'r_vaddr' from the resource_map directly instead of using > rman_set_virtual() on the original resource. (At some point I would like > to have each 'struct resource' keep a list of all the mappings created > from it and to forcefully invalidate/free any existing mappings that > still remain when a 'struct resource' is freed. I imagine as part of > that I might end up with some assertions about the embedded resource > fields in the 'struct resource' being self-consistent). Does the following diff seem better: diff --git a/sys/dev/xen/bus/xenpv.c b/sys/dev/xen/bus/xenpv.c index 91004039a85e..748ead7f2f41 100644 --- a/sys/dev/xen/bus/xenpv.c +++ b/sys/dev/xen/bus/xenpv.c @@ -120,7 +120,8 @@ xenpv_alloc_physmem(device_t dev, device_t child, int *res_id, size_t size) { struct resource *res; vm_paddr_t phys_addr; - void *virt_addr; + struct resource_map_request margs; + struct resource_map map; int error; res = bus_alloc_resource(child, SYS_RES_MEMORY, res_id, LOW_MEM_LIMIT, @@ -135,9 +136,16 @@ xenpv_alloc_physmem(device_t dev, device_t child, int *res_id, size_t size) bus_release_resource(child, SYS_RES_MEMORY, *res_id, res); return (NULL); } - virt_addr = pmap_mapdev_attr(phys_addr, size, VM_MEMATTR_XEN); - KASSERT(virt_addr != NULL, ("Failed to create linear mappings")); - rman_set_virtual(res, virt_addr); + + resource_init_map_request(&margs); + margs.memattr = VM_MEMATTR_XEN; + error = bus_map_resource(child, SYS_RES_MEMORY, res, &margs, &map); + if (error != 0) { + vm_phys_fictitious_unreg_range(phys_addr, phys_addr + size); + bus_release_resource(child, SYS_RES_MEMORY, *res_id, res); + return (NULL); + } + rman_set_mapping(res, &map); return (res); } @@ -146,14 +154,14 @@ static int xenpv_free_physmem(device_t dev, device_t child, int res_id, struct resource *res) { vm_paddr_t phys_addr; - vm_offset_t virt_addr; + struct resource_map map; size_t size; phys_addr = rman_get_start(res); size = rman_get_size(res); - virt_addr = (vm_offset_t)rman_get_virtual(res); + rman_get_mapping(res, &map); - pmap_unmapdev(virt_addr, size); + bus_unmap_resource(child, SYS_RES_MEMORY, res, &map); vm_phys_fictitious_unreg_range(phys_addr, phys_addr + size); return (bus_release_resource(child, SYS_RES_MEMORY, res_id, res)); } I also wonder if rman_set_mapping should clear the RF_UNMAPPED flag from the resource? Or the content of the flags field is opaque from rman PoV and it should be the caller of rman_set_mapping the one to clear the flag? (xenpv_alloc_physmem in the code above) It's kind of pointless to copy the bus_unmap_resource logic in xenpv_free_physmem when it could be done by bus_release_resource if the RF_UNMAPPED flag was cleared. Thanks, Roger.