diff mbox

[RFC,v7,1/7] KVM: api: pass the devid in the msi routing entry

Message ID 1468848357-2331-2-git-send-email-eric.auger@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eric Auger July 18, 2016, 1:25 p.m. UTC 
On ARM, the MSI msg (address and data) comes along with
out-of-band device ID information. The device ID encodes the
device that writes the MSI msg. Let's convey the device id in
kvm_irq_routing_msi and use KVM_MSI_VALID_DEVID flag value in
kvm_irq_routing_entry to indicate the msi devid is populated.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>

---

v6 -> v7:
- Added Andre's R-b

v4 -> v5:
- some rephrasing in api.txt according to Christoffer's comments
v2 -> v3:
- replace usage of KVM_IRQ_ROUTING_EXTENDED_MSI type by
  usage of KVM_MSI_VALID_DEVID flag
- add note about KVM_CAP_MSI_DEVID capability

v1 -> v2:
- devid id passed in kvm_irq_routing_msi instead of in
  kvm_irq_routing_entry

RFC -> PATCH
- remove kvm_irq_routing_extended_msi and use union instead
---
 Documentation/virtual/kvm/api.txt | 19 +++++++++++++++++--
 include/uapi/linux/kvm.h          |  5 ++++-
 2 files changed, 21 insertions(+), 3 deletions(-)

Comments

Radim Krčmář July 21, 2016, 4:01 p.m. UTC | #1 
2016-07-18 13:25+0000, Eric Auger:
> On ARM, the MSI msg (address and data) comes along with
> out-of-band device ID information. The device ID encodes the
> device that writes the MSI msg. Let's convey the device id in
> kvm_irq_routing_msi and use KVM_MSI_VALID_DEVID flag value in
> kvm_irq_routing_entry to indicate the msi devid is populated.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> 
> ---
> 
> v6 -> v7:
> - Added Andre's R-b
> 
> v4 -> v5:
> - some rephrasing in api.txt according to Christoffer's comments
> v2 -> v3:
> - replace usage of KVM_IRQ_ROUTING_EXTENDED_MSI type by
>   usage of KVM_MSI_VALID_DEVID flag
> - add note about KVM_CAP_MSI_DEVID capability
> 
> v1 -> v2:
> - devid id passed in kvm_irq_routing_msi instead of in
>   kvm_irq_routing_entry
> 
> RFC -> PATCH
> - remove kvm_irq_routing_extended_msi and use union instead
> ---
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> @@ -1479,9 +1483,20 @@ struct kvm_irq_routing_msi {
>  	__u32 address_lo;
>  	__u32 address_hi;
>  	__u32 data;
> -	__u32 pad;
> +	union {
> +		__u32 pad;
> +		__u32 devid;
> +	};
>  };
>  
> +devid: If KVM_MSI_VALID_DEVID is set, contains a unique device identifier
> +       for the device that wrote the MSI message.
> +       For PCI, this is usually a BFD identifier in the lower 16 bits.
> +
> +The per-VM KVM_CAP_MSI_DEVID capability advertises the requirement to
> +provide the device ID. If this capability is not set, userland cannot
> +rely on the kernel to allow the KVM_MSI_VALID_DEVID flag being set.

It would be better to enforce this mentioned dependency on set
KVM_CAP_MSI_DEVID, but is the dependency even required?
It seems we were checking flags for zero, so KVM_MSI_VALID_DEVID
couldn't have been set by old userspaces, therefor it is ok to only make
it depend only on the presence of KVM_CAP_MSI_DEVID, like the patch does
now.  (I assume KVM_MSI_VALID_DEVID and KVM_CAP_MSI_DEVID are being
merged at the same time.)

Then there would be little point in having KVM_CAP_MSI_DEVID enableable,
so does enabling KVM_CAP_MSI_DEVID mean that every MSI must have a valid
devid?

Thanks.

---
I'm confused about the purpose behind two dynamic flags that seem to do
that same thing, but those are just nitpicks, the API looks good in
general.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andre Przywara July 21, 2016, 4:43 p.m. UTC | #2 
Hi Radim,

