Message ID | 20211115110851.1462-1-roger.pau@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [for-4.16] freebsd/privcmd: fix MMAP_RESOURCE ioctl definition | expand |
On 15/11/2021 11:08, Roger Pau Monne wrote: > Current ioctl definition was wrong in both FreeBSD and Xen sources, as > the MMAP_RESOURCE ioctl needs to copy back the size of the resource > when passed a zero address and size. FreeBSD encodes in the definition > of the ioctl number whether parameters should be copied in (W) and/or > copied out (R). The current definition for MMAP_RESOURCE is lacking > the copy out part (R), and thus the call to query the size of a > resource would always return 0. > > This change will break the current ioctl interface, the tools can > however fall back to using the foreign memory interface in order to > map resources from guests. > > This was a shortcoming from when the hypercall and ioctl gained the > ability to query the size of the resources, as originally the > MMAP_RESOURCE ioctl didn't need to copy out any data. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Cc: Ian Jackson <iwj@xenproject.org> > > The change only affects FreeBSD, and it's only a change in a > definition of an ioctl, so it's unlikely to break existing code logic. > Without this change Xen tools won't be able to use the MMAP_RESOURCE > ioctl. I guess you found this while trying to fix test-resource, in which case a further argument for the change is "the unit tests now pass on FreeBSD" ? ~Andrew
On Mon, Nov 15, 2021 at 12:03:26PM +0000, Andrew Cooper wrote: > On 15/11/2021 11:08, Roger Pau Monne wrote: > > Current ioctl definition was wrong in both FreeBSD and Xen sources, as > > the MMAP_RESOURCE ioctl needs to copy back the size of the resource > > when passed a zero address and size. FreeBSD encodes in the definition > > of the ioctl number whether parameters should be copied in (W) and/or > > copied out (R). The current definition for MMAP_RESOURCE is lacking > > the copy out part (R), and thus the call to query the size of a > > resource would always return 0. > > > > This change will break the current ioctl interface, the tools can > > however fall back to using the foreign memory interface in order to > > map resources from guests. > > > > This was a shortcoming from when the hypercall and ioctl gained the > > ability to query the size of the resources, as originally the > > MMAP_RESOURCE ioctl didn't need to copy out any data. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > Cc: Ian Jackson <iwj@xenproject.org> > > > > The change only affects FreeBSD, and it's only a change in a > > definition of an ioctl, so it's unlikely to break existing code logic. > > Without this change Xen tools won't be able to use the MMAP_RESOURCE > > ioctl. > > I guess you found this while trying to fix test-resource, in which case > a further argument for the change is "the unit tests now pass on FreeBSD" ? Indeed. It seems like this is the only instance of a resource size query that we have implemented so far. Thanks, Roger.
On 15/11/2021 12:19, Roger Pau Monné wrote: > On Mon, Nov 15, 2021 at 12:03:26PM +0000, Andrew Cooper wrote: >> On 15/11/2021 11:08, Roger Pau Monne wrote: >>> Current ioctl definition was wrong in both FreeBSD and Xen sources, as >>> the MMAP_RESOURCE ioctl needs to copy back the size of the resource >>> when passed a zero address and size. FreeBSD encodes in the definition >>> of the ioctl number whether parameters should be copied in (W) and/or >>> copied out (R). The current definition for MMAP_RESOURCE is lacking >>> the copy out part (R), and thus the call to query the size of a >>> resource would always return 0. >>> >>> This change will break the current ioctl interface, the tools can >>> however fall back to using the foreign memory interface in order to >>> map resources from guests. >>> >>> This was a shortcoming from when the hypercall and ioctl gained the >>> ability to query the size of the resources, as originally the >>> MMAP_RESOURCE ioctl didn't need to copy out any data. >>> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >>> --- >>> Cc: Ian Jackson <iwj@xenproject.org> >>> >>> The change only affects FreeBSD, and it's only a change in a >>> definition of an ioctl, so it's unlikely to break existing code logic. >>> Without this change Xen tools won't be able to use the MMAP_RESOURCE >>> ioctl. >> I guess you found this while trying to fix test-resource, in which case >> a further argument for the change is "the unit tests now pass on FreeBSD" ? > Indeed. It seems like this is the only instance of a resource size > query that we have implemented so far. Well - it's used on the domain create path ever since being introduced to avoid the "shoot a hole in a superpage in order to map the grant table" problem, but it does have a fallback which is probably why it went unnoticed. ~Andrew
Roger Pau Monne writes ("[PATCH for-4.16] freebsd/privcmd: fix MMAP_RESOURCE ioctl definition"): > Current ioctl definition was wrong in both FreeBSD and Xen sources, as > the MMAP_RESOURCE ioctl needs to copy back the size of the resource > when passed a zero address and size. FreeBSD encodes in the definition > of the ioctl number whether parameters should be copied in (W) and/or > copied out (R). The current definition for MMAP_RESOURCE is lacking > the copy out part (R), and thus the call to query the size of a > resource would always return 0. > > This change will break the current ioctl interface, the tools can > however fall back to using the foreign memory interface in order to > map resources from guests. > > This was a shortcoming from when the hypercall and ioctl gained the > ability to query the size of the resources, as originally the > MMAP_RESOURCE ioctl didn't need to copy out any data. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Cc: Ian Jackson <iwj@xenproject.org> > > The change only affects FreeBSD, and it's only a change in a > definition of an ioctl, so it's unlikely to break existing code logic. > Without this change Xen tools won't be able to use the MMAP_RESOURCE > ioctl. Release-Acked-by: Ian Jackson <iwj@xenproject.org>
diff --git a/tools/include/xen-sys/FreeBSD/privcmd.h b/tools/include/xen-sys/FreeBSD/privcmd.h index 649ad443c7..70cee3db68 100644 --- a/tools/include/xen-sys/FreeBSD/privcmd.h +++ b/tools/include/xen-sys/FreeBSD/privcmd.h @@ -84,7 +84,7 @@ typedef struct ioctl_privcmd_dmop privcmd_dm_op_t; #define IOCTL_PRIVCMD_MMAPBATCH \ _IOWR('E', 1, struct ioctl_privcmd_mmapbatch) #define IOCTL_PRIVCMD_MMAP_RESOURCE \ - _IOW('E', 2, struct ioctl_privcmd_mmapresource) + _IOWR('E', 2, struct ioctl_privcmd_mmapresource) #define IOCTL_PRIVCMD_DM_OP \ _IOW('E', 3, struct ioctl_privcmd_dmop) #define IOCTL_PRIVCMD_RESTRICT \
Current ioctl definition was wrong in both FreeBSD and Xen sources, as the MMAP_RESOURCE ioctl needs to copy back the size of the resource when passed a zero address and size. FreeBSD encodes in the definition of the ioctl number whether parameters should be copied in (W) and/or copied out (R). The current definition for MMAP_RESOURCE is lacking the copy out part (R), and thus the call to query the size of a resource would always return 0. This change will break the current ioctl interface, the tools can however fall back to using the foreign memory interface in order to map resources from guests. This was a shortcoming from when the hypercall and ioctl gained the ability to query the size of the resources, as originally the MMAP_RESOURCE ioctl didn't need to copy out any data. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Ian Jackson <iwj@xenproject.org> The change only affects FreeBSD, and it's only a change in a definition of an ioctl, so it's unlikely to break existing code logic. Without this change Xen tools won't be able to use the MMAP_RESOURCE ioctl. --- tools/include/xen-sys/FreeBSD/privcmd.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)