Patchwork [v7,26/27] KVM: Document errors for KVM_GET_ONE_REG and KVM_SET_ONE_REG

login
register
mail settings
Submitter Dave Martin
Date March 29, 2019, 1 p.m.
Message ID <1553864452-15080-27-git-send-email-Dave.Martin@arm.com>
Download mbox | patch
Permalink /patch/761195/
State New
Headers show

Comments

Dave Martin - March 29, 2019, 1 p.m.
KVM_GET_ONE_REG and KVM_SET_ONE_REG return some error codes that
are not documented (but hopefully not surprising either).  To give
an indication of what these may mean, this patch adds brief
documentation.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 Documentation/virtual/kvm/api.txt | 6 ++++++
 1 file changed, 6 insertions(+)
Andrew Jones - April 3, 2019, 8:27 p.m.
On Fri, Mar 29, 2019 at 01:00:51PM +0000, Dave Martin wrote:
> KVM_GET_ONE_REG and KVM_SET_ONE_REG return some error codes that
> are not documented (but hopefully not surprising either).  To give
> an indication of what these may mean, this patch adds brief
> documentation.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
>  Documentation/virtual/kvm/api.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 2d4f7ce..cd920dd 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -1871,6 +1871,9 @@ Architectures: all
>  Type: vcpu ioctl
>  Parameters: struct kvm_one_reg (in)
>  Returns: 0 on success, negative value on failure
> +Errors:
> +  ENOENT:   no such register
> +  EINVAL:   other errors, such as bad size encoding for a known register
>  
>  struct kvm_one_reg {
>         __u64 id;
> @@ -2192,6 +2195,9 @@ Architectures: all
>  Type: vcpu ioctl
>  Parameters: struct kvm_one_reg (in and out)
>  Returns: 0 on success, negative value on failure
> +Errors:
> +  ENOENT:   no such register
> +  EINVAL:   other errors, such as bad size encoding for a known register
>  
>  This ioctl allows to receive the value of a single register implemented
>  in a vcpu. The register to read is indicated by the "id" field of the
> -- 
> 2.1.4
>

Are we sure all architectures have these, and only these errors? A quick
grep indicates not. I'm not sure we can document this easily here due to
it addressing all architectures at once. Maybe we could add arch-specific
subsections, but I'm not sure it's worth it.

drew
Dave Martin - April 4, 2019, 8:35 a.m.
On Wed, Apr 03, 2019 at 10:27:46PM +0200, Andrew Jones wrote:
> On Fri, Mar 29, 2019 at 01:00:51PM +0000, Dave Martin wrote:
> > KVM_GET_ONE_REG and KVM_SET_ONE_REG return some error codes that
> > are not documented (but hopefully not surprising either).  To give
> > an indication of what these may mean, this patch adds brief
> > documentation.
> > 
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > ---
> >  Documentation/virtual/kvm/api.txt | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> > index 2d4f7ce..cd920dd 100644
> > --- a/Documentation/virtual/kvm/api.txt
> > +++ b/Documentation/virtual/kvm/api.txt
> > @@ -1871,6 +1871,9 @@ Architectures: all
> >  Type: vcpu ioctl
> >  Parameters: struct kvm_one_reg (in)
> >  Returns: 0 on success, negative value on failure
> > +Errors:
> > +  ENOENT:   no such register
> > +  EINVAL:   other errors, such as bad size encoding for a known register
> >  
> >  struct kvm_one_reg {
> >         __u64 id;
> > @@ -2192,6 +2195,9 @@ Architectures: all
> >  Type: vcpu ioctl
> >  Parameters: struct kvm_one_reg (in and out)
> >  Returns: 0 on success, negative value on failure
> > +Errors:
> > +  ENOENT:   no such register
> > +  EINVAL:   other errors, such as bad size encoding for a known register
> >  
> >  This ioctl allows to receive the value of a single register implemented
> >  in a vcpu. The register to read is indicated by the "id" field of the
> > -- 
> > 2.1.4
> >
> 
> Are we sure all architectures have these, and only these errors? A quick
> grep indicates not. I'm not sure we can document this easily here due to
> it addressing all architectures at once. Maybe we could add arch-specific
> subsections, but I'm not sure it's worth it.

Error codes are generally indicative at best, and rarely mutually
exclusive in a given situation.

Writing a caveat to say that you shouldn't make assumptions about
precisely what error code will be returned in a given situation would
look odd: that really applies widely all over the kernel/user ABI, not
just here.

_Maybe_ everything is exhaustively correct in this file already, but
given the size of it, and the fact that many things are implemented
per-arch, the chance of this seems zero.

I could add "invalid register ID" for EINVAL.  This allows some
ambiguity about which error code applies for a nonexistent register.

Alternatively, we could revert this documentation instead.

It seemed appropriate to document the EPERM error for finalization
ordering (added later), but since correct userspace code should never
see this maybe it's reasonable to leave it undocumented(?)

Cheers
---Dave
Andrew Jones - April 4, 2019, 9:34 a.m.
On Thu, Apr 04, 2019 at 09:35:26AM +0100, Dave Martin wrote:
> On Wed, Apr 03, 2019 at 10:27:46PM +0200, Andrew Jones wrote:
> > On Fri, Mar 29, 2019 at 01:00:51PM +0000, Dave Martin wrote:
> > > KVM_GET_ONE_REG and KVM_SET_ONE_REG return some error codes that
> > > are not documented (but hopefully not surprising either).  To give
> > > an indication of what these may mean, this patch adds brief
> > > documentation.
> > > 
> > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > ---
> > >  Documentation/virtual/kvm/api.txt | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> > > index 2d4f7ce..cd920dd 100644
> > > --- a/Documentation/virtual/kvm/api.txt
> > > +++ b/Documentation/virtual/kvm/api.txt
> > > @@ -1871,6 +1871,9 @@ Architectures: all
> > >  Type: vcpu ioctl
> > >  Parameters: struct kvm_one_reg (in)
> > >  Returns: 0 on success, negative value on failure
> > > +Errors:
> > > +  ENOENT:   no such register
> > > +  EINVAL:   other errors, such as bad size encoding for a known register
> > >  
> > >  struct kvm_one_reg {
> > >         __u64 id;
> > > @@ -2192,6 +2195,9 @@ Architectures: all
> > >  Type: vcpu ioctl
> > >  Parameters: struct kvm_one_reg (in and out)
> > >  Returns: 0 on success, negative value on failure
> > > +Errors:
> > > +  ENOENT:   no such register
> > > +  EINVAL:   other errors, such as bad size encoding for a known register
> > >  
> > >  This ioctl allows to receive the value of a single register implemented
> > >  in a vcpu. The register to read is indicated by the "id" field of the
> > > -- 
> > > 2.1.4
> > >
> > 
> > Are we sure all architectures have these, and only these errors? A quick
> > grep indicates not. I'm not sure we can document this easily here due to
> > it addressing all architectures at once. Maybe we could add arch-specific
> > subsections, but I'm not sure it's worth it.
> 
> Error codes are generally indicative at best, and rarely mutually
> exclusive in a given situation.
> 
> Writing a caveat to say that you shouldn't make assumptions about
> precisely what error code will be returned in a given situation would
> look odd: that really applies widely all over the kernel/user ABI, not
> just here.
> 
> _Maybe_ everything is exhaustively correct in this file already, but
> given the size of it, and the fact that many things are implemented
> per-arch, the chance of this seems zero.
> 
> I could add "invalid register ID" for EINVAL.  This allows some
> ambiguity about which error code applies for a nonexistent register.
> 
> Alternatively, we could revert this documentation instead.

Yeah, I vote we just drop this patch. It could add more confusion
when people start here to determine what to grep and then, when
grepping for it, can't find any occurrences, as would be the case
for ENOENT on at least a couple arches.

Thanks,
drew

> 
> It seemed appropriate to document the EPERM error for finalization
> ordering (added later), but since correct userspace code should never
> see this maybe it's reasonable to leave it undocumented(?)
> 
> Cheers
> ---Dave
Dave Martin - April 4, 2019, 9:38 a.m.
On Thu, Apr 04, 2019 at 10:34:15AM +0100, Andrew Jones wrote:
> On Thu, Apr 04, 2019 at 09:35:26AM +0100, Dave Martin wrote:
> > On Wed, Apr 03, 2019 at 10:27:46PM +0200, Andrew Jones wrote:
> > > On Fri, Mar 29, 2019 at 01:00:51PM +0000, Dave Martin wrote:
> > > > KVM_GET_ONE_REG and KVM_SET_ONE_REG return some error codes that
> > > > are not documented (but hopefully not surprising either).  To give
> > > > an indication of what these may mean, this patch adds brief
> > > > documentation.
> > > >
> > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > > ---
> > > >  Documentation/virtual/kvm/api.txt | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> > > > index 2d4f7ce..cd920dd 100644
> > > > --- a/Documentation/virtual/kvm/api.txt
> > > > +++ b/Documentation/virtual/kvm/api.txt
> > > > @@ -1871,6 +1871,9 @@ Architectures: all
> > > >  Type: vcpu ioctl
> > > >  Parameters: struct kvm_one_reg (in)
> > > >  Returns: 0 on success, negative value on failure
> > > > +Errors:
> > > > +  ENOENT:   no such register
> > > > +  EINVAL:   other errors, such as bad size encoding for a known register
> > > >
> > > >  struct kvm_one_reg {
> > > >         __u64 id;
> > > > @@ -2192,6 +2195,9 @@ Architectures: all
> > > >  Type: vcpu ioctl
> > > >  Parameters: struct kvm_one_reg (in and out)
> > > >  Returns: 0 on success, negative value on failure
> > > > +Errors:
> > > > +  ENOENT:   no such register
> > > > +  EINVAL:   other errors, such as bad size encoding for a known register
> > > >
> > > >  This ioctl allows to receive the value of a single register implemented
> > > >  in a vcpu. The register to read is indicated by the "id" field of the
> > > > --
> > > > 2.1.4
> > > >
> > >
> > > Are we sure all architectures have these, and only these errors? A quick
> > > grep indicates not. I'm not sure we can document this easily here due to
> > > it addressing all architectures at once. Maybe we could add arch-specific
> > > subsections, but I'm not sure it's worth it.
> >
> > Error codes are generally indicative at best, and rarely mutually
> > exclusive in a given situation.
> >
> > Writing a caveat to say that you shouldn't make assumptions about
> > precisely what error code will be returned in a given situation would
> > look odd: that really applies widely all over the kernel/user ABI, not
> > just here.
> >
> > _Maybe_ everything is exhaustively correct in this file already, but
> > given the size of it, and the fact that many things are implemented
> > per-arch, the chance of this seems zero.
> >
> > I could add "invalid register ID" for EINVAL.  This allows some
> > ambiguity about which error code applies for a nonexistent register.
> >
> > Alternatively, we could revert this documentation instead.
>
> Yeah, I vote we just drop this patch. It could add more confusion
> when people start here to determine what to grep and then, when
> grepping for it, can't find any occurrences, as would be the case
> for ENOENT on at least a couple arches.

I guess so.  It's not like we're claiming this ioctl _can't_ fail.
So leaving interpretation of the errno values to common sense is
reasonable.

> > It seemed appropriate to document the EPERM error for finalization
> > ordering (added later), but since correct userspace code should never
> > see this maybe it's reasonable to leave it undocumented(?)

I assume you mean to remove this too?

I don't think correctly written userspace should be relying on seeing
this error code (as opposed to e.g., KVM_GET_REG_LIST's E2BIG which you
must observe in order to allocate the right amount of memory for the reg
list).

Cheers
---Dave
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Andrew Jones - April 4, 2019, 9:45 a.m.
On Thu, Apr 04, 2019 at 09:38:49AM +0000, Dave P Martin wrote:
> On Thu, Apr 04, 2019 at 10:34:15AM +0100, Andrew Jones wrote:
> > On Thu, Apr 04, 2019 at 09:35:26AM +0100, Dave Martin wrote:
> > > On Wed, Apr 03, 2019 at 10:27:46PM +0200, Andrew Jones wrote:
> > > > On Fri, Mar 29, 2019 at 01:00:51PM +0000, Dave Martin wrote:
> > > > > KVM_GET_ONE_REG and KVM_SET_ONE_REG return some error codes that
> > > > > are not documented (but hopefully not surprising either).  To give
> > > > > an indication of what these may mean, this patch adds brief
> > > > > documentation.
> > > > >
> > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > > > ---
> > > > >  Documentation/virtual/kvm/api.txt | 6 ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> > > > > index 2d4f7ce..cd920dd 100644
> > > > > --- a/Documentation/virtual/kvm/api.txt
> > > > > +++ b/Documentation/virtual/kvm/api.txt
> > > > > @@ -1871,6 +1871,9 @@ Architectures: all
> > > > >  Type: vcpu ioctl
> > > > >  Parameters: struct kvm_one_reg (in)
> > > > >  Returns: 0 on success, negative value on failure
> > > > > +Errors:
> > > > > +  ENOENT:   no such register
> > > > > +  EINVAL:   other errors, such as bad size encoding for a known register
> > > > >
> > > > >  struct kvm_one_reg {
> > > > >         __u64 id;
> > > > > @@ -2192,6 +2195,9 @@ Architectures: all
> > > > >  Type: vcpu ioctl
> > > > >  Parameters: struct kvm_one_reg (in and out)
> > > > >  Returns: 0 on success, negative value on failure
> > > > > +Errors:
> > > > > +  ENOENT:   no such register
> > > > > +  EINVAL:   other errors, such as bad size encoding for a known register
> > > > >
> > > > >  This ioctl allows to receive the value of a single register implemented
> > > > >  in a vcpu. The register to read is indicated by the "id" field of the
> > > > > --
> > > > > 2.1.4
> > > > >
> > > >
> > > > Are we sure all architectures have these, and only these errors? A quick
> > > > grep indicates not. I'm not sure we can document this easily here due to
> > > > it addressing all architectures at once. Maybe we could add arch-specific
> > > > subsections, but I'm not sure it's worth it.
> > >
> > > Error codes are generally indicative at best, and rarely mutually
> > > exclusive in a given situation.
> > >
> > > Writing a caveat to say that you shouldn't make assumptions about
> > > precisely what error code will be returned in a given situation would
> > > look odd: that really applies widely all over the kernel/user ABI, not
> > > just here.
> > >
> > > _Maybe_ everything is exhaustively correct in this file already, but
> > > given the size of it, and the fact that many things are implemented
> > > per-arch, the chance of this seems zero.
> > >
> > > I could add "invalid register ID" for EINVAL.  This allows some
> > > ambiguity about which error code applies for a nonexistent register.
> > >
> > > Alternatively, we could revert this documentation instead.
> >
> > Yeah, I vote we just drop this patch. It could add more confusion
> > when people start here to determine what to grep and then, when
> > grepping for it, can't find any occurrences, as would be the case
> > for ENOENT on at least a couple arches.
> 
> I guess so.  It's not like we're claiming this ioctl _can't_ fail.
> So leaving interpretation of the errno values to common sense is
> reasonable.
> 
> > > It seemed appropriate to document the EPERM error for finalization
> > > ordering (added later), but since correct userspace code should never
> > > see this maybe it's reasonable to leave it undocumented(?)
> 
> I assume you mean to remove this too?

I haven't reviewed that stuff yet, so I don't yet have an opinion :)
I'll get through it all today, but I'm trashing on mail and bugzilla
this morning, so progress on the review hasn't started back up since
last night yet...

drew

> 
> I don't think correctly written userspace should be relying on seeing
> this error code (as opposed to e.g., KVM_GET_REG_LIST's E2BIG which you
> must observe in order to allocate the right amount of memory for the reg
> list).
> 
> Cheers
> ---Dave
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Patch

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 2d4f7ce..cd920dd 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -1871,6 +1871,9 @@  Architectures: all
 Type: vcpu ioctl
 Parameters: struct kvm_one_reg (in)
 Returns: 0 on success, negative value on failure
+Errors:
+  ENOENT:   no such register
+  EINVAL:   other errors, such as bad size encoding for a known register
 
 struct kvm_one_reg {
        __u64 id;
@@ -2192,6 +2195,9 @@  Architectures: all
 Type: vcpu ioctl
 Parameters: struct kvm_one_reg (in and out)
 Returns: 0 on success, negative value on failure
+Errors:
+  ENOENT:   no such register
+  EINVAL:   other errors, such as bad size encoding for a known register
 
 This ioctl allows to receive the value of a single register implemented
 in a vcpu. The register to read is indicated by the "id" field of the