On 21/07/16 17:01, Radim Krčmář wrote:
> 2016-07-18 13:25+0000, Eric Auger:
>> On ARM, the MSI msg (address and data) comes along with
>> out-of-band device ID information. The device ID encodes the
>> device that writes the MSI msg. Let's convey the device id in
>> kvm_irq_routing_msi and use KVM_MSI_VALID_DEVID flag value in
>> kvm_irq_routing_entry to indicate the msi devid is populated.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>>
>> ---
>>
>> v6 -> v7:
>> - Added Andre's R-b
>>
>> v4 -> v5:
>> - some rephrasing in api.txt according to Christoffer's comments
>> v2 -> v3:
>> - replace usage of KVM_IRQ_ROUTING_EXTENDED_MSI type by
>>   usage of KVM_MSI_VALID_DEVID flag
>> - add note about KVM_CAP_MSI_DEVID capability
>>
>> v1 -> v2:
>> - devid id passed in kvm_irq_routing_msi instead of in
>>   kvm_irq_routing_entry
>>
>> RFC -> PATCH
>> - remove kvm_irq_routing_extended_msi and use union instead
>> ---
>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>> @@ -1479,9 +1483,20 @@ struct kvm_irq_routing_msi {
>>  	__u32 address_lo;
>>  	__u32 address_hi;
>>  	__u32 data;
>> -	__u32 pad;
>> +	union {
>> +		__u32 pad;
>> +		__u32 devid;
>> +	};
>>  };
>>  
>> +devid: If KVM_MSI_VALID_DEVID is set, contains a unique device identifier
>> +       for the device that wrote the MSI message.
>> +       For PCI, this is usually a BFD identifier in the lower 16 bits.
>> +
>> +The per-VM KVM_CAP_MSI_DEVID capability advertises the requirement to
>> +provide the device ID. If this capability is not set, userland cannot
>> +rely on the kernel to allow the KVM_MSI_VALID_DEVID flag being set.
> 
> It would be better to enforce this mentioned dependency on set
> KVM_CAP_MSI_DEVID, but is the dependency even required?
> It seems we were checking flags for zero, so KVM_MSI_VALID_DEVID
> couldn't have been set by old userspaces, therefor it is ok to only make
> it depend only on the presence of KVM_CAP_MSI_DEVID, like the patch does
> now.  (I assume KVM_MSI_VALID_DEVID and KVM_CAP_MSI_DEVID are being
> merged at the same time.)
> 
> Then there would be little point in having KVM_CAP_MSI_DEVID enableable,
> so does enabling KVM_CAP_MSI_DEVID mean that every MSI must have a valid
> devid?

KVM_CAP_MSI_DEVID tells userland that it's fine to set the
KVM_MSI_VALID_DEVID flag (because the kernel would bark otherwise).

KVM_MSI_VALID_DEVID tells the kernel that there is some meaningful
device ID data in the field formerly known as "pad".

IIRC we started with the VALID_DEVID flag, then found that we need the
CAP because we repurposed the pad field.

Does that make sense? Admittedly this _is_ confusing ;-)

Cheers,
Andre.


> 
> Thanks.
> 
> ---
> I'm confused about the purpose behind two dynamic flags that seem to do
> that same thing, but those are just nitpicks, the API looks good in
> general.
> 
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Radim Krčmář July 21, 2016, 5:15 p.m. UTC | #3 
2016-07-21 17:43+0100, Andre Przywara:
> Hi Radim,
> 
> On 21/07/16 17:01, Radim Krčmář wrote:
>> 2016-07-18 13:25+0000, Eric Auger:
>>> On ARM, the MSI msg (address and data) comes along with
>>> out-of-band device ID information. The device ID encodes the
>>> device that writes the MSI msg. Let's convey the device id in
>>> kvm_irq_routing_msi and use KVM_MSI_VALID_DEVID flag value in
>>> kvm_irq_routing_entry to indicate the msi devid is populated.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>>>
>>> ---
>>>  
>>> +devid: If KVM_MSI_VALID_DEVID is set, contains a unique device identifier
>>> +       for the device that wrote the MSI message.
>>> +       For PCI, this is usually a BFD identifier in the lower 16 bits.
>>> +
>>> +The per-VM KVM_CAP_MSI_DEVID capability advertises the requirement to
>>> +provide the device ID. If this capability is not set, userland cannot
>>> +rely on the kernel to allow the KVM_MSI_VALID_DEVID flag being set.
>> 
>> It would be better to enforce this mentioned dependency on set
>> KVM_CAP_MSI_DEVID, but is the dependency even required?
>> It seems we were checking flags for zero, so KVM_MSI_VALID_DEVID
>> couldn't have been set by old userspaces, therefor it is ok to only make
>> it depend only on the presence of KVM_CAP_MSI_DEVID, like the patch does
>> now.  (I assume KVM_MSI_VALID_DEVID and KVM_CAP_MSI_DEVID are being
>> merged at the same time.)
>> 
>> Then there would be little point in having KVM_CAP_MSI_DEVID enableable,
>> so does enabling KVM_CAP_MSI_DEVID mean that every MSI must have a valid
>> devid?
> 
> KVM_CAP_MSI_DEVID tells userland that it's fine to set the
> KVM_MSI_VALID_DEVID flag (because the kernel would bark otherwise).
> 
> KVM_MSI_VALID_DEVID tells the kernel that there is some meaningful
> device ID data in the field formerly known as "pad".
> 
> IIRC we started with the VALID_DEVID flag, then found that we need the
> CAP because we repurposed the pad field.
> 
> Does that make sense? Admittedly this _is_ confusing ;-)

It does, thanks.
Some capability is need and I thought that KVM_CAP_MSI_DEVID has to be
enabled by userspace before KVM_MSI_VALID_DEVID can be used, which isn't
the case.  It is enabled conditionally based on vgic ITS ... my bad.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Auger July 21, 2016, 8:48 p.m. UTC | #4 
Hi,

On 21/07/2016 19:15, Radim Krčmář wrote:
> 2016-07-21 17:43+0100, Andre Przywara:
>> Hi Radim,
>>
>> On 21/07/16 17:01, Radim Krčmář wrote:
>>> 2016-07-18 13:25+0000, Eric Auger:
>>>> On ARM, the MSI msg (address and data) comes along with
>>>> out-of-band device ID information. The device ID encodes the
>>>> device that writes the MSI msg. Let's convey the device id in
>>>> kvm_irq_routing_msi and use KVM_MSI_VALID_DEVID flag value in
>>>> kvm_irq_routing_entry to indicate the msi devid is populated.
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>>>>
>>>> ---
>>>>  
>>>> +devid: If KVM_MSI_VALID_DEVID is set, contains a unique device identifier
>>>> +       for the device that wrote the MSI message.
>>>> +       For PCI, this is usually a BFD identifier in the lower 16 bits.
>>>> +
>>>> +The per-VM KVM_CAP_MSI_DEVID capability advertises the requirement to
>>>> +provide the device ID. If this capability is not set, userland cannot
>>>> +rely on the kernel to allow the KVM_MSI_VALID_DEVID flag being set.
>>>
>>> It would be better to enforce this mentioned dependency on set
>>> KVM_CAP_MSI_DEVID, but is the dependency even required?
>>> It seems we were checking flags for zero, so KVM_MSI_VALID_DEVID
>>> couldn't have been set by old userspaces, therefor it is ok to only make
>>> it depend only on the presence of KVM_CAP_MSI_DEVID, like the patch does
>>> now.  (I assume KVM_MSI_VALID_DEVID and KVM_CAP_MSI_DEVID are being
>>> merged at the same time.)
>>>
>>> Then there would be little point in having KVM_CAP_MSI_DEVID enableable,
>>> so does enabling KVM_CAP_MSI_DEVID mean that every MSI must have a valid
>>> devid?
>>
>> KVM_CAP_MSI_DEVID tells userland that it's fine to set the
>> KVM_MSI_VALID_DEVID flag (because the kernel would bark otherwise).
>>
>> KVM_MSI_VALID_DEVID tells the kernel that there is some meaningful
>> device ID data in the field formerly known as "pad".
>>
>> IIRC we started with the VALID_DEVID flag, then found that we need the
>> CAP because we repurposed the pad field.
>>
>> Does that make sense? Admittedly this _is_ confusing ;-)
> 
> It does, thanks.
> Some capability is need and I thought that KVM_CAP_MSI_DEVID has to be
> enabled by userspace before KVM_MSI_VALID_DEVID can be used, which isn't
> the case.  It is enabled conditionally based on vgic ITS ... my bad.
> 

Great

Thanks Andre for the clarification

Eric
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index f60b137..0065c8e 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -1468,7 +1468,11 @@  struct kvm_irq_routing_entry {
 #define KVM_IRQ_ROUTING_S390_ADAPTER 3
 #define KVM_IRQ_ROUTING_HV_SINT 4
 
-No flags are specified so far, the corresponding field must be set to zero.
+flags:
+- KVM_MSI_VALID_DEVID: used along with KVM_IRQ_ROUTING_MSI
+  routing entry type, specifies that the devid field contains
+  a valid value.
+- zero otherwise
 
 struct kvm_irq_routing_irqchip {
 	__u32 irqchip;
@@ -1479,9 +1483,20 @@  struct kvm_irq_routing_msi {
 	__u32 address_lo;
 	__u32 address_hi;
 	__u32 data;
-	__u32 pad;
+	union {
+		__u32 pad;
+		__u32 devid;
+	};
 };
 
+devid: If KVM_MSI_VALID_DEVID is set, contains a unique device identifier
+       for the device that wrote the MSI message.
+       For PCI, this is usually a BFD identifier in the lower 16 bits.
+
+The per-VM KVM_CAP_MSI_DEVID capability advertises the requirement to
+provide the device ID. If this capability is not set, userland cannot
+rely on the kernel to allow the KVM_MSI_VALID_DEVID flag being set.
+
 struct kvm_irq_routing_s390_adapter {
 	__u64 ind_addr;
 	__u64 summary_addr;
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index d8c4c32..eb22208 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -879,7 +879,10 @@  struct kvm_irq_routing_msi {
 	__u32 address_lo;
 	__u32 address_hi;
 	__u32 data;
-	__u32 pad;
+	union {
+		__u32 pad;
+		__u32 devid;
+	};
 };
 
 struct kvm_irq_routing_s390_adapter {