qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Filip Navara <filip.navara@gmail.com>
To: Gleb Natapov <gleb@redhat.com>
Cc: qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH] Register usb-uhci reset function.
Date: Wed, 17 Jun 2009 15:03:58 +0200	[thread overview]
Message-ID: <5b31733c0906170603k34de010fu1f01e9cda1277c8f@mail.gmail.com> (raw)
In-Reply-To: <20090617121209.GA19508@redhat.com>

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

On Wed, Jun 17, 2009 at 2:12 PM, Gleb Natapov <gleb@redhat.com> wrote:
[snip]

> >
> > That's why all the paths where the level in device emulation can change
> has
> > to be covered, so the information in piix3 stays accurate. What I'm
> saying
> > is that the reset callback is not one of the points where you can call
> > qemu_set_irq since the state of other devices is unknown at that point
> (for
> > the purpose of simulating interrupt levels). When the reset callbacks are
> I understand what you are saying, but the logic that emulates level irqs
> from qemu_irq() edge (in piix3) is not a part of device emulation, so it
> is not obvious to me that system reset should reset it and bring it to
> unknown state. Line state change may reach IRQ chip before or after reset,
> but
> this doesn't matter since IRQ chip will be in knows state in both cases.
>

Actually I want to bring it to a known state. It may not be equivalent to
what happens in real HW, but it's compatible behavior and deterministic,
which makes debugging easier.

> done with their work, all the simulated interrupt levels at all layers
> have
> > to be restored to known state. This state is currently with all the
> levels
> > set to zero. If you call qemu_set_irq during the reset callbacks the
> > interrupt levels recorded in the device tree may end up in uncertain
> state.
> That may happen on real HW too. OS will get spurious interrupt, that's it.


Agreed.

That's what I said earlier (or at least what I thought), you would get
spurious interrupts. It may have other consequences which I can't think of,
right now.

[snip]

> > To change the level you first have to have the whole device tree topology
> in
> > known state. In the reset callback that's not the case. If there was a
> Level change (as seen by other device) can happen only after or before
> reset, not during reset. In both cases the device state is known.


Hmm, makes sense. I suppose that if none of the bus/PIC/etc. devices altered
their "interrupt level" state during the reset and each individual device
brought it's IRQ line down it would work too. As long as the interrupts are
communicated to the CPU after the reset in a correct way. I'm just not
convinced that it's the right way. My rationale is that on power-up the
interrupt levels in PIC/PCI/etc. are set to zero and they only change as the
emulation is executed. So why should the reset be different? Doing it in a
different way on reset sounds fishy to me, especially since it involves more
code for no obvious benefit. Moreover APIC and lot of other devices clear
the interrupt state now, so you'd have to change them too.

[snip]

> > I'm suggesting not to call it at all and always reset the recorded
> > "interrupt levels" all the way down the device tree on reset. If there is
> a
> By doing this you are setting device irq line without asking device. It
> will work since assumption that irq level is zero after reset is usually
> correct.


Yes, but the emulation is not running at that point, so it's harmless.


> > device which sets the IRQ line high on reset then we should introduce the
> > "late" reset/init callback.
> >
> Yes, calling set_irq(1) on reset may not achieve this since irq chip may
> be reset after set_irq() was called.


Right, exactly my point.

[snip]

> > If it's not clear then let's make it clear. Real HW communicates
> interrupt
> > levels and detects edges, QEMU communicates edges and simulates levels,
> so
> > it has to work differently than on real HW in this particular case.
> >
> > The system reset callback would reset device registers to a known state
> as
> > specified by the HW documentation. The interrupt levels would be driven
> low
> > at this point on the devices which simulate "interrupt levels" (buses,
> PIC,
> > etc.) - as if "0" was sent to the copper wire ;)
> >
> > The "late" reset/init callback would drive the IRQs (and GPIOs) high if
> > necessary once the system is in a known state, ie. after all reset
> callbacks
> > have successfully completed.
> >
> Basically you want to simulate "dead" period when devices ignore their
> inputs for
> some time after reset (like on real HW).


Sort of, but in a deterministic way.


> > Hot-unplug callback doesn't need reset the registers of the device, it
> only
> > has to drive the IRQ low on the bus - ie. call *_set_irq(0) for the
> specific
> > bus. In theory that could be done at the bus level.
> >
> I should be done at bus level indeed. The device is disconnected at that
> point.


Agreed.


> > Feel free to disagree or correct me. Suggestions welcome.
> >
> Not disagreeing but this still is not enough, for instance there are
> devices that have different state after power-up and reset.


Let's document it once we (including the maintainers) agree on something. At
least that would set the precedent for future.

[snip]

> >
> > I don't remember the exact contents of 2/3, sorry.
> >
> http://lists.gnu.org/archive/html/qemu-devel/2009-06/msg00342.html
>

Looks good to me.

Best regards,
Filip Navara

[-- Attachment #2: Type: text/html, Size: 7347 bytes --]

      reply	other threads:[~2009-06-17 13:04 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-06-16 12:47 [Qemu-devel] [PATCH] Register usb-uhci reset function Gleb Natapov
2009-06-16 17:14 ` Paul Brook
2009-06-16 17:37   ` Gleb Natapov
2009-06-16 18:41     ` Paul Brook
2009-06-16 19:11       ` Gleb Natapov
2009-06-16 19:38         ` Paul Brook
2009-06-16 22:57           ` Zachary Amsden
2009-06-17  8:12           ` Gleb Natapov
2009-06-16 18:02   ` Blue Swirl
2009-06-16 19:19 ` Anthony Liguori
2009-06-16 19:26   ` Gleb Natapov
2009-06-17  9:07 ` Filip Navara
2009-06-17  9:43   ` Gleb Natapov
2009-06-17 10:17     ` Filip Navara
2009-06-17 11:06       ` Gleb Natapov
2009-06-17 11:25         ` Dor Laor
2009-06-17 11:39           ` Gleb Natapov
2009-06-17 11:50             ` Filip Navara
2009-06-17 11:36         ` Filip Navara
2009-06-17 12:12           ` Gleb Natapov
2009-06-17 13:03             ` Filip Navara [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5b31733c0906170603k34de010fu1f01e9cda1277c8f@mail.gmail.com \
    --to=filip.navara@gmail.com \
    --cc=gleb@redhat.com \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).