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

[Eric DeVolder <eric.devolder@oracle.com>]

On 01/18/2017 04:47 AM, Wei Liu wrote:
> On Wed, Jan 18, 2017 at 03:45:54AM -0700, Jan Beulich wrote:
>>>>> On 18.01.17 at 11:37, <wei.liu2@citrix.com> wrote:
>>> On Wed, Jan 18, 2017 at 03:19:49AM -0700, Jan Beulich wrote:
>>>>>>> On 17.01.17 at 18:29, <eric.devolder@oracle.com> 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.
>

Hi. I have made the two changes and recreated the entire patch.
It has been posted as v4.
If a delta patch against v3 was what was desired, then please
let me know and I'll provide.

Note my handling of the test_bit() scenario is to explicitly check
for return value of 1, so any value other than 1 returns 0.

Regards,
Eric


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