On Thu, Oct 25, 2012 at 9:34 PM, Jan Kiszka <jan.kis...@siemens.com> wrote: > On 2012-10-24 09:29, liu ping fan wrote: >> On Tue, Oct 23, 2012 at 5:04 PM, Jan Kiszka <jan.kis...@siemens.com> wrote: >>> On 2012-10-22 11:23, Liu Ping Fan wrote: >>>> Use local lock to protect e1000. When calling the system function, >>>> dropping the fine lock before acquiring the big lock. This will >>>> introduce broken device state, which need extra effort to fix. >>>> >>>> Signed-off-by: Liu Ping Fan <pingf...@linux.vnet.ibm.com> >>>> --- >>>> hw/e1000.c | 24 +++++++++++++++++++++++- >>>> 1 files changed, 23 insertions(+), 1 deletions(-) >>>> >>>> diff --git a/hw/e1000.c b/hw/e1000.c >>>> index ae8a6c5..5eddab5 100644 >>>> --- a/hw/e1000.c >>>> +++ b/hw/e1000.c >>>> @@ -85,6 +85,7 @@ typedef struct E1000State_st { >>>> NICConf conf; >>>> MemoryRegion mmio; >>>> MemoryRegion io; >>>> + QemuMutex e1000_lock; >>>> >>>> uint32_t mac_reg[0x8000]; >>>> uint16_t phy_reg[0x20]; >>>> @@ -223,13 +224,27 @@ static const uint32_t mac_reg_init[] = { >>>> static void >>>> set_interrupt_cause(E1000State *s, int index, uint32_t val) >>>> { >>>> + QemuThread *t; >>>> + >>>> if (val && (E1000_DEVID >= E1000_DEV_ID_82547EI_MOBILE)) { >>>> /* Only for 8257x */ >>>> val |= E1000_ICR_INT_ASSERTED; >>>> } >>>> s->mac_reg[ICR] = val; >>>> s->mac_reg[ICS] = val; >>>> - qemu_set_irq(s->dev.irq[0], (s->mac_reg[IMS] & s->mac_reg[ICR]) != 0); >>>> + >>>> + t = pthread_getspecific(qemu_thread_key); >>>> + if (t->context_type == 1) { >>>> + qemu_mutex_unlock(&s->e1000_lock); >>>> + qemu_mutex_lock_iothread(); >>>> + } >>>> + if (DEVICE(s)->state < DEV_STATE_STOPPING) { >>>> + qemu_set_irq(s->dev.irq[0], (s->mac_reg[IMS] & s->mac_reg[ICR]) >>>> != 0); >>>> + } >>>> + if (t->context_type == 1) { >>>> + qemu_mutex_unlock_iothread(); >>>> + qemu_mutex_lock(&s->e1000_lock); >>>> + } >>> >>> This is ugly for many reasons. First of all, it is racy as the register >>> content may change while dropping the device lock, no? Then you would >>> raise or clear an IRQ spuriously. >>> >>> Second, it clearly shows that we need to address lock-less IRQ delivery. >>> Almost nothing is won if we have to take the global lock again to push >>> an IRQ event to the guest. I'm repeating myself, but the problem to be >>> solved here is almost identical to fast IRQ delivery for assigned >>> devices (which we only address pretty ad-hoc for PCI so far). >>> >> Interesting, could you show me more detail about it, so I can google... > > No need to look that far, just grep for pci_device_route_intx_to_irq, > pci_device_set_intx_routing_notifier and related functions in the code. > I think, the major point here is to bypass the delivery process among the irq chipset during runtime. Right?
Thanks and regards, pingfan > Jan > > -- > Siemens AG, Corporate Technology, CT RTC ITP SDP-DE > Corporate Competence Center Embedded Linux