On 2012-10-29 06:24, liu ping fan wrote: > 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?
Right. Jan -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux