linux-kernel - Re: [PATCH 3/3] KVM: SVM: Add irqchip_split() checks before enabling AVIC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 12 Sep 2017 15:17:27 +0200
From:   Radim Krčmář <rkrcmar@...hat.com>
To:     Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        pbonzini@...hat.com, joro@...tes.org
Subject: Re: [PATCH 3/3] KVM: SVM: Add irqchip_split() checks before enabling
 AVIC

2017-09-12 01:59-0700, Suravee Suthikulpanit:
> On 9/8/17 08:53, Radim Krčmář wrote:
>> 2017-09-05 22:39-0500, Suravee Suthikulpanit:
>> > SVM AVIC hardware accelerates guest write to APIC_EOI register
>> > (for edge-trigger interrupt), which means it does not trap to KVM.
>> > 
>> > So, only enable SVM AVIC only in split irqchip mode.
>> > (e.g. launching qemu w/ option '-machine kernel_irqchip=split').
>> 
>> Yeah, hacking TMR to get the VM exit could result in future bugs.
>> We have to push split irqchip as the deafult in userspaces with this
>> change.
> 
> Actually, I'm not quite sure about the advantages/disadvantages with split
> irqchip, and how it would affect other cases, and why it was not used as
> default currently.

The main advantage of split irqchip is that we're moving code out of the
kernel, and QEMU's irqchip currently has more features too.

I think it is not the default as the support for split irqchip is recent
(v4.3) and has lower performance, so it is only used in cases that need
the extra features.

> > > +		pr_debug("%s: Disable AVIC due to non-split irqchip.\n",
> > > +			 __func__);
> > 
> > There is going to be too much of those.  pr_debug_once() would be a
> > better notification.  We can also report it in svm_get_enable_apicv().
> 
> pr_debug_once does not use dynamic debug APIs. I think I can call pr_debug
> only when vcpu_id == 0.

I see, the rest uses dynamic debug.  It is not printing by default, so
v1 is ok.  (I'd rather remove the line than to add a condition.)

Thanks.