Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On 2012-09-05 06:33, Matthew Ogilvie wrote:
According to later discussion, the main issue is actually the input
IRQ behavior on a high to low transition; hence the following fixes
both the test program and the Microport UNIX problem:
diff --git a/hw/i8259.c b/hw/i8259.c
index 6587666..c011787 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -143,22 +143,23 @@ static void pic_set_irq(void *opaque, int irq, int
level)
if (s-elcr mask) {
/* level triggered */
if (level) {
s-irr |= mask;
s-last_irr |= mask;
} else {
s-irr = ~mask;
s-last_irr = ~mask;
}
} else {
/* edge triggered */
if (level) {
if ((s-last_irr mask) == 0) {
s-irr |= mask;
}
s-last_irr |= mask;
} else {
+s-irr = ~mask;
s-last_irr = ~mask;
}
}
pic_update_irq(s);
}
Perhaps it would be worth it to swap around the ifs a little bit
to avoid the (!level) duplication, and clarify that the only difference
is in the low to high transition?
Yes, refactoring would be welcome. But I would suggest to do this in two
patches, leaving the above change (+ changelog that explains the reason)
in its current clarity.
Hurray, no vmstate subsections needed! :)
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On 09/03/2012 07:33 PM, Paolo Bonzini wrote: Il 03/09/2012 18:30, Avi Kivity ha scritto: The values above are what every user of the PIC cascaded on our targets must program to use them. So We will find them in the state once any relevant guest code was able to run (e.g. the BIOS). Suppose the bios has not run yet? Then you transmit the subsection. And the migration fails. Needlessly, since icw3 == 0 doesn't affect guest operation. But the point of subsections is to succeed migration in the common case, assuming there is more than one case that doesn't affect guest operation. According to the patch, if icw3 == 4 !(eclr 4), then behaviour will change. With the standard configuration, if two pci interrupts hit at once, then before the patch irr.2 will be clear, and afterwards set. So we do have a behavioural change. Is the rest of the code masking this change under the standard configuration? -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
Il 04/09/2012 10:16, Avi Kivity ha scritto: But the point of subsections is to succeed migration in the common case, assuming there is more than one case that doesn't affect guest operation. According to the patch, if icw3 == 4 !(eclr 4), then behaviour will change. With the standard configuration, if two pci interrupts hit at once, then before the patch irr.2 will be clear, and afterwards set. So we do have a behavioural change. Is the rest of the code masking this change under the standard configuration? No, it is not masking the change. The assumption is that nothing should care about irr.2 or isr.2, because nothing attaches an handler to the cascade interrupt. You have to choose between assuming this, and breaking backwards migration. I would rather break backwards migration, but others disagree... Paolo
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On 09/04/2012 12:15 PM, Paolo Bonzini wrote: Il 04/09/2012 10:16, Avi Kivity ha scritto: But the point of subsections is to succeed migration in the common case, assuming there is more than one case that doesn't affect guest operation. According to the patch, if icw3 == 4 !(eclr 4), then behaviour will change. With the standard configuration, if two pci interrupts hit at once, then before the patch irr.2 will be clear, and afterwards set. So we do have a behavioural change. Is the rest of the code masking this change under the standard configuration? No, it is not masking the change. The assumption is that nothing should care about irr.2 or isr.2, because nothing attaches an handler to the cascade interrupt. Won't the next call to pic_get_irq() notice the difference in s-irr? You have to choose between assuming this, and breaking backwards migration. I would rather break backwards migration, but others disagree... Normally I'd agree, but if the only known breakee is a 1987 guest then I'd make an exception. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On Tue, 4 Sep 2012, Avi Kivity wrote: On 09/04/2012 12:15 PM, Paolo Bonzini wrote: Il 04/09/2012 10:16, Avi Kivity ha scritto: But the point of subsections is to succeed migration in the common case, assuming there is more than one case that doesn't affect guest operation. According to the patch, if icw3 == 4 !(eclr 4), then behaviour will change. With the standard configuration, if two pci interrupts hit at once, then before the patch irr.2 will be clear, and afterwards set. So we do have a behavioural change. Is the rest of the code masking this change under the standard configuration? No, it is not masking the change. The assumption is that nothing should care about irr.2 or isr.2, because nothing attaches an handler to the cascade interrupt. Won't the next call to pic_get_irq() notice the difference in s-irr? You have to choose between assuming this, and breaking backwards migration. I would rather break backwards migration, but others disagree... Normally I'd agree, but if the only known breakee is a 1987 guest then I'd make an exception. Another one affected by this is OpenStep 4.2 (probably NeXTstep and Rhapsody too) which are not exactly recent either but there are more than only one breakee. Regards, BALATON Zoltan
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On 09/04/2012 12:29 PM, BALATON Zoltan wrote: On Tue, 4 Sep 2012, Avi Kivity wrote: On 09/04/2012 12:15 PM, Paolo Bonzini wrote: Il 04/09/2012 10:16, Avi Kivity ha scritto: But the point of subsections is to succeed migration in the common case, assuming there is more than one case that doesn't affect guest operation. According to the patch, if icw3 == 4 !(eclr 4), then behaviour will change. With the standard configuration, if two pci interrupts hit at once, then before the patch irr.2 will be clear, and afterwards set. So we do have a behavioural change. Is the rest of the code masking this change under the standard configuration? No, it is not masking the change. The assumption is that nothing should care about irr.2 or isr.2, because nothing attaches an handler to the cascade interrupt. Won't the next call to pic_get_irq() notice the difference in s-irr? You have to choose between assuming this, and breaking backwards migration. I would rather break backwards migration, but others disagree... Normally I'd agree, but if the only known breakee is a 1987 guest then I'd make an exception. Another one affected by this is OpenStep 4.2 (probably NeXTstep and Rhapsody too) which are not exactly recent either but there are more than only one breakee. Those are all filed under esoterics. I don't mean to say we shouldn't care about them, but there are likely to be a lot more users doing backwards migration than users running those guests, let alone migrating them (forwards or backwards). The pragmatic choice is clear. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On 2012-09-04 11:37, Avi Kivity wrote: On 09/04/2012 12:29 PM, BALATON Zoltan wrote: On Tue, 4 Sep 2012, Avi Kivity wrote: On 09/04/2012 12:15 PM, Paolo Bonzini wrote: Il 04/09/2012 10:16, Avi Kivity ha scritto: But the point of subsections is to succeed migration in the common case, assuming there is more than one case that doesn't affect guest operation. According to the patch, if icw3 == 4 !(eclr 4), then behaviour will change. With the standard configuration, if two pci interrupts hit at once, then before the patch irr.2 will be clear, and afterwards set. So we do have a behavioural change. Is the rest of the code masking this change under the standard configuration? No, it is not masking the change. The assumption is that nothing should care about irr.2 or isr.2, because nothing attaches an handler to the cascade interrupt. Won't the next call to pic_get_irq() notice the difference in s-irr? You have to choose between assuming this, and breaking backwards migration. I would rather break backwards migration, but others disagree... Normally I'd agree, but if the only known breakee is a 1987 guest then I'd make an exception. Another one affected by this is OpenStep 4.2 (probably NeXTstep and Rhapsody too) which are not exactly recent either but there are more than only one breakee. Those are all filed under esoterics. I don't mean to say we shouldn't care about them, but there are likely to be a lot more users doing backwards migration than users running those guests, let alone migrating them (forwards or backwards). The pragmatic choice is clear. BTW, did anyone actually test backward migration recently? I thought to remember I effectively broke it in 1.1 with some changes to the i8259 (or was it the PIT?) vmstate, and no one really cared about this or my first proposals to fix it. Jan -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
Il 04/09/2012 11:51, Jan Kiszka ha scritto: I don't mean to say we shouldn't care about them, but there are likely to be a lot more users doing backwards migration than users running those guests, let alone migrating them (forwards or backwards). The pragmatic choice is clear. BTW, did anyone actually test backward migration recently? I thought to remember I effectively broke it in 1.1 with some changes to the i8259 (or was it the PIT?) vmstate, and no one really cared about this or my first proposals to fix it. Correct: commit ce967e2 (i8254: Rework fix interaction with HPET in legacy mode, 2012-02-01) bumped the PIT version from 2 to 3. RTC changes will break it more in 1.3. Honestly, backwards migration only works on enterprise qemu-kvm because it is tested only there. And so far the only sample across major releases is that RHEL6-RHEL5 migration didn't work. Here the choice is between changing guest behavior by defaulting to 4/2, and always transmitting the subsection by defaulting to 0/0. The latter makes the subsection useless, so at that point we might as well bump the version number and board said flight to the Pacific. Paolo
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On 09/04/2012 01:06 PM, Paolo Bonzini wrote: Il 04/09/2012 11:51, Jan Kiszka ha scritto: I don't mean to say we shouldn't care about them, but there are likely to be a lot more users doing backwards migration than users running those guests, let alone migrating them (forwards or backwards). The pragmatic choice is clear. BTW, did anyone actually test backward migration recently? I thought to remember I effectively broke it in 1.1 with some changes to the i8259 (or was it the PIT?) vmstate, and no one really cared about this or my first proposals to fix it. Correct: commit ce967e2 (i8254: Rework fix interaction with HPET in legacy mode, 2012-02-01) bumped the PIT version from 2 to 3. Gah, this is 1.1, so there's no way to fix it. RTC changes will break it more in 1.3. Honestly, backwards migration only works on enterprise qemu-kvm because it is tested only there. And so far the only sample across major releases is that RHEL6-RHEL5 migration didn't work. I expect that we will want 7-6, though we may have to settle for minor releases only. Here the choice is between changing guest behavior by defaulting to 4/2, and always transmitting the subsection by defaulting to 0/0. The latter makes the subsection useless, so at that point we might as well bump the version number and board said flight to the Pacific. So we do the 4/2 thing. I expect if we go they'll tell us they don't do backwards flights. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
Il 04/09/2012 16:29, Maciej W. Rozycki ha scritto:
So first of all, the *output* of the 8259A is always edge triggered,
regardless of whether it's the master or one of the slaves (only one slave
is used in the PC/AT architecture, but up to eight are supported; the
PC/XT had none).
I swear I read all your message :) but this seems to be the crux. It means
that something like this ought to fix the bug too. Matthew, can you post
your code or test it?
diff --git a/hw/i8259.c b/hw/i8259.c
index 53daf78..3dc1dff 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -104,12 +104,11 @@ static void pic_update_irq(PICCommonState *s)
int irq;
irq = pic_get_irq(s);
+qemu_irq_lower(s-int_out[0]);
if (irq = 0) {
DPRINTF(pic%d: imr=%x irr=%x padd=%d\n,
s-master ? 0 : 1, s-imr, s-irr, s-priority_add);
qemu_irq_raise(s-int_out[0]);
-} else {
-qemu_irq_lower(s-int_out[0]);
}
}
The logic of the in-kernel 8259 is a bit different, but something
like this should do it, too:
diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 81cf4fa..feb6d5b 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -174,9 +174,11 @@ static void pic_update_irq(struct kvm_pic *s)
/*
* if irq request by slave pic, signal master PIC
*/
+ pic_set_irq1(s-pics[0], 2, 0);
pic_set_irq1(s-pics[0], 2, 1);
+ } else if (s-pics[0].irr (1 2))
pic_set_irq1(s-pics[0], 2, 0);
- }
+
irq = pic_get_irq(s-pics[0]);
pic_irq_request(s-kvm, irq = 0);
}
Both patches untested.
Paolo
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On 2012-09-04 16:42, Paolo Bonzini wrote:
Il 04/09/2012 16:29, Maciej W. Rozycki ha scritto:
So first of all, the *output* of the 8259A is always edge triggered,
regardless of whether it's the master or one of the slaves (only one slave
is used in the PC/AT architecture, but up to eight are supported; the
PC/XT had none).
I swear I read all your message :) but this seems to be the crux. It means
that something like this ought to fix the bug too. Matthew, can you post
your code or test it?
diff --git a/hw/i8259.c b/hw/i8259.c
index 53daf78..3dc1dff 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -104,12 +104,11 @@ static void pic_update_irq(PICCommonState *s)
int irq;
irq = pic_get_irq(s);
+qemu_irq_lower(s-int_out[0]);
if (irq = 0) {
DPRINTF(pic%d: imr=%x irr=%x padd=%d\n,
s-master ? 0 : 1, s-imr, s-irr, s-priority_add);
qemu_irq_raise(s-int_out[0]);
-} else {
-qemu_irq_lower(s-int_out[0]);
}
}
I don't think this can be correct in all scenario. E.g., we also call
pic_update_irq in case a level-triggered input was updated but didn't
change the output state of the PIC (high-high transition). With your
patch, the output will now generate edges.
What I'm trying to understand and translate from the description is
rather note that for inputs a high-to-low transition cancels the
interrupt as in the level-triggered mode. This is surely not what we do
right now. OTOH, I'm afraid that switching to this mode in the PIC can
cause problems elsewhere, with devices that actually inject short
low-high-low signals. Still wrapping my head around it...
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On 2012-09-04 19:41, Maciej W. Rozycki wrote: On Tue, 4 Sep 2012, Jan Kiszka wrote: What I'm trying to understand and translate from the description is rather note that for inputs a high-to-low transition cancels the interrupt as in the level-triggered mode. This is surely not what we do right now. OTOH, I'm afraid that switching to this mode in the PIC can cause problems elsewhere, with devices that actually inject short low-high-low signals. Still wrapping my head around it... That won't work reliably with true 8259A hardware -- for an Ok, then we have to scan our code base for such device models that won't survive with real 8259A hardware. That can only be devices attached to edge-only inputs of the PIC, namely the PIT, the keyboard controller, the RTC and FPU emulation. They basically need to generate high-low-high transitions on new events, instead of low-high-low (via qemu_irq_pulse e.g.). I'm I on the right track? Thanks, Jan edge-triggered interrupt to propagate up to the CPU first there must be a low-to-high transition and then the high logic state must be maintained up until the start of the second INTA cycle. If the interrupt request drops before then (e.g. because CPU interrupts have been masked or a higher-priority 8259A has been serviced), then the corresponding IRR bit is cleared and either the interrupt is missed altogether or, if the CPU has already accepted the interrupt and started the first INTA cycle, then the spurious vector is supplied and no ISR bit is set. To put it in different words: the only actual difference between edge-triggered and level-triggered interrupts in the 8259A is that the formers require a leading edge to record another interrupt. For both trigger modes the high level has to be maintained until the second INTA cycle for the interrupt to be correctly delivered to the CPU and also in both trigger modes a trailing edge cancels the interrupt. This is unlike the traditional edge-triggered mode where the level does not have to be maintained once a leading edge has been correctly recorded (there is usually spike filtering logic implemented on such IRQ inputs so appropriate timings have to be met; because of its unusual interpretation the 8259A obviously does not require such logic). The edge detector logic is also drawn in the 8259A datasheet (that for a change used to be available from one of the Intel sites in the PDF form) and I believe the functionality described can be inferred from that by the curious enough. ;) Maciej -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On 2012-09-04 20:27, Maciej W. Rozycki wrote: On Tue, 4 Sep 2012, Jan Kiszka wrote: What I'm trying to understand and translate from the description is rather note that for inputs a high-to-low transition cancels the interrupt as in the level-triggered mode. This is surely not what we do right now. OTOH, I'm afraid that switching to this mode in the PIC can cause problems elsewhere, with devices that actually inject short low-high-low signals. Still wrapping my head around it... That won't work reliably with true 8259A hardware -- for an Ok, then we have to scan our code base for such device models that won't survive with real 8259A hardware. That can only be devices attached to edge-only inputs of the PIC, namely the PIT, the keyboard controller, the RTC and FPU emulation. They basically need to generate high-low-high transitions on new events, instead of low-high-low (via qemu_irq_pulse e.g.). I'm I on the right track? They shouldn't be the problem unless we've got an implementation problem: * The 8254 PIT is normally configured in mode 2 or 3 in the PC/AT architecture. In the former its output is high (active) all the time except from one (last) clock cycle. In the latter a wave that has a duty cycle close or equal to 0.5 (depending on whether the divider is odd or even) is produced, so no short pulses either. I don't remember the other four modes -- have a look at the datasheet if interested, but I reckon they're not really compatible with the wiring anyway, e.g. the gate is hardwired enabled. * The 8042 keyboard controller requires an interrupt acknowledge (by reading its data port, normally at the address 0x60 in the PC/AT port I/O space) to clear its output, so no concern here either (this is BTW how many years ago I actually learnt the hard way how the edge-triggered mode works in the 8259A -- reading the KBC's data port in the main program a tight loop with the keyboard interrupt enabled will lead to a spurious interrupt each time data is supplied). I believe it's otherwise edge-triggered though (I don't have an 8042 datasheet handy, I would have to dig for a hardcopy that I left abroad). * The MC146818 RTC interrupt is level triggered and has to be acknowledged by reading Register C. * The i287/i387 FPU interrupt (in the PC/AT compatibility mode) is latched and requires the location at the address 0xf0 in the PC/AT port I/O space to be poked at to acknowledge. It can therefore be considered level-triggered (overall the design is busted and you have to be very careful not to freeze the system when handling the FPU error this way rather than via the FPU exception). Did I miss anything? Nope, and it looks like I was too pessimistic. I've checked PIT, 8042, and RTC, and they all set their output levels properly. So it should be safe to start clearing an IRQ on falling edges at a PIC input. Jan -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On Tue, 4 Sep 2012, Jan Kiszka wrote: What I'm trying to understand and translate from the description is rather note that for inputs a high-to-low transition cancels the interrupt as in the level-triggered mode. This is surely not what we do right now. OTOH, I'm afraid that switching to this mode in the PIC can cause problems elsewhere, with devices that actually inject short low-high-low signals. Still wrapping my head around it... That won't work reliably with true 8259A hardware -- for an edge-triggered interrupt to propagate up to the CPU first there must be a low-to-high transition and then the high logic state must be maintained up until the start of the second INTA cycle. If the interrupt request drops before then (e.g. because CPU interrupts have been masked or a higher-priority 8259A has been serviced), then the corresponding IRR bit is cleared and either the interrupt is missed altogether or, if the CPU has already accepted the interrupt and started the first INTA cycle, then the spurious vector is supplied and no ISR bit is set. To put it in different words: the only actual difference between edge-triggered and level-triggered interrupts in the 8259A is that the formers require a leading edge to record another interrupt. For both trigger modes the high level has to be maintained until the second INTA cycle for the interrupt to be correctly delivered to the CPU and also in both trigger modes a trailing edge cancels the interrupt. This is unlike the traditional edge-triggered mode where the level does not have to be maintained once a leading edge has been correctly recorded (there is usually spike filtering logic implemented on such IRQ inputs so appropriate timings have to be met; because of its unusual interpretation the 8259A obviously does not require such logic). The edge detector logic is also drawn in the 8259A datasheet (that for a change used to be available from one of the Intel sites in the PDF form) and I believe the functionality described can be inferred from that by the curious enough. ;) Maciej
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On Mon, 3 Sep 2012, Jan Kiszka wrote: - Qemu output (without this patch): elcr=0c00 cmdRead ummask mask sti irq15 unmask DONE But on real hardware, the master seems to treat IRQ2 as level triggered, That is not universally true, however in reality it does not matter, more on that below. and doesn't deliver an interrupt to the CPU until the slave unmasks IRQ14. I've tried this on several machines, including a non-PCI 486 that does not seem to have ELCR registers. - Typical output (pentium/pentium pro/dual AMD Athlon/etc; slight variations in some elcr bits depending on machine, but bit 0x04 is always clear): elcr=0c20 cmdRead unmask mask sti unmask irq14 DONE - 486 without elcr (just an ISA bus): elcr=e0e0 cmdRead unmask mask sti unmask irq14 DONE - One failure: It doesn't boot properly (no output) with a USB floppy drive on my Intel Core I7. Guess: The test program just barely fits in a sector, with no room for any tables (partition/etc) that BIOS might check for if it isn't an original, native floppy drive. --- I've found a few descriptions of programming the i8259. I was mostly following the official 8259A PROGRAMMABLE INTERRUPT CONTROLLER (8259A 8259A-2) by Intel. But it also doesn't mention any trigger mode changes for the cascading input lines. The datasheet is vague on the subject and there is apparently a common misconception on how the 8259A trigger modes work. PIC core modifications, especially the ELCR (originally added for EISA support), seen in chipsets obfuscate the matter further. So first of all, the *output* of the 8259A is always edge triggered, regardless of whether it's the master or one of the slaves (only one slave is used in the PC/AT architecture, but up to eight are supported; the PC/XT had none). If once an interrupt has been served any interrupts remain in the pending state, then the 8259A deasserts its output briefly before reasserting it. Secondly, for inputs the original 8259A only allowed a global (i.e. for all at once) edge/level selection via ICW1. That somehow had to be compatible with the master/slave mode. However as of the 8259A edge-triggered in Intel's nomenclature meant: asserted with a low-to-high transition at the time an interrupt is registered and then kept high until the interrupt is served via one of the EOI mechanisms (there are a few, including AEOI) or goes away unhandled. This is important for the output because the x86 architecture's definition for the INT input is level-triggered. The edge-triggered mode was also the only one supported for inputs with the original 8080-compatible 8259; note that for inputs a high-to-low transition cancels the interrupt as in the level-triggered mode. The level-triggered mode was then added with the usual meaning -- essentially copying the interrupt input onto the output, ORing with the other inputs. So thus defined edge-triggered mode is in fact compatible with both edge-triggered and level-triggered inputs -- the former work by means of observing the low-to-high transition and the latter also work by only seeing the interrupt go away briefly -- but that does not really matter for level-triggered interrupts as the 8259A still knows an IRQ is pending and if a race causes an INTA cycle to arrive at the time the PIC's output is low it will still correctly serve the vector (I think in practice it does not really happen as the vector is served with the second INTA cycle only and by then the interrupt output must have recovered). There were some interesting errata in some early embedded 8259A cores in EISA chipses where the brief deassertion of the output did not happen. That worked just fine when wired directly to an x86 processor (because as noted above the INT input is level-triggered), however when connected to an i82489DX discrete APIC, that interprets the edge-triggered mode in the traditional way and implies that mode for inputs cascaded to 8259A, the problem had to be worked around in hardware, by adding extra logic to regenerate the missing high-to-low-to-high transition. Some of the information presented here was only noted by Intel in APIC documentation; I may track down and quote references, but these documents have never been available online anyway. Did that help? Maciej
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On Tue, 4 Sep 2012, Jan Kiszka wrote: What I'm trying to understand and translate from the description is rather note that for inputs a high-to-low transition cancels the interrupt as in the level-triggered mode. This is surely not what we do right now. OTOH, I'm afraid that switching to this mode in the PIC can cause problems elsewhere, with devices that actually inject short low-high-low signals. Still wrapping my head around it... That won't work reliably with true 8259A hardware -- for an Ok, then we have to scan our code base for such device models that won't survive with real 8259A hardware. That can only be devices attached to edge-only inputs of the PIC, namely the PIT, the keyboard controller, the RTC and FPU emulation. They basically need to generate high-low-high transitions on new events, instead of low-high-low (via qemu_irq_pulse e.g.). I'm I on the right track? They shouldn't be the problem unless we've got an implementation problem: * The 8254 PIT is normally configured in mode 2 or 3 in the PC/AT architecture. In the former its output is high (active) all the time except from one (last) clock cycle. In the latter a wave that has a duty cycle close or equal to 0.5 (depending on whether the divider is odd or even) is produced, so no short pulses either. I don't remember the other four modes -- have a look at the datasheet if interested, but I reckon they're not really compatible with the wiring anyway, e.g. the gate is hardwired enabled. * The 8042 keyboard controller requires an interrupt acknowledge (by reading its data port, normally at the address 0x60 in the PC/AT port I/O space) to clear its output, so no concern here either (this is BTW how many years ago I actually learnt the hard way how the edge-triggered mode works in the 8259A -- reading the KBC's data port in the main program a tight loop with the keyboard interrupt enabled will lead to a spurious interrupt each time data is supplied). I believe it's otherwise edge-triggered though (I don't have an 8042 datasheet handy, I would have to dig for a hardcopy that I left abroad). * The MC146818 RTC interrupt is level triggered and has to be acknowledged by reading Register C. * The i287/i387 FPU interrupt (in the PC/AT compatibility mode) is latched and requires the location at the address 0xf0 in the PC/AT port I/O space to be poked at to acknowledge. It can therefore be considered level-triggered (overall the design is busted and you have to be very careful not to freeze the system when handling the FPU error this way rather than via the FPU exception). Did I miss anything? Maciej
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On Tue, Sep 04, 2012 at 04:42:35PM +0200, Paolo Bonzini wrote:
Il 04/09/2012 16:29, Maciej W. Rozycki ha scritto:
So first of all, the *output* of the 8259A is always edge triggered,
regardless of whether it's the master or one of the slaves (only one slave
is used in the PC/AT architecture, but up to eight are supported; the
PC/XT had none).
I swear I read all your message :) but this seems to be the crux. It means
that something like this ought to fix the bug too. Matthew, can you post
your code or test it?
The test program source can be downloaded from
http://home.comcast.net/~mmogilvi/downloads/i8259-imr-test-2012-09-02.tar.bz2
I intend to rewrite it for kvm-unit-tests as you suggested earlier, but
likely not before this weekend.
diff --git a/hw/i8259.c b/hw/i8259.c
index 53daf78..3dc1dff 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -104,12 +104,11 @@ static void pic_update_irq(PICCommonState *s)
int irq;
irq = pic_get_irq(s);
+qemu_irq_lower(s-int_out[0]);
if (irq = 0) {
DPRINTF(pic%d: imr=%x irr=%x padd=%d\n,
s-master ? 0 : 1, s-imr, s-irr, s-priority_add);
qemu_irq_raise(s-int_out[0]);
-} else {
-qemu_irq_lower(s-int_out[0]);
}
}
This doesn't work; the master still locks onto the original low to high
edge, and doesn't cancel it on the high to low edge. I haven't had a
chance to look into or test your (or any other) KVM patches yet,
although I'll probably get to it this weekend.
According to later discussion, the main issue is actually the input
IRQ behavior on a high to low transition; hence the following fixes
both the test program and the Microport UNIX problem:
diff --git a/hw/i8259.c b/hw/i8259.c
index 6587666..c011787 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -143,22 +143,23 @@ static void pic_set_irq(void *opaque, int irq, int level)
if (s-elcr mask) {
/* level triggered */
if (level) {
s-irr |= mask;
s-last_irr |= mask;
} else {
s-irr = ~mask;
s-last_irr = ~mask;
}
} else {
/* edge triggered */
if (level) {
if ((s-last_irr mask) == 0) {
s-irr |= mask;
}
s-last_irr |= mask;
} else {
+s-irr = ~mask;
s-last_irr = ~mask;
}
}
pic_update_irq(s);
}
Perhaps it would be worth it to swap around the ifs a little bit
to avoid the (!level) duplication, and clarify that the only difference
is in the low to high transition?
- Matthew Ogilvie
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
Il 03/09/2012 04:56, Matthew Ogilvie ha scritto: Although I haven't found any specs that say so, on real hardware I have a test program that shows if you mask off the slave interrupt (say IRQ14) in the IMR after it has already been raised, the master (IRQ2) gets canceled (that is, IRQ2 acts like it is level triggered). Without this patch, qemu delivers a spurious interrupt (IRQ15) instead when running the test program. Signed-off-by: Matthew Ogilvie mmogilvi_q...@miniinfo.net Nice testing, thanks! KVM's i8259 emulation can be patched at arch/x86/kvm/i8259.c in the Linux tree. You can write a test for kvm-unit-tests. These tests can be written in C, including interrupt handlers (see lib/x86/isr.h). The git tree is at git://git.kernel.org/pub/scm/virt/kvm/kvm-unit-tests.git. Paolo
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
Am 03.09.2012 04:56, schrieb Matthew Ogilvie:
diff --git a/hw/i8259_common.c b/hw/i8259_common.c
index ab3d98b..dcde5f2 100644
--- a/hw/i8259_common.c
+++ b/hw/i8259_common.c
[...]
@@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common = {
VMSTATE_UINT8(isr, PICCommonState),
VMSTATE_UINT8(priority_add, PICCommonState),
VMSTATE_UINT8(irq_base, PICCommonState),
+VMSTATE_UINT8(icw3, PICCommonState),
VMSTATE_UINT8(read_reg_select, PICCommonState),
VMSTATE_UINT8(poll, PICCommonState),
VMSTATE_UINT8(special_mask, PICCommonState),
Additional VMState needs to be versioned by incrementing .version_id and
by specifying the new version number here. Otherwise it breaks migration.
Regards,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On 2012-09-03 04:56, Matthew Ogilvie wrote: Although I haven't found any specs that say so, on real hardware I have a test program that shows if you mask off the slave interrupt (say IRQ14) in the IMR after it has already been raised, the master (IRQ2) gets canceled (that is, IRQ2 acts like it is level triggered). Without this patch, qemu delivers a spurious interrupt (IRQ15) instead when running the test program. Signed-off-by: Matthew Ogilvie mmogilvi_q...@miniinfo.net --- I've written a test program (in the form of a floppy disk boot sector) that demonstrates that qemu's emulation of IRQ2 propagation from the slave i8259 to the master does not work correctly when the CPU has interrupts disabled and it masks off the original interrupt (IRQ14) in the slave's IMR register. This was based on simplifying breakage observed when trying to run an old Microport UNIX system (ca 1987). Earlier I speculated that maybe the ELCR bit for IRQ2 was incorrect, but now I don't think changing the bit (from the target's perspective) would be a good idea. See below. Yes, you cannot set IRQ0..2 to level triggered mode on modern chipsets, look e.g. at the ICH9 spec: In edge mode, (bit[x] = 0), the interrupt is recognized by a low to high transition. In level mode (bit[x] = 1), the interrupt is recognized by a high level. The cascade channel, IRQ2, the heart beat timer (IRQ0), and the keyboard controller (IRQ1), cannot be put into level mode. So, fiddling with ELCR from guest perspective cannot be the solution. You can download the source code for the test program from http://home.comcast.net/~mmogilvi/downloads/i8259-imr-test-2012-09-02.tar.bz2 It can be compiled using a standard GNU i386 toolchain on Linux. As Paolo said, we are better off with a KVM unit test. That should be loadable via grub co. on real HW as well. The heart of the test program is: Initialization is also important to exclude leftovers of the BIOS. cli # i8259:imm: mask off everything except IRQ2 movb $0xfb,%al # master (only IRQ2 clear) outb %al,$0x21 movb $0xff,%al # slave outb %al,$0xa1 mov $.msgCmdRead,%ax call print call initIrqHandlers call scheduleIrq14 call .largeDelay # note: IRQ14 raised while this is waiting mov $.msgUnmask,%ax call print movb $0x3f,%al # unmask IRQ14 and IRQ15 outb %al,$0xa1 call .largeDelay # (probably not important) mov $.msgMask,%ax call print movb $0xff,%al # mask IRQ14 and IRQ15 again outb %al,$0xa1 And now check IRR of the master by reading it back. That would be a smoking gun if bit 2 is set before and cleared afterward. call .largeDelay # (probably not important) mov $.msgSti,%ax call print sti call .largeDelay # note: spurious interrupt (IRQ15) from qemu cli mov $.msgUnmask,%ax call print movb $0x3f,%al # unmask IRQ14 and IRQ15 outb %al,$0xa1 sti call .largeDelay # we should finally see IRQ14 here? # DONE: cli movw $.msgDone, %ax call print .Lhalt: hlt jmp .Lhalt In qemu, the master treats IRQ2 as edge triggered, and delivers a spurious interrupt if the CPU re-enables interrupts after the slave's IMR is masked off (it also doesn't seem to deliver the real interrupt when the IMR is unmasked, but maybe that is because I'm not correctly acknowledging the spurious interrupt). Yes, that is required. - Qemu output (without this patch): elcr=0c00 cmdRead ummask mask sti irq15 unmask DONE But on real hardware, the master seems to treat IRQ2 as level triggered, and doesn't deliver an interrupt to the CPU until the slave unmasks IRQ14. I've tried this on several machines, including a non-PCI 486 that does not seem to have ELCR registers. - Typical output (pentium/pentium pro/dual AMD Athlon/etc; slight variations in some elcr bits depending on machine, but bit 0x04 is always clear): elcr=0c20 cmdRead unmask mask sti unmask irq14 DONE - 486 without elcr (just an ISA bus): elcr=e0e0 cmdRead unmask mask sti unmask irq14 DONE - One failure: It doesn't boot properly (no output) with a USB floppy drive on my Intel Core I7. Guess: The test program just barely fits in a sector, with no room for any tables (partition/etc) that BIOS might check for if it isn't an original, native floppy drive. --- I've found a few descriptions of programming the i8259. I was mostly following the official 8259A PROGRAMMABLE INTERRUPT CONTROLLER (8259A 8259A-2) by Intel. But it also doesn't mention any trigger mode changes for the cascading input lines. The closest thing I've found to a detailed
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On 2012-09-03 10:51, Jan Kiszka wrote:
diff --git a/hw/i8259_common.c b/hw/i8259_common.c
index ab3d98b..dcde5f2 100644
--- a/hw/i8259_common.c
+++ b/hw/i8259_common.c
@@ -33,6 +33,7 @@ void pic_reset_common(PICCommonState *s)
s-isr = 0;
s-priority_add = 0;
s-irq_base = 0;
+s-icw3 = 0;
s-read_reg_select = 0;
s-poll = 0;
s-special_mask = 0;
@@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common = {
VMSTATE_UINT8(isr, PICCommonState),
VMSTATE_UINT8(priority_add, PICCommonState),
VMSTATE_UINT8(irq_base, PICCommonState),
+VMSTATE_UINT8(icw3, PICCommonState),
Let's add this as an optional subsection, only written when it's not
0x04 (for a master) or 0x2 (for a slave). See target-i386/machine.c for
examples, or read docs/migration.txt. That will mean you need to set
icw3 to 0x4 before loading a vmstate (= pre_load handler).
Oh, and this code affects also the kvm-pic. So you have to maintain icw3
for that model as well, setting it to the PC defaults. Please test KVM
after your changes, including migration.
Jan
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
Il 03/09/2012 10:51, Jan Kiszka ha scritto: The only thing that worries me is that we just consider the PC so far while the i8259 is also used on different architectures (PPC, MIPS, Alpha?). Why is this a problem? All of them use IRQ2 for a cascade, and initialize icw3 to 0x4/0x2 (I checked OpenBIOS, rth's palcode for Alpha, and Linux). BTW, from the palcode it looks like Alpha wants LTIM=1, so it would be nice to implement that one as well: /* ??? MILO initializes the PIC as edge triggered; I do not know how SRM initializes them. However, Linux seems to expect that these are level triggered. That may be a kernel bug, but level triggers are more reliable anyway so lets go with that. */ /* Initialize level triggers. The CY82C693UB that's on real alpha hardware doesn't have this; this is a PIIX extension. However, QEMU doesn't implement regular level triggers. */ outb(0xff, PORT_PIC2_ELCR); outb(0xff, PORT_PIC1_ELCR); Paolo
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On 2012-09-03 11:34, Paolo Bonzini wrote: Il 03/09/2012 10:51, Jan Kiszka ha scritto: The only thing that worries me is that we just consider the PC so far while the i8259 is also used on different architectures (PPC, MIPS, Alpha?). Why is this a problem? All of them use IRQ2 for a cascade, and initialize icw3 to 0x4/0x2 (I checked OpenBIOS, rth's palcode for Alpha, and Linux). IRQ2 is already hard-coded in QEMU (we had to patch this for our machine emulation, but less in recent versions), that is not the point. I'm concerned about the behavioral changes we are discussing here, ie. the special handling of cascading interrupt inputs. BTW, from the palcode it looks like Alpha wants LTIM=1, so it would be nice to implement that one as well: /* ??? MILO initializes the PIC as edge triggered; I do not know how SRM initializes them. However, Linux seems to expect that these are level triggered. That may be a kernel bug, but level triggers are more reliable anyway so lets go with that. */ /* Initialize level triggers. The CY82C693UB that's on real alpha hardware doesn't have this; this is a PIIX extension. However, QEMU doesn't implement regular level triggers. */ outb(0xff, PORT_PIC2_ELCR); outb(0xff, PORT_PIC1_ELCR); Just takes someone to write the patch, as usual. :) Jan -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
Il 03/09/2012 12:34, Jan Kiszka ha scritto: Why is this a problem? All of them use IRQ2 for a cascade, and initialize icw3 to 0x4/0x2 (I checked OpenBIOS, rth's palcode for Alpha, and Linux). IRQ2 is already hard-coded in QEMU (we had to patch this for our machine emulation, but less in recent versions), that is not the point. I'm concerned about the behavioral changes we are discussing here, ie. the special handling of cascading interrupt inputs. Yeah, it's quite interesting that the behavior isn't mentioned in the 8259 datasheets. Still in retrospect it's hard to see how it can possibly work with edge-triggered cascaded inputs. Paolo
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On 2012-09-03 13:11, Paolo Bonzini wrote: Il 03/09/2012 12:34, Jan Kiszka ha scritto: Why is this a problem? All of them use IRQ2 for a cascade, and initialize icw3 to 0x4/0x2 (I checked OpenBIOS, rth's palcode for Alpha, and Linux). IRQ2 is already hard-coded in QEMU (we had to patch this for our machine emulation, but less in recent versions), that is not the point. I'm concerned about the behavioral changes we are discussing here, ie. the special handling of cascading interrupt inputs. Yeah, it's quite interesting that the behavior isn't mentioned in the 8259 datasheets. Still in retrospect it's hard to see how it can possibly work with edge-triggered cascaded inputs. As I said: by avoiding the pattern Matthew generated in his test case. That's not impossible. All the other OSes running fine against out PIC models are proving this. Jan -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On 09/03/2012 11:40 AM, Andreas Färber wrote:
Am 03.09.2012 04:56, schrieb Matthew Ogilvie:
diff --git a/hw/i8259_common.c b/hw/i8259_common.c
index ab3d98b..dcde5f2 100644
--- a/hw/i8259_common.c
+++ b/hw/i8259_common.c
[...]
@@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common = {
VMSTATE_UINT8(isr, PICCommonState),
VMSTATE_UINT8(priority_add, PICCommonState),
VMSTATE_UINT8(irq_base, PICCommonState),
+VMSTATE_UINT8(icw3, PICCommonState),
VMSTATE_UINT8(read_reg_select, PICCommonState),
VMSTATE_UINT8(poll, PICCommonState),
VMSTATE_UINT8(special_mask, PICCommonState),
Additional VMState needs to be versioned by incrementing .version_id and
by specifying the new version number here. Otherwise it breaks migration.
And incrementing the version ID breaks backwards migration. The correct
solution is subsections, copying Juan and booking a trip to the Mariana
trench.
--
error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
Avi Kivity a...@redhat.com wrote:
On 09/03/2012 11:40 AM, Andreas Färber wrote:
Am 03.09.2012 04:56, schrieb Matthew Ogilvie:
diff --git a/hw/i8259_common.c b/hw/i8259_common.c
index ab3d98b..dcde5f2 100644
--- a/hw/i8259_common.c
+++ b/hw/i8259_common.c
[...]
@@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common = {
VMSTATE_UINT8(isr, PICCommonState),
VMSTATE_UINT8(priority_add, PICCommonState),
VMSTATE_UINT8(irq_base, PICCommonState),
+VMSTATE_UINT8(icw3, PICCommonState),
VMSTATE_UINT8(read_reg_select, PICCommonState),
VMSTATE_UINT8(poll, PICCommonState),
VMSTATE_UINT8(special_mask, PICCommonState),
Additional VMState needs to be versioned by incrementing .version_id and
by specifying the new version number here. Otherwise it breaks migration.
For the subsection, only sending this when icw3 != 0 is enough? I am
searching for a test about when we need to send it.
And incrementing the version ID breaks backwards migration. The correct
solution is subsections, copying Juan and booking a trip to the Mariana
trench.
Book also for me, no need for the return ticket.
Later, Juan.
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On 2012-09-03 17:42, Juan Quintela wrote:
Avi Kivity a...@redhat.com wrote:
On 09/03/2012 11:40 AM, Andreas Färber wrote:
Am 03.09.2012 04:56, schrieb Matthew Ogilvie:
diff --git a/hw/i8259_common.c b/hw/i8259_common.c
index ab3d98b..dcde5f2 100644
--- a/hw/i8259_common.c
+++ b/hw/i8259_common.c
[...]
@@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common = {
VMSTATE_UINT8(isr, PICCommonState),
VMSTATE_UINT8(priority_add, PICCommonState),
VMSTATE_UINT8(irq_base, PICCommonState),
+VMSTATE_UINT8(icw3, PICCommonState),
VMSTATE_UINT8(read_reg_select, PICCommonState),
VMSTATE_UINT8(poll, PICCommonState),
VMSTATE_UINT8(special_mask, PICCommonState),
Additional VMState needs to be versioned by incrementing .version_id and
by specifying the new version number here. Otherwise it breaks migration.
For the subsection, only sending this when icw3 != 0 is enough? I am
searching for a test about when we need to send it.
See my reply on this topic in the other branch of this thread.
And incrementing the version ID breaks backwards migration. The correct
solution is subsections, copying Juan and booking a trip to the Mariana
trench.
Book also for me, no need for the return ticket.
Hey, this scenario is rather harmless (famous last words...). ;)
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On 09/03/2012 06:42 PM, Juan Quintela wrote:
Avi Kivity a...@redhat.com wrote:
On 09/03/2012 11:40 AM, Andreas Färber wrote:
Am 03.09.2012 04:56, schrieb Matthew Ogilvie:
diff --git a/hw/i8259_common.c b/hw/i8259_common.c
index ab3d98b..dcde5f2 100644
--- a/hw/i8259_common.c
+++ b/hw/i8259_common.c
[...]
@@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common = {
VMSTATE_UINT8(isr, PICCommonState),
VMSTATE_UINT8(priority_add, PICCommonState),
VMSTATE_UINT8(irq_base, PICCommonState),
+VMSTATE_UINT8(icw3, PICCommonState),
VMSTATE_UINT8(read_reg_select, PICCommonState),
VMSTATE_UINT8(poll, PICCommonState),
VMSTATE_UINT8(special_mask, PICCommonState),
Additional VMState needs to be versioned by incrementing .version_id and
by specifying the new version number here. Otherwise it breaks migration.
For the subsection, only sending this when icw3 != 0 is enough? I am
searching for a test about when we need to send it.
Looks like the optimal condition is ((s-icw3 ~s-eclr) != 0) (i.e.
bit set in icw3 but clear in eclr).
--
error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On 2012-09-03 17:52, Avi Kivity wrote:
On 09/03/2012 06:42 PM, Juan Quintela wrote:
Avi Kivity a...@redhat.com wrote:
On 09/03/2012 11:40 AM, Andreas Färber wrote:
Am 03.09.2012 04:56, schrieb Matthew Ogilvie:
diff --git a/hw/i8259_common.c b/hw/i8259_common.c
index ab3d98b..dcde5f2 100644
--- a/hw/i8259_common.c
+++ b/hw/i8259_common.c
[...]
@@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common = {
VMSTATE_UINT8(isr, PICCommonState),
VMSTATE_UINT8(priority_add, PICCommonState),
VMSTATE_UINT8(irq_base, PICCommonState),
+VMSTATE_UINT8(icw3, PICCommonState),
VMSTATE_UINT8(read_reg_select, PICCommonState),
VMSTATE_UINT8(poll, PICCommonState),
VMSTATE_UINT8(special_mask, PICCommonState),
Additional VMState needs to be versioned by incrementing .version_id and
by specifying the new version number here. Otherwise it breaks migration.
For the subsection, only sending this when icw3 != 0 is enough? I am
searching for a test about when we need to send it.
Looks like the optimal condition is ((s-icw3 ~s-eclr) != 0) (i.e.
bit set in icw3 but clear in eclr).
The standard PC values are optimal: 4 for master, 2 for slave.
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On 09/03/2012 06:54 PM, Jan Kiszka wrote:
On 2012-09-03 17:52, Avi Kivity wrote:
On 09/03/2012 06:42 PM, Juan Quintela wrote:
Avi Kivity a...@redhat.com wrote:
On 09/03/2012 11:40 AM, Andreas Färber wrote:
Am 03.09.2012 04:56, schrieb Matthew Ogilvie:
diff --git a/hw/i8259_common.c b/hw/i8259_common.c
index ab3d98b..dcde5f2 100644
--- a/hw/i8259_common.c
+++ b/hw/i8259_common.c
[...]
@@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common =
{
VMSTATE_UINT8(isr, PICCommonState),
VMSTATE_UINT8(priority_add, PICCommonState),
VMSTATE_UINT8(irq_base, PICCommonState),
+VMSTATE_UINT8(icw3, PICCommonState),
VMSTATE_UINT8(read_reg_select, PICCommonState),
VMSTATE_UINT8(poll, PICCommonState),
VMSTATE_UINT8(special_mask, PICCommonState),
Additional VMState needs to be versioned by incrementing .version_id and
by specifying the new version number here. Otherwise it breaks migration.
For the subsection, only sending this when icw3 != 0 is enough? I am
searching for a test about when we need to send it.
Looks like the optimal condition is ((s-icw3 ~s-eclr) != 0) (i.e.
bit set in icw3 but clear in eclr).
The standard PC values are optimal: 4 for master, 2 for slave.
Can you explain why? I saw that icw3 is always ORed with eclr, so my
condition will catch exactly those cases where a change in behaviour
occurs, and no more.
--
error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On 2012-09-03 17:57, Avi Kivity wrote:
On 09/03/2012 06:54 PM, Jan Kiszka wrote:
On 2012-09-03 17:52, Avi Kivity wrote:
On 09/03/2012 06:42 PM, Juan Quintela wrote:
Avi Kivity a...@redhat.com wrote:
On 09/03/2012 11:40 AM, Andreas Färber wrote:
Am 03.09.2012 04:56, schrieb Matthew Ogilvie:
diff --git a/hw/i8259_common.c b/hw/i8259_common.c
index ab3d98b..dcde5f2 100644
--- a/hw/i8259_common.c
+++ b/hw/i8259_common.c
[...]
@@ -111,6 +112,7 @@ static const VMStateDescription vmstate_pic_common
= {
VMSTATE_UINT8(isr, PICCommonState),
VMSTATE_UINT8(priority_add, PICCommonState),
VMSTATE_UINT8(irq_base, PICCommonState),
+VMSTATE_UINT8(icw3, PICCommonState),
VMSTATE_UINT8(read_reg_select, PICCommonState),
VMSTATE_UINT8(poll, PICCommonState),
VMSTATE_UINT8(special_mask, PICCommonState),
Additional VMState needs to be versioned by incrementing .version_id and
by specifying the new version number here. Otherwise it breaks migration.
For the subsection, only sending this when icw3 != 0 is enough? I am
searching for a test about when we need to send it.
Looks like the optimal condition is ((s-icw3 ~s-eclr) != 0) (i.e.
bit set in icw3 but clear in eclr).
The standard PC values are optimal: 4 for master, 2 for slave.
Can you explain why? I saw that icw3 is always ORed with eclr, so my
condition will catch exactly those cases where a change in behaviour
occurs, and no more.
The values above are what every user of the PIC cascaded on our targets
must program to use them. So We will find them in the state once any
relevant guest code was able to run (e.g. the BIOS).
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On 09/03/2012 07:02 PM, Jan Kiszka wrote: Looks like the optimal condition is ((s-icw3 ~s-eclr) != 0) (i.e. bit set in icw3 but clear in eclr). The standard PC values are optimal: 4 for master, 2 for slave. Can you explain why? I saw that icw3 is always ORed with eclr, so my condition will catch exactly those cases where a change in behaviour occurs, and no more. The values above are what every user of the PIC cascaded on our targets must program to use them. So We will find them in the state once any relevant guest code was able to run (e.g. the BIOS). Suppose the bios has not run yet? -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
Il 03/09/2012 18:15, Avi Kivity ha scritto:
The values above are what every user of the PIC cascaded on our targets
must program to use them. So We will find them in the state once any
relevant guest code was able to run (e.g. the BIOS).
Suppose the bios has not run yet?
Then you transmit the subsection.
BTW this also means that simply checking against eclr|icw3 is wrong; the
right test is:
* against elcr if !s-master
* against elcr|icw3 if s-master
This makes precomputing the value more appealing.
Similarly, perhaps this:
if (s-special_fully_nested_mode s-master) {
mask = ~(1 2);
}
should be changed to
if (s-special_fully_nested_mode s-master) {
mask = ~s-icw3;
}
?
Paolo
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On 2012-09-03 18:15, Avi Kivity wrote: On 09/03/2012 07:02 PM, Jan Kiszka wrote: Looks like the optimal condition is ((s-icw3 ~s-eclr) != 0) (i.e. bit set in icw3 but clear in eclr). The standard PC values are optimal: 4 for master, 2 for slave. Can you explain why? I saw that icw3 is always ORed with eclr, so my condition will catch exactly those cases where a change in behaviour occurs, and no more. The values above are what every user of the PIC cascaded on our targets must program to use them. So We will find them in the state once any relevant guest code was able to run (e.g. the BIOS). Suppose the bios has not run yet? Then we must save the 0. Your logic will force us to save in all standard cases (ELCR's bit for IRQ2 must not be set by a guest). So it's not really helpful. Jan -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On 09/03/2012 07:23 PM, Paolo Bonzini wrote: Il 03/09/2012 18:15, Avi Kivity ha scritto: The values above are what every user of the PIC cascaded on our targets must program to use them. So We will find them in the state once any relevant guest code was able to run (e.g. the BIOS). Suppose the bios has not run yet? Then you transmit the subsection. And the migration fails. Needlessly, since icw3 == 0 doesn't affect guest operation. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
Il 03/09/2012 18:30, Avi Kivity ha scritto: The values above are what every user of the PIC cascaded on our targets must program to use them. So We will find them in the state once any relevant guest code was able to run (e.g. the BIOS). Suppose the bios has not run yet? Then you transmit the subsection. And the migration fails. Needlessly, since icw3 == 0 doesn't affect guest operation. But the point of subsections is to succeed migration in the common case, assuming there is more than one case that doesn't affect guest operation. Paolo
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
On 2012-09-03 18:33, Paolo Bonzini wrote: Il 03/09/2012 18:30, Avi Kivity ha scritto: The values above are what every user of the PIC cascaded on our targets must program to use them. So We will find them in the state once any relevant guest code was able to run (e.g. the BIOS). Suppose the bios has not run yet? Then you transmit the subsection. And the migration fails. Needlessly, since icw3 == 0 doesn't affect guest operation. But the point of subsections is to succeed migration in the common case, assuming there is more than one case that doesn't affect guest operation. The point is that the common case is icw3 = 4 (for the master), not 0. And if we don't save that case, we must save the rest. If we don't save this as well, we lose state information. This can't work. Jan -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register
Il 03/09/2012 18:40, Jan Kiszka ha scritto: And the migration fails. Needlessly, since icw3 == 0 doesn't affect guest operation. But the point of subsections is to succeed migration in the common case, assuming there is more than one case that doesn't affect guest operation. The point is that the common case is icw3 = 4 (for the master), not 0. And if we don't save that case, we must save the rest. If we don't save this as well, we lose state information. This can't work. I was agreeing with you, not Avi. :) Paolo
