Re: [Qemu-devel] [PATCH 1/2] add support for KVM_CAP_SPLIT_IRQCHIP

[Matt Gingell]

Hi Eric,

Thanks for your comments. I’m submitting a v2 based on your feedback.

Matt

> On Nov 13, 2015, at 4:11 PM, Eric Blake <address@hidden> wrote:
> 
> On 11/13/2015 04:25 PM, Matt Gingell wrote:
> 
> [meta-comment:]  This patch was sent without an In-Reply-To header tying
> it back to the 0/2 cover letter, which means mailers render it as its
> own thread.  Git 'send-email' can do proper threading with the right
> settings (although I'm not sure exactly what to suggest tweaking, if the
> defaults aren't already right).
> 
>> This patch adds the initial plumbing for split IRQ chip mode via
>> KVM_CAP_SPLIT_IRQCHIP. In addition to option processing, a number of
>> kvm_*_in_kernel macros are defined to help clarify which component is
>> where.
>> 
>> Signed-off-by: Matt Gingell <address@hidden>
>> ---
> 
>> -static void machine_set_kernel_irqchip(Object *obj, bool value, Error 
>> **errp)
>> +static void machine_set_kernel_irqchip(Object *obj, Visitor *v,
>> +                                       void *opaque, const char *name,
>> +                                       Error **errp)
>> {
>> -    MachineState *ms = MACHINE(obj);
>> -
>> -    ms->kernel_irqchip_allowed = value;
>> -    ms->kernel_irqchip_required = value;
>> +    OnOffSplit mode;
>> +    MachineState *ms = MACHINE(obj);
>> +
>> +    visit_type_OnOffSplit(v, &mode, name, errp);
>> +    switch (mode) {
> 
> 'mode' starts life uninitialized, and the current contract of
> visit_type_OnOffSplit (or any visit_type_Enum, for that matter) is that
> it is left unchanged except on success (see
> qapi/qapi-visit-core.c:input_type_enum() for the implementation).
> However, you are blindly calling switch without checking whether an
> error was reported, which could lead to spurious code behavior depending
> on what was previously on the stack.
> 
> Arguably, we could fix the qapi generator for visit_type_Enum() to set
> the parameter to the sentinel _MAX value on error, to make buggy code
> like this less likely to do the wrong things on uninitialized input.  But
> I don't know if that is a bug worth fixing during 2.5 hardfreeze (it's
> more likely to expose other bugs, but why not let sleeping dogs lie
> rather than risk regressions where things happened to work by chance).
> 
> So, your choices are to pre-initialize mode to a sane sentinel value, or
> else check for error before switching on mode (and remember that you
> can't check errp directly, but would need a local err, since the caller
> may have passed in errp==NULL).  Or in code, either:
> 
> OnOffSplit mode = ON_OFF_SPLIT_MAX;
> visit_type_OnOffSplit(v, &mode, name, errp);
> switch (mode) {
> case ON_OFF_SPLIT_MAX:
> // flag invalid input; errp already contains potentially nice message
> 
> or:
> 
> OnOffSplit mode;
> Error *err = NULL;
> visit_type_OnOffSplit(v, &mode, name, &err);
> if (err) {
>    // handle err, perhaps with error_propagate()
> } else {
>    switch (mode) {
>    case ON_OFF_SPLIT_ON:
>    case ON_OFF_SPLIT_OFF:
>    case ON_OFF_SPLIT_SPLIT:
>       // handle
>       break;
>    default:
>       abort(); // unreachable
> }
> 
>> +    case ON_OFF_SPLIT_ON:
>> +        ms->kernel_irqchip_allowed = true;
>> +        ms->kernel_irqchip_required = true;
>> +        ms->kernel_irqchip_split = false;
>> +        break;
>> +    case ON_OFF_SPLIT_OFF:
>> +        ms->kernel_irqchip_allowed = false;
>> +        ms->kernel_irqchip_required = false;
>> +        ms->kernel_irqchip_split = false;
>> +        break;
>> +    case ON_OFF_SPLIT_SPLIT:
>> +        ms->kernel_irqchip_allowed = true;
>> +        ms->kernel_irqchip_required = true;
>> +        ms->kernel_irqchip_split = true;
>> +        break;
>> +    default:
>> +        error_report("Option 'kernel-irqchip' must be one of on|off|split");
>> +        exit(1);
> 
> Eww. Why are you calling exit() in a function that already returns errp?
> Shouldn't you instead just bubble the error up the stack?
> 
> 
>> +++ b/qapi/common.json
>> @@ -114,3 +114,19 @@
>> ##
>> { 'enum': 'OnOffAuto',
>>   'data': [ 'auto', 'on', 'off' ] }
>> +
>> +##
>> +# @OnOffSplit
>> +#
>> +# An enumeration of three values: on, off, and split
>> +#
>> +# @on: Enabled
>> +#
>> +# @off: Disabled
>> +#
>> +# @split: Mixed
>> +#
>> +# Since: 2.6
>> +##
>> +{ 'enum': 'OnOffSplit',
>> +  'data': [ 'on', 'off', 'split' ] }
> 
> Use of qapi for the enum mapping looks sane.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>