Re: [PATCH v1 10/11] KVM: x86: add KVM_CAP_X2APIC_API

[Radim KrÄmÃÅ]

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ÃÅ <rkrcmar@xxxxxxxxxx>
>> ---
>>  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.