Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20180727144448.9606-1-vkuznets@redhat.com>
References: <20180727144448.9606-1-vkuznets@redhat.com>
From:   Jim Mattson <jmattson@google.com>
Date:   Fri, 27 Jul 2018 09:48:42 -0700
Message-ID: <CALMp9eSaiw7wMMhzKZQpM141h6zEz=shu5P++BerbQBTVyFdYA@mail.gmail.com>
Subject: Re: [PATCH RFC] x86/kvm/lapic: always disable MMIO interface in
 x2APIC mode
To:     Vitaly Kuznetsov <vkuznets@redhat.com>
Cc:     kvm list <kvm@vger.kernel.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= <rkrcmar@redhat.com>,
        "the arch/x86 maintainers" <x86@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On a physical machine, I would expect the default local APIC page to
fall in the PCI hole, so it would be correct to sink writes and to
return all ones for reads. Does qemu implement a PCI hole, and does
this address fall into it?

On Fri, Jul 27, 2018 at 7:44 AM, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> When VMX is used with flexpriority disabled (because of no support or
> if disabled with module parameter) MMIO interface to lAPIC is still
> available in x2APIC mode while it shouldn't be (kvm-unit-tests):
>
> PASS: apic_disable: Local apic enabled in x2APIC mode
> PASS: apic_disable: CPUID.1H:EDX.APIC[bit 9] is set
> FAIL: apic_disable: *0xfee00030: 50014
>
> The issue appears because we basically do nothing while switching to
> x2APIC mode when APIC access page is not used. apic_mmio_{read,write}
> only check if lAPIC is disabled before proceeding to actual write.
>
> When APIC access is virtualized we correctly manipulate with VMX controls
> in vmx_set_virtual_apic_mode() and we don't get vmexits from memory writes
> in x2APIC mode so there's no issue.
>
> Disabling MMIO interface seems to be easy. The question is: what do we
> do with these reads and writes? If we add apic_x2apic_mode() check to
> apic_mmio_in_range() and return -EOPNOTSUPP these reads and writes will
> go to userspace. When lAPIC is in kernel, Qemu uses this interface to
> inject MSIs only (see kvm_apic_mem_write() in hw/i386/kvm/apic.c). This
> somehow works with disabled lAPIC but when we're in xAPIC mode we will
> get a real injected MSI from every write to lAPIC. Not good.
>
> The simplest solution seems to be to just ignore writes to the region
> and return ~0 for all reads when we're in x2APIC mode. This is what this
> patch does. However, this approach is inconsistent with what currently
> happens when flexpriority is enabled: we allocate APIC access page and
> create KVM memory region so in x2APIC modes all reads and writes go to
> this pre-allocated page which is, btw, the same for all vCPUs.
>
> The other solution would be to pre-allocate a 'shadow' page for lAPICs
> in disabled/x2APIC mode and re-direct all reads and writes there. I'm,
> however, not convinced this is a good thing to do.
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  arch/x86/kvm/lapic.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index b5cd8465d44f..89b59d6410ae 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1291,9 +1291,8 @@ EXPORT_SYMBOL_GPL(kvm_lapic_reg_read);
>
>  static int apic_mmio_in_range(struct kvm_lapic *apic, gpa_t addr)
>  {
> -       return kvm_apic_hw_enabled(apic) &&
> -           addr >= apic->base_address &&
> -           addr < apic->base_address + LAPIC_MMIO_LENGTH;
> +       return addr >= apic->base_address &&
> +               addr < apic->base_address + LAPIC_MMIO_LENGTH;
>  }
>
>  static int apic_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
> @@ -1305,6 +1304,11 @@ static int apic_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
>         if (!apic_mmio_in_range(apic, address))
>                 return -EOPNOTSUPP;
>
> +       if (!kvm_apic_hw_enabled(apic) || apic_x2apic_mode(apic)) {
> +               memset(data, 0xff, len);
> +               return 0;
> +       }
> +
>         kvm_lapic_reg_read(apic, offset, len, data);
>
>         return 0;
> @@ -1864,6 +1868,9 @@ static int apic_mmio_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
>         if (!apic_mmio_in_range(apic, address))
>                 return -EOPNOTSUPP;
>
> +       if (!kvm_apic_hw_enabled(apic) || apic_x2apic_mode(apic))
> +               return 0;
> +
>         /*
>          * APIC register must be aligned on 128-bits boundary.
>          * 32/64/128 bits registers must be accessed thru 32 bits.
> --
> 2.14.4
>