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ář <rkrc...@redhat.com>
>> ---
>> 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.
