[Qemu-devel] [PATCH 1/3] Add rtc reset function.

[Qemu-devel] [PATCH 1/3] Add rtc reset function.

[Gleb Natapov]

On reset:
Periodic Interrupt Enable (PIE) bit is cleared to zero
Alarm Interrupt Enable (AIE) bit is cleared to zero
Update ended Interrupt Flag (UF) bit is cleared to zero
Interrupt Request status Flag (IRQF) bit is cleared to zero
Periodic Interrupt Flag (PF) bit is cleared to zero
Alarm Interrupt Flag (AF) bit is cleared to zero
Square Wave output Enable (SQWE) zero

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 hw/mc146818rtc.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 888b85a..98a2273 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -568,6 +568,22 @@ static int rtc_load_td(QEMUFile *f, void *opaque, int version_id)
 }
 #endif
 
+static void rtc_reset(void *opaque)
+{
+    RTCState *s = opaque;
+
+    /* clear PIE,AIE,SQWE on reset */
+    s->cmos_data[RTC_REG_B] &= ~((1<<6) | (1<<5) | (1<<3));
+
+    /* clear UF,IRQF,PF,AF on reset */
+    s->cmos_data[RTC_REG_C] &= ~((1<<4) | (1<<7) | (1<<6) | (1<<5));
+
+#ifdef TARGET_I386
+    if (rtc_td_hack)
+	    s->irq_coalesced = 0;
+#endif
+}
+
 RTCState *rtc_init_sqw(int base, qemu_irq irq, qemu_irq sqw_irq, int base_year)
 {
     RTCState *s;
@@ -606,6 +622,8 @@ RTCState *rtc_init_sqw(int base, qemu_irq irq, qemu_irq sqw_irq, int base_year)
     if (rtc_td_hack)
         register_savevm("mc146818rtc-td", base, 1, rtc_save_td, rtc_load_td, s);
 #endif
+    qemu_register_reset(rtc_reset, 0, s);
+
     return s;
 }
 
@@ -721,5 +739,6 @@ RTCState *rtc_mm_init(target_phys_addr_t base, int it_shift, qemu_irq irq,
     if (rtc_td_hack)
         register_savevm("mc146818rtc-td", base, 1, rtc_save_td, rtc_load_td, s);
 #endif
+    qemu_register_reset(rtc_reset, 0, s);
     return s;
 }
-- 
1.6.2.1

[Qemu-devel] [PATCH 2/3] Add pci_bus_reset() function.

[Gleb Natapov]

To reset internal irq handling data structures.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
Signed-off-by: Yaniv Kamay <ykamay@redhat.com>
---
 hw/pci.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 02b335f..89fefdf 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -88,6 +88,21 @@ static int  pcibus_load(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
+static void pci_bus_reset(void *opaque)
+{
+    PCIBus *bus = (PCIBus *)opaque;
+    int i;
+
+    for (i = 0; i < bus->nirq; i++) {
+        bus->irq_count[i] = 0;
+    }
+    for (i = 0; i < PCI_DEVICES_MAX; i++) {
+        if (bus->devices[i])
+            memset(bus->devices[i]->irq_state, 0,
+                   sizeof(bus->devices[i]->irq_state));
+    }
+}
+
 PCIBus *pci_register_bus(DeviceState *parent, const char *name,
                          pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
                          qemu_irq *pic, int devfn_min, int nirq)
@@ -106,6 +121,7 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
     bus->next = first_bus;
     first_bus = bus;
     register_savevm("PCIBUS", nbus++, 1, pcibus_save, pcibus_load, bus);
+    qemu_register_reset(pci_bus_reset, 0, bus);
     return bus;
 }
 
-- 
1.6.2.1

[Qemu-devel] [PATCH 3/3] Call piix3_reset() on system reset.

[Gleb Natapov]

