DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 23F4F5F739
Date: Tue, 12 Sep 2017 15:17:27 +0200
From: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
To: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, pbonzini@redhat.com,
        joro@8bytes.org
Subject: Re: [PATCH 3/3] KVM: SVM: Add irqchip_split() checks before enabling
 AVIC
Message-ID: <20170912131727.GA24326@flask>
References: <1504669169-4919-1-git-send-email-suravee.suthikulpanit@amd.com>
 <1504669169-4919-4-git-send-email-suravee.suthikulpanit@amd.com>
 <20170908155324.GB14748@flask>
 <9475950b-566f-9425-5de9-9e6677e41043@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <9475950b-566f-9425-5de9-9e6677e41043@amd.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1536
Lines: 37

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.