Re: [Qemu-devel] [RFC][PATCH 11/45] msi: Factor out delivery hook
On 2011-10-17 15:48, Michael S. Tsirkin wrote: On Mon, Oct 17, 2011 at 03:41:44PM +0200, Avi Kivity wrote: On 10/17/2011 03:41 PM, Michael S. Tsirkin wrote: On Mon, Oct 17, 2011 at 01:15:56PM +0200, Jan Kiszka wrote: On 2011-10-17 12:56, Avi Kivity wrote: On 10/17/2011 11:27 AM, Jan Kiszka wrote: So far we deliver MSI messages by writing them into the target MMIO area. This reflects what happens on hardware, but imposes some limitations on the emulation when introducing KVM in-kernel irqchip models. For those we will need to track the message origin. Why do we need to track the message origin? Emulated interrupt remapping? The origin holds the routing cache which we need to track if the message already has a route (and that without searching long lists) and to update that route instead of add another one. Hmm, yes, but if the device does stl_phys or something like this, it won't work with irqchip, will it? And it should, ideally. Why not? it will fall back to the apic path, and use the local routing cache entry there. Does it still work with irqchip enabled? I didn't realize ... Yep, as MSI requests that land in the APIC MMIO page are also fed into msi_deliver and will take the normal path from there on. Jan signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [RFC][PATCH 11/45] msi: Factor out delivery hook
On Mon, Oct 17, 2011 at 09:15:47PM +0200, Jan Kiszka wrote:
On 2011-10-17 15:43, Michael S. Tsirkin wrote:
On Mon, Oct 17, 2011 at 11:27:45AM +0200, Jan Kiszka wrote:
diff --git a/hw/msi.c b/hw/msi.c
index 3c7ebc3..9055155 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -40,6 +40,14 @@
/* Flag for interrupt controller to declare MSI/MSI-X support */
bool msi_supported;
+static void msi_unsupported(MSIMessage *msg)
+{
+/* If we get here, the board failed to register a delivery handler. */
+abort();
+}
+
+void (*msi_deliver)(MSIMessage *msg) = msi_unsupported;
+
How about we set this to NULL, and check it instead of the bool
flag?
Yeah. I will introduce
bool msi_supported(void)
{
return msi_deliver != msi_unsupported;
}
OK?
Jan
Looks a bit weird ...
NULL is a pretty standard value for an invalid pointer, isn't it?
--
MST
Re: [Qemu-devel] [RFC][PATCH 11/45] msi: Factor out delivery hook
On 2011-10-18 14:05, Michael S. Tsirkin wrote:
On Mon, Oct 17, 2011 at 09:15:47PM +0200, Jan Kiszka wrote:
On 2011-10-17 15:43, Michael S. Tsirkin wrote:
On Mon, Oct 17, 2011 at 11:27:45AM +0200, Jan Kiszka wrote:
diff --git a/hw/msi.c b/hw/msi.c
index 3c7ebc3..9055155 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -40,6 +40,14 @@
/* Flag for interrupt controller to declare MSI/MSI-X support */
bool msi_supported;
+static void msi_unsupported(MSIMessage *msg)
+{
+/* If we get here, the board failed to register a delivery handler. */
+abort();
+}
+
+void (*msi_deliver)(MSIMessage *msg) = msi_unsupported;
+
How about we set this to NULL, and check it instead of the bool
flag?
Yeah. I will introduce
bool msi_supported(void)
{
return msi_deliver != msi_unsupported;
}
OK?
Jan
Looks a bit weird ...
NULL is a pretty standard value for an invalid pointer, isn't it?
Save us the runtime check and is equally expressive and readable IMHO.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [RFC][PATCH 11/45] msi: Factor out delivery hook
On Tue, Oct 18, 2011 at 02:23:29PM +0200, Jan Kiszka wrote:
On 2011-10-18 14:05, Michael S. Tsirkin wrote:
On Mon, Oct 17, 2011 at 09:15:47PM +0200, Jan Kiszka wrote:
On 2011-10-17 15:43, Michael S. Tsirkin wrote:
On Mon, Oct 17, 2011 at 11:27:45AM +0200, Jan Kiszka wrote:
diff --git a/hw/msi.c b/hw/msi.c
index 3c7ebc3..9055155 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -40,6 +40,14 @@
/* Flag for interrupt controller to declare MSI/MSI-X support */
bool msi_supported;
+static void msi_unsupported(MSIMessage *msg)
+{
+/* If we get here, the board failed to register a delivery handler.
*/
+abort();
+}
+
+void (*msi_deliver)(MSIMessage *msg) = msi_unsupported;
+
How about we set this to NULL, and check it instead of the bool
flag?
Yeah. I will introduce
bool msi_supported(void)
{
return msi_deliver != msi_unsupported;
}
OK?
Jan
Looks a bit weird ...
NULL is a pretty standard value for an invalid pointer, isn't it?
Save us the runtime check and is equally expressive and readable IMHO.
Jan
Do we need to check?
NULL dereference leads to a crash just as surely...
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [RFC][PATCH 11/45] msi: Factor out delivery hook
On 2011-10-18 14:38, Michael S. Tsirkin wrote:
On Tue, Oct 18, 2011 at 02:23:29PM +0200, Jan Kiszka wrote:
On 2011-10-18 14:05, Michael S. Tsirkin wrote:
On Mon, Oct 17, 2011 at 09:15:47PM +0200, Jan Kiszka wrote:
On 2011-10-17 15:43, Michael S. Tsirkin wrote:
On Mon, Oct 17, 2011 at 11:27:45AM +0200, Jan Kiszka wrote:
diff --git a/hw/msi.c b/hw/msi.c
index 3c7ebc3..9055155 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -40,6 +40,14 @@
/* Flag for interrupt controller to declare MSI/MSI-X support */
bool msi_supported;
+static void msi_unsupported(MSIMessage *msg)
+{
+/* If we get here, the board failed to register a delivery handler.
*/
+abort();
+}
+
+void (*msi_deliver)(MSIMessage *msg) = msi_unsupported;
+
How about we set this to NULL, and check it instead of the bool
flag?
Yeah. I will introduce
bool msi_supported(void)
{
return msi_deliver != msi_unsupported;
}
OK?
Jan
Looks a bit weird ...
NULL is a pretty standard value for an invalid pointer, isn't it?
Save us the runtime check and is equally expressive and readable IMHO.
Jan
Do we need to check?
NULL dereference leads to a crash just as surely...
There is no NULL state of msi_deliver. A) it would execute
msi_unsupported if all goes wrong (which will abort) and B)
msi_supported() is supposed to protect us in the absence of bugs from
ever executing msi_deliver() if it points to msi_unsupported.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [RFC][PATCH 11/45] msi: Factor out delivery hook
On Tue, 18 Oct 2011, Michael S. Tsirkin wrote:
On Tue, Oct 18, 2011 at 02:23:29PM +0200, Jan Kiszka wrote:
On 2011-10-18 14:05, Michael S. Tsirkin wrote:
On Mon, Oct 17, 2011 at 09:15:47PM +0200, Jan Kiszka wrote:
On 2011-10-17 15:43, Michael S. Tsirkin wrote:
On Mon, Oct 17, 2011 at 11:27:45AM +0200, Jan Kiszka wrote:
diff --git a/hw/msi.c b/hw/msi.c
index 3c7ebc3..9055155 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -40,6 +40,14 @@
/* Flag for interrupt controller to declare MSI/MSI-X support */
bool msi_supported;
+static void msi_unsupported(MSIMessage *msg)
+{
+/* If we get here, the board failed to register a delivery
handler. */
+abort();
+}
+
+void (*msi_deliver)(MSIMessage *msg) = msi_unsupported;
+
How about we set this to NULL, and check it instead of the bool
flag?
Yeah. I will introduce
bool msi_supported(void)
{
return msi_deliver != msi_unsupported;
}
OK?
Jan
Looks a bit weird ...
NULL is a pretty standard value for an invalid pointer, isn't it?
Save us the runtime check and is equally expressive and readable IMHO.
Jan
Do we need to check?
NULL dereference leads to a crash just as surely...
Not universally (not on AIX for instance (read)).
--
mailto:av1...@comtv.ru
Re: [Qemu-devel] [RFC][PATCH 11/45] msi: Factor out delivery hook
On Tue, Oct 18, 2011 at 04:44:47PM +0400, malc wrote:
On Tue, 18 Oct 2011, Michael S. Tsirkin wrote:
On Tue, Oct 18, 2011 at 02:23:29PM +0200, Jan Kiszka wrote:
On 2011-10-18 14:05, Michael S. Tsirkin wrote:
On Mon, Oct 17, 2011 at 09:15:47PM +0200, Jan Kiszka wrote:
On 2011-10-17 15:43, Michael S. Tsirkin wrote:
On Mon, Oct 17, 2011 at 11:27:45AM +0200, Jan Kiszka wrote:
diff --git a/hw/msi.c b/hw/msi.c
index 3c7ebc3..9055155 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -40,6 +40,14 @@
/* Flag for interrupt controller to declare MSI/MSI-X support */
bool msi_supported;
+static void msi_unsupported(MSIMessage *msg)
+{
+/* If we get here, the board failed to register a delivery
handler. */
+abort();
+}
+
+void (*msi_deliver)(MSIMessage *msg) = msi_unsupported;
+
How about we set this to NULL, and check it instead of the bool
flag?
Yeah. I will introduce
bool msi_supported(void)
{
return msi_deliver != msi_unsupported;
}
OK?
Jan
Looks a bit weird ...
NULL is a pretty standard value for an invalid pointer, isn't it?
Save us the runtime check and is equally expressive and readable IMHO.
Jan
Do we need to check?
NULL dereference leads to a crash just as surely...
Not universally (not on AIX for instance (read)).
This is a NULL function call though :)
Anyway, this was just nitpicking. Do it any way you like.
--
mailto:av1...@comtv.ru
Re: [Qemu-devel] [RFC][PATCH 11/45] msi: Factor out delivery hook
On 10/17/2011 11:27 AM, Jan Kiszka wrote: So far we deliver MSI messages by writing them into the target MMIO area. This reflects what happens on hardware, but imposes some limitations on the emulation when introducing KVM in-kernel irqchip models. For those we will need to track the message origin. Why do we need to track the message origin? Emulated interrupt remapping? Moreover, different architecture or accelerators may want to overload the delivery handler. Therefore, this commit introduces a delivery hook that is called by the MSI/MSI-X layer when devices send normal messages, but also on spurious deliveries that ended up on the APIC MMIO handler. Our default delivery handler for APIC-based PCs then dispatches between real MSIs and other DMA requests that happened to take the MSI patch. 'path' -static void apic_send_msi(target_phys_addr_t addr, uint32_t data) +void apic_deliver_msi(MSIMessage *msg) In general, it is better these days to pass small structures by value. Not sure what the gain is from intercepting the msi just before the stl_phys() vs. in the apic handler. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [RFC][PATCH 11/45] msi: Factor out delivery hook
On 2011-10-17 12:56, Avi Kivity wrote: On 10/17/2011 11:27 AM, Jan Kiszka wrote: So far we deliver MSI messages by writing them into the target MMIO area. This reflects what happens on hardware, but imposes some limitations on the emulation when introducing KVM in-kernel irqchip models. For those we will need to track the message origin. Why do we need to track the message origin? Emulated interrupt remapping? The origin holds the routing cache which we need to track if the message already has a route (and that without searching long lists) and to update that route instead of add another one. Moreover, different architecture or accelerators may want to overload the delivery handler. Therefore, this commit introduces a delivery hook that is called by the MSI/MSI-X layer when devices send normal messages, but also on spurious deliveries that ended up on the APIC MMIO handler. Our default delivery handler for APIC-based PCs then dispatches between real MSIs and other DMA requests that happened to take the MSI patch. 'path' -static void apic_send_msi(target_phys_addr_t addr, uint32_t data) +void apic_deliver_msi(MSIMessage *msg) In general, it is better these days to pass small structures by value. OK, will adjust this. Not sure what the gain is from intercepting the msi just before the stl_phys() vs. in the apic handler. APIC is x86-specific, MSI is not. I think Xen will also want to make use of this hook. I originally though of using it for the KVM in-kernel models as well, but I will now establish a callback at APIC-level (upstream will look differently from qemu-kvm in this regard). Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [RFC][PATCH 11/45] msi: Factor out delivery hook
On 10/17/2011 01:15 PM, Jan Kiszka wrote: On 2011-10-17 12:56, Avi Kivity wrote: On 10/17/2011 11:27 AM, Jan Kiszka wrote: So far we deliver MSI messages by writing them into the target MMIO area. This reflects what happens on hardware, but imposes some limitations on the emulation when introducing KVM in-kernel irqchip models. For those we will need to track the message origin. Why do we need to track the message origin? Emulated interrupt remapping? The origin holds the routing cache which we need to track if the message already has a route (and that without searching long lists) and to update that route instead of add another one. Okay, having read more of the code I understand this better. The approach of providing an explicit cache entry, while more intrusive, is simpler (at least, without std::unordered_map). However you do need destructors for the cache to let the core know that it can't reference it anymore. Not sure what the gain is from intercepting the msi just before the stl_phys() vs. in the apic handler. APIC is x86-specific, MSI is not. I think Xen will also want to make use of this hook. I originally though of using it for the KVM in-kernel models as well, but I will now establish a callback at APIC-level (upstream will look differently from qemu-kvm in this regard). But you still have to handle it the the platform interrupt controller (or whatever processes msi messages) since you can still DMA there. So you don't get away from doing it there anyway. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [RFC][PATCH 11/45] msi: Factor out delivery hook
On 2011-10-17 13:22, Avi Kivity wrote: On 10/17/2011 01:15 PM, Jan Kiszka wrote: On 2011-10-17 12:56, Avi Kivity wrote: On 10/17/2011 11:27 AM, Jan Kiszka wrote: So far we deliver MSI messages by writing them into the target MMIO area. This reflects what happens on hardware, but imposes some limitations on the emulation when introducing KVM in-kernel irqchip models. For those we will need to track the message origin. Why do we need to track the message origin? Emulated interrupt remapping? The origin holds the routing cache which we need to track if the message already has a route (and that without searching long lists) and to update that route instead of add another one. Okay, having read more of the code I understand this better. The approach of providing an explicit cache entry, while more intrusive, is simpler (at least, without std::unordered_map). However you do need destructors for the cache to let the core know that it can't reference it anymore. See my other mail. Not sure what the gain is from intercepting the msi just before the stl_phys() vs. in the apic handler. APIC is x86-specific, MSI is not. I think Xen will also want to make use of this hook. I originally though of using it for the KVM in-kernel models as well, but I will now establish a callback at APIC-level (upstream will look differently from qemu-kvm in this regard). But you still have to handle it the the platform interrupt controller (or whatever processes msi messages) since you can still DMA there. So you don't get away from doing it there anyway. Right, but that's the slow path (which is still handled - on x86 via the MMIO region the APIC still maintains). Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [RFC][PATCH 11/45] msi: Factor out delivery hook
On 10/17/2011 01:29 PM, Jan Kiszka wrote: APIC is x86-specific, MSI is not. I think Xen will also want to make use of this hook. I originally though of using it for the KVM in-kernel models as well, but I will now establish a callback at APIC-level (upstream will look differently from qemu-kvm in this regard). But you still have to handle it the the platform interrupt controller (or whatever processes msi messages) since you can still DMA there. So you don't get away from doing it there anyway. Right, but that's the slow path (which is still handled - on x86 via the MMIO region the APIC still maintains). It's handled by caching and immediately uncaching the MSIMessage/kvm route relationship? Can you post a git tree? It will be easier for me to understand the whole thing this way. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [RFC][PATCH 11/45] msi: Factor out delivery hook
On Mon, Oct 17, 2011 at 01:15:56PM +0200, Jan Kiszka wrote: On 2011-10-17 12:56, Avi Kivity wrote: On 10/17/2011 11:27 AM, Jan Kiszka wrote: So far we deliver MSI messages by writing them into the target MMIO area. This reflects what happens on hardware, but imposes some limitations on the emulation when introducing KVM in-kernel irqchip models. For those we will need to track the message origin. Why do we need to track the message origin? Emulated interrupt remapping? The origin holds the routing cache which we need to track if the message already has a route (and that without searching long lists) and to update that route instead of add another one. Hmm, yes, but if the device does stl_phys or something like this, it won't work with irqchip, will it? And it should, ideally. -- MST
Re: [Qemu-devel] [RFC][PATCH 11/45] msi: Factor out delivery hook
On 10/17/2011 03:41 PM, Michael S. Tsirkin wrote: On Mon, Oct 17, 2011 at 01:15:56PM +0200, Jan Kiszka wrote: On 2011-10-17 12:56, Avi Kivity wrote: On 10/17/2011 11:27 AM, Jan Kiszka wrote: So far we deliver MSI messages by writing them into the target MMIO area. This reflects what happens on hardware, but imposes some limitations on the emulation when introducing KVM in-kernel irqchip models. For those we will need to track the message origin. Why do we need to track the message origin? Emulated interrupt remapping? The origin holds the routing cache which we need to track if the message already has a route (and that without searching long lists) and to update that route instead of add another one. Hmm, yes, but if the device does stl_phys or something like this, it won't work with irqchip, will it? And it should, ideally. Why not? it will fall back to the apic path, and use the local routing cache entry there. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [RFC][PATCH 11/45] msi: Factor out delivery hook
On Mon, Oct 17, 2011 at 11:27:45AM +0200, Jan Kiszka wrote:
diff --git a/hw/msi.c b/hw/msi.c
index 3c7ebc3..9055155 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -40,6 +40,14 @@
/* Flag for interrupt controller to declare MSI/MSI-X support */
bool msi_supported;
+static void msi_unsupported(MSIMessage *msg)
+{
+/* If we get here, the board failed to register a delivery handler. */
+abort();
+}
+
+void (*msi_deliver)(MSIMessage *msg) = msi_unsupported;
+
How about we set this to NULL, and check it instead of the bool
flag?
--
MSt
Re: [Qemu-devel] [RFC][PATCH 11/45] msi: Factor out delivery hook
On Mon, Oct 17, 2011 at 03:41:44PM +0200, Avi Kivity wrote: On 10/17/2011 03:41 PM, Michael S. Tsirkin wrote: On Mon, Oct 17, 2011 at 01:15:56PM +0200, Jan Kiszka wrote: On 2011-10-17 12:56, Avi Kivity wrote: On 10/17/2011 11:27 AM, Jan Kiszka wrote: So far we deliver MSI messages by writing them into the target MMIO area. This reflects what happens on hardware, but imposes some limitations on the emulation when introducing KVM in-kernel irqchip models. For those we will need to track the message origin. Why do we need to track the message origin? Emulated interrupt remapping? The origin holds the routing cache which we need to track if the message already has a route (and that without searching long lists) and to update that route instead of add another one. Hmm, yes, but if the device does stl_phys or something like this, it won't work with irqchip, will it? And it should, ideally. Why not? it will fall back to the apic path, and use the local routing cache entry there. Does it still work with irqchip enabled? I didn't realize ... -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [RFC][PATCH 11/45] msi: Factor out delivery hook
On 2011-10-17 15:43, Michael S. Tsirkin wrote:
On Mon, Oct 17, 2011 at 11:27:45AM +0200, Jan Kiszka wrote:
diff --git a/hw/msi.c b/hw/msi.c
index 3c7ebc3..9055155 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -40,6 +40,14 @@
/* Flag for interrupt controller to declare MSI/MSI-X support */
bool msi_supported;
+static void msi_unsupported(MSIMessage *msg)
+{
+/* If we get here, the board failed to register a delivery handler. */
+abort();
+}
+
+void (*msi_deliver)(MSIMessage *msg) = msi_unsupported;
+
How about we set this to NULL, and check it instead of the bool
flag?
Yeah. I will introduce
bool msi_supported(void)
{
return msi_deliver != msi_unsupported;
}
OK?
Jan
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [RFC][PATCH 11/45] msi: Factor out delivery hook
On 2011-10-17 14:14, Avi Kivity wrote: Can you post a git tree? It will be easier for me to understand the whole thing this way. Pushed current state to git://git.kiszka.org/qemu-kvm.git queues/msi Jan signature.asc Description: OpenPGP digital signature
