| Message ID | 20220111150318.22570-3-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/libs: decouple more from mini-os | expand |
On 11/01/2022 15:03, Juergen Gross wrote: > libgnttab is using implementation details of Mini-OS. Change that by > letting libgnttab use the new alloc_file_type() and get_file_from_fd() > functions and the generic dev pointer of struct file from Mini-OS. > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > V2: > - add alloc_file_type() support > --- > tools/libs/gnttab/minios.c | 68 +++++++++++++++++++++++++++----------- > 1 file changed, 48 insertions(+), 20 deletions(-) > > diff --git a/tools/libs/gnttab/minios.c b/tools/libs/gnttab/minios.c > index f78caadd30..c19f339c8c 100644 > --- a/tools/libs/gnttab/minios.c > +++ b/tools/libs/gnttab/minios.c > @@ -28,18 +28,53 @@ > #include <sys/mman.h> > > #include <errno.h> > +#include <malloc.h> > #include <unistd.h> > > #include "private.h" > > -void minios_gnttab_close_fd(int fd); > +int minios_gnttab_close_fd(int fd); Again, like evtchn, no need to forward declare. However, I've only just realised... > + > +int minios_gnttab_close_fd(int fd) > +{ > + struct file *file = get_file_from_fd(fd); > + > + gntmap_fini(file->dev); > + free(file->dev); > + > + return 0; > +} The only reason this doesn't break the build is because the declaration is not in a header. After this change, you've got the function returning int here, but declared as returning void as far as MiniOS is concerned. Furthermore, we cannot fix this mess atomically now that minios has moved into a separate repo. It's tolerable from an ABI point of view on x86, but I don't know for certain on other architectures. The least bad way I can think of doing this would be to leave void minios_gnttab_close_fd(int fd) exactly as it was, and instead of converting it's use here, use a separate static function straight away for the file ops. (Will be necessary anyway if you like my suggestion of passing file too). Then in the whole function in "tools/libs: final cleanup making mini-os callbacks static", rather than just making it static. ~Andrew
On 11.01.22 21:08, Andrew Cooper wrote: > On 11/01/2022 15:03, Juergen Gross wrote: >> libgnttab is using implementation details of Mini-OS. Change that by >> letting libgnttab use the new alloc_file_type() and get_file_from_fd() >> functions and the generic dev pointer of struct file from Mini-OS. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> V2: >> - add alloc_file_type() support >> --- >> tools/libs/gnttab/minios.c | 68 +++++++++++++++++++++++++++----------- >> 1 file changed, 48 insertions(+), 20 deletions(-) >> >> diff --git a/tools/libs/gnttab/minios.c b/tools/libs/gnttab/minios.c >> index f78caadd30..c19f339c8c 100644 >> --- a/tools/libs/gnttab/minios.c >> +++ b/tools/libs/gnttab/minios.c >> @@ -28,18 +28,53 @@ >> #include <sys/mman.h> >> >> #include <errno.h> >> +#include <malloc.h> >> #include <unistd.h> >> >> #include "private.h" >> >> -void minios_gnttab_close_fd(int fd); >> +int minios_gnttab_close_fd(int fd); > > Again, like evtchn, no need to forward declare. > > > However, I've only just realised... > >> + >> +int minios_gnttab_close_fd(int fd) >> +{ >> + struct file *file = get_file_from_fd(fd); >> + >> + gntmap_fini(file->dev); >> + free(file->dev); >> + >> + return 0; >> +} > > The only reason this doesn't break the build is because the declaration > is not in a header. After this change, you've got the function > returning int here, but declared as returning void as far as MiniOS is > concerned. > > Furthermore, we cannot fix this mess atomically now that minios has > moved into a separate repo. It's tolerable from an ABI point of view on > x86, but I don't know for certain on other architectures. Mini-OS is x86 only right now (well, it has some Arm parts in it, but it is not in a state to be usable on Arm). > The least bad way I can think of doing this would be to leave void > minios_gnttab_close_fd(int fd) exactly as it was, and instead of > converting it's use here, use a separate static function straight away > for the file ops. (Will be necessary anyway if you like my suggestion > of passing file too). Then in the whole function in "tools/libs: final > cleanup making mini-os callbacks static", rather than just making it static. I can change it if you really want. Juergen
On 12/01/2022 07:27, Juergen Gross wrote: > On 11.01.22 21:08, Andrew Cooper wrote: > >> The least bad way I can think of doing this would be to leave void >> minios_gnttab_close_fd(int fd) exactly as it was, and instead of >> converting it's use here, use a separate static function straight away >> for the file ops. (Will be necessary anyway if you like my suggestion >> of passing file too). Then in the whole function in "tools/libs: final >> cleanup making mini-os callbacks static", rather than just making it >> static. > > I can change it if you really want. Well, it will happen as a natural side effect of passing file* rather than fd, but I do think it is a safer course of action. ~Andrew
diff --git a/tools/libs/gnttab/minios.c b/tools/libs/gnttab/minios.c index f78caadd30..c19f339c8c 100644 --- a/tools/libs/gnttab/minios.c +++ b/tools/libs/gnttab/minios.c @@ -28,18 +28,53 @@ #include <sys/mman.h> #include <errno.h> +#include <malloc.h> #include <unistd.h> #include "private.h" -void minios_gnttab_close_fd(int fd); +int minios_gnttab_close_fd(int fd); + +int minios_gnttab_close_fd(int fd) +{ + struct file *file = get_file_from_fd(fd); + + gntmap_fini(file->dev); + free(file->dev); + + return 0; +} + +static struct file_ops gnttab_ops = { + .name = "gnttab", + .close = minios_gnttab_close_fd, +}; int osdep_gnttab_open(xengnttab_handle *xgt) { - int fd = alloc_fd(FTYPE_GNTMAP); - if ( fd == -1 ) + int fd; + struct file *file; + struct gntmap *gntmap; + static unsigned int ftype_gnttab; + + if ( !ftype_gnttab ) + ftype_gnttab = alloc_file_type(&gnttab_ops); + + gntmap = malloc(sizeof(*gntmap)); + if ( !gntmap ) + return -1; + + fd = alloc_fd(ftype_gnttab); + file = get_file_from_fd(fd); + + if ( !file ) + { + free(gntmap); return -1; - gntmap_init(&files[fd].gntmap); + } + + file->dev = gntmap; + gntmap_init(gntmap); xgt->fd = fd; return 0; } @@ -52,28 +87,22 @@ int osdep_gnttab_close(xengnttab_handle *xgt) return close(xgt->fd); } -void minios_gnttab_close_fd(int fd) -{ - gntmap_fini(&files[fd].gntmap); - files[fd].type = FTYPE_NONE; -} - void *osdep_gnttab_grant_map(xengnttab_handle *xgt, uint32_t count, int flags, int prot, uint32_t *domids, uint32_t *refs, uint32_t notify_offset, evtchn_port_t notify_port) { - int fd = xgt->fd; + struct file *file = get_file_from_fd(xgt->fd); int stride = 1; + if (flags & XENGNTTAB_GRANT_MAP_SINGLE_DOMAIN) stride = 0; if (notify_offset != -1 || notify_port != -1) { errno = ENOSYS; return NULL; } - return gntmap_map_grant_refs(&files[fd].gntmap, - count, domids, stride, + return gntmap_map_grant_refs(file->dev, count, domids, stride, refs, prot & PROT_WRITE); } @@ -81,11 +110,10 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count) { - int fd = xgt->fd; + struct file *file = get_file_from_fd(xgt->fd); int ret; - ret = gntmap_munmap(&files[fd].gntmap, - (unsigned long) start_address, - count); + + ret = gntmap_munmap(file->dev, (unsigned long) start_address, count); if (ret < 0) { errno = -ret; return -1; @@ -95,10 +123,10 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt, int osdep_gnttab_set_max_grants(xengnttab_handle *xgt, uint32_t count) { - int fd = xgt->fd; + struct file *file = get_file_from_fd(xgt->fd); int ret; - ret = gntmap_set_max_grants(&files[fd].gntmap, - count); + + ret = gntmap_set_max_grants(file->dev, count); if (ret < 0) { errno = -ret; return -1;
libgnttab is using implementation details of Mini-OS. Change that by letting libgnttab use the new alloc_file_type() and get_file_from_fd() functions and the generic dev pointer of struct file from Mini-OS. Signed-off-by: Juergen Gross <jgross@suse.com> --- V2: - add alloc_file_type() support --- tools/libs/gnttab/minios.c | 68 +++++++++++++++++++++++++++----------- 1 file changed, 48 insertions(+), 20 deletions(-)