Re: [Qemu-devel] [RFC PATCH v2] kvm: x86: ignore ioapic polarity

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Feb 27, 2014 at 11:06:17PM -0500, Gabriel L. Somlo wrote:
> Both QEMU and KVM have already accumulated a significant number of
> optimizations based on the hard-coded assumption that ioapic polarity
> will always use the ActiveHigh convention, where the logical and
> physical states of level-triggered irq lines always match (i.e.,
> active(asserted) == high == 1, inactive == low == 0). QEMU guests
> are expected to follow directions given via ACPI and configure the
> ioapic with polarity 0 (ActiveHigh). However, even when misbehaving
> guests (e.g. OS X <= 10.9) set the ioapic polarity to 1 (ActiveLow),
> QEMU will still use the ActiveHigh signaling convention when
> interfacing with KVM.
> 
> This patch modifies KVM to completely ignore ioapic polarity as set by
> the guest OS, enabling misbehaving guests to work alongside those which
> comply with the ActiveHigh polarity specified by QEMU's ACPI tables.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Gabriel L. Somlo <somlo@cmu.edu>
> ---
> 
> On Fri, Feb 28, 2014 at 12:31:27AM +0100, Paolo Bonzini wrote:
> >>>This is a much better description.  Can you turn it into a patch to
> >>>Documentation/virtual/kvm/api.txt and a more complete commit
> >>>message?
> 
> OK, let me know what you all think of this version.

Looks good to me.

Acked-by: Michael S. Tsirkin <mst@redhat.com>



> > If you change ACPI tables to ActiveLow, and leave the polarity
> > inversion in hw/intc/ioapic.c, then it will matter.
> 
> I only changed ACPI to ActiveLow to cause Linux to misbehave in the
> same way OS X does, it was a temporary/test hack. ACPI should stay
> ActiveHigh to reflect the rest of QEMU/KVM, and misbehaving guest OSs
> should ignore it at their own peril :)

I actually think now that you've fixed intc.c as well, we should change
it as the next step.



> > >(Hmmm, maybe this one of the reasons I never got OS X to boot without
> > >-enable-kvm... I should look at the QEMU hw/intc/ioapic*.c, and see
> > >if *it* cares about guest-configured polarity, and maybe get it to
> > >*stop* caring :)
> 
> > Exactly my point. :)
> 
> So this patch gets the accelerated path to work regardless of how
> well-behaved a guest OS may or may not be w.r.t. ACPI and polarity.
> I'll try to find out whether it's possible to be *extra* nice to these
> types of guests by also allowing them to ignore ACPI polarity when not
> using acceleration :)
> 
> Thanks again,
>   Gabriel
> 
>  Documentation/virtual/kvm/api.txt | 18 ++++++++++++++++++
>  include/uapi/linux/kvm.h          |  1 +
>  virt/kvm/ioapic.c                 |  1 -
>  3 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 6cd63a9..0492a94 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -1365,6 +1365,24 @@ struct kvm_irq_routing_msi {
>  	__u32 pad;
>  };
>  
> +NOTE: For each level-triggered interrupt managed by a virtual ioapic,
> +the guest OS may set a polarity value (bit 13 of each corresponding I/O
> +redirection table register). The polarity bit defines the relationship
> +between an irq line's logical state (active/asserted = 1, inactive = 0)
> +and its physical state (high = 1, low = 0). When the polarity bit is 0
> +(ActiveHigh), logical and physical states are matched (active == high == 1,
> +inactive == low == 0). When polarity is set to 1 (ActiveLow), logical and
> +physical state are opposites (logical == !physical). Typically, guests are
> +expected to use the same polarity across all level-triggered interrupts,
> +as directed by ACPI. Historically, both KVM and QEMU have accumulated a
> +significant number of optimizations based on the hard-coded assumption
> +that polarity will always be set to ActiveHigh. When interfacing with KVM,
> +QEMU will use the ActiveHigh convention for all level-triggered irqs,
> +regardless of how the guest OS has configured the polarity bits in the
> +ioapic registers. As such, KVM must also ignore these bits, and always act
> +as if the logical and physical states of an irq line exactly match each
> +other (i.e., follow the ActiveHigh convention regardless of polarity).
> +
>  
>  4.53 KVM_ASSIGN_SET_MSIX_NR
>  
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 932d7f2..5bfc0e6 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -675,6 +675,7 @@ struct kvm_ppc_smmu_info {
>  #define KVM_CAP_SPAPR_MULTITCE 94
>  #define KVM_CAP_EXT_EMUL_CPUID 95
>  #define KVM_CAP_HYPERV_TIME 96
> +#define KVM_CAP_X86_IOAPIC_POLARITY_IGNORED 97
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index ce9ed99..1539d37 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -328,7 +328,6 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
>  	irq_level = __kvm_irq_line_state(&ioapic->irq_states[irq],
>  					 irq_source_id, level);
>  	entry = ioapic->redirtbl[irq];
> -	irq_level ^= entry.fields.polarity;
>  	if (!irq_level) {
>  		ioapic->irr &= ~mask;
>  		ret = 1;
> -- 
> 1.8.1.4