Re: [Xen-devel] [PATCH v3] kexec: implement STATUS hypercall to check if image is loaded

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Jan 18, 2017 at 03:45:54AM -0700, Jan Beulich wrote:
> >>> On 18.01.17 at 11:37, <wei.liu2@xxxxxxxxxx> wrote:
> > On Wed, Jan 18, 2017 at 03:19:49AM -0700, Jan Beulich wrote:
> >> >>> On 17.01.17 at 18:29, <eric.devolder@xxxxxxxxxx> wrote:
> >> > --- a/xen/common/kexec.c
> >> > +++ b/xen/common/kexec.c
> >> > @@ -1169,6 +1169,22 @@ static int 
> >> > kexec_unload(XEN_GUEST_HANDLE_PARAM(void) uarg)
> >> >      return kexec_do_unload(&unload);
> >> >  }
> >> >  
> >> > +static int kexec_status(XEN_GUEST_HANDLE_PARAM(void) uarg)
> >> > +{
> >> > +    xen_kexec_status_t status;
> >> > +    int base, bit;
> >> > +
> >> > +    if ( unlikely(copy_from_guest(&status, uarg, 1)) )
> >> > +        return -EFAULT;
> >> > +
> >> > +    /* No need to check KEXEC_FLAG_IN_PROGRESS. */
> >> > +
> >> > +    if ( kexec_load_get_bits(status.type, &base, &bit) )
> >> > +        return -EINVAL;
> >> > +
> >> > +    return test_bit(bit, &kexec_flags);
> >> 
> >> In the public header you promise to return zero or one here (unless
> >> an error occurs), which requires the use of !!. Please see x86's
> >> implementation of the function for how/when there can actually be
> >> other non-zero values returned here (in particular all ones, which
> >> would resolve to -EPERM).
> >> 
> >> > --- a/xen/include/public/kexec.h
> >> > +++ b/xen/include/public/kexec.h
> >> > @@ -227,6 +227,19 @@ typedef struct xen_kexec_unload {
> >> >  } xen_kexec_unload_t;
> >> >  DEFINE_XEN_GUEST_HANDLE(xen_kexec_unload_t);
> >> >  
> >> > +/*
> >> > + * Figure out whether we have an image loaded. A return value of
> >> > + * zero indicates no image loaded. A return value of one
> >> > + * indicates an image is loaded. A negative return value
> >> > + * indicates an error.
> >> > + *
> >> > + * Type must be one of KEXEC_TYPE_DEFAULT or KEXEC_TYPE_CRASH.
> >> > + */
> >> > +#define KEXEC_CMD_kexec_status 6
> >> > +typedef struct xen_kexec_status {
> >> > +    uint8_t type;
> >> > +} xen_kexec_status_t;
> >> > +DEFINE_XEN_GUEST_HANDLE(xen_kexec_status_t);
> >> >  #else /* __XEN_INTERFACE_VERSION__ < 0x00040400 */
> >> 
> >> There was a blank line above here before your addition, and you
> >> shouldn't eliminate it (making quickly scanning over the file harder).
> >> 
> >> I guess both items are simple enough to fix while committing.
> > 
> > Oops, I already committed this patch with Andrew's review. A follow-up
> > patch is appreciated. Thanks.
> 
> Well, I suppose that was directed at Eric ...
> 

Yes. That was for Eric.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel