[PATCH RFC] x86/kvm/lapic: always disable MMIO interface in x2APIC mode

[PATCH RFC] x86/kvm/lapic: always disable MMIO interface in x2APIC mode

[Vitaly Kuznetsov]

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

Re: [PATCH RFC] x86/kvm/lapic: always disable MMIO interface in x2APIC mode

[Jim Mattson]

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
>

Re: [PATCH RFC] x86/kvm/lapic: always disable MMIO interface in x2APIC mode

[Paolo Bonzini]

On 27/07/2018 18:48, Jim Mattson wrote:
> 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?

It does implement a PCI hole, but when using the kernel LAPIC it expects
that only devices write to that range; therefore that address doesn't
fall into the PCI hole, and instead it generates an MSIs.

Paolo

Re: [PATCH RFC] x86/kvm/lapic: always disable MMIO interface in x2APIC mode

[Vitaly Kuznetsov]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 27/07/2018 18:48, Jim Mattson wrote:
>> 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?
>
> It does implement a PCI hole, but when using the kernel LAPIC it expects
> that only devices write to that range; therefore that address doesn't
> fall into the PCI hole, and instead it generates an MSIs.

Yes, and that's why I believe it's correct to never forward lapic
reads/writes from KVM to userspace when lapic is in kernel.

"RFC" was mostly about the inconsistency with the case when APIC access
page is in use. To be 100% correct I would suggest to somehow make it
behave like MMIO hole in case we're in x2APIC/disabled mode too.

-- 
  Vitaly

Re: [PATCH RFC] x86/kvm/lapic: always disable MMIO interface in x2APIC mode

[Paolo Bonzini]

On 30/07/2018 11:14, Vitaly Kuznetsov wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 27/07/2018 18:48, Jim Mattson wrote:
>>> 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?
>>
>> It does implement a PCI hole, but when using the kernel LAPIC it expects
>> that only devices write to that range; therefore that address doesn't
>> fall into the PCI hole, and instead it generates an MSIs.
> 
> Yes, and that's why I believe it's correct to never forward lapic
> reads/writes from KVM to userspace when lapic is in kernel.
> 
> "RFC" was mostly about the inconsistency with the case when APIC access
> page is in use. To be 100% correct I would suggest to somehow make it
> behave like MMIO hole in case we're in x2APIC/disabled mode too.
> 

FWIW it is possible to move the MSI memory region from system memory to
the PCI address space in QEMU, however I'm worried about backwards
compatibility.

Vitaly, perhaps you could resubmit this patch, and provide a
KVM_CAP_DISABLE_QUIRKS switch that would make apic_mmio_{read,write}
return -EOPNOTSUPP in this case?

Thanks,

Paolo

Re: [PATCH RFC] x86/kvm/lapic: always disable MMIO interface in x2APIC mode

[Vitaly Kuznetsov]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 30/07/2018 11:14, Vitaly Kuznetsov wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> On 27/07/2018 18:48, Jim Mattson wrote:
>>>> 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?
>>>
>>> It does implement a PCI hole, but when using the kernel LAPIC it expects
>>> that only devices write to that range; therefore that address doesn't
>>> fall into the PCI hole, and instead it generates an MSIs.
>> 
>> Yes, and that's why I believe it's correct to never forward lapic
>> reads/writes from KVM to userspace when lapic is in kernel.
>> 
>> "RFC" was mostly about the inconsistency with the case when APIC access
>> page is in use. To be 100% correct I would suggest to somehow make it
>> behave like MMIO hole in case we're in x2APIC/disabled mode too.
>> 
>
> FWIW it is possible to move the MSI memory region from system memory to
> the PCI address space in QEMU, however I'm worried about backwards
> compatibility.
>

You know better :-)

> Vitaly, perhaps you could resubmit this patch, and provide a
> KVM_CAP_DISABLE_QUIRKS switch that would make apic_mmio_{read,write}
> return -EOPNOTSUPP in this case?

Just to make sure I understand,

we introduce a KVM_QUIRK_LAPIC_DISABLED_MMIO bit and will be emulating
MMIO hole in KVM till Qemu is able to deal with reads/writes passed to
it correctly?

-- 
  Vitaly

Re: [PATCH RFC] x86/kvm/lapic: always disable MMIO interface in x2APIC mode

[Paolo Bonzini]

On 02/08/2018 15:30, Vitaly Kuznetsov wrote:
> 
>> Vitaly, perhaps you could resubmit this patch, and provide a
>> KVM_CAP_DISABLE_QUIRKS switch that would make apic_mmio_{read,write}
>> return -EOPNOTSUPP in this case?
> Just to make sure I understand,
> 
> we introduce a KVM_QUIRK_LAPIC_DISABLED_MMIO bit and will be emulating
> MMIO hole in KVM till Qemu is able to deal with reads/writes passed to
> it correctly?

Yes, or forever, whichever comes first.

Paolo