Also zero pci_irq_levels on reset to avoid stuck irq after reset.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
Signed-off-by: Yaniv Kamay <ykamay@redhat.com>
---
 hw/piix_pci.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 914a65a..7b545ac 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -267,6 +267,8 @@ static void piix3_reset(PCIDevice *d)
     pci_conf[0xab] = 0x00;
     pci_conf[0xac] = 0x00;
     pci_conf[0xae] = 0x00;
+
+    memset(pci_irq_levels, 0, sizeof(pci_irq_levels));
 }
 
 static void piix4_reset(PCIDevice *d)
@@ -339,6 +341,7 @@ int piix3_init(PCIBus *bus, int devfn)
         PCI_HEADER_TYPE_NORMAL | PCI_HEADER_TYPE_MULTI_FUNCTION; // header_type = PCI_multifunction, generic
 
     piix3_reset(d);
+    qemu_register_reset(piix3_reset, 0, d);
     return d->devfn;
 }
 
-- 
1.6.2.1

[Qemu-devel] Re: [PATCH 1/3] Add rtc reset function.

[Jan Kiszka]

Gleb Natapov wrote:
> On reset:
> Periodic Interrupt Enable (PIE) bit is cleared to zero
> Alarm Interrupt Enable (AIE) bit is cleared to zero
> Update ended Interrupt Flag (UF) bit is cleared to zero
> Interrupt Request status Flag (IRQF) bit is cleared to zero
> Periodic Interrupt Flag (PF) bit is cleared to zero
> Alarm Interrupt Flag (AF) bit is cleared to zero
> Square Wave output Enable (SQWE) zero
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
>  hw/mc146818rtc.c |   19 +++++++++++++++++++
>  1 files changed, 19 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
> index 888b85a..98a2273 100644
> --- a/hw/mc146818rtc.c
> +++ b/hw/mc146818rtc.c
> @@ -568,6 +568,22 @@ static int rtc_load_td(QEMUFile *f, void *opaque, int version_id)
>  }
>  #endif
>  
> +static void rtc_reset(void *opaque)
> +{
> +    RTCState *s = opaque;
> +
> +    /* clear PIE,AIE,SQWE on reset */
> +    s->cmos_data[RTC_REG_B] &= ~((1<<6) | (1<<5) | (1<<3));
> +
> +    /* clear UF,IRQF,PF,AF on reset */
> +    s->cmos_data[RTC_REG_C] &= ~((1<<4) | (1<<7) | (1<<6) | (1<<5));

Didn't check what the rest of this file does, but maybe it's a chance to
give those bits names, ie. #define something.

> +
> +#ifdef TARGET_I386
> +    if (rtc_td_hack)
> +	    s->irq_coalesced = 0;
> +#endif
> +}
> +
>  RTCState *rtc_init_sqw(int base, qemu_irq irq, qemu_irq sqw_irq, int base_year)
>  {
>      RTCState *s;
> @@ -606,6 +622,8 @@ RTCState *rtc_init_sqw(int base, qemu_irq irq, qemu_irq sqw_irq, int base_year)
>      if (rtc_td_hack)
>          register_savevm("mc146818rtc-td", base, 1, rtc_save_td, rtc_load_td, s);
>  #endif
> +    qemu_register_reset(rtc_reset, 0, s);
> +
>      return s;
>  }
>  
> @@ -721,5 +739,6 @@ RTCState *rtc_mm_init(target_phys_addr_t base, int it_shift, qemu_irq irq,
>      if (rtc_td_hack)
>          register_savevm("mc146818rtc-td", base, 1, rtc_save_td, rtc_load_td, s);
>  #endif
> +    qemu_register_reset(rtc_reset, 0, s);
>      return s;
>  }

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 1/3] Add rtc reset function.

[Gleb Natapov]

On Mon, Jun 08, 2009 at 04:05:35PM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On reset:
> > Periodic Interrupt Enable (PIE) bit is cleared to zero
> > Alarm Interrupt Enable (AIE) bit is cleared to zero
> > Update ended Interrupt Flag (UF) bit is cleared to zero
> > Interrupt Request status Flag (IRQF) bit is cleared to zero
> > Periodic Interrupt Flag (PF) bit is cleared to zero
> > Alarm Interrupt Flag (AF) bit is cleared to zero
> > Square Wave output Enable (SQWE) zero
> > 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > ---
> >  hw/mc146818rtc.c |   19 +++++++++++++++++++
> >  1 files changed, 19 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
> > index 888b85a..98a2273 100644
> > --- a/hw/mc146818rtc.c
> > +++ b/hw/mc146818rtc.c
> > @@ -568,6 +568,22 @@ static int rtc_load_td(QEMUFile *f, void *opaque, int version_id)
> >  }
> >  #endif
> >  
> > +static void rtc_reset(void *opaque)
> > +{
> > +    RTCState *s = opaque;
> > +
> > +    /* clear PIE,AIE,SQWE on reset */
> > +    s->cmos_data[RTC_REG_B] &= ~((1<<6) | (1<<5) | (1<<3));
> > +
> > +    /* clear UF,IRQF,PF,AF on reset */
> > +    s->cmos_data[RTC_REG_C] &= ~((1<<4) | (1<<7) | (1<<6) | (1<<5));
> 
> Didn't check what the rest of this file does, but maybe it's a chance to
> give those bits names, ie. #define something.
> 
Part of them already has names, I'll define rest.

> > +
> > +#ifdef TARGET_I386
> > +    if (rtc_td_hack)
> > +	    s->irq_coalesced = 0;
> > +#endif
> > +}
> > +
> >  RTCState *rtc_init_sqw(int base, qemu_irq irq, qemu_irq sqw_irq, int base_year)
> >  {
> >      RTCState *s;
> > @@ -606,6 +622,8 @@ RTCState *rtc_init_sqw(int base, qemu_irq irq, qemu_irq sqw_irq, int base_year)
> >      if (rtc_td_hack)
> >          register_savevm("mc146818rtc-td", base, 1, rtc_save_td, rtc_load_td, s);
> >  #endif
> > +    qemu_register_reset(rtc_reset, 0, s);
> > +
> >      return s;
> >  }
> >  
> > @@ -721,5 +739,6 @@ RTCState *rtc_mm_init(target_phys_addr_t base, int it_shift, qemu_irq irq,
> >      if (rtc_td_hack)
> >          register_savevm("mc146818rtc-td", base, 1, rtc_save_td, rtc_load_td, s);
> >  #endif
> > +    qemu_register_reset(rtc_reset, 0, s);
> >      return s;
> >  }
> 
> Jan
> 
> -- 
> Siemens AG, Corporate Technology, CT SE 2
> Corporate Competence Center Embedded Linux

--
			Gleb.

Re: [Qemu-devel] [PATCH 2/3] Add pci_bus_reset() function.

[Avi Kivity]

Gleb Natapov wrote:
> To reset internal irq handling data structures.
>
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> Signed-off-by: Yaniv Kamay <ykamay@redhat.com>
> ---
>  hw/pci.c |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
>
> diff --git a/hw/pci.c b/hw/pci.c
> index 02b335f..89fefdf 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -88,6 +88,21 @@ static int  pcibus_load(QEMUFile *f, void *opaque, int version_id)
>      return 0;
>  }
>  
> +static void pci_bus_reset(void *opaque)
> +{
> +    PCIBus *bus = (PCIBus *)opaque;
> +    int i;
> +
> +    for (i = 0; i < bus->nirq; i++) {
> +        bus->irq_count[i] = 0;
> +    }
> +    for (i = 0; i < PCI_DEVICES_MAX; i++) {
> +        if (bus->devices[i])
> +            memset(bus->devices[i]->irq_state, 0,
> +                   sizeof(bus->devices[i]->irq_state));
> +    }
> +}
> +
>   

Shouldn't each device's reset function bring its line low, thus zeroing 
the irq_state naturally?

If not, we have a bug somewhere.  Note we have exactly the same issue 
with save/restore.

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

Re: [Qemu-devel] [PATCH 2/3] Add pci_bus_reset() function.

[Gleb Natapov]

On Mon, Jun 08, 2009 at 05:35:11PM +0300, Avi Kivity wrote:
> Gleb Natapov wrote:
>> To reset internal irq handling data structures.
>>
>> Signed-off-by: Gleb Natapov <gleb@redhat.com>
>> Signed-off-by: Yaniv Kamay <ykamay@redhat.com>
>> ---
>>  hw/pci.c |   16 ++++++++++++++++
>>  1 files changed, 16 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/pci.c b/hw/pci.c
>> index 02b335f..89fefdf 100644
>> --- a/hw/pci.c
>> +++ b/hw/pci.c
>> @@ -88,6 +88,21 @@ static int  pcibus_load(QEMUFile *f, void *opaque, int version_id)
>>      return 0;
>>  }
>>  +static void pci_bus_reset(void *opaque)
>> +{
>> +    PCIBus *bus = (PCIBus *)opaque;
>> +    int i;
>> +
>> +    for (i = 0; i < bus->nirq; i++) {
>> +        bus->irq_count[i] = 0;
>> +    }
>> +    for (i = 0; i < PCI_DEVICES_MAX; i++) {
>> +        if (bus->devices[i])
>> +            memset(bus->devices[i]->irq_state, 0,
>> +                   sizeof(bus->devices[i]->irq_state));
>> +    }
>> +}
>> +
>>   
>
> Shouldn't each device's reset function bring its line low, thus zeroing  
> the irq_state naturally?
>
> If not, we have a bug somewhere.  Note we have exactly the same issue  
> with save/restore.
>
They should, but I haven't found one that does.

--
			Gleb.

Re: [Qemu-devel] [PATCH 2/3] Add pci_bus_reset() function.

[Yaniv Kamay]

Gleb Natapov wrote:
> On Mon, Jun 08, 2009 at 05:35:11PM +0300, Avi Kivity wrote:
>   
>> Gleb Natapov wrote:
>>     
>>> To reset internal irq handling data structures.
>>>
>>> Signed-off-by: Gleb Natapov <gleb@redhat.com>
>>> Signed-off-by: Yaniv Kamay <ykamay@redhat.com>
>>> ---
>>>  hw/pci.c |   16 ++++++++++++++++
>>>  1 files changed, 16 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/pci.c b/hw/pci.c
>>> index 02b335f..89fefdf 100644
>>> --- a/hw/pci.c
>>> +++ b/hw/pci.c
>>> @@ -88,6 +88,21 @@ static int  pcibus_load(QEMUFile *f, void *opaque, int version_id)
>>>      return 0;
>>>  }
>>>  +static void pci_bus_reset(void *opaque)
>>> +{
>>> +    PCIBus *bus = (PCIBus *)opaque;
>>> +    int i;
>>> +
>>> +    for (i = 0; i < bus->nirq; i++) {
>>> +        bus->irq_count[i] = 0;
>>> +    }
>>> +    for (i = 0; i < PCI_DEVICES_MAX; i++) {
>>> +        if (bus->devices[i])
>>> +            memset(bus->devices[i]->irq_state, 0,
>>> +                   sizeof(bus->devices[i]->irq_state));
>>> +    }
>>> +}
>>> +
>>>   
>>>       
>> Shouldn't each device's reset function bring its line low, thus zeroing  
>> the irq_state naturally?
>>
>> If not, we have a bug somewhere.  Note we have exactly the same issue  
>> with save/restore.
>>
>>     
> They should, but I haven't found one that does.
>   

This is matter of definitions and not right or wrong, Since PCI in 
current imp save and load its state then it only logical that PCI will 
also reset it state.

Yaniv

> --
> 			Gleb.
>   

Re: [Qemu-devel] [PATCH 2/3] Add pci_bus_reset() function.

[Dor Laor]

Gleb Natapov wrote:
> On Mon, Jun 08, 2009 at 05:35:11PM +0300, Avi Kivity wrote:
>   
>> Gleb Natapov wrote:
>>     
>>> To reset internal irq handling data structures.
>>>
>>> Signed-off-by: Gleb Natapov <gleb@redhat.com>
>>> Signed-off-by: Yaniv Kamay <ykamay@redhat.com>
>>> ---
>>>  hw/pci.c |   16 ++++++++++++++++
>>>  1 files changed, 16 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/pci.c b/hw/pci.c
>>> index 02b335f..89fefdf 100644
>>> --- a/hw/pci.c
>>> +++ b/hw/pci.c
>>> @@ -88,6 +88,21 @@ static int  pcibus_load(QEMUFile *f, void *opaque, int version_id)
>>>      return 0;
>>>  }
>>>  +static void pci_bus_reset(void *opaque)
>>> +{
>>> +    PCIBus *bus = (PCIBus *)opaque;
>>> +    int i;
>>> +
>>> +    for (i = 0; i < bus->nirq; i++) {
>>> +        bus->irq_count[i] = 0;
>>> +    }
>>> +    for (i = 0; i < PCI_DEVICES_MAX; i++) {
>>> +        if (bus->devices[i])
>>> +            memset(bus->devices[i]->irq_state, 0,
>>> +                   sizeof(bus->devices[i]->irq_state));
>>> +    }
>>> +}
>>> +
>>>   
>>>       
>> Shouldn't each device's reset function bring its line low, thus zeroing  
>> the irq_state naturally?
>>
>> If not, we have a bug somewhere.  Note we have exactly the same issue  
>> with save/restore.
>>
>>     
> They should, but I haven't found one that does.
>   
virtio does and so do many others. Sad thing is that all should do it 
since the line is shared.
e1000 and rtl8139 do not register a reset handler.

Maybe we should make it a required callback for pci_qdev_register? or better
have every (pci?) device register several callback together.

Re: [Qemu-devel] [PATCH 2/3] Add pci_bus_reset() function.

[Gleb Natapov]

On Tue, Jun 09, 2009 at 12:42:16AM +0300, Dor Laor wrote:
> Gleb Natapov wrote:
>> On Mon, Jun 08, 2009 at 05:35:11PM +0300, Avi Kivity wrote:
>>   
>>> Gleb Natapov wrote:
>>>     
>>>> To reset internal irq handling data structures.
>>>>
>>>> Signed-off-by: Gleb Natapov <gleb@redhat.com>
>>>> Signed-off-by: Yaniv Kamay <ykamay@redhat.com>
>>>> ---
>>>>  hw/pci.c |   16 ++++++++++++++++
>>>>  1 files changed, 16 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/hw/pci.c b/hw/pci.c
>>>> index 02b335f..89fefdf 100644
>>>> --- a/hw/pci.c
>>>> +++ b/hw/pci.c
>>>> @@ -88,6 +88,21 @@ static int  pcibus_load(QEMUFile *f, void *opaque, int version_id)
>>>>      return 0;
>>>>  }
>>>>  +static void pci_bus_reset(void *opaque)
>>>> +{
>>>> +    PCIBus *bus = (PCIBus *)opaque;
>>>> +    int i;
>>>> +
>>>> +    for (i = 0; i < bus->nirq; i++) {
>>>> +        bus->irq_count[i] = 0;
>>>> +    }
>>>> +    for (i = 0; i < PCI_DEVICES_MAX; i++) {
>>>> +        if (bus->devices[i])
>>>> +            memset(bus->devices[i]->irq_state, 0,
>>>> +                   sizeof(bus->devices[i]->irq_state));
>>>> +    }
>>>> +}
>>>> +
>>>>         
>>> Shouldn't each device's reset function bring its line low, thus 
>>> zeroing  the irq_state naturally?
>>>
>>> If not, we have a bug somewhere.  Note we have exactly the same issue 
>>>  with save/restore.
>>>
>>>     
>> They should, but I haven't found one that does.
>>   
> virtio does and so do many others. Sad thing is that all should do it  
> since the line is shared.
> e1000 and rtl8139 do not register a reset handler.
>
So somebody (not me) should go and fix all others.

> Maybe we should make it a required callback for pci_qdev_register? or better
> have every (pci?) device register several callback together.
May be.

--
			Gleb.

Re: [Qemu-devel] [PATCH 2/3] Add pci_bus_reset() function.

[Blue Swirl]

[-- Attachment #1: Type: text/plain, Size: 2098 bytes --]

On 6/9/09, Gleb Natapov <gleb@redhat.com> wrote:
> On Tue, Jun 09, 2009 at 12:42:16AM +0300, Dor Laor wrote:
>  > Gleb Natapov wrote:
>  >> On Mon, Jun 08, 2009 at 05:35:11PM +0300, Avi Kivity wrote:
>  >>
>  >>> Gleb Natapov wrote:
>  >>>
>  >>>> To reset internal irq handling data structures.
>  >>>>
>  >>>> Signed-off-by: Gleb Natapov <gleb@redhat.com>
>  >>>> Signed-off-by: Yaniv Kamay <ykamay@redhat.com>
>  >>>> ---
>  >>>>  hw/pci.c |   16 ++++++++++++++++
>  >>>>  1 files changed, 16 insertions(+), 0 deletions(-)
>  >>>>
>  >>>> diff --git a/hw/pci.c b/hw/pci.c
>  >>>> index 02b335f..89fefdf 100644
>  >>>> --- a/hw/pci.c
>  >>>> +++ b/hw/pci.c
>  >>>> @@ -88,6 +88,21 @@ static int  pcibus_load(QEMUFile *f, void *opaque, int version_id)
>  >>>>      return 0;
>  >>>>  }
>  >>>>  +static void pci_bus_reset(void *opaque)
>  >>>> +{
>  >>>> +    PCIBus *bus = (PCIBus *)opaque;
>  >>>> +    int i;
>  >>>> +
>  >>>> +    for (i = 0; i < bus->nirq; i++) {
>  >>>> +        bus->irq_count[i] = 0;
>  >>>> +    }
>  >>>> +    for (i = 0; i < PCI_DEVICES_MAX; i++) {
>  >>>> +        if (bus->devices[i])
>  >>>> +            memset(bus->devices[i]->irq_state, 0,
>  >>>> +                   sizeof(bus->devices[i]->irq_state));
>  >>>> +    }
>  >>>> +}
>  >>>> +
>  >>>>
>  >>> Shouldn't each device's reset function bring its line low, thus
>  >>> zeroing  the irq_state naturally?
>  >>>
>  >>> If not, we have a bug somewhere.  Note we have exactly the same issue
>  >>>  with save/restore.
>  >>>
>  >>>
>  >> They should, but I haven't found one that does.
>  >>
>  > virtio does and so do many others. Sad thing is that all should do it
>  > since the line is shared.
>  > e1000 and rtl8139 do not register a reset handler.
>  >
>
> So somebody (not me) should go and fix all others.

Here's a 5 min patch to add reset to e1000 and rtl8139. Not too difficult?

>  > Maybe we should make it a required callback for pci_qdev_register? or better
>  > have every (pci?) device register several callback together.
>
> May be.

If we go to this, I'd also make savevm/loadvm mandatory.

[-- Attachment #2: 0001-Register-reset-functions-for-e1000-and-rtl8139.patch --]
[-- Type: application/x-patch, Size: 2683 bytes --]

Re: [Qemu-devel] [PATCH 2/3] Add pci_bus_reset() function.

[Gleb Natapov]

On Tue, Jun 09, 2009 at 06:07:00PM +0300, Blue Swirl wrote:
> On 6/9/09, Gleb Natapov <gleb@redhat.com> wrote:
> > On Tue, Jun 09, 2009 at 12:42:16AM +0300, Dor Laor wrote:
> >  > Gleb Natapov wrote:
> >  >> On Mon, Jun 08, 2009 at 05:35:11PM +0300, Avi Kivity wrote:
> >  >>
> >  >>> Gleb Natapov wrote:
> >  >>>
> >  >>>> To reset internal irq handling data structures.
> >  >>>>
> >  >>>> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> >  >>>> Signed-off-by: Yaniv Kamay <ykamay@redhat.com>
> >  >>>> ---
> >  >>>>  hw/pci.c |   16 ++++++++++++++++
> >  >>>>  1 files changed, 16 insertions(+), 0 deletions(-)
> >  >>>>
> >  >>>> diff --git a/hw/pci.c b/hw/pci.c
> >  >>>> index 02b335f..89fefdf 100644
> >  >>>> --- a/hw/pci.c
> >  >>>> +++ b/hw/pci.c
> >  >>>> @@ -88,6 +88,21 @@ static int  pcibus_load(QEMUFile *f, void *opaque, int version_id)
> >  >>>>      return 0;
> >  >>>>  }
> >  >>>>  +static void pci_bus_reset(void *opaque)
> >  >>>> +{
> >  >>>> +    PCIBus *bus = (PCIBus *)opaque;
> >  >>>> +    int i;
> >  >>>> +
> >  >>>> +    for (i = 0; i < bus->nirq; i++) {
> >  >>>> +        bus->irq_count[i] = 0;
> >  >>>> +    }
> >  >>>> +    for (i = 0; i < PCI_DEVICES_MAX; i++) {
> >  >>>> +        if (bus->devices[i])
> >  >>>> +            memset(bus->devices[i]->irq_state, 0,
> >  >>>> +                   sizeof(bus->devices[i]->irq_state));
> >  >>>> +    }
> >  >>>> +}
> >  >>>> +
> >  >>>>
> >  >>> Shouldn't each device's reset function bring its line low, thus
> >  >>> zeroing  the irq_state naturally?
> >  >>>
> >  >>> If not, we have a bug somewhere.  Note we have exactly the same issue
> >  >>>  with save/restore.
> >  >>>
> >  >>>
> >  >> They should, but I haven't found one that does.
> >  >>
> >  > virtio does and so do many others. Sad thing is that all should do it
> >  > since the line is shared.
> >  > e1000 and rtl8139 do not register a reset handler.
> >  >
> >
> > So somebody (not me) should go and fix all others.
> 
> Here's a 5 min patch to add reset to e1000 and rtl8139. Not too difficult?
> 
Very handy. I just discovered that e1000 continue to write to memory
after OS reboot with some OSes. Will check if this fixes it.


> >  > Maybe we should make it a required callback for pci_qdev_register? or better
> >  > have every (pci?) device register several callback together.
> >
> > May be.
> 
> If we go to this, I'd also make savevm/loadvm mandatory.

We need this for device hot unplug. We have to be able to reset only one
device at a time.

--
			Gleb.

Re: [Qemu-devel] [PATCH 2/3] Add pci_bus_reset() function.

[Gleb Natapov]

On Tue, Jun 09, 2009 at 06:07:00PM +0300, Blue Swirl wrote:
> Here's a 5 min patch to add reset to e1000 and rtl8139. Not too difficult?
> 
By the way I see that you don't lower irq line in e1000 reset handler
(haven't looked at rtl8139 one).

--
			Gleb.

Re: [Qemu-devel] [PATCH 2/3] Add pci_bus_reset() function.

[Blue Swirl]

[-- Attachment #1: Type: text/plain, Size: 376 bytes --]

On 6/9/09, Gleb Natapov <gleb@redhat.com> wrote:
> On Tue, Jun 09, 2009 at 06:07:00PM +0300, Blue Swirl wrote:
>
> > Here's a 5 min patch to add reset to e1000 and rtl8139. Not too difficult?
>  >
>
> By the way I see that you don't lower irq line in e1000 reset handler
>  (haven't looked at rtl8139 one).

Also on loadvm the irq line should be toggled. Fixed in this patch.

[-- Attachment #2: 0001-Update-irqs-on-reset-and-device-load.patch --]
[-- Type: application/x-patch, Size: 1481 bytes --]