| Message ID | d13aab98dc5c08fa2473511e006365b3a4bbee3f.1318843693.git.jan.kiszka@siemens.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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.
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.
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.
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?
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
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
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
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
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?
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
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
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
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)).
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:av1474@comtv.ru
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. Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- hw/apic.c | 19 ++++++++++++------- hw/apic.h | 1 + hw/msi.c | 10 +++++++++- hw/msi.h | 2 ++ hw/msix.c | 2 +- hw/pc.c | 11 +++++++++++ 6 files changed, 36 insertions(+), 9 deletions(-) diff --git a/hw/apic.c b/hw/apic.c index e43219f..c1d557d 100644 --- a/hw/apic.c +++ b/hw/apic.c @@ -19,6 +19,7 @@ #include "hw.h" #include "apic.h" #include "ioapic.h" +#include "msi.h" #include "qemu-timer.h" #include "host-utils.h" #include "sysbus.h" @@ -803,13 +804,15 @@ static uint32_t apic_mem_readl(void *opaque, target_phys_addr_t addr) return val; } -static void apic_send_msi(target_phys_addr_t addr, uint32_t data) +void apic_deliver_msi(MSIMessage *msg) { - uint8_t dest = (addr & MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT; - uint8_t vector = (data & MSI_DATA_VECTOR_MASK) >> MSI_DATA_VECTOR_SHIFT; - uint8_t dest_mode = (addr >> MSI_ADDR_DEST_MODE_SHIFT) & 0x1; - uint8_t trigger_mode = (data >> MSI_DATA_TRIGGER_SHIFT) & 0x1; - uint8_t delivery = (data >> MSI_DATA_DELIVERY_MODE_SHIFT) & 0x7; + uint8_t dest = + (msg->address & MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT; + uint8_t vector = + (msg->data & MSI_DATA_VECTOR_MASK) >> MSI_DATA_VECTOR_SHIFT; + uint8_t dest_mode = (msg->address >> MSI_ADDR_DEST_MODE_SHIFT) & 0x1; + uint8_t trigger_mode = (msg->data >> MSI_DATA_TRIGGER_SHIFT) & 0x1; + uint8_t delivery = (msg->data >> MSI_DATA_DELIVERY_MODE_SHIFT) & 0x7; /* XXX: Ignore redirection hint. */ apic_deliver_irq(dest, dest_mode, delivery, vector, trigger_mode); } @@ -825,7 +828,9 @@ static void apic_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val) * APIC is connected directly to the CPU. * Mapping them on the global bus happens to work because * MSI registers are reserved in APIC MMIO and vice versa. */ - apic_send_msi(addr, val); + MSIMessage msg = { .address = addr, .data = val }; + + msi_deliver(&msg); return; } diff --git a/hw/apic.h b/hw/apic.h index c398c83..fa848fd 100644 --- a/hw/apic.h +++ b/hw/apic.h @@ -18,6 +18,7 @@ void cpu_set_apic_tpr(DeviceState *s, uint8_t val); uint8_t cpu_get_apic_tpr(DeviceState *s); void apic_init_reset(DeviceState *s); void apic_sipi(DeviceState *s); +void apic_deliver_msi(MSIMessage *msg); /* pc.c */ int cpu_is_bsp(CPUState *env); 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; + /* If we get rid of cap allocator, we won't need this. */ static inline uint8_t msi_cap_sizeof(uint16_t flags) { @@ -381,7 +389,7 @@ void msi_notify(PCIDevice *dev, unsigned int vector) "notify vector 0x%x" " address: 0x%"PRIx64" data: 0x%"PRIx32"\n", vector, msg.address, msg.data); - stl_le_phys(msg.address, msg.data); + msi_deliver(&msg); } /* Normally called by pci_default_write_config(). */ diff --git a/hw/msi.h b/hw/msi.h index 22e3932..f3152f3 100644 --- a/hw/msi.h +++ b/hw/msi.h @@ -46,4 +46,6 @@ static inline bool msi_present(const PCIDevice *dev) return dev->cap_present & QEMU_PCI_CAP_MSI; } +extern void (*msi_deliver)(MSIMessage *msg); + #endif /* QEMU_MSI_H */ diff --git a/hw/msix.c b/hw/msix.c index 50fa504..08cc526 100644 --- a/hw/msix.c +++ b/hw/msix.c @@ -478,7 +478,7 @@ void msix_notify(PCIDevice *dev, unsigned vector) msix_message_from_vector(dev, vector, &msg); - stl_le_phys(msg.address, msg.data); + msi_deliver(&msg); } void msix_reset(PCIDevice *dev) diff --git a/hw/pc.c b/hw/pc.c index 768a20c..7d29a4a 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -24,6 +24,7 @@ #include "hw.h" #include "pc.h" #include "apic.h" +#include "msi.h" #include "fdc.h" #include "ide.h" #include "pci.h" @@ -102,6 +103,15 @@ void isa_irq_handler(void *opaque, int n, int level) qemu_set_irq(isa->ioapic[n], level); }; +static void pc_msi_deliver(MSIMessage *msg) +{ + if ((msg->address & 0xfff00000) == MSI_ADDR_BASE) { + apic_deliver_msi(msg); + } else { + stl_phys(msg->address, msg->data); + } +} + static void ioport80_write(void *opaque, uint32_t addr, uint32_t data) { } @@ -889,6 +899,7 @@ static DeviceState *apic_init(void *env, uint8_t apic_id) on the global memory bus. */ /* XXX: what if the base changes? */ sysbus_mmio_map(d, 0, MSI_ADDR_BASE); + msi_deliver = pc_msi_deliver; apic_mapped = 1; }