GNTTABOP_unmap_grant_ref doc

GNTTABOP_unmap_grant_ref doc

[David Scott]

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

Re: GNTTABOP_unmap_grant_ref doc

[Stefano Stabellini]

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.

Re: GNTTABOP_unmap_grant_ref doc

[Ian Campbell]

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

Re: GNTTABOP_unmap_grant_ref doc

[Keir Fraser]

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

Re: GNTTABOP_unmap_grant_ref doc

[David Vrabel]

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