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

[Avi Kivity <avi@redhat.com>]

On 10/22/2012 11:23 AM, 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 <pingfank@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;

Can call it '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 way too complicated for device model authors.  There's no way to
get it correct.

If mmio dispatch needs to call a non-thread-safe subsystem, it must
acquire the big lock:

Something like

e1000_mmio_read()
{
    if (index < NREADOPS && macreg_readops[index]){
        macreg_lockops[index].lock(s);
        ret = macreg_readops[index](s, index);
        macreg_lockops[index].unlock(s);
    }
    DBGOUT(UNKNOWN, "MMIO unknown read addr=0x%08x\n", index<<2);

}

Where .lock() either locks just the local lock, or both locks.  As
subsystems are converted to be thread safe, we can remove this.



-- 
error compiling committee.c: too many arguments to function