Date: Thu, 12 Aug 2021 14:46:21 -0700 From: John Baldwin <jhb@FreeBSD.org> To: =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= <royger@FreeBSD.org> 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: <11d56b94-7fb9-740d-93b0-fab3c3dad6a7@FreeBSD.org> In-Reply-To: <YRVT9yDNHhh2vnyv@Air-de-Roger> References: <202108120724.17C7OJ8h043708@gitrepo.freebsd.org> <d1f8a587-5b7f-91bd-3daf-eba1a1c77bab@FreeBSD.org> <YRVD/0OwIlRqtBZE@Air-de-Roger> <32e784a3-9c96-9818-8fc3-a5ad85643f2c@FreeBSD.org> <YRVT9yDNHhh2vnyv@Air-de-Roger>
next in thread | previous in thread | raw e-mail | index | archive | help
On 8/12/21 10:01 AM, Roger Pau Monné wrote: > 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é <royger@FreeBSD.org> >>>>> AuthorDate: 2021-04-09 09:31:44 +0000 >>>>> Commit: Roger Pau Monné <royger@FreeBSD.org> >>>>> 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. Huh, I hadn't really imagined that use case, but it would be nice to not have to touch the release path and optimize this case when you aren't doing a subset mapping. I would be fine with changing rman_set_mapping to clear the flag. In fact, arguably it would be cleaner (but maybe just one more one-line change?) to have new resources always have RF_UNMAPPED set even if it wasn't passed into the rman_reserve_bounds() (you would do it in the case where you allocate a resource before the RF_SHAREABLE loop) and then only clear it if rman_set_mapping() is called. -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?11d56b94-7fb9-740d-93b0-fab3c3dad6a7>