| Message ID | 1435592237-17924-2-git-send-email-eric.auger@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello! > -----Original Message----- > From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On Behalf Of Eric Auger > Sent: Monday, June 29, 2015 6:37 PM > To: eric.auger@st.com; eric.auger@linaro.org; linux-arm-kernel@lists.infradead.org; > marc.zyngier@arm.com; christoffer.dall@linaro.org; andre.przywara@arm.com; > kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org > Cc: linux-kernel@vger.kernel.org; patches@linaro.org; p.fedin@samsung.com; pbonzini@redhat.com > Subject: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi > > On ARM, the MSI msg (address and data) comes along with > out-of-band device ID information. The device ID encodes the device > that composes the MSI msg. Let's create a new routing entry type, > dubbed KVM_IRQ_ROUTING_EXTENDED_MSI and use the __u32 pad space > to convey the device ID. > > Signed-off-by: Eric Auger <eric.auger@linaro.org> > > --- > > RFC -> PATCH > - remove kvm_irq_routing_extended_msi and use union instead > --- > Documentation/virtual/kvm/api.txt | 9 ++++++++- > include/uapi/linux/kvm.h | 6 +++++- > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt > index d20fd94..6426ae9 100644 > --- a/Documentation/virtual/kvm/api.txt > +++ b/Documentation/virtual/kvm/api.txt > @@ -1414,7 +1414,10 @@ struct kvm_irq_routing_entry { > __u32 gsi; > __u32 type; > __u32 flags; > - __u32 pad; > + union { > + __u32 pad; > + __u32 devid; > + }; > union { > struct kvm_irq_routing_irqchip irqchip; > struct kvm_irq_routing_msi msi; devid is actually a part of MSI bunch. Shouldn't it be a part of struct kvm_irq_routing_msi then? It also has reserved pad. > @@ -1427,6 +1430,10 @@ struct kvm_irq_routing_entry { > #define KVM_IRQ_ROUTING_IRQCHIP 1 > #define KVM_IRQ_ROUTING_MSI 2 > #define KVM_IRQ_ROUTING_S390_ADAPTER 3 > +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4 > + > +In case of KVM_IRQ_ROUTING_EXTENDED_MSI routing type, devid is used to convey > +the device ID. > > No flags are specified so far, the corresponding field must be set to zero. What if we use KVM_MSI_VALID_DEVID flag instead of new KVM_IRQ_ROUTING_EXTENDED_MSI definition? I believe this would make an API more consistent and introduce less new definitions. > > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > index 2a23705..8484681 100644 > --- a/include/uapi/linux/kvm.h > +++ b/include/uapi/linux/kvm.h > @@ -841,12 +841,16 @@ struct kvm_irq_routing_s390_adapter { > #define KVM_IRQ_ROUTING_IRQCHIP 1 > #define KVM_IRQ_ROUTING_MSI 2 > #define KVM_IRQ_ROUTING_S390_ADAPTER 3 > +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4 > > struct kvm_irq_routing_entry { > __u32 gsi; > __u32 type; > __u32 flags; > - __u32 pad; > + union { > + __u32 pad; > + __u32 devid; > + }; > union { > struct kvm_irq_routing_irqchip irqchip; > struct kvm_irq_routing_msi msi; > -- > 1.9.1 > > -- > 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 Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia -- 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
Hello! > What if we use KVM_MSI_VALID_DEVID flag instead of new KVM_IRQ_ROUTING_EXTENDED_MSI > definition? I > believe this would make an API more consistent and introduce less new definitions. I have just found one more flaw in your implementation. If you take a look at irqfd_wakeup()... --- cut --- /* An event has been signaled, inject an interrupt */ if (irq.type == KVM_IRQ_ROUTING_MSI) kvm_set_msi(&irq, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1, false); else schedule_work(&irqfd->inject); --- cut --- You apparently missed KVM_IRQ_ROUTING_EXTENDED_MSI here, as well as in irqfd_update(). But, if you accept my API proposal, this becomes irrelevant. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia -- 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
Hi Pavel, On 07/02/2015 09:26 AM, Pavel Fedin wrote: > Hello! > >> -----Original Message----- >> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On Behalf Of Eric Auger >> Sent: Monday, June 29, 2015 6:37 PM >> To: eric.auger@st.com; eric.auger@linaro.org; linux-arm-kernel@lists.infradead.org; >> marc.zyngier@arm.com; christoffer.dall@linaro.org; andre.przywara@arm.com; >> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org; patches@linaro.org; p.fedin@samsung.com; pbonzini@redhat.com >> Subject: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi >> >> On ARM, the MSI msg (address and data) comes along with >> out-of-band device ID information. The device ID encodes the device >> that composes the MSI msg. Let's create a new routing entry type, >> dubbed KVM_IRQ_ROUTING_EXTENDED_MSI and use the __u32 pad space >> to convey the device ID. >> >> Signed-off-by: Eric Auger <eric.auger@linaro.org> >> >> --- >> >> RFC -> PATCH >> - remove kvm_irq_routing_extended_msi and use union instead >> --- >> Documentation/virtual/kvm/api.txt | 9 ++++++++- >> include/uapi/linux/kvm.h | 6 +++++- >> 2 files changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt >> index d20fd94..6426ae9 100644 >> --- a/Documentation/virtual/kvm/api.txt >> +++ b/Documentation/virtual/kvm/api.txt >> @@ -1414,7 +1414,10 @@ struct kvm_irq_routing_entry { >> __u32 gsi; >> __u32 type; >> __u32 flags; >> - __u32 pad; >> + union { >> + __u32 pad; >> + __u32 devid; >> + }; >> union { >> struct kvm_irq_routing_irqchip irqchip; >> struct kvm_irq_routing_msi msi; > > devid is actually a part of MSI bunch. Shouldn't it be a part of struct kvm_irq_routing_msi then? > It also has reserved pad. Well this makes sense to me to associate the devid to the msi and put devid in the pad field of struct kvm_irq_routing_msi. André, Christoffer, would you agree on this change? - I would like to avoid doing/undoing things ;-) - > >> @@ -1427,6 +1430,10 @@ struct kvm_irq_routing_entry { >> #define KVM_IRQ_ROUTING_IRQCHIP 1 >> #define KVM_IRQ_ROUTING_MSI 2 >> #define KVM_IRQ_ROUTING_S390_ADAPTER 3 >> +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4 >> + >> +In case of KVM_IRQ_ROUTING_EXTENDED_MSI routing type, devid is used to convey >> +the device ID. >> >> No flags are specified so far, the corresponding field must be set to zero. > > What if we use KVM_MSI_VALID_DEVID flag instead of new KVM_IRQ_ROUTING_EXTENDED_MSI definition? I > believe this would make an API more consistent and introduce less new definitions. do you mean using type == KVM_IRQ_ROUTING_MSI and flag == KVM_MSI_VALID_DEVID? Not sure this is simpler/clearer. s390 paved the way for new routing entry types. I add a new one here. Another solution may be to use new KVM_IRQ_ROUTING_EXTENDED_MSI type and add struct kvm_msi ext_msi in kvm_irq_routing_entry union. It is 8 words as well. But most probably this is even uglier. Let's see if this thread is heading to a consensus... Best Regards Eric > >> >> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h >> index 2a23705..8484681 100644 >> --- a/include/uapi/linux/kvm.h >> +++ b/include/uapi/linux/kvm.h >> @@ -841,12 +841,16 @@ struct kvm_irq_routing_s390_adapter { >> #define KVM_IRQ_ROUTING_IRQCHIP 1 >> #define KVM_IRQ_ROUTING_MSI 2 >> #define KVM_IRQ_ROUTING_S390_ADAPTER 3 >> +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4 >> >> struct kvm_irq_routing_entry { >> __u32 gsi; >> __u32 type; >> __u32 flags; >> - __u32 pad; >> + union { >> + __u32 pad; >> + __u32 devid; >> + }; >> union { >> struct kvm_irq_routing_irqchip irqchip; >> struct kvm_irq_routing_msi msi; >> -- >> 1.9.1 >> >> -- >> 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 > > Kind regards, > Pavel Fedin > Expert Engineer > Samsung Electronics Research center Russia > -- 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
On 07/02/2015 10:41 AM, Pavel Fedin wrote: > Hello! > >> What if we use KVM_MSI_VALID_DEVID flag instead of new KVM_IRQ_ROUTING_EXTENDED_MSI >> definition? I >> believe this would make an API more consistent and introduce less new definitions. > > I have just found one more flaw in your implementation. If you take a look at irqfd_wakeup()... > --- cut --- > /* An event has been signaled, inject an interrupt */ > if (irq.type == KVM_IRQ_ROUTING_MSI) > kvm_set_msi(&irq, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1, > false); > else > schedule_work(&irqfd->inject); > --- cut --- > You apparently missed KVM_IRQ_ROUTING_EXTENDED_MSI here, as well as in irqfd_update(). But, if you > accept my API proposal, this becomes irrelevant. Hi Pavel, thanks for spotting this bug. Whatever the user-api API choice I will respin shortly fixing this plus the one reported by André. Thanks for the review. Best Regards Eric > > Kind regards, > Pavel Fedin > Expert Engineer > Samsung Electronics Research center Russia > > -- 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
Hi Eric, On 02/07/15 15:49, Eric Auger wrote: > Hi Pavel, > On 07/02/2015 09:26 AM, Pavel Fedin wrote: >> Hello! >> >>> -----Original Message----- >>> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On Behalf Of Eric Auger >>> Sent: Monday, June 29, 2015 6:37 PM >>> To: eric.auger@st.com; eric.auger@linaro.org; linux-arm-kernel@lists.infradead.org; >>> marc.zyngier@arm.com; christoffer.dall@linaro.org; andre.przywara@arm.com; >>> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org >>> Cc: linux-kernel@vger.kernel.org; patches@linaro.org; p.fedin@samsung.com; pbonzini@redhat.com >>> Subject: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi >>> >>> On ARM, the MSI msg (address and data) comes along with >>> out-of-band device ID information. The device ID encodes the device >>> that composes the MSI msg. Let's create a new routing entry type, >>> dubbed KVM_IRQ_ROUTING_EXTENDED_MSI and use the __u32 pad space >>> to convey the device ID. >>> >>> Signed-off-by: Eric Auger <eric.auger@linaro.org> >>> >>> --- >>> >>> RFC -> PATCH >>> - remove kvm_irq_routing_extended_msi and use union instead >>> --- >>> Documentation/virtual/kvm/api.txt | 9 ++++++++- >>> include/uapi/linux/kvm.h | 6 +++++- >>> 2 files changed, 13 insertions(+), 2 deletions(-) >>> >>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt >>> index d20fd94..6426ae9 100644 >>> --- a/Documentation/virtual/kvm/api.txt >>> +++ b/Documentation/virtual/kvm/api.txt >>> @@ -1414,7 +1414,10 @@ struct kvm_irq_routing_entry { >>> __u32 gsi; >>> __u32 type; >>> __u32 flags; >>> - __u32 pad; >>> + union { >>> + __u32 pad; >>> + __u32 devid; >>> + }; >>> union { >>> struct kvm_irq_routing_irqchip irqchip; >>> struct kvm_irq_routing_msi msi; >> >> devid is actually a part of MSI bunch. Shouldn't it be a part of struct kvm_irq_routing_msi then? >> It also has reserved pad. > Well this makes sense to me to associate the devid to the msi and put > devid in the pad field of struct kvm_irq_routing_msi. > > André, Christoffer, would you agree on this change? - I would like to > avoid doing/undoing things ;-) - Yes, that makes sense to me. TBH I haven't had a closer look at the patches yet, but clearly devid belongs into struct kvm_irq_routing_msi. >> >>> @@ -1427,6 +1430,10 @@ struct kvm_irq_routing_entry { >>> #define KVM_IRQ_ROUTING_IRQCHIP 1 >>> #define KVM_IRQ_ROUTING_MSI 2 >>> #define KVM_IRQ_ROUTING_S390_ADAPTER 3 >>> +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4 >>> + >>> +In case of KVM_IRQ_ROUTING_EXTENDED_MSI routing type, devid is used to convey >>> +the device ID. >>> >>> No flags are specified so far, the corresponding field must be set to zero. >> >> What if we use KVM_MSI_VALID_DEVID flag instead of new KVM_IRQ_ROUTING_EXTENDED_MSI definition? I >> believe this would make an API more consistent and introduce less new definitions. > do you mean using type == KVM_IRQ_ROUTING_MSI and flag == > KVM_MSI_VALID_DEVID? Not sure this is simpler/clearer. s390 paved the > way for new routing entry types. I add a new one here. I tend to agree with Pavel's solution. When hacking IRQ routing support into kvmtool I saw that it's nasty being forced to differentiate between the two MSI routing types. Actually userland should be able to query the kernel about what kind of routing it requires. Also there is the issue that we must _not_ set the flag on x86, since that breaks older kernels (due to that check that Eric removes in 3/7). So from my point of view the cleanest solution would be to always use KVM_IRQ_ROUTING_MSI, and add the device ID if the kernel needs it (true for ITS guests, false for GICv2M, x86, ...) I am looking for a clever solution for this now. Cheers, Andre. > > Another solution may be to use new KVM_IRQ_ROUTING_EXTENDED_MSI type and > add struct kvm_msi ext_msi in kvm_irq_routing_entry union. It is 8 words > as well. But most probably this is even uglier. > > Let's see if this thread is heading to a consensus... > > Best Regards > > Eric >> >>> >>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h >>> index 2a23705..8484681 100644 >>> --- a/include/uapi/linux/kvm.h >>> +++ b/include/uapi/linux/kvm.h >>> @@ -841,12 +841,16 @@ struct kvm_irq_routing_s390_adapter { >>> #define KVM_IRQ_ROUTING_IRQCHIP 1 >>> #define KVM_IRQ_ROUTING_MSI 2 >>> #define KVM_IRQ_ROUTING_S390_ADAPTER 3 >>> +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4 >>> >>> struct kvm_irq_routing_entry { >>> __u32 gsi; >>> __u32 type; >>> __u32 flags; >>> - __u32 pad; >>> + union { >>> + __u32 pad; >>> + __u32 devid; >>> + }; >>> union { >>> struct kvm_irq_routing_irqchip irqchip; >>> struct kvm_irq_routing_msi msi; >>> -- >>> 1.9.1 >>> >>> -- >>> 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 >> >> Kind regards, >> Pavel Fedin >> Expert Engineer >> Samsung Electronics Research center Russia >> > -- 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
Hi Andre, On 07/02/2015 05:14 PM, Andre Przywara wrote: > Hi Eric, > > On 02/07/15 15:49, Eric Auger wrote: >> Hi Pavel, >> On 07/02/2015 09:26 AM, Pavel Fedin wrote: >>> Hello! >>> >>>> -----Original Message----- >>>> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On Behalf Of Eric Auger >>>> Sent: Monday, June 29, 2015 6:37 PM >>>> To: eric.auger@st.com; eric.auger@linaro.org; linux-arm-kernel@lists.infradead.org; >>>> marc.zyngier@arm.com; christoffer.dall@linaro.org; andre.przywara@arm.com; >>>> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org >>>> Cc: linux-kernel@vger.kernel.org; patches@linaro.org; p.fedin@samsung.com; pbonzini@redhat.com >>>> Subject: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi >>>> >>>> On ARM, the MSI msg (address and data) comes along with >>>> out-of-band device ID information. The device ID encodes the device >>>> that composes the MSI msg. Let's create a new routing entry type, >>>> dubbed KVM_IRQ_ROUTING_EXTENDED_MSI and use the __u32 pad space >>>> to convey the device ID. >>>> >>>> Signed-off-by: Eric Auger <eric.auger@linaro.org> >>>> >>>> --- >>>> >>>> RFC -> PATCH >>>> - remove kvm_irq_routing_extended_msi and use union instead >>>> --- >>>> Documentation/virtual/kvm/api.txt | 9 ++++++++- >>>> include/uapi/linux/kvm.h | 6 +++++- >>>> 2 files changed, 13 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt >>>> index d20fd94..6426ae9 100644 >>>> --- a/Documentation/virtual/kvm/api.txt >>>> +++ b/Documentation/virtual/kvm/api.txt >>>> @@ -1414,7 +1414,10 @@ struct kvm_irq_routing_entry { >>>> __u32 gsi; >>>> __u32 type; >>>> __u32 flags; >>>> - __u32 pad; >>>> + union { >>>> + __u32 pad; >>>> + __u32 devid; >>>> + }; >>>> union { >>>> struct kvm_irq_routing_irqchip irqchip; >>>> struct kvm_irq_routing_msi msi; >>> >>> devid is actually a part of MSI bunch. Shouldn't it be a part of struct kvm_irq_routing_msi then? >>> It also has reserved pad. >> Well this makes sense to me to associate the devid to the msi and put >> devid in the pad field of struct kvm_irq_routing_msi. >> >> André, Christoffer, would you agree on this change? - I would like to >> avoid doing/undoing things ;-) - > > Yes, that makes sense to me. TBH I haven't had a closer look at the > patches yet, but clearly devid belongs into struct kvm_irq_routing_msi. thanks for your quick reply. OK so let's go with that change. > >>> >>>> @@ -1427,6 +1430,10 @@ struct kvm_irq_routing_entry { >>>> #define KVM_IRQ_ROUTING_IRQCHIP 1 >>>> #define KVM_IRQ_ROUTING_MSI 2 >>>> #define KVM_IRQ_ROUTING_S390_ADAPTER 3 >>>> +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4 >>>> + >>>> +In case of KVM_IRQ_ROUTING_EXTENDED_MSI routing type, devid is used to convey >>>> +the device ID. >>>> >>>> No flags are specified so far, the corresponding field must be set to zero. >>> >>> What if we use KVM_MSI_VALID_DEVID flag instead of new KVM_IRQ_ROUTING_EXTENDED_MSI definition? I >>> believe this would make an API more consistent and introduce less new definitions. >> do you mean using type == KVM_IRQ_ROUTING_MSI and flag == >> KVM_MSI_VALID_DEVID? Not sure this is simpler/clearer. s390 paved the >> way for new routing entry types. I add a new one here. > > I tend to agree with Pavel's solution. When hacking IRQ routing support > into kvmtool I saw that it's nasty being forced to differentiate between > the two MSI routing types. Actually userland should be able to query the > kernel about what kind of routing it requires. Also there is the issue > that we must _not_ set the flag on x86, since that breaks older kernels > (due to that check that Eric removes in 3/7). > So from my point of view the cleanest solution would be to always use > KVM_IRQ_ROUTING_MSI, and add the device ID if the kernel needs it (true > for ITS guests, false for GICv2M, x86, ...) > I am looking for a clever solution for this now. OK thanks for sharing. I need some more time to study qemu code too. - Eric > > Cheers, > Andre. > >> >> Another solution may be to use new KVM_IRQ_ROUTING_EXTENDED_MSI type and >> add struct kvm_msi ext_msi in kvm_irq_routing_entry union. It is 8 words >> as well. But most probably this is even uglier. > >> >> Let's see if this thread is heading to a consensus... >> >> Best Regards >> >> Eric >>> >>>> >>>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h >>>> index 2a23705..8484681 100644 >>>> --- a/include/uapi/linux/kvm.h >>>> +++ b/include/uapi/linux/kvm.h >>>> @@ -841,12 +841,16 @@ struct kvm_irq_routing_s390_adapter { >>>> #define KVM_IRQ_ROUTING_IRQCHIP 1 >>>> #define KVM_IRQ_ROUTING_MSI 2 >>>> #define KVM_IRQ_ROUTING_S390_ADAPTER 3 >>>> +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4 >>>> >>>> struct kvm_irq_routing_entry { >>>> __u32 gsi; >>>> __u32 type; >>>> __u32 flags; >>>> - __u32 pad; >>>> + union { >>>> + __u32 pad; >>>> + __u32 devid; >>>> + }; >>>> union { >>>> struct kvm_irq_routing_irqchip irqchip; >>>> struct kvm_irq_routing_msi msi; >>>> -- >>>> 1.9.1 >>>> >>>> -- >>>> 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 >>> >>> Kind regards, >>> Pavel Fedin >>> Expert Engineer >>> Samsung Electronics Research center Russia >>> >> -- 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
Hello!
> OK thanks for sharing. I need some more time to study qemu code too.
I am currently working on supporting this in qemu. Not ready yet, need some time. But, with API i
suggest, things are really much-much simpler.
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
--
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
On 07/02/2015 05:39 PM, Pavel Fedin wrote: > Hello! > >> OK thanks for sharing. I need some more time to study qemu code too. > > I am currently working on supporting this in qemu. Not ready yet, need some time. But, with API i > suggest, things are really much-much simpler. OK so both of you say the same thing. Will respin accordingly Eric > > Kind regards, > Pavel Fedin > Expert Engineer > Samsung Electronics Research center Russia > > -- 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
Hi Pavel, On 02/07/15 08:26, Pavel Fedin wrote: > Hello! > >> -----Original Message----- >> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On Behalf Of Eric Auger >> Sent: Monday, June 29, 2015 6:37 PM >> To: eric.auger@st.com; eric.auger@linaro.org; linux-arm-kernel@lists.infradead.org; >> marc.zyngier@arm.com; christoffer.dall@linaro.org; andre.przywara@arm.com; >> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org; patches@linaro.org; p.fedin@samsung.com; pbonzini@redhat.com >> Subject: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi >> >> On ARM, the MSI msg (address and data) comes along with >> out-of-band device ID information. The device ID encodes the device >> that composes the MSI msg. Let's create a new routing entry type, >> dubbed KVM_IRQ_ROUTING_EXTENDED_MSI and use the __u32 pad space >> to convey the device ID. >> >> Signed-off-by: Eric Auger <eric.auger@linaro.org> >> >> --- >> >> RFC -> PATCH >> - remove kvm_irq_routing_extended_msi and use union instead >> --- >> Documentation/virtual/kvm/api.txt | 9 ++++++++- >> include/uapi/linux/kvm.h | 6 +++++- >> 2 files changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt >> index d20fd94..6426ae9 100644 >> --- a/Documentation/virtual/kvm/api.txt >> +++ b/Documentation/virtual/kvm/api.txt >> @@ -1414,7 +1414,10 @@ struct kvm_irq_routing_entry { >> __u32 gsi; >> __u32 type; >> __u32 flags; >> - __u32 pad; >> + union { >> + __u32 pad; >> + __u32 devid; >> + }; >> union { >> struct kvm_irq_routing_irqchip irqchip; >> struct kvm_irq_routing_msi msi; > > devid is actually a part of MSI bunch. Shouldn't it be a part of struct kvm_irq_routing_msi then? > It also has reserved pad. > >> @@ -1427,6 +1430,10 @@ struct kvm_irq_routing_entry { >> #define KVM_IRQ_ROUTING_IRQCHIP 1 >> #define KVM_IRQ_ROUTING_MSI 2 >> #define KVM_IRQ_ROUTING_S390_ADAPTER 3 >> +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4 >> + >> +In case of KVM_IRQ_ROUTING_EXTENDED_MSI routing type, devid is used to convey >> +the device ID. >> >> No flags are specified so far, the corresponding field must be set to zero. > > What if we use KVM_MSI_VALID_DEVID flag instead of new KVM_IRQ_ROUTING_EXTENDED_MSI definition? I > believe this would make an API more consistent and introduce less new definitions. I like this approach, but it runs into problems: As you read above the current documentation says that the flags field must be zero and the current KVM_SET_GSI_ROUTING handler bails out if it isn't. So userland would need to know whether it's safe to set that field. Introducing a new KVM_CAP_... value seems overkill if we could just have a new routing entry type. So we could still reuse the existing struct kvm_irq_routing_msi (and extend that with the devid field), but we would have to add a new routing type number. Maybe we could collapse this into the existing MSI type + flag when handing it further down the kernel? Cheers, Andre. > >> >> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h >> index 2a23705..8484681 100644 >> --- a/include/uapi/linux/kvm.h >> +++ b/include/uapi/linux/kvm.h >> @@ -841,12 +841,16 @@ struct kvm_irq_routing_s390_adapter { >> #define KVM_IRQ_ROUTING_IRQCHIP 1 >> #define KVM_IRQ_ROUTING_MSI 2 >> #define KVM_IRQ_ROUTING_S390_ADAPTER 3 >> +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4 >> >> struct kvm_irq_routing_entry { >> __u32 gsi; >> __u32 type; >> __u32 flags; >> - __u32 pad; >> + union { >> + __u32 pad; >> + __u32 devid; >> + }; >> union { >> struct kvm_irq_routing_irqchip irqchip; >> struct kvm_irq_routing_msi msi; >> -- >> 1.9.1 >> >> -- >> 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 > > Kind regards, > Pavel Fedin > Expert Engineer > Samsung Electronics Research center Russia > -- 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
Hi! > OK so both of you say the same thing. Will respin accordingly You may also want to add this: Tested-by: Pavel Fedin <p.fedin@samsung.com> Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia -- 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
On 07/03/2015 05:29 PM, Pavel Fedin wrote: > Hi! > >> OK so both of you say the same thing. Will respin accordingly > > You may also want to add this: > Tested-by: Pavel Fedin <p.fedin@samsung.com> Thanks Pavel for the intent. However since I am going to change the uapi and correct the bug you spotted out, this will need to be tested again. T-b is applied when the code is stable and bug-free I think ;-) Best Regards Eric > > Kind regards, > Pavel Fedin > Expert Engineer > Samsung Electronics Research center Russia > -- 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
Hi, On 03/07/15 10:05, Andre Przywara wrote: > Hi Pavel, > > On 02/07/15 08:26, Pavel Fedin wrote: >> Hello! >> >>> -----Original Message----- >>> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On Behalf Of Eric Auger >>> Sent: Monday, June 29, 2015 6:37 PM >>> To: eric.auger@st.com; eric.auger@linaro.org; linux-arm-kernel@lists.infradead.org; >>> marc.zyngier@arm.com; christoffer.dall@linaro.org; andre.przywara@arm.com; >>> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org >>> Cc: linux-kernel@vger.kernel.org; patches@linaro.org; p.fedin@samsung.com; pbonzini@redhat.com >>> Subject: [PATCH 1/7] KVM: api: add kvm_irq_routing_extended_msi >>> >>> On ARM, the MSI msg (address and data) comes along with >>> out-of-band device ID information. The device ID encodes the device >>> that composes the MSI msg. Let's create a new routing entry type, >>> dubbed KVM_IRQ_ROUTING_EXTENDED_MSI and use the __u32 pad space >>> to convey the device ID. >>> >>> Signed-off-by: Eric Auger <eric.auger@linaro.org> >>> >>> --- >>> >>> RFC -> PATCH >>> - remove kvm_irq_routing_extended_msi and use union instead >>> --- >>> Documentation/virtual/kvm/api.txt | 9 ++++++++- >>> include/uapi/linux/kvm.h | 6 +++++- >>> 2 files changed, 13 insertions(+), 2 deletions(-) >>> >>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt >>> index d20fd94..6426ae9 100644 >>> --- a/Documentation/virtual/kvm/api.txt >>> +++ b/Documentation/virtual/kvm/api.txt >>> @@ -1414,7 +1414,10 @@ struct kvm_irq_routing_entry { >>> __u32 gsi; >>> __u32 type; >>> __u32 flags; >>> - __u32 pad; >>> + union { >>> + __u32 pad; >>> + __u32 devid; >>> + }; >>> union { >>> struct kvm_irq_routing_irqchip irqchip; >>> struct kvm_irq_routing_msi msi; >> >> devid is actually a part of MSI bunch. Shouldn't it be a part of struct kvm_irq_routing_msi then? >> It also has reserved pad. >> >>> @@ -1427,6 +1430,10 @@ struct kvm_irq_routing_entry { >>> #define KVM_IRQ_ROUTING_IRQCHIP 1 >>> #define KVM_IRQ_ROUTING_MSI 2 >>> #define KVM_IRQ_ROUTING_S390_ADAPTER 3 >>> +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4 >>> + >>> +In case of KVM_IRQ_ROUTING_EXTENDED_MSI routing type, devid is used to convey >>> +the device ID. >>> >>> No flags are specified so far, the corresponding field must be set to zero. >> >> What if we use KVM_MSI_VALID_DEVID flag instead of new KVM_IRQ_ROUTING_EXTENDED_MSI definition? I >> believe this would make an API more consistent and introduce less new definitions. > > I like this approach, but it runs into problems: > As you read above the current documentation says that the flags field > must be zero and the current KVM_SET_GSI_ROUTING handler bails out if it > isn't. So userland would need to know whether it's safe to set that > field. Introducing a new KVM_CAP_... value seems overkill if we could > just have a new routing entry type. So we could still reuse the existing > struct kvm_irq_routing_msi (and extend that with the devid field), but > we would have to add a new routing type number. > Maybe we could collapse this into the existing MSI type + flag when > handing it further down the kernel? FWIW, I gave this a try, this doesn't look to bad. I carried the new type down till virt/kvm/arm/vgic.c:kvm_set_routing_entry(), where the EXTENDED type got turned back into the normal MSI type while setting the flag in the internal struct kvm_kernel_irq_routing_entry. This keeps the new type only to the userland facing side, with the kernel code staying mostly the same. Together with a new KVM_CAP_MSIS_REQUIRE_DEVID capability I can now drive both GICv2M and ITS emulation from the same userland base in a sane manner. If someone wants to have a look now, tell me, otherwise I will wait for Eric's upcoming code drop and comment on that then. Cheers, Andre. > > Cheers, > Andre. > >> >>> >>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h >>> index 2a23705..8484681 100644 >>> --- a/include/uapi/linux/kvm.h >>> +++ b/include/uapi/linux/kvm.h >>> @@ -841,12 +841,16 @@ struct kvm_irq_routing_s390_adapter { >>> #define KVM_IRQ_ROUTING_IRQCHIP 1 >>> #define KVM_IRQ_ROUTING_MSI 2 >>> #define KVM_IRQ_ROUTING_S390_ADAPTER 3 >>> +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4 >>> >>> struct kvm_irq_routing_entry { >>> __u32 gsi; >>> __u32 type; >>> __u32 flags; >>> - __u32 pad; >>> + union { >>> + __u32 pad; >>> + __u32 devid; >>> + }; >>> union { >>> struct kvm_irq_routing_irqchip irqchip; >>> struct kvm_irq_routing_msi msi; >>> -- >>> 1.9.1 >>> >>> -- >>> 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 >> >> Kind regards, >> Pavel Fedin >> Expert Engineer >> Samsung Electronics Research center Russia >> -- 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
Hello! > I like this approach, but it runs into problems: > As you read above the current documentation says that the flags field > must be zero and the current KVM_SET_GSI_ROUTING handler bails out if it > isn't. So userland would need to know whether it's safe to set that > field. This problem does not exist because: a) Older platforms do not need this flag, so they expect to get zero. b) ARM64 + GICv3 platform cannot work without this flag. This is perfectly OK combination IMO. Userland just knows, whether it needs to supply device ID or not. For example, my modified qemu now has kvm_msi_flags global variable which defaults to 0. ITS code, then, if activated, changes it to KVM_MSI_VALID_DEVID, and qemu starts supplying device IDs to the related calls. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia -- 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
Hi Pavel, On 06/07/15 07:42, Pavel Fedin wrote: > Hello! > >> I like this approach, but it runs into problems: >> As you read above the current documentation says that the flags field >> must be zero and the current KVM_SET_GSI_ROUTING handler bails out if it >> isn't. So userland would need to know whether it's safe to set that >> field. > > This problem does not exist because: > a) Older platforms do not need this flag, so they expect to get zero. > b) ARM64 + GICv3 platform cannot work without this flag. > > This is perfectly OK combination IMO. Userland just knows, whether it needs to supply device ID or > not. For example, my modified qemu now has kvm_msi_flags global variable which defaults to 0. ITS > code, then, if activated, changes it to KVM_MSI_VALID_DEVID, and qemu starts supplying device IDs to > the related calls. Well, I had this solution before in kvmtool: If ARM && ITS then set the flag. But I wasn't really happy with this, as the IRQ routing, setup and injection code is rather architecture agnostic (implementing the generic KVM interface), so spraying in some architecture hacks sounded not very elegant. Also as the flag describes a rather generic feature (provide an unique device ID), I'd rather avoid to make this an ARM hack. That being said this is not a show stopper for me, so if the others are happy with this, I will go down your road. Cheers, Andre. -- 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
On Mon, Jul 06, 2015 at 09:30:20AM +0100, Andre Przywara wrote: > Hi Pavel, > > On 06/07/15 07:42, Pavel Fedin wrote: > > Hello! > > > >> I like this approach, but it runs into problems: > >> As you read above the current documentation says that the flags field > >> must be zero and the current KVM_SET_GSI_ROUTING handler bails out if it > >> isn't. So userland would need to know whether it's safe to set that > >> field. > > > > This problem does not exist because: > > a) Older platforms do not need this flag, so they expect to get zero. > > b) ARM64 + GICv3 platform cannot work without this flag. > > > > This is perfectly OK combination IMO. Userland just knows, whether it needs to supply device ID or > > not. For example, my modified qemu now has kvm_msi_flags global variable which defaults to 0. ITS > > code, then, if activated, changes it to KVM_MSI_VALID_DEVID, and qemu starts supplying device IDs to > > the related calls. > > Well, I had this solution before in kvmtool: If ARM && ITS then set the > flag. But I wasn't really happy with this, as the IRQ routing, setup and > injection code is rather architecture agnostic (implementing the generic > KVM interface), so spraying in some architecture hacks sounded not very > elegant. > Also as the flag describes a rather generic feature (provide an unique > device ID), I'd rather avoid to make this an ARM hack. > > That being said this is not a show stopper for me, so if the others are > happy with this, I will go down your road. > There must be some way for userspace to discover if it's valid to set the flag or not; either through a well-defined error-code probing mechanism for KVM_SET_GSI_ROUTING or through advertising a capability. -Christoffer -- 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
Hi Christoffer, On 06/07/15 10:30, Christoffer Dall wrote: > On Mon, Jul 06, 2015 at 09:30:20AM +0100, Andre Przywara wrote: >> Hi Pavel, >> >> On 06/07/15 07:42, Pavel Fedin wrote: >>> Hello! >>> >>>> I like this approach, but it runs into problems: >>>> As you read above the current documentation says that the flags field >>>> must be zero and the current KVM_SET_GSI_ROUTING handler bails out if it >>>> isn't. So userland would need to know whether it's safe to set that >>>> field. >>> >>> This problem does not exist because: >>> a) Older platforms do not need this flag, so they expect to get zero. >>> b) ARM64 + GICv3 platform cannot work without this flag. >>> >>> This is perfectly OK combination IMO. Userland just knows, whether it needs to supply device ID or >>> not. For example, my modified qemu now has kvm_msi_flags global variable which defaults to 0. ITS >>> code, then, if activated, changes it to KVM_MSI_VALID_DEVID, and qemu starts supplying device IDs to >>> the related calls. >> >> Well, I had this solution before in kvmtool: If ARM && ITS then set the >> flag. But I wasn't really happy with this, as the IRQ routing, setup and >> injection code is rather architecture agnostic (implementing the generic >> KVM interface), so spraying in some architecture hacks sounded not very >> elegant. >> Also as the flag describes a rather generic feature (provide an unique >> device ID), I'd rather avoid to make this an ARM hack. >> >> That being said this is not a show stopper for me, so if the others are >> happy with this, I will go down your road. >> > There must be some way for userspace to discover if it's valid to set > the flag or not; either through a well-defined error-code probing > mechanism for KVM_SET_GSI_ROUTING or through advertising a capability. Right, makes sense, I was wondering about this requirement earlier, but couldn't find really good prior art in the code (KVM_GET_VCPU_EVENTS seems to be bad example). So I think we get along with a new VM specific capability like KVM_CAP_MSIS_REQUIRE_DEVID. This isn't strictly a "capability" (as it's more a requirement), but I guess it fits here anyway. It has to be per-VM, as a GICv2M guest does not need it, but an ITS guest does. We can use this very flag for both the KVM_SIGNAL_MSI and the KVM_SET_GSI_ROUTING ioctl, so interface churn is kept minimal. Does that make sense? Actually I have implemented this already last week, I will send it out along with a v2 of the ITS emulation later this week. Cheers, Andre. -- 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
On Mon, Jul 06, 2015 at 11:05:26AM +0100, Andre Przywara wrote: > Hi Christoffer, > > On 06/07/15 10:30, Christoffer Dall wrote: > > On Mon, Jul 06, 2015 at 09:30:20AM +0100, Andre Przywara wrote: > >> Hi Pavel, > >> > >> On 06/07/15 07:42, Pavel Fedin wrote: > >>> Hello! > >>> > >>>> I like this approach, but it runs into problems: > >>>> As you read above the current documentation says that the flags field > >>>> must be zero and the current KVM_SET_GSI_ROUTING handler bails out if it > >>>> isn't. So userland would need to know whether it's safe to set that > >>>> field. > >>> > >>> This problem does not exist because: > >>> a) Older platforms do not need this flag, so they expect to get zero. > >>> b) ARM64 + GICv3 platform cannot work without this flag. > >>> > >>> This is perfectly OK combination IMO. Userland just knows, whether it needs to supply device ID or > >>> not. For example, my modified qemu now has kvm_msi_flags global variable which defaults to 0. ITS > >>> code, then, if activated, changes it to KVM_MSI_VALID_DEVID, and qemu starts supplying device IDs to > >>> the related calls. > >> > >> Well, I had this solution before in kvmtool: If ARM && ITS then set the > >> flag. But I wasn't really happy with this, as the IRQ routing, setup and > >> injection code is rather architecture agnostic (implementing the generic > >> KVM interface), so spraying in some architecture hacks sounded not very > >> elegant. > >> Also as the flag describes a rather generic feature (provide an unique > >> device ID), I'd rather avoid to make this an ARM hack. > >> > >> That being said this is not a show stopper for me, so if the others are > >> happy with this, I will go down your road. > >> > > There must be some way for userspace to discover if it's valid to set > > the flag or not; either through a well-defined error-code probing > > mechanism for KVM_SET_GSI_ROUTING or through advertising a capability. > > Right, makes sense, I was wondering about this requirement earlier, but > couldn't find really good prior art in the code (KVM_GET_VCPU_EVENTS > seems to be bad example). > So I think we get along with a new VM specific capability like > KVM_CAP_MSIS_REQUIRE_DEVID. This isn't strictly a "capability" (as it's > more a requirement), but I guess it fits here anyway. It has to be > per-VM, as a GICv2M guest does not need it, but an ITS guest does. > We can use this very flag for both the KVM_SIGNAL_MSI and the > KVM_SET_GSI_ROUTING ioctl, so interface churn is kept minimal. > > Does that make sense? > > Actually I have implemented this already last week, I will send it out > along with a v2 of the ITS emulation later this week. > I don't view it as 'the kernel requires this' but as 'the kernel will not complain with arbitrary error code if you set the devid flag' capability, and it's up to userspace (as usual) to provide the correct arguments for things to work, and up to the kernel to ensure we don't crash the system etc. Thus, if you want to advertise it as a capability, I would rather call it KVM_CAP_MSI_DEVID. The question is if userspace code that sets the devid flag will anyway depend on some discovery mechanism of whether or not the kernel supports arm64 irqfd etc. and if so, can we be sure to add the required support at once in the kernel so that EINVAL never means 'you set the flags field on the ioctl on an old kernel'? This smells an awful lot like a capability to me. -Christoffer -- 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
On 06/07/2015 12:37, Christoffer Dall wrote: > I don't view it as 'the kernel requires this' but as 'the kernel will > not complain with arbitrary error code if you set the devid flag' > capability, and it's up to userspace (as usual) to provide the correct > arguments for things to work, and up to the kernel to ensure we don't > crash the system etc. > > Thus, if you want to advertise it as a capability, I would rather call > it KVM_CAP_MSI_DEVID. I agree. Does userspace know that ITS guests always require devid? I guess it's okay to return -EINVAL if the userspace doesn't set the flag but the virtual hardware requires it. Paolo > The question is if userspace code that sets the devid flag will anyway > depend on some discovery mechanism of whether or not the kernel supports > arm64 irqfd etc. and if so, can we be sure to add the required support > at once in the kernel so that EINVAL never means 'you set the flags > field on the ioctl on an old kernel'? > > This smells an awful lot like a capability to me. -- 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
Hi Paolo, thanks for looking at this! On 06/07/15 12:07, Paolo Bonzini wrote: > > > On 06/07/2015 12:37, Christoffer Dall wrote: >> I don't view it as 'the kernel requires this' but as 'the kernel will >> not complain with arbitrary error code if you set the devid flag' >> capability, and it's up to userspace (as usual) to provide the correct >> arguments for things to work, and up to the kernel to ensure we don't >> crash the system etc. >> >> Thus, if you want to advertise it as a capability, I would rather call >> it KVM_CAP_MSI_DEVID. > > I agree. Does userspace know that ITS guests always require devid? Well, as we are about to implement this: yes. But the issue is that MSI injection and GSI routing code is generic PCI code in userland (at least in kvmtool, guess in QEMU, too), so I don't want to pull in any kind of ARM specific code in there. The idea is to always provide the device ID from the PCI code (for PCI devices it's just the B/D/F triplet), but only send it to the kernel if needed. Querying a KVM capability is perfectly fine for this IMO. > I > guess it's okay to return -EINVAL if the userspace doesn't set the flag > but the virtual hardware requires it. Yes, that is what I do in the kernel implementation. And that is perfectly fine: the ITS emulation does not work without a device ID, the ITS driver in the guest assigns the very same payload (and address) to different devices, so there is no way to tell the MSIs apart without a unique device ID. Thanks, Andre. > > Paolo > >> The question is if userspace code that sets the devid flag will anyway >> depend on some discovery mechanism of whether or not the kernel supports >> arm64 irqfd etc. and if so, can we be sure to add the required support >> at once in the kernel so that EINVAL never means 'you set the flags >> field on the ioctl on an old kernel'? >> >> This smells an awful lot like a capability to me. > -- 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
On 06/07/2015 13:23, Andre Przywara wrote: > Hi Paolo, > > thanks for looking at this! > > On 06/07/15 12:07, Paolo Bonzini wrote: >> >> >> On 06/07/2015 12:37, Christoffer Dall wrote: >>> I don't view it as 'the kernel requires this' but as 'the kernel will >>> not complain with arbitrary error code if you set the devid flag' >>> capability, and it's up to userspace (as usual) to provide the correct >>> arguments for things to work, and up to the kernel to ensure we don't >>> crash the system etc. >>> >>> Thus, if you want to advertise it as a capability, I would rather call >>> it KVM_CAP_MSI_DEVID. >> >> I agree. Does userspace know that ITS guests always require devid? > > Well, as we are about to implement this: yes. But the issue is that MSI > injection and GSI routing code is generic PCI code in userland (at least > in kvmtool, guess in QEMU, too), so I don't want to pull in any kind of > ARM specific code in there. The idea is to always provide the device ID > from the PCI code (for PCI devices it's just the B/D/F triplet), but > only send it to the kernel if needed. Querying a KVM capability is > perfectly fine for this IMO. Yes, I agree. Paolo -- 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
On Mon, Jul 06, 2015 at 12:23:19PM +0100, Andre Przywara wrote: > Hi Paolo, > > thanks for looking at this! > > On 06/07/15 12:07, Paolo Bonzini wrote: > > > > > > On 06/07/2015 12:37, Christoffer Dall wrote: > >> I don't view it as 'the kernel requires this' but as 'the kernel will > >> not complain with arbitrary error code if you set the devid flag' > >> capability, and it's up to userspace (as usual) to provide the correct > >> arguments for things to work, and up to the kernel to ensure we don't > >> crash the system etc. > >> > >> Thus, if you want to advertise it as a capability, I would rather call > >> it KVM_CAP_MSI_DEVID. > > > > I agree. Does userspace know that ITS guests always require devid? > > Well, as we are about to implement this: yes. But the issue is that MSI > injection and GSI routing code is generic PCI code in userland (at least > in kvmtool, guess in QEMU, too), so I don't want to pull in any kind of > ARM specific code in there. The idea is to always provide the device ID > from the PCI code (for PCI devices it's just the B/D/F triplet), but > only send it to the kernel if needed. Querying a KVM capability is > perfectly fine for this IMO. > > > I > > guess it's okay to return -EINVAL if the userspace doesn't set the flag > > but the virtual hardware requires it. > > Yes, that is what I do in the kernel implementation. And that is > perfectly fine: the ITS emulation does not work without a device ID, the > ITS driver in the guest assigns the very same payload (and address) to > different devices, so there is no way to tell the MSIs apart without a > unique device ID. > Just so I'm sure I understand: The way the kernel differentiates between no-devid and devid==0, is whether or not the devid flag is set, correct? -Christoffer -- 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
Hi! > > Well, as we are about to implement this: yes. But the issue is that MSI > > injection and GSI routing code is generic PCI code in userland (at least > > in kvmtool, guess in QEMU, too), so I don't want to pull in any kind of > > ARM specific code in there. The idea is to always provide the device ID > > from the PCI code (for PCI devices it's just the B/D/F triplet), but > > only send it to the kernel if needed. Querying a KVM capability is > > perfectly fine for this IMO. > > Yes, I agree. Actually, we already have this capability, it's KVM_CAP_IRQ_ROUTING. If we have this capability, and want to use irqfds with GICv3, we need to set KVM_MSI_VALID_DEVID. And there is no other way to use irqfds with GICv3. Just for example, this is what i have done in qemu: --- cut --- int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg, PCIDevice *dev) { struct kvm_irq_routing_entry kroute = {}; int virq; if (kvm_gsi_direct_mapping()) { return kvm_arch_msi_data_to_gsi(msg.data); } if (!kvm_gsi_routing_enabled()) { return -ENOSYS; } virq = kvm_irqchip_get_virq(s); if (virq < 0) { return virq; } kroute.gsi = virq; kroute.type = KVM_IRQ_ROUTING_MSI; kroute.u.msi.address_lo = (uint32_t)msg.address; kroute.u.msi.address_hi = msg.address >> 32; kroute.u.msi.data = le32_to_cpu(msg.data); kroute.flags = kvm_msi_flags; if (kroute.flags & KVM_MSI_VALID_DEVID) { kroute.u.msi.devid = (pci_bus_num(dev->bus) << 8) | dev->devfn; } if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data)) { kvm_irqchip_release_virq(s, virq); return -EINVAL; } kvm_add_routing_entry(s, &kroute); kvm_irqchip_commit_routes(s); return virq; } --- cut --- ITS code in qemu just does: ---cut --- msi_supported = true; kvm_msi_flags = KVM_MSI_VALID_DEVID; kvm_msi_via_irqfd_allowed = kvm_has_gsi_routing(); kvm_gsi_routing_allowed = kvm_msi_via_irqfd_allowed; --- cut --- I set KVM_MSI_VALID_DEVID unconditionally here just because it will never be checked if kvm_msi_via_irqfd_allowed is false, it's just qemu specifics. The more canonical form would perhaps be: --- cut --- if (kvm_has_gsi_routing()) { kvm_msi_flags = KVM_MSI_VALID_DEVID; kvm_gsi_routing_allowed = true; kvm_msi_via_irqfd_allowed = true; } --- cut --- I can post my sets as RFCs to qemu mailing list, if you want to take a look at the whole change set. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia -- 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
> Just so I'm sure I understand: The way the kernel differentiates between > no-devid and devid==0, is whether or not the devid flag is set, correct? Yes, exactly. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia -- 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
Hi all, On 07/06/2015 03:32 PM, Pavel Fedin wrote: > Hi! > >>> Well, as we are about to implement this: yes. But the issue is that MSI >>> injection and GSI routing code is generic PCI code in userland (at least >>> in kvmtool, guess in QEMU, too), so I don't want to pull in any kind of >>> ARM specific code in there. The idea is to always provide the device ID >>> from the PCI code (for PCI devices it's just the B/D/F triplet), but >>> only send it to the kernel if needed. Querying a KVM capability is >>> perfectly fine for this IMO. >> >> Yes, I agree. > > Actually, we already have this capability, it's KVM_CAP_IRQ_ROUTING. If we have this capability, > and want to use irqfds with GICv3, we need to set KVM_MSI_VALID_DEVID. And there is no other way to > use irqfds with GICv3. > Just for example, this is what i have done in qemu: > --- cut --- > int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg, PCIDevice *dev) > { > struct kvm_irq_routing_entry kroute = {}; > int virq; > > if (kvm_gsi_direct_mapping()) { > return kvm_arch_msi_data_to_gsi(msg.data); > } > > if (!kvm_gsi_routing_enabled()) { > return -ENOSYS; > } > > virq = kvm_irqchip_get_virq(s); > if (virq < 0) { > return virq; > } > > kroute.gsi = virq; > kroute.type = KVM_IRQ_ROUTING_MSI; > kroute.u.msi.address_lo = (uint32_t)msg.address; > kroute.u.msi.address_hi = msg.address >> 32; > kroute.u.msi.data = le32_to_cpu(msg.data); > kroute.flags = kvm_msi_flags; > if (kroute.flags & KVM_MSI_VALID_DEVID) { > kroute.u.msi.devid = (pci_bus_num(dev->bus) << 8) | dev->devfn; > } > > if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data)) { > kvm_irqchip_release_virq(s, virq); > return -EINVAL; > } > > kvm_add_routing_entry(s, &kroute); > kvm_irqchip_commit_routes(s); > > return virq; > } > --- cut --- > > ITS code in qemu just does: > > ---cut --- > msi_supported = true; > kvm_msi_flags = KVM_MSI_VALID_DEVID; > kvm_msi_via_irqfd_allowed = kvm_has_gsi_routing(); > kvm_gsi_routing_allowed = kvm_msi_via_irqfd_allowed; > --- cut --- > > I set KVM_MSI_VALID_DEVID unconditionally here just because it will never be checked if > kvm_msi_via_irqfd_allowed is false, it's just qemu specifics. The more canonical form would perhaps > be: > --- cut --- > if (kvm_has_gsi_routing()) { > kvm_msi_flags = KVM_MSI_VALID_DEVID; Personally I prefer a capability rather than hardcoding a global variable value in the qemu interrupt controller code. All the more so typically there is KVM GSI routing cap that could/should? be queried instead of hardcoding the value I think. So not sure whether we eventually concluded;-) - introduce a KVM_CAP_MSI_DEVID capability? All OK except Pavel not convinced? - userspaces puts the devid in struct kvm_irq_routing_msi pad field: consensus (we do not intrduce a new kvm_irq_routing_ext_msi) - userspace tells it conveyed a devid by setting A) the kvm_irq_routing_entry's field? B) the kvm_irq_routing_entry's type no consensus. If there is a cap, does it really matter? Best Regards Eric > kvm_gsi_routing_allowed = true; > kvm_msi_via_irqfd_allowed = true; > } > --- cut --- > > I can post my sets as RFCs to qemu mailing list, if you want to take a look at the whole change > set. > > Kind regards, > Pavel Fedin > Expert Engineer > Samsung Electronics Research center Russia > > -- 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
On 06/07/15 13:08, Christoffer Dall wrote: > On Mon, Jul 06, 2015 at 12:23:19PM +0100, Andre Przywara wrote: >> Hi Paolo, >> >> thanks for looking at this! >> >> On 06/07/15 12:07, Paolo Bonzini wrote: >>> >>> >>> On 06/07/2015 12:37, Christoffer Dall wrote: >>>> I don't view it as 'the kernel requires this' but as 'the kernel will >>>> not complain with arbitrary error code if you set the devid flag' >>>> capability, and it's up to userspace (as usual) to provide the correct >>>> arguments for things to work, and up to the kernel to ensure we don't >>>> crash the system etc. >>>> >>>> Thus, if you want to advertise it as a capability, I would rather call >>>> it KVM_CAP_MSI_DEVID. >>> >>> I agree. Does userspace know that ITS guests always require devid? >> >> Well, as we are about to implement this: yes. But the issue is that MSI >> injection and GSI routing code is generic PCI code in userland (at least >> in kvmtool, guess in QEMU, too), so I don't want to pull in any kind of >> ARM specific code in there. The idea is to always provide the device ID >> from the PCI code (for PCI devices it's just the B/D/F triplet), but >> only send it to the kernel if needed. Querying a KVM capability is >> perfectly fine for this IMO. >> >>> I >>> guess it's okay to return -EINVAL if the userspace doesn't set the flag >>> but the virtual hardware requires it. >> >> Yes, that is what I do in the kernel implementation. And that is >> perfectly fine: the ITS emulation does not work without a device ID, the >> ITS driver in the guest assigns the very same payload (and address) to >> different devices, so there is no way to tell the MSIs apart without a >> unique device ID. >> > Just so I'm sure I understand: The way the kernel differentiates between > no-devid and devid==0, is whether or not the devid flag is set, correct? Yes, that is the idea. The plan for the implementation is like this: 1) If the kernel does not need the device ID (x86, GICv2M), it does not care about the flag or the value at all. 2) In case for ITS on ARM64, the kernel returns an error is the flag is not set. Cheers, Andre. -- 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
Hi Pavel, On 06/07/15 14:32, Pavel Fedin wrote: > Hi! > >>> Well, as we are about to implement this: yes. But the issue is that MSI >>> injection and GSI routing code is generic PCI code in userland (at least >>> in kvmtool, guess in QEMU, too), so I don't want to pull in any kind of >>> ARM specific code in there. The idea is to always provide the device ID >>> from the PCI code (for PCI devices it's just the B/D/F triplet), but >>> only send it to the kernel if needed. Querying a KVM capability is >>> perfectly fine for this IMO. >> >> Yes, I agree. > > Actually, we already have this capability, it's KVM_CAP_IRQ_ROUTING. If we have this capability, > and want to use irqfds with GICv3, we need to set KVM_MSI_VALID_DEVID. This is the connection that I don't like: We make the decision to support a flag on a generic KVM interface dependent on some particular device emulation (for some very specific architecture, also). > And there is no other way to > use irqfds with GICv3. For now: yes, but I fail to see why the GICv3 is so special that is justifies an extra handling in the KVM interrupt routing code. If it is special, lets name it explicitly why: we need a device ID. > Just for example, this is what i have done in qemu: > --- cut --- > int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg, PCIDevice *dev) > { > struct kvm_irq_routing_entry kroute = {}; > int virq; > > if (kvm_gsi_direct_mapping()) { > return kvm_arch_msi_data_to_gsi(msg.data); > } > > if (!kvm_gsi_routing_enabled()) { > return -ENOSYS; > } > > virq = kvm_irqchip_get_virq(s); > if (virq < 0) { > return virq; > } > > kroute.gsi = virq; > kroute.type = KVM_IRQ_ROUTING_MSI; > kroute.u.msi.address_lo = (uint32_t)msg.address; > kroute.u.msi.address_hi = msg.address >> 32; > kroute.u.msi.data = le32_to_cpu(msg.data); > kroute.flags = kvm_msi_flags; > if (kroute.flags & KVM_MSI_VALID_DEVID) { > kroute.u.msi.devid = (pci_bus_num(dev->bus) << 8) | dev->devfn; > } Wouldn't: if (kvm_vm_check_extension(s, KVM_CAP_MSI_DEVID)) { kroute.flags = KVM_MSI_VALID_DEVID; kroute.u.msi.devid = (pci_bus_num(dev->bus) << 8) | dev->devfn; } be saner (without a global variable)? That would make the interface more consistent, with a new flag being protected by a new capability. Cheers, Andre. > if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data)) { > kvm_irqchip_release_virq(s, virq); > return -EINVAL; > } > > kvm_add_routing_entry(s, &kroute); > kvm_irqchip_commit_routes(s); > > return virq; > } -- 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
Salut Eric, .... >> ITS code in qemu just does: >> >> ---cut --- >> msi_supported = true; >> kvm_msi_flags = KVM_MSI_VALID_DEVID; >> kvm_msi_via_irqfd_allowed = kvm_has_gsi_routing(); >> kvm_gsi_routing_allowed = kvm_msi_via_irqfd_allowed; >> --- cut --- >> >> I set KVM_MSI_VALID_DEVID unconditionally here just because it will never be checked if >> kvm_msi_via_irqfd_allowed is false, it's just qemu specifics. The more canonical form would perhaps >> be: >> --- cut --- >> if (kvm_has_gsi_routing()) { >> kvm_msi_flags = KVM_MSI_VALID_DEVID; > Personally I prefer a capability rather than hardcoding a global > variable value in the qemu interrupt controller code. All the more so > typically there is KVM GSI routing cap that could/should? be queried > instead of hardcoding the value I think. > > So not sure whether we eventually concluded;-) > - introduce a KVM_CAP_MSI_DEVID capability? All OK except Pavel not > convinced? OK for me. > - userspaces puts the devid in struct kvm_irq_routing_msi pad field: > consensus (we do not intrduce a new kvm_irq_routing_ext_msi) OK for me. > - userspace tells it conveyed a devid by setting > A) the kvm_irq_routing_entry's field? You mean kvm_irq_routing_entry's "flags" here? > B) the kvm_irq_routing_entry's type So personally I don't like it so much to use the generic flags field to specify the meaning within one particular type only. Using a new type instead seems to be more consistent, reusing an existing struct for that sounds even better. As written before (and coded in my branch) we can collapse that into the existing MSI type while translating that into the kernel internal routing structure to make the kernel code changes minimal. > no consensus. If there is a cap, does it really matter? I guess not. But I prefer the new type anyway, as it also has a known error path for older kernels. Cheers, Andre. -- 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
On 06/07/2015 17:37, Andre Przywara wrote: > Wouldn't: > if (kvm_vm_check_extension(s, KVM_CAP_MSI_DEVID)) { > kroute.flags = KVM_MSI_VALID_DEVID; > kroute.u.msi.devid = (pci_bus_num(dev->bus) << 8) | dev->devfn; > } > > be saner (without a global variable)? > That would make the interface more consistent, with a new flag being > protected by a new capability. I agree that your version is niceer, but you still need to cache the kvm_vm_check_extension result... in a global variable. :) Paolo -- 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
On 06/07/15 16:54, Paolo Bonzini wrote: > > > On 06/07/2015 17:37, Andre Przywara wrote: >> Wouldn't: >> if (kvm_vm_check_extension(s, KVM_CAP_MSI_DEVID)) { >> kroute.flags = KVM_MSI_VALID_DEVID; >> kroute.u.msi.devid = (pci_bus_num(dev->bus) << 8) | dev->devfn; >> } >> >> be saner (without a global variable)? >> That would make the interface more consistent, with a new flag being >> protected by a new capability. > > I agree that your version is niceer, but you still need to cache the > kvm_vm_check_extension result... in a global variable. :) I used a static variable in a wrapper function in kvmtool ;-) TBH my argument wasn't so much about global variables (just saw that QEMU seems to use them already), but more about a consistent and architecture agnostic interface. Ciao! Andre. -- 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
On 07/06/2015 05:52 PM, Andre Przywara wrote: > Salut Eric, > > .... > >>> ITS code in qemu just does: >>> >>> ---cut --- >>> msi_supported = true; >>> kvm_msi_flags = KVM_MSI_VALID_DEVID; >>> kvm_msi_via_irqfd_allowed = kvm_has_gsi_routing(); >>> kvm_gsi_routing_allowed = kvm_msi_via_irqfd_allowed; >>> --- cut --- >>> >>> I set KVM_MSI_VALID_DEVID unconditionally here just because it will never be checked if >>> kvm_msi_via_irqfd_allowed is false, it's just qemu specifics. The more canonical form would perhaps >>> be: >>> --- cut --- >>> if (kvm_has_gsi_routing()) { >>> kvm_msi_flags = KVM_MSI_VALID_DEVID; >> Personally I prefer a capability rather than hardcoding a global >> variable value in the qemu interrupt controller code. All the more so >> typically there is KVM GSI routing cap that could/should? be queried >> instead of hardcoding the value I think. >> >> So not sure whether we eventually concluded;-) >> - introduce a KVM_CAP_MSI_DEVID capability? All OK except Pavel not >> convinced? > > OK for me. > >> - userspaces puts the devid in struct kvm_irq_routing_msi pad field: >> consensus (we do not intrduce a new kvm_irq_routing_ext_msi) > > OK for me. > >> - userspace tells it conveyed a devid by setting >> A) the kvm_irq_routing_entry's field? > > You mean kvm_irq_routing_entry's "flags" here? yes!! > >> B) the kvm_irq_routing_entry's type > > So personally I don't like it so much to use the generic flags field to > specify the meaning within one particular type only. Using a new type > instead seems to be more consistent, reusing an existing struct for that > sounds even better. > As written before (and coded in my branch) we can collapse that into the > existing MSI type while translating that into the kernel internal > routing structure to make the kernel code changes minimal. > >> no consensus. If there is a cap, does it really matter? > > I guess not. But I prefer the new type anyway, as it also has a known > error path for older kernels. I am fine with the new type too. Eric > > Cheers, > Andre. > -- 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
Hello! > Wouldn't: > if (kvm_vm_check_extension(s, KVM_CAP_MSI_DEVID)) { > kroute.flags = KVM_MSI_VALID_DEVID; > kroute.u.msi.devid = (pci_bus_num(dev->bus) << 8) | dev->devfn; > } > > be saner (without a global variable)? No it would not, because: a) kvm_vm_check_extension() calls ioctl every time, therefore it's slow. But, well, doesn't really matter because it's possible to check for the capability once in generic code, and cache it. b) Capability is a global thing as far as i understand. The kernel either has it, or doesn't have. However, whether we want this flag or not, depends also on what GIC model we use. GICv2(m) doesn't want it, GICv3 does. qemu actually has two sets of flags: one set actually specifies capabilities, another set enables use of these capabilities. But, well, you can make GICv2 kernel code simply ignoring it instead of bailing out if flags != 0. And add the capability for ARM64 architecture (ARM32 can't use GICv3, can it?). And this will work and it'll be OK. So, i'm not against it, and if you want it, you can do it. I just want to point that it is not strictly necessary to add new APIs, because existing ones are pretty much enough. But, you are the architects here, so you of course can do it if you want. It's just me being not a big fan of adding APIs without which it's completely possible to live. Below i'm answering to Eric's comment, because my reply is tightly coupled with this one. > So not sure whether we eventually concluded;-) > - introduce a KVM_CAP_MSI_DEVID capability? All OK except Pavel not > convinced? See above. I'm not against it, i just don't think it's necessary. You can do it if you want, it actually won't change things much. > - userspaces puts the devid in struct kvm_irq_routing_msi pad field: > consensus (we do not intrduce a new kvm_irq_routing_ext_msi) Yes. > - userspace tells it conveyed a devid by setting > A) the kvm_irq_routing_entry's field? > B) the kvm_irq_routing_entry's type > no consensus. If there is a cap, does it really matter? It has absolutely nothing to do with the cap. My argument here is the same as above again - why adding new API's / definitions? We already have KVM_MSI_VALID_DEVID and we already have 'flags' field. Using them would just make the API more consistent because KVM_SIGNAL_MSI already uses them in absolutely the same manner. That's my point and nothing more. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia -- 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
Hi! > I guess not. But I prefer the new type anyway, as it also has a known > error path for older kernels. flags != 0 has known error path too, and it's absolutely the same. Sorry, read this after writing my previous reply, so this is a short addendum. I see lots of people agreed on a new type. If my argument about reusing existing definitions is not enough, you can ignore it. Three people beat one definitely. :) And yes, since we are talking about it, actually KVM_MSI_VALID_DEVID flag is not yet a part of mainline, so it's not set in stone. Then, perhaps you could throw it away completely and invent KVM_SIGNAL_EXT_MSI ioctl for sending MSIs with device ID. This would also be consistent IMO. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia -- 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
Hi Pavel, On 07/07/2015 09:23 AM, Pavel Fedin wrote: > Hi! > >> I guess not. But I prefer the new type anyway, as it also has a known >> error path for older kernels. > > flags != 0 has known error path too, and it's absolutely the same. > Sorry, read this after writing my previous reply, so this is a short addendum. > > I see lots of people agreed on a new type. If my argument about reusing existing definitions is not > enough, you can ignore it. Three people beat one definitely. :) OK. let's move forward and use this new type. I will repost soon so everyone can re-check the fit at kvmtool/qemu. Thanks Eric > And yes, since we are talking about it, actually KVM_MSI_VALID_DEVID flag is not yet a part of > mainline, so it's not set in stone. Then, perhaps you could throw it away completely and invent > KVM_SIGNAL_EXT_MSI ioctl for sending MSIs with device ID. This would also be consistent IMO. > > Kind regards, > Pavel Fedin > Expert Engineer > Samsung Electronics Research center Russia > > -- 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
Good morning Pavel, On 07/07/15 08:16, Pavel Fedin wrote: > Hello! > >> Wouldn't: >> if (kvm_vm_check_extension(s, KVM_CAP_MSI_DEVID)) { >> kroute.flags = KVM_MSI_VALID_DEVID; >> kroute.u.msi.devid = (pci_bus_num(dev->bus) << 8) | dev->devfn; >> } >> >> be saner (without a global variable)? > > No it would not, because: > a) kvm_vm_check_extension() calls ioctl every time, therefore it's slow. But, well, doesn't really > matter because it's possible to check for the capability once in generic code, and cache it. Indeed, as mentioned before I have it in a wrapper function with a static variable. > b) Capability is a global thing as far as i understand. The kernel either has it, or doesn't have. There are two flavours of capabilities: global and per-VM ones, depending on which fd you are issuing the ioctl. The per-VM ones are just not very widely used yet (I only found PowerPC doing so). > However, whether we want this flag or not, depends also on what GIC model we use. GICv2(m) doesn't > want it, GICv3 does. qemu actually has two sets of flags: one set actually specifies capabilities, > another set enables use of these capabilities. That's why I do a per-VM capability check and I do it as late as possible to let the GIC initialize first (hence the wrapper function). > But, well, you can make GICv2 kernel code simply ignoring it instead of bailing out if flags != 0. > And add the capability for ARM64 architecture (ARM32 can't use GICv3, can it?). And this will work > and it'll be OK. So, i'm not against it, and if you want it, you can do it. I just want to point > that it is not strictly necessary to add new APIs, because existing ones are pretty much enough. As said before I don't like the idea of inferring the validity of a flag by some hard-coded dependencies like "GICv3 on ARM64". I guess ARM(32) will get GICv3 support sooner or later (I think I saw patches to do so already). So as soon as a kernel supports it, we automatically get the support from userland without changing a single line there. Also what tells you that no other architecture or IRQ controller will ever need a device ID? I just don't want to end up with something like: (GICV3 && ARM64) || (GICV3 && ARM && KERNEL>4.4) || (SuperIRQC && i986) or (ARM || ARM64) && HAS_IRQ_ROUTING Instead: If the kernel needs it, it tells you. Full stop. > But, you are the architects here, so you of course can do it if you want. > It's just me being not a > big fan of adding APIs without which it's completely possible to live. > > Below i'm answering to Eric's comment, because my reply is tightly coupled with this one. > >> So not sure whether we eventually concluded;-) >> - introduce a KVM_CAP_MSI_DEVID capability? All OK except Pavel not >> convinced? > > See above. I'm not against it, i just don't think it's necessary. You can do it if you want, it > actually won't change things much. > >> - userspaces puts the devid in struct kvm_irq_routing_msi pad field: >> consensus (we do not intrduce a new kvm_irq_routing_ext_msi) > > Yes. > >> - userspace tells it conveyed a devid by setting >> A) the kvm_irq_routing_entry's field? >> B) the kvm_irq_routing_entry's type >> no consensus. If there is a cap, does it really matter? > > It has absolutely nothing to do with the cap. My argument here is the same as above again - why > adding new API's / definitions? We already have KVM_MSI_VALID_DEVID and we already have 'flags' > field. Using them would just make the API more consistent because KVM_SIGNAL_MSI already uses them > in absolutely the same manner. That's my point and nothing more. To be honest it's me to blame here to not having introduced the capability earlier. At the moment ARM has a different code path for KVM_SIGNAL_MSI, which does not bail out if the flag field is set. With Eric's patches this changes and we use the irqchip.c generic code, which returns -EINVAL atm. So I plan to introduce this capability already with the ITS emulation series, so we can just pick it up in the IRQ routing series. So we now have already two users of this, if that makes more sense. Cheers, Andre. -- 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
> I just don't want to end up with something like: > (GICV3 && ARM64) || (GICV3 && ARM && KERNEL>4.4) || (SuperIRQC && i986) > or > (ARM || ARM64) && HAS_IRQ_ROUTING > > Instead: If the kernel needs it, it tells you. Full stop. Agree. > To be honest it's me to blame here to not having introduced the > capability earlier. At the moment ARM has a different code path for > KVM_SIGNAL_MSI, which does not bail out if the flag field is set. With > Eric's patches this changes and we use the irqchip.c generic code, which > returns -EINVAL atm. So I plan to introduce this capability already with > the ITS emulation series, so we can just pick it up in the IRQ routing > series. Then may be you follow https://lkml.org/lkml/2015/7/7/115 and replace flag with something like KVM_SIGNAL_EXT_MSI ioctl ? After all you were one of people who voted against using flags. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia -- 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 --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt index d20fd94..6426ae9 100644 --- a/Documentation/virtual/kvm/api.txt +++ b/Documentation/virtual/kvm/api.txt @@ -1414,7 +1414,10 @@ struct kvm_irq_routing_entry { __u32 gsi; __u32 type; __u32 flags; - __u32 pad; + union { + __u32 pad; + __u32 devid; + }; union { struct kvm_irq_routing_irqchip irqchip; struct kvm_irq_routing_msi msi; @@ -1427,6 +1430,10 @@ struct kvm_irq_routing_entry { #define KVM_IRQ_ROUTING_IRQCHIP 1 #define KVM_IRQ_ROUTING_MSI 2 #define KVM_IRQ_ROUTING_S390_ADAPTER 3 +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4 + +In case of KVM_IRQ_ROUTING_EXTENDED_MSI routing type, devid is used to convey +the device ID. No flags are specified so far, the corresponding field must be set to zero. diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index 2a23705..8484681 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -841,12 +841,16 @@ struct kvm_irq_routing_s390_adapter { #define KVM_IRQ_ROUTING_IRQCHIP 1 #define KVM_IRQ_ROUTING_MSI 2 #define KVM_IRQ_ROUTING_S390_ADAPTER 3 +#define KVM_IRQ_ROUTING_EXTENDED_MSI 4 struct kvm_irq_routing_entry { __u32 gsi; __u32 type; __u32 flags; - __u32 pad; + union { + __u32 pad; + __u32 devid; + }; union { struct kvm_irq_routing_irqchip irqchip; struct kvm_irq_routing_msi msi;
On ARM, the MSI msg (address and data) comes along with out-of-band device ID information. The device ID encodes the device that composes the MSI msg. Let's create a new routing entry type, dubbed KVM_IRQ_ROUTING_EXTENDED_MSI and use the __u32 pad space to convey the device ID. Signed-off-by: Eric Auger <eric.auger@linaro.org> --- RFC -> PATCH - remove kvm_irq_routing_extended_msi and use union instead --- Documentation/virtual/kvm/api.txt | 9 ++++++++- include/uapi/linux/kvm.h | 6 +++++- 2 files changed, 13 insertions(+), 2 deletions(-)