| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 22 Feb 2018 14:48:27 +1100
From: "Alastair D'Silva" <alastair@....ibm.com>
To: Balbir Singh <bsingharora@...il.com>
Cc: linuxppc-dev <linuxppc-dev@...abs.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Arnd Bergmann <arnd@...db.de>, frederic.barrat@...ibm.com,
Greg KH <gregkh@...uxfoundation.org>,
Andrew Donnellan <andrew.donnellan@....ibm.com>
Subject: Re: [PATCH] ocxl: Add get_metadata IOCTL to share OCXL information
to userspace
On Thu, 2018-02-22 at 14:41 +1100, Balbir Singh wrote:
> On Thu, Feb 22, 2018 at 10:32 AM, Alastair D'Silva <alastair@....ibm.
> com> wrote:
> >
> > On Wed, 2018-02-21 at 17:43 +1100, Balbir Singh wrote:
> > > On Wed, Feb 21, 2018 at 3:57 PM, Alastair D'Silva <alastair@....i
> > > bm.c
> > > om> wrote:
> > > > From: Alastair D'Silva <alastair@...ilva.org>
> > > >
> > > > Some required information is not exposed to userspace currently
> > > > (eg. the
> > > > PASID), pass this information back, along with other
> > > > information
> > > > which
> > > > is currently communicated via sysfs, which saves some parsing
> > > > effort in
> > > > userspace.
> > > >
> > > > Signed-off-by: Alastair D'Silva <alastair@...ilva.org>
> > > > ---
> > > > drivers/misc/ocxl/file.c | 27 +++++++++++++++++++++++++++
> > > > include/uapi/misc/ocxl.h | 22 ++++++++++++++++++++++
> > > > 2 files changed, 49 insertions(+)
> > > >
> > > > diff --git a/drivers/misc/ocxl/file.c
> > > > b/drivers/misc/ocxl/file.c
> > > > index d9aa407db06a..11514a8444e5 100644
> > > > --- a/drivers/misc/ocxl/file.c
> > > > +++ b/drivers/misc/ocxl/file.c
> > > > @@ -102,10 +102,32 @@ static long afu_ioctl_attach(struct
> > > > ocxl_context *ctx,
> > > > return rc;
> > > > }
> > > >
> > > > +static long afu_ioctl_get_metadata(struct ocxl_context *ctx,
> > > > + struct ocxl_ioctl_get_metadata __user *uarg)
> > >
> > > Why do we call this metadata? Isn't this an afu_descriptor?
> > >
> >
> > It's metadata for the descriptor.
>
> I meant metadata is too generic, could we have other types of
> metadata in OCXL?
>
I don't believe so, we would instead expand the scope of this IOCTL
using version & space available from the reserved fields.
> >
> > > > +{
> > > > + struct ocxl_ioctl_get_metadata arg;
> > > > +
> > > > + memset(&arg, 0, sizeof(arg));
> > > > +
> > > > + arg.version = 0;
> > >
> > > Does it make sense to have version 0? Even if does, you can
> > > afford
> > > to skip initialization due to the memset above. I prefer that
> > > versions
> > > start with 1
> > >
> >
> > Setting it to 0 is for the reader, not the compiler. I'm not clear
> > on
> > the benefit of starting the version at 1, could you clarify?
>
> How do I distinguish between version number never set and 0?
>
The version number is always set. If the IOCTL doesn't exist, the ioctl
call will error instead.
> >
> > > > +
> > > > + arg.afu_version_major = ctx->afu->config.version_major;
> > > > + arg.afu_version_minor = ctx->afu->config.version_minor;
> > > > + arg.pasid = ctx->pasid;
> > > > + arg.pp_mmio_size = ctx->afu->config.pp_mmio_stride;
> > > > + arg.global_mmio_size = ctx->afu-
> > > > >config.global_mmio_size;
> > > > +
> > > > + if (copy_to_user(uarg, &arg, sizeof(arg)))
> > > > + return -EFAULT;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > #define CMD_STR(x) (x == OCXL_IOCTL_ATTACH ? "ATTACH"
> > > > : \
> > > > x == OCXL_IOCTL_IRQ_ALLOC ? "IRQ_ALLOC"
> > > > : \
> > > > x == OCXL_IOCTL_IRQ_FREE ? "IRQ_FREE"
> > > > : \
> > > > x == OCXL_IOCTL_IRQ_SET_FD ?
> > > > "IRQ_SET_FD"
> > > > : \
> > > > + x == OCXL_IOCTL_GET_METADATA ?
> > > > "GET_METADATA" : \
> > > > "UNKNOWN")
> > > >
> > > > static long afu_ioctl(struct file *file, unsigned int cmd,
> > > > @@ -157,6 +179,11 @@ static long afu_ioctl(struct file *file,
> > > > unsigned int cmd,
> > > > irq_fd.eventfd);
> > > > break;
> > > >
> > > > + case OCXL_IOCTL_GET_METADATA:
> > > > + rc = afu_ioctl_get_metadata(ctx,
> > > > + (struct ocxl_ioctl_get_metadata
> > > > __user *) args);
> > > > + break;
> > > > +
> > > > default:
> > > > rc = -EINVAL;
> > > > }
> > > > diff --git a/include/uapi/misc/ocxl.h
> > > > b/include/uapi/misc/ocxl.h
> > > > index 4b0b0b756f3e..16e1f48ce280 100644
> > > > --- a/include/uapi/misc/ocxl.h
> > > > +++ b/include/uapi/misc/ocxl.h
> > > > @@ -32,6 +32,27 @@ struct ocxl_ioctl_attach {
> > > > __u64 reserved3;
> > > > };
> > > >
> > > > +/*
> > > > + * Version contains the version of the struct.
> > > > + * Versions will always be backwards compatible, that is, new
> > > > versions will not
> > > > + * alter existing fields
> > > > + */
> > > > +struct ocxl_ioctl_get_metadata {
> > >
> > > This sounds more like a function name, do we need it to be
> > > _get_metdata?
> > >
> >
> > It pretty much is a function, it returns to userspace metadata
> > about
> > the descriptor being operated on.
> >
>
> It has a verb indicating action
I misunderstood, I had named the struct to match the IOCTL, but that
isn't necessary. I'll update it in the next patch.
--
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819
Powered by blists - more mailing lists