From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752242AbcGANZV (ORCPT ); Fri, 1 Jul 2016 09:25:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36791 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752023AbcGANZT (ORCPT ); Fri, 1 Jul 2016 09:25:19 -0400 Date: Fri, 1 Jul 2016 15:25:14 +0200 From: Radim =?utf-8?B?S3LEjW3DocWZ?= To: Paolo Bonzini Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, "Lan, Tianyu" , Igor Mammedov , Jan Kiszka , Peter Xu Subject: Re: [PATCH v1 10/11] KVM: x86: add KVM_CAP_X2APIC_API Message-ID: <20160701132514.GD27840@potion> References: <20160630205429.16480-1-rkrcmar@redhat.com> <20160630205429.16480-11-rkrcmar@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Fri, 01 Jul 2016 13:25:18 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 2016-07-01 10:24+0200, Paolo Bonzini: > On 30/06/2016 22:54, Radim Krčmář wrote: >> KVM_CAP_X2APIC_API can be enabled to extend APIC ID in get/set ioctl and MSI >> addresses to 32 bits. Both are needed to support x2APIC. >> >> The capability has to be toggleable and disabled by default, because get/set >> ioctl shifted and truncated APIC ID to 8 bits by using a non-standard protocol >> inspired by xAPIC and the change is not backward-compatible. >> >> Changes to MSI addresses follow the format used by interrupt remapping unit. >> The upper address word, that used to be 0, contains upper 24 bits of the LAPIC >> address in its upper 24 bits. Lower 8 bits are reserved as 0. >> Using the upper address word is not backward-compatible either as we didn't >> check that userspace zeroed the word. Reserved bits are still not explicitly >> checked, but non-zero data will affect LAPIC addresses, which will cause a bug. >> >> Signed-off-by: Radim Krčmář >> --- >> v1: >> * rewritten with a toggleable capability [Paolo] >> * dropped MSI_ADDR_EXT_DEST_ID to enforce reserved bits >> >> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt > [Rewritten documentation] Will apply, thanks. >> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h >> @@ -1365,7 +1367,7 @@ bool kvm_intr_is_single_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq, >> struct kvm_vcpu **dest_vcpu); >> >> void kvm_set_msi_irq(struct kvm_kernel_irq_routing_entry *e, >> - struct kvm_lapic_irq *irq); >> + struct kvm_lapic_irq *irq, bool x2apic_api); > > Just pass a struct kvm as the first argument. Ok. >> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c >> @@ -111,12 +111,17 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src, >> } >> >> void kvm_set_msi_irq(struct kvm_kernel_irq_routing_entry *e, >> - struct kvm_lapic_irq *irq) >> + struct kvm_lapic_irq *irq, bool x2apic_api) >> { >> trace_kvm_msi_set_irq(e->msi.address_lo, e->msi.data); >> >> irq->dest_id = (e->msi.address_lo & >> MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT; >> + if (x2apic_api) >> + /* MSI_ADDR_EXT_DEST_ID() is omitted to introduce bugs on >> + * userspaces that set reserved bits 0-7. >> + */ > > Reread Rusty's API design guidelines and come back. ;) I still consider it as an improvement over not checking at all. ;) > Seriously, please validate the address_hi at both places > (KVM_SET_GSI_ROUTING and KVM_SIGNAL_MSI) and WARN here if you get > non-zero bits 7-0. This is of course better, will do necessary changes. >> + irq->dest_id |= e->msi.address_hi; >> irq->vector = (e->msi.data & >> MSI_DATA_VECTOR_MASK) >> MSI_DATA_VECTOR_SHIFT; >> irq->dest_mode = (1 << MSI_ADDR_DEST_MODE_SHIFT) & e->msi.address_lo; >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> @@ -3799,6 +3800,17 @@ split_irqchip_unlock: >> + case KVM_CAP_X2APIC_API: { >> + struct kvm_enable_cap valid = {.cap = KVM_CAP_X2APIC_API}; >> + >> + r = -EINVAL; >> + if (memcmp(cap, &valid, sizeof(valid))) >> + break; > > Nice trick, and strict argument checking in general is a good idea. > However it's ugly to do it only for KVM_CAP_X2APIC_API and we've really > bad at strict argument checking elsewhere. For consistency, please > check that args[0] is zero, and forgo other violations. :( Ok. Thanks.