From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [PATCH v1 10/11] KVM: x86: add KVM_CAP_X2APIC_API Date: Fri, 1 Jul 2016 15:25:14 +0200 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-Transfer-Encoding: QUOTED-PRINTABLE Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, "Lan, Tianyu" , Igor Mammedov , Jan Kiszka , Peter Xu To: Paolo Bonzini Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org 2016-07-01 10:24+0200, Paolo Bonzini: > On 30/06/2016 22:54, Radim Kr=C4=8Dm=C3=A1=C5=99 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. >>=20 >> 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-standar= d protocol >> inspired by xAPIC and the change is not backward-compatible. >>=20 >> Changes to MSI addresses follow the format used by interrupt remappi= ng 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 e= xplicitly >> checked, but non-zero data will affect LAPIC addresses, which will c= ause a bug. >>=20 >> Signed-off-by: Radim Kr=C4=8Dm=C3=A1=C5=99 >> --- >> v1: >> * rewritten with a toggleable capability [Paolo] >> * dropped MSI_ADDR_EXT_DEST_ID to enforce reserved bits >>=20 >> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtu= al/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); >> =20 >> 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); >=20 > 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, >> } >> =20 >> 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); >> =20 >> irq->dest_id =3D (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. >> + */ >=20 > 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 |=3D e->msi.address_hi; >> irq->vector =3D (e->msi.data & >> MSI_DATA_VECTOR_MASK) >> MSI_DATA_VECTOR_SHIFT; >> irq->dest_mode =3D (1 << MSI_ADDR_DEST_MODE_SHIFT) & e->msi.addres= s_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 =3D {.cap =3D KVM_CAP_X2APIC_API}; >> + >> + r =3D -EINVAL; >> + if (memcmp(cap, &valid, sizeof(valid))) >> + break; >=20 > 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 real= ly > bad at strict argument checking elsewhere. For consistency, please > check that args[0] is zero, and forgo other violations. :( Ok. Thanks.