| Message ID | 20230830103754.36461-18-zhenzhong.duan@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | vfio: Adopt iommufd | expand |
On Wed, Aug 30, 2023 at 06:37:49PM +0800, Zhenzhong Duan wrote: > From: Yi Liu <yi.l.liu@intel.com> > > /dev/vfio/devices/vfioX may not exist. In that case it is still possible > to open /dev/char/$major:$minor instead. Add helper function to abstract > the cdev open. > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > MAINTAINERS | 6 ++++ > include/qemu/char_dev.h | 16 +++++++++++ > util/chardev_open.c | 61 +++++++++++++++++++++++++++++++++++++++++ Using the same naming scheme for the .c and .h is strongly desired. > util/meson.build | 1 + > 4 files changed, 84 insertions(+) > create mode 100644 include/qemu/char_dev.h > create mode 100644 util/chardev_open.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 04663fbb6f..74d18593fe 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3372,6 +3372,12 @@ S: Maintained > F: include/qemu/iova-tree.h > F: util/iova-tree.c > > +cdev Open > +M: Yi Liu <yi.l.liu@intel.com> > +S: Maintained > +F: include/qemu/char_dev.h > +F: util/chardev_open.c > + > diff --git a/util/chardev_open.c b/util/chardev_open.c > new file mode 100644 > index 0000000000..d03e415131 > --- /dev/null > +++ b/util/chardev_open.c > @@ -0,0 +1,61 @@ > +/* > + * Copyright (C) 2023 Intel Corporation. > + * Copyright (c) 2019, Mellanox Technologies. All rights reserved. > + * > + * Authors: Yi Liu <yi.l.liu@intel.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2. See > + * the COPYING file in the top-level directory. > + * > + * Copied from > + * https://github.com/linux-rdma/rdma-core/blob/master/util/open_cdev.c > + * > + */ Since this is GPL-2.0-only, IMHO it would be preferrable to keep it out of the util/ directory, as we're aiming to not add further 2.0 only code, except for specific subdirs. This only appears to be used by code under hw/vfio/, whcih is one of the dirs still permitting 2.0-only code. So I think better to keep this file where it is used. > +#ifndef _GNU_SOURCE > +#define _GNU_SOURCE > +#endif This is set globally for building all files in QEMU > +#include "qemu/osdep.h" > +#include "qemu/char_dev.h" > + > +static int open_cdev_internal(const char *path, dev_t cdev) > +{ > + struct stat st; > + int fd; > + > + fd = qemu_open_old(path, O_RDWR); > + if (fd == -1) { > + return -1; > + } > + if (fstat(fd, &st) || !S_ISCHR(st.st_mode) || > + (cdev != 0 && st.st_rdev != cdev)) { > + close(fd); > + return -1; > + } > + return fd; > +} > + > +static int open_cdev_robust(dev_t cdev) > +{ > + char *devpath; g_autofree for this... > + int ret; > + > + /* > + * This assumes that udev is being used and is creating the /dev/char/ > + * symlinks. > + */ > + devpath = g_strdup_printf("/dev/char/%u:%u", major(cdev), minor(cdev)); > + ret = open_cdev_internal(devpath, cdev); > + g_free(devpath); ...avoids the need for g_free, and also avoids the need for the intermediate 'ret' variable. > + return ret; > +} > + > +int open_cdev(const char *devpath, dev_t cdev) > +{ > + int fd; > + > + fd = open_cdev_internal(devpath, cdev); > + if (fd == -1 && cdev != 0) { > + return open_cdev_robust(cdev); > + } > + return fd; > +} > diff --git a/util/meson.build b/util/meson.build > index a375160286..d5313d858f 100644 > --- a/util/meson.build > +++ b/util/meson.build > @@ -107,6 +107,7 @@ if have_block > util_ss.add(files('filemonitor-stub.c')) > endif > util_ss.add(when: 'CONFIG_LINUX', if_true: files('vfio-helpers.c')) > + util_ss.add(when: 'CONFIG_LINUX', if_true: files('chardev_open.c')) > endif > > if cpu == 'aarch64' > -- > 2.34.1 > > With regards, Daniel
On Wed, Sep 20, 2023 at 01:39:02PM +0100, Daniel P. Berrangé wrote: > > diff --git a/util/chardev_open.c b/util/chardev_open.c > > new file mode 100644 > > index 0000000000..d03e415131 > > --- /dev/null > > +++ b/util/chardev_open.c > > @@ -0,0 +1,61 @@ > > +/* > > + * Copyright (C) 2023 Intel Corporation. > > + * Copyright (c) 2019, Mellanox Technologies. All rights reserved. > > + * > > + * Authors: Yi Liu <yi.l.liu@intel.com> > > + * > > + * This work is licensed under the terms of the GNU GPL, version 2. See > > + * the COPYING file in the top-level directory. > > + * > > + * Copied from > > + * https://github.com/linux-rdma/rdma-core/blob/master/util/open_cdev.c > > + * > > + */ > > Since this is GPL-2.0-only, IMHO it would be preferrable to keep it > out of the util/ directory, as we're aiming to not add further 2.0 > only code, except for specific subdirs. This only appears to be used > by code under hw/vfio/, whcih is one of the dirs still permitting > 2.0-only code. So I think better to keep this file where it is used. The copyright comment above is not fully accurate. The original code is under the "OpenIB" dual license, you can choose to take it using the OpenIB BSD license text: * Redistribution and use in source and binary forms, with or * without modification, are permitted provided that the following * conditions are met: * * - Redistributions of source code must retain the above * copyright notice, this list of conditions and the following * disclaimer. * * - Redistributions in binary form must reproduce the above * copyright notice, this list of conditions and the following * disclaimer in the documentation and/or other materials * provided with the distribution. * * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. And drop reference to GPL if that is what qemu desires. Jason
On Wed, Sep 20, 2023 at 09:53:46AM -0300, Jason Gunthorpe wrote: > On Wed, Sep 20, 2023 at 01:39:02PM +0100, Daniel P. Berrangé wrote: > > > > diff --git a/util/chardev_open.c b/util/chardev_open.c > > > new file mode 100644 > > > index 0000000000..d03e415131 > > > --- /dev/null > > > +++ b/util/chardev_open.c > > > @@ -0,0 +1,61 @@ > > > +/* > > > + * Copyright (C) 2023 Intel Corporation. > > > + * Copyright (c) 2019, Mellanox Technologies. All rights reserved. > > > + * > > > + * Authors: Yi Liu <yi.l.liu@intel.com> > > > + * > > > + * This work is licensed under the terms of the GNU GPL, version 2. See > > > + * the COPYING file in the top-level directory. > > > + * > > > + * Copied from > > > + * https://github.com/linux-rdma/rdma-core/blob/master/util/open_cdev.c > > > + * > > > + */ > > > > Since this is GPL-2.0-only, IMHO it would be preferrable to keep it > > out of the util/ directory, as we're aiming to not add further 2.0 > > only code, except for specific subdirs. This only appears to be used > > by code under hw/vfio/, whcih is one of the dirs still permitting > > 2.0-only code. So I think better to keep this file where it is used. > > The copyright comment above is not fully accurate. > > The original code is under the "OpenIB" dual license, you can choose > to take it using the OpenIB BSD license text: > > * Redistribution and use in source and binary forms, with or > * without modification, are permitted provided that the following > * conditions are met: > * > * - Redistributions of source code must retain the above > * copyright notice, this list of conditions and the following > * disclaimer. > * > * - Redistributions in binary form must reproduce the above > * copyright notice, this list of conditions and the following > * disclaimer in the documentation and/or other materials > * provided with the distribution. > * > * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND > * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS > * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN > * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN > * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE > * SOFTWARE. > > And drop reference to GPL if that is what qemu desires. Simplest is probably just to copy the original license header as-is, and thus preserve the GPL OR BSD choice. With regards, Daniel
>-----Original Message----- >From: Daniel P. Berrangé <berrange@redhat.com> >Sent: Wednesday, September 20, 2023 8:39 PM >Subject: Re: [PATCH v1 17/22] util/char_dev: Add open_cdev() > >On Wed, Aug 30, 2023 at 06:37:49PM +0800, Zhenzhong Duan wrote: >> From: Yi Liu <yi.l.liu@intel.com> >> >> /dev/vfio/devices/vfioX may not exist. In that case it is still possible >> to open /dev/char/$major:$minor instead. Add helper function to abstract >> the cdev open. >> >> Suggested-by: Jason Gunthorpe <jgg@nvidia.com> >> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> MAINTAINERS | 6 ++++ >> include/qemu/char_dev.h | 16 +++++++++++ >> util/chardev_open.c | 61 +++++++++++++++++++++++++++++++++++++++++ > >Using the same naming scheme for the .c and .h is strongly desired. Got it. > >> util/meson.build | 1 + >> 4 files changed, 84 insertions(+) >> create mode 100644 include/qemu/char_dev.h >> create mode 100644 util/chardev_open.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 04663fbb6f..74d18593fe 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -3372,6 +3372,12 @@ S: Maintained >> F: include/qemu/iova-tree.h >> F: util/iova-tree.c >> >> +cdev Open >> +M: Yi Liu <yi.l.liu@intel.com> >> +S: Maintained >> +F: include/qemu/char_dev.h >> +F: util/chardev_open.c >> + > > >> diff --git a/util/chardev_open.c b/util/chardev_open.c >> new file mode 100644 >> index 0000000000..d03e415131 >> --- /dev/null >> +++ b/util/chardev_open.c >> @@ -0,0 +1,61 @@ >> +/* >> + * Copyright (C) 2023 Intel Corporation. >> + * Copyright (c) 2019, Mellanox Technologies. All rights reserved. >> + * >> + * Authors: Yi Liu <yi.l.liu@intel.com> >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2. See >> + * the COPYING file in the top-level directory. >> + * >> + * Copied from >> + * https://github.com/linux-rdma/rdma-core/blob/master/util/open_cdev.c >> + * >> + */ > >Since this is GPL-2.0-only, IMHO it would be preferrable to keep it >out of the util/ directory, as we're aiming to not add further 2.0 >only code, except for specific subdirs. This only appears to be used >by code under hw/vfio/, whcih is one of the dirs still permitting >2.0-only code. So I think better to keep this file where it is used. I'll copy the original license header to preserve the GPL OR BSD choice. As it's not restricted by GPL-2.0-only now, I plan to keep it in util/. Let me know if you still prefer to move to hw/vifo/. > >> +#ifndef _GNU_SOURCE >> +#define _GNU_SOURCE >> +#endif > >This is set globally for building all files in QEMU Will remove it. > >> +#include "qemu/osdep.h" >> +#include "qemu/char_dev.h" >> + >> +static int open_cdev_internal(const char *path, dev_t cdev) >> +{ >> + struct stat st; >> + int fd; >> + >> + fd = qemu_open_old(path, O_RDWR); >> + if (fd == -1) { >> + return -1; >> + } >> + if (fstat(fd, &st) || !S_ISCHR(st.st_mode) || >> + (cdev != 0 && st.st_rdev != cdev)) { >> + close(fd); >> + return -1; >> + } >> + return fd; >> +} >> + >> +static int open_cdev_robust(dev_t cdev) >> +{ >> + char *devpath; > >g_autofree for this... Will do. > >> + int ret; >> + >> + /* >> + * This assumes that udev is being used and is creating the /dev/char/ >> + * symlinks. >> + */ >> + devpath = g_strdup_printf("/dev/char/%u:%u", major(cdev), minor(cdev)); >> + ret = open_cdev_internal(devpath, cdev); >> + g_free(devpath); > >...avoids the need for g_free, and also avoids the need for >the intermediate 'ret' variable. Yes. Thanks Zhenzhong
diff --git a/MAINTAINERS b/MAINTAINERS index 04663fbb6f..74d18593fe 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3372,6 +3372,12 @@ S: Maintained F: include/qemu/iova-tree.h F: util/iova-tree.c +cdev Open +M: Yi Liu <yi.l.liu@intel.com> +S: Maintained +F: include/qemu/char_dev.h +F: util/chardev_open.c + elf2dmp M: Viktor Prutyanov <viktor.prutyanov@phystech.edu> S: Maintained diff --git a/include/qemu/char_dev.h b/include/qemu/char_dev.h new file mode 100644 index 0000000000..6580d351c6 --- /dev/null +++ b/include/qemu/char_dev.h @@ -0,0 +1,16 @@ +/* + * QEMU Chardev Helper + * + * Copyright (C) 2023 Intel Corporation. + * + * Authors: Yi Liu <yi.l.liu@intel.com> + * + * This work is licensed under the terms of the GNU GPL, version 2. See + * the COPYING file in the top-level directory. + */ + +#ifndef QEMU_CHARDEV_HELPERS_H +#define QEMU_CHARDEV_HELPERS_H + +int open_cdev(const char *devpath, dev_t cdev); +#endif diff --git a/util/chardev_open.c b/util/chardev_open.c new file mode 100644 index 0000000000..d03e415131 --- /dev/null +++ b/util/chardev_open.c @@ -0,0 +1,61 @@ +/* + * Copyright (C) 2023 Intel Corporation. + * Copyright (c) 2019, Mellanox Technologies. All rights reserved. + * + * Authors: Yi Liu <yi.l.liu@intel.com> + * + * This work is licensed under the terms of the GNU GPL, version 2. See + * the COPYING file in the top-level directory. + * + * Copied from + * https://github.com/linux-rdma/rdma-core/blob/master/util/open_cdev.c + * + */ +#ifndef _GNU_SOURCE +#define _GNU_SOURCE +#endif +#include "qemu/osdep.h" +#include "qemu/char_dev.h" + +static int open_cdev_internal(const char *path, dev_t cdev) +{ + struct stat st; + int fd; + + fd = qemu_open_old(path, O_RDWR); + if (fd == -1) { + return -1; + } + if (fstat(fd, &st) || !S_ISCHR(st.st_mode) || + (cdev != 0 && st.st_rdev != cdev)) { + close(fd); + return -1; + } + return fd; +} + +static int open_cdev_robust(dev_t cdev) +{ + char *devpath; + int ret; + + /* + * This assumes that udev is being used and is creating the /dev/char/ + * symlinks. + */ + devpath = g_strdup_printf("/dev/char/%u:%u", major(cdev), minor(cdev)); + ret = open_cdev_internal(devpath, cdev); + g_free(devpath); + return ret; +} + +int open_cdev(const char *devpath, dev_t cdev) +{ + int fd; + + fd = open_cdev_internal(devpath, cdev); + if (fd == -1 && cdev != 0) { + return open_cdev_robust(cdev); + } + return fd; +} diff --git a/util/meson.build b/util/meson.build index a375160286..d5313d858f 100644 --- a/util/meson.build +++ b/util/meson.build @@ -107,6 +107,7 @@ if have_block util_ss.add(files('filemonitor-stub.c')) endif util_ss.add(when: 'CONFIG_LINUX', if_true: files('vfio-helpers.c')) + util_ss.add(when: 'CONFIG_LINUX', if_true: files('chardev_open.c')) endif if cpu == 'aarch64'