Re: [Qemu-devel] [patch v4 12/16] e1000: apply fine lock on e1000

[Jan Kiszka]

On 2012-10-29 06:24, liu ping fan wrote:
> On Thu, Oct 25, 2012 at 9:34 PM, Jan Kiszka <address@hidden> wrote:
>> On 2012-10-24 09:29, liu ping fan wrote:
>>> On Tue, Oct 23, 2012 at 5:04 PM, Jan Kiszka <address@hidden> 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 <address@hidden>
>>>>> ---
>>>>>  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