Re: [Qemu-devel] RFC: Add support for KVM_CAP_SPLIT_IRQCHIP

[Eric Blake]

On 10/29/2015 02:25 PM, Matt Gingell wrote:
> Hi,
> 
> The following patch adds support for the new KVM split irqchip
> interface discussed recently on the KVM mailing list.
> 
> [kvm] KVM: x86: Split the APIC from the rest of IRQCHIP.
> http://www.mailbrowse.com/kvm/138252.html
> 
> Our testing found some issues with the implementation of split IRQ
> chip in the kernel, and without changes to address those it will not
> be possible to test this. We've been able to do some preliminary
> testing of the QEMU against our local kernel though and I'm able to
> boot Linux and Windows with KVM_CAP_SPLIT_IRQCHIP enabled.
> 
> While we work on getting the KVM piece ready to submit, I'd appreciate
> any feedback or discussion on the user space portion.
> 
> Thanks,
> Matt Gingell
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index f4db340..3c14e78 100644

Your diff doesn't include the usual '---' separator and diffstat
provided by 'git send-email'; making it a bit harder to see at a glance
what your patch touches.

I'm just doing an interface review, since I happened to notice that it
touched a .json file.

> +++ 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.5 (???)

The question marks should not be in the final patch (but this is an RFC,
so that's okay).  On the other hand, you've missed soft freeze for 2.5,
so it may end up being something we add for 2.6 instead.

> +##
> +{ 'enum': 'OnOffSplit',
> +  'data': [ 'on', 'off', 'split' ] }

Nothing in the user interface seems to use this new enum, so you are
just using it internally.  That's okay; it's not the first time.


> @@ -54,10 +55,15 @@ This is used to enable an accelerator. Depending on the 
> target architecture,
>  kvm, xen, or tcg can be available. By default, tcg is used. If there is more
>  than one accelerator specified, the next one is used if the previous one 
> fails
>  to initialize.
> +<<<<<<<
>  @item kernel_irqchip=on|off
>  Enables in-kernel irqchip support for the chosen accelerator when available.
>  @item gfx_passthru=on|off
>  Enables IGD GFX passthrough support for the chosen machine when available.
> +=======
> address@hidden kernel_irqchip=on|off|split
> +Controls in-kernel irqchip support for the chosen accelerator when available.
> +>>>>>>>

Umm, you really don't want merge markers in your commit.


> @@ -2799,6 +2805,20 @@ void kvm_arch_init_irq_routing(KVMState *s)
>       */
>      kvm_msi_via_irqfd_allowed = true;
>      kvm_gsi_routing_allowed = true;
> +
> +    if (kvm_irqchip_is_split()) {
> +        int i;
> +
> +        /* If the ioapic is in QEMU and the lapics are in KVM, reserve
> +           MSI routes for signaling interrupts to the local apics. */
> +        for (i = 0; i < IOAPIC_NUM_PINS; i++) {
> +            struct MSIMessage msg = { 0x0, 0x0 };
> +            if (kvm_irqchip_add_msi_route(s, msg) < 0) {
> +                fprintf(stderr, "Could not enable split IRQ mode.");
> +                exit(-1);

exit(-1) is usually NOT what you want (yes, xargs has a special case
when $? is 255 - but it is seldom used).  You probably want exit(1).

We are trying to avoid the addition of new fprintf(stderr), and instead
use error_report(), in part because it gives more consistent output
(such as the option to prepend timestamps).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature