On Fri, Mar 13, 2015 at 02:28:09PM -0600, Alex Williamson wrote:
> On Thu, 2015-03-12 at 15:21 +1100, David Gibson wrote:
> > On Thu, Mar 12, 2015 at 02:16:42PM +1100, Gavin Shan wrote:
> > > On Thu, Mar 12, 2015 at 11:57:21AM +1100, David Gibson wrote:
> > > >On Wed, Mar 11, 2015 at 05:34:11PM +1100, Gavin Shan wrote:
> > > >> The patch adds one more EEH sub-command (VFIO_EEH_PE_INJECT_ERR)
> > > >> to inject the specified EEH error, which is represented by
> > > >> (struct vfio_eeh_pe_err), to the indicated PE for testing purpose.
> > > >>
> > > >> Signed-off-by: Gavin Shan <gwshan@xxxxxxxxxxxxxxxxxx>
> > > >> ---
> > > >> Documentation/vfio.txt | 47 ++++++++++++++++++++++++++++++-------------
> > > >> drivers/vfio/vfio_spapr_eeh.c | 14 +++++++++++++
> > > >> include/uapi/linux/vfio.h | 34 ++++++++++++++++++++++++++++++-
> > > >> 3 files changed, 80 insertions(+), 15 deletions(-)
> > > >>
> > > >> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
> > > >> index 96978ec..2e7f736 100644
> > > >> --- a/Documentation/vfio.txt
> > > >> +++ b/Documentation/vfio.txt
> > > >> @@ -328,7 +328,13 @@ So 4 additional ioctls have been added:
> > > >>
> > > >> The code flow from the example above should be slightly changed:
> > > >>
> > > >> - struct vfio_eeh_pe_op pe_op = { .argsz = sizeof(pe_op), .flags = 0 };
> > > >> + struct vfio_eeh_pe_op *pe_op;
> > > >> + struct vfio_eeh_pe_err *pe_err;
> > > >> +
> > > >> + pe_op = malloc(sizeof(*pe_op) + sizeof(*pe_err));
> > > >> + pe_err = (void *)pe_op + sizeof(*pe_op);
> > > >> + pe_op->argsz = sizeof(*pe_op) + sizeof(*pe_err);
> > > >
> > > >Surely that argsz can't be correct for most of the operations. The
> > > >extended structure should only be there for the error inject ioctl,
> > > >yes?
> > > >
> > >
> > > argsz isn't appropriate for most cases because kernel has the check
> > > "expected_argsz < passed_argsz", not "expected_argsz ==
> > > passed_argsz".
> >
> > It works for now, but if any of those calls was extended with more
> > data, it would break horribly. By setting the argsz greater than
> > necessary, you're effectively passing uninitialized data to the
> > ioctl(). At the moment, the ioctl() ignores it, but the whole point
> > of the argsz value is that in the future, it might not.
>
> argsz tells us how much data the user is passing, we're always going to
> need to figure out what the extra data is, so I don't really see the
> point of this objection. In fact, it might make use of this interface
> quite a bit easier if vfio_eeh_pe_op ended with a union including
> vfio_eeh_pe_err. op == VFIO_EEH_PE_INJECT_ERR defines that the user has
> passed vfio_eeh_pe_err in the union, other ops may add new unions later.
*thinks*
Yeah, I missed the fact that all these ioctls are using a common
arguments structure. In which case, yes, absolutely the error field
should be added to that structure rather than bolted on afterwards.
A union is the obvious way to do that, but I think we should make sure
to include a padding field in that union as well, so we've got some
room to move if we both need to add other union options and add common
things after the union in future.
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
pgpTO2X9jV04V.pgp
Description: PGP signature
