Hi, I''ve been working on the Mirage (mini-OS + OCaml runtime) blkback implementation. It seems to be working much more happily now but I was a bit surprised by the GNTTABOP_unmap_grant_ref doc -- maybe I misread it. In ./xen/include/public/grant_table.h it says /* * GNTTABOP_unmap_grant_ref: Destroy one or more grant-reference mappings * tracked by <handle>. If <host_addr> or <dev_bus_addr> is zero, that * field is ignored. If non-zero, they must refer to a device/host mapping * that is tracked by <handle> * NOTES: * 1. The call may fail in an undefined manner if either mapping is not * tracked by <handle>. * 3. After executing a batch of unmaps, it is guaranteed that no stale * mappings will remain in the device or host TLBs. */ When I read "If <host_addr> or <dev_bus_addr> is zero, that field is ignored" "The call may fail in an undefined manner if either mapping is not tracked by <handle>" I thought, "great, I''ll not bother tracking anything except the handle and set both fields to zero". However if I set host_addr to zero then my frontend complains bitterly gnttab_stubs.c: WARNING: g.e. 53 still in use! (19) If I store the host_addr and fill it in during the unmap, everything seems to work. So did I misread the doc? Or did it mean exclusive-or i.e. you must fill in either one of these two fields? Or have I done something stupid elsewhere (always possible)? If it turns out that I have misread the doc, I''ll happily send a patch to improve the text (once I''m sure I understand what it should say...) Thanks! Dave
On Wed, 24 Jul 2013, David Scott wrote:> Hi, > > I''ve been working on the Mirage (mini-OS + OCaml runtime) blkback > implementation. It seems to be working much more happily now but I was a bit > surprised by the GNTTABOP_unmap_grant_ref doc -- maybe I misread it. > > In ./xen/include/public/grant_table.h it says > > /* > * GNTTABOP_unmap_grant_ref: Destroy one or more grant-reference mappings > * tracked by <handle>. If <host_addr> or <dev_bus_addr> is zero, that > * field is ignored. If non-zero, they must refer to a device/host mapping > * that is tracked by <handle> > * NOTES: > * 1. The call may fail in an undefined manner if either mapping is not > * tracked by <handle>. > * 3. After executing a batch of unmaps, it is guaranteed that no stale > * mappings will remain in the device or host TLBs. > */ > > When I read > "If <host_addr> or <dev_bus_addr> is zero, that field is ignored" > "The call may fail in an undefined manner if either mapping is not tracked > by <handle>" > > I thought, "great, I''ll not bother tracking anything except the handle and set > both fields to zero". However if I set host_addr to zero then my frontend > complains bitterly > > gnttab_stubs.c: WARNING: g.e. 53 still in use! (19) > > If I store the host_addr and fill it in during the unmap, everything seems to > work. > > So did I misread the doc? Or did it mean exclusive-or i.e. you must fill in > either one of these two fields? Or have I done something stupid elsewhere > (always possible)? > > If it turns out that I have misread the doc, I''ll happily send a patch to > improve the text (once I''m sure I understand what it should say...)From reading the implementation it woudl seem clear that host_addr is actually needed. In fact: if ( (op->host_addr != 0) && (op->flags & GNTMAP_host_map) ) { if ( (rc = replace_grant_host_mapping(op->host_addr, op->frame, op->new_addr, op->pteval, op->flags)) < 0 ) goto unmap_out; no unmapping is done if host_addr is 0.
On 24/07/13 13:49, David Scott wrote:> Hi, > > I''ve been working on the Mirage (mini-OS + OCaml runtime) blkback > implementation. It seems to be working much more happily now but I was a > bit surprised by the GNTTABOP_unmap_grant_ref doc -- maybe I misread it. > > In ./xen/include/public/grant_table.h it says > > /* > * GNTTABOP_unmap_grant_ref: Destroy one or more grant-reference mappings > * tracked by <handle>. If <host_addr> or <dev_bus_addr> is zero, that > * field is ignored. If non-zero, they must refer to a device/host mapping > * that is tracked by <handle> > * NOTES: > * 1. The call may fail in an undefined manner if either mapping is not > * tracked by <handle>. > * 3. After executing a batch of unmaps, it is guaranteed that no stale > * mappings will remain in the device or host TLBs. > */ > > When I read > "If <host_addr> or <dev_bus_addr> is zero, that field is ignored" > "The call may fail in an undefined manner if either mapping is not > tracked by <handle>" > > I thought, "great, I''ll not bother tracking anything except the handle > and set both fields to zero". However if I set host_addr to zero then my > frontend complains bitterly > > gnttab_stubs.c: WARNING: g.e. 53 still in use! (19) > > If I store the host_addr and fill it in during the unmap, everything > seems to work. > > So did I misread the doc? Or did it mean exclusive-or i.e. you must fill > in either one of these two fields? Or have I done something stupid > elsewhere (always possible)?If host_addr is non-zero, a host (cpu) mapping is cleared. If dev_bus_addr is non-zero a device (iommu) mapping is cleared. If both are zero, GNTTAB_unmap_grant_ref is a no-op. David
On Wed, 2013-07-24 at 18:31 +0100, Stefano Stabellini wrote:> On Wed, 24 Jul 2013, David Scott wrote: > > Hi, > > > > I''ve been working on the Mirage (mini-OS + OCaml runtime) blkback > > implementation. It seems to be working much more happily now but I was a bit > > surprised by the GNTTABOP_unmap_grant_ref doc -- maybe I misread it. > > > > In ./xen/include/public/grant_table.h it says > > > > /* > > * GNTTABOP_unmap_grant_ref: Destroy one or more grant-reference mappings > > * tracked by <handle>. If <host_addr> or <dev_bus_addr> is zero, that > > * field is ignored. If non-zero, they must refer to a device/host mapping > > * that is tracked by <handle> > > * NOTES: > > * 1. The call may fail in an undefined manner if either mapping is not > > * tracked by <handle>. > > * 3. After executing a batch of unmaps, it is guaranteed that no stale > > * mappings will remain in the device or host TLBs. > > */ > > > > When I read > > "If <host_addr> or <dev_bus_addr> is zero, that field is ignored" > > "The call may fail in an undefined manner if either mapping is not tracked > > by <handle>" > > > > I thought, "great, I''ll not bother tracking anything except the handle and set > > both fields to zero". However if I set host_addr to zero then my frontend > > complains bitterly > > > > gnttab_stubs.c: WARNING: g.e. 53 still in use! (19) > > > > If I store the host_addr and fill it in during the unmap, everything seems to > > work. > > > > So did I misread the doc? Or did it mean exclusive-or i.e. you must fill in > > either one of these two fields? Or have I done something stupid elsewhere > > (always possible)? > > > > If it turns out that I have misread the doc, I''ll happily send a patch to > > improve the text (once I''m sure I understand what it should say...) > > From reading the implementation it woudl seem clear that host_addr is > actually needed.In fact I think you need to pass in whatever you passed to the original mapping operation, because op->flags here is not an argument from the unmap call but is actually a reference to the original flags used to make the mapping. So if you made the mapping with GNTMAP_host_map then you need to pass the host_addr on unmap. Likewise if you used GNTMAP_device_map then you need to specify dev_bus_addr. I''ve added Keir to the CC since he''s the ultimate authority on these things. Ian.> In fact: > > if ( (op->host_addr != 0) && (op->flags & GNTMAP_host_map) ) > { > if ( (rc = replace_grant_host_mapping(op->host_addr, > op->frame, op->new_addr, > op->pteval, > op->flags)) < 0 ) > goto unmap_out; > > > no unmapping is done if host_addr is 0. > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
On 29/07/2013 09:42, "Ian Campbell" <Ian.Campbell@citrix.com> wrote:>>> If it turns out that I have misread the doc, I''ll happily send a patch to >>> improve the text (once I''m sure I understand what it should say...) >> >> From reading the implementation it woudl seem clear that host_addr is >> actually needed. > > In fact I think you need to pass in whatever you passed to the original > mapping operation, because op->flags here is not an argument from the > unmap call but is actually a reference to the original flags used to > make the mapping. So if you made the mapping with GNTMAP_host_map then > you need to pass the host_addr on unmap. Likewise if you used > GNTMAP_device_map then you need to specify dev_bus_addr. > > I''ve added Keir to the CC since he''s the ultimate authority on these > things.The documentation is simply vague. GNTTABOP_unmap_grant_ref will only undo mappings that it is given the address of. If you provide a zero address in either field, that field is ignored. If you provide only zero addresses, then GNTTABOP_unmap_grant_ref does no work at all! But you now worked all this out already of course. ;) -- Keir