Re: [Qemu-devel] [PATCH 2/2] char: i.MX: Add support for "TX complete" interrupt
Of all the gin joints in all the towns in all the world, Andrey Smirnov had to
walk into mine at 12:11 on Thursday 15 March 2018 and say:
> Add support for "TX complete"/TXDC interrupt generate by real HW since
> it is needed to support guests other than Linux.
>
> Based on the patch by Bill Paul as found here:
> https://bugs.launchpad.net/qemu/+bug/1753314
>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-...@nongnu.org
> Cc: Bill Paul <wp...@windriver.com>
> Cc: Peter Maydell <peter.mayd...@linaro.org>
> Signed-off-by: Andrey Smirnov <andrew.smir...@gmail.com>
> ---
>
> Bill:
>
> I only tested this with i.MX7/Linux guest combo and am hoping you can
> take this and previous patch and give it a try on your VxWorks setup.
I tried it and it seems to work as well as my original patch did. There is one
thing I noticed, which is that when printing a lot of output to the console,
sometimes the output will stall until you press a key to generate an input
interrupt, and then it resumes. This happens with my original patch too. My
suspicion is that this has to do with the lack of emulation of the FIFO
feature of the i.MX6 UART, but I'm not certain. (It's still much better than
not having it work at all, so I didn't really mind this. :) ) All I know for
sure is that the stalls don't happen on real hardware.
FYI, I uploaded a sample VxWorks image that you can use for testing. You can
find it here:
http://people.freebsd.org/~wpaul/qemu/sabrelite
The file vxWorks is the kernel, which is really all you need. This is an SMP
image so you need to run QEMU with the -smp 4 option. The uVxWorks image and
.dtb file are used on a real board with U-Boot and the bootm command. The
qemu_imx.sh script contains the options I use for testing. I usually do:
% qemu_imx.sh vxWorks
You can download all the files at once by grabbing:
http://people.freebsd.org/~wpaul/qemu/sabrelite/vxworks_test.zip
At the -> prompt, you can type the following things (among others):
-> i -- show running task info
-> vxbIoctlShow -- show VxBus device tree
-> vxbDevShow -- show VxBus device tree in a different format
-> vxbDrvShow -- show VxBus drivers
-> vxbIntShow -- show information about interrupt bindings
-> vmContextShow -- show MMU mappings
-> memShow -- show heap usage
-> ifconfig -- show network interfaces
-> routec "show" -- show network routing table
-> netstat "-a" -- show network socket info
The image also includes network support. You can use ifconfig to set the enet0
interface's addresss like this:
-> ifconfig "enet0 10.0.2.15 netmask 255.255.255.0 up"
You should then be able to telnet to the VxWorks image from the host machine
by doing:
% telnet localhost 10023
To exit the telnet session, type logout:
-> logout
NOTE: this only works if you've patched the imx_fec module to fix the
interrupt vector bug that I also reported.
If you run vxbIoctlShow, which generates a lot of output, you should be able
to see the stall condition I'm talking about. If you don't, then maybe it has
something to do with me running QEMU on FreeBSD.
> Peter:
>
> Bill is the author of original code, so I am not sure how to handle
> Signed-off-by from him. I'd like to add it to this patch, but since
> his original submission to launchpad didn't have one it is currently
> omitted.
My original goal was to report the bug and provide as much info as I could on
how to maybe fix it, and let somebody else make a proper fix/patch.
-Bill
> include/hw/char/imx_serial.h | 3 +++
> hw/char/imx_serial.c | 20 +---
> 2 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/char/imx_serial.h b/include/hw/char/imx_serial.h
> index baeec3183f..5b99cee7cf 100644
> --- a/include/hw/char/imx_serial.h
> +++ b/include/hw/char/imx_serial.h
> @@ -67,6 +67,8 @@
> #define UCR2_RXEN (1<<1)/* Receiver enable */
> #define UCR2_SRST (1<<0)/* Reset complete */
>
> +#define UCR4_TCEN BIT(3)/* TX complete interrupt enable */
> +
> #define UTS1_TXEMPTY(1<<6)
> #define UTS1_RXEMPTY(1<<5)
> #define UTS1_TXFULL (1<<4)
> @@ -95,6 +97,7 @@ typedef struct IMXSerialState {
> uint32_t ubmr;
> uint32_t ubrc;
> uint32_t ucr3;
> +uint32_t ucr4;
>
> qemu_irq irq;
> CharBackend chr;
> diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c
> index d1e8586280..1e5540472b 100644
> --- a/hw/char/imx_serial.c
> +++ b/hw/char/imx_serial.c
> @@ -37,8 +37,8 @@
>
> static const VMStateDescription vmstate_imx_serial = {
> .name = TYPE_IMX_SERIAL,
> -.version_id = 1,
> -.minimum_version_id = 1,
> +.version_id = 2,
> +.minimum_version_id = 2,
Re: [Qemu-devel] [PATCH 2/2] char: i.MX: Add support for "TX complete" interrupt
Of all the gin joints in all the towns in all the world, Bill Paul had to walk into mine at 13:45 on Thursday 15 March 2018 and say: > Of all the gin joints in all the towns in all the world, Andrey Smirnov had > to > > walk into mine at 12:11 on Thursday 15 March 2018 and say: > > Add support for "TX complete"/TXDC interrupt generate by real HW since > > it is needed to support guests other than Linux. > > > > Based on the patch by Bill Paul as found here: > > https://bugs.launchpad.net/qemu/+bug/1753314 > > > > Cc: qemu-devel@nongnu.org > > Cc: qemu-...@nongnu.org > > Cc: Bill Paul <wp...@windriver.com> > > Cc: Peter Maydell <peter.mayd...@linaro.org> > > Signed-off-by: Andrey Smirnov <andrew.smir...@gmail.com> > > --- > > > > Bill: > > > > I only tested this with i.MX7/Linux guest combo and am hoping you can > > take this and previous patch and give it a try on your VxWorks setup. > > I tried it and it seems to work as well as my original patch did. There is > one thing I noticed, which is that when printing a lot of output to the > console, sometimes the output will stall until you press a key to generate > an input interrupt, and then it resumes. This happens with my original > patch too. My suspicion is that this has to do with the lack of emulation > of the FIFO feature of the i.MX6 UART, but I'm not certain. (It's still > much better than not having it work at all, so I didn't really mind this. > :) ) All I know for sure is that the stalls don't happen on real hardware. In retrospect, I think the problem may not be with the UART emulation. I went back and created a uniprocessor VxWorks image instead of an SMP image, and it doesn't seem to have the same stall behavior. I added a vxWorks_up image to the URL below so that you can try that too. It may well be that having more than a 1 byte FIFO would fix the problem with the SMP image too, but I don't want to force you to implement that. If you want to try to fix the issue, great, otherwise I am happy with the patch as it is, so I will provide this: Signed-off-by: Bill Paul <wp...@windriver.com> -Bill > > FYI, I uploaded a sample VxWorks image that you can use for testing. You > can find it here: > > http://people.freebsd.org/~wpaul/qemu/sabrelite > > The file vxWorks is the kernel, which is really all you need. This is an > SMP image so you need to run QEMU with the -smp 4 option. The uVxWorks > image and .dtb file are used on a real board with U-Boot and the bootm > command. The qemu_imx.sh script contains the options I use for testing. I > usually do: > > % qemu_imx.sh vxWorks > > You can download all the files at once by grabbing: > > http://people.freebsd.org/~wpaul/qemu/sabrelite/vxworks_test.zip > > At the -> prompt, you can type the following things (among others): > > -> i -- show running task info > -> vxbIoctlShow -- show VxBus device tree > -> vxbDevShow -- show VxBus device tree in a different format > -> vxbDrvShow -- show VxBus drivers > -> vxbIntShow -- show information about interrupt bindings > -> vmContextShow -- show MMU mappings > -> memShow -- show heap usage > -> ifconfig -- show network interfaces > -> routec "show" -- show network routing table > -> netstat "-a" -- show network socket info > > The image also includes network support. You can use ifconfig to set the > enet0 interface's addresss like this: > > -> ifconfig "enet0 10.0.2.15 netmask 255.255.255.0 up" > > You should then be able to telnet to the VxWorks image from the host > machine by doing: > > % telnet localhost 10023 > > To exit the telnet session, type logout: > > -> logout > > NOTE: this only works if you've patched the imx_fec module to fix the > interrupt vector bug that I also reported. > > If you run vxbIoctlShow, which generates a lot of output, you should be > able to see the stall condition I'm talking about. If you don't, then > maybe it has something to do with me running QEMU on FreeBSD. > > > Peter: > > > > Bill is the author of original code, so I am not sure how to handle > > Signed-off-by from him. I'd like to add it to this patch, but since > > his original submission to launchpad didn't have one it is currently > > omitted. > > My original goal was to report the bug and provide as much info as I could > on how to maybe fix it, and let somebody else make a proper fix/patch. > > -Bill > > > include/hw/char/imx_serial.h | 3 +++ > > hw/char/imx_serial.c | 20 +--- > > 2 files changed, 20 insertions(+), 3 deletions(-)
[Qemu-devel] [Bug 1753314] Re: UART in sabrelite machine simulation doesn't work with VxWorks 7
As I said before: "I'm not submitting this as a patch to the development list as I'm not fully certain it complies with the hardware spec and doesn't break any other functionality." What I'm trying to say here is that while I may have been able to cobble together a hack to make the UART nominally compatible with VxWorks, I do not understand the hardware or QEMU well enough to really fix this the right way. Even with my hack, every once in a while when printing lots of data on the console, the output from the UART will stall unless I press a key on the serial port, and I don't know why it does that. I did try to investigate it a little but wasn't able to make much progress. (My suspicion is that it has something to do with the fact that the imx_serial module doesn't implement FIFO support, but even if that's true I don't know how to fix it.) Even so, I figured it was still worth it to attach my changes to the bug report so that somebody who is better at this than me could use it as a starting point, and so that anybody else who might want to experiment with VxWorks using the QEMU sabrelite machine model wouldn't be totally stuck like I was. In short, the changes I made are good enough for my own needs (the output stalls don't happen often enough to really bother me), but they're not a fully debugged fix. That's why I filed a bug report instead of just submitting a patch in the first place: I wanted somebody sexier than me to create a fully debugged fix. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1753314 Title: UART in sabrelite machine simulation doesn't work with VxWorks 7 Status in QEMU: New Bug description: The imx_serial.c driver currently implements only partial support for the i.MX6 UART hardware. (I understand it's a work in progress and that's fine.) dIn particular, it does not implement support for the Transmit Complete Interrupt Enable bit in the UCR4 register. The VxWorks 7 i.MX6 serial driver depends on the behavior of this bit in actual hardware in order to send characters through the UART correctly. The result is that with the current machine model, VxWorks will boot and run in QEMU but it's unable to print any characters to the console serial port. I have produced a small patch for the imx_serial.c module to make it nominally functional with VxWorks 7. It works well enough to allow the boot banner to appear and for the user to interact with the target shell. I'm not submitting this as a patch to the development list as I'm not fully certain it complies with the hardware spec and doesn't break any other functionality. I would prefer if the maintainer (or someone) reviewed it for any issues/refinements first. I'm attaching the patch to this bug report. A copy can also be obtained from: http://people.freebsd.org/~wpaul/qemu/imx_serial.zip This patch was generated against QEMU 2.11.0 but also works with QEMU 2.11.1. My host environment is FreeBSD/amd64 11.1-RELEASE with QEMU 2.11.0/2.11.11 built from source. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1753314/+subscriptions
Re: [Qemu-devel] [PATCH] fsl-imx6: Swap Ethernet interrupt defines
Of all the gin joints in all the towns in all the world, Guenter Roeck had to
walk into mine at 13:38 on Friday 09 March 2018 and say:
[...]
> > > > Do I have that right?
> > >
> > > Pretty much.
> >
> > There may be a 4th option.
> >
> > Since older kernels work because they were looking at vector 151, you
> > could patch the imx_fec.c module so that when it signals an event for
> > vector 150, it also signals vector 151 too. This would keep newer
> > versions of QEMU "bug- compatible" with older versions while also fixing
> > support for newer Linux kernels and other guests (e.g. VxWorks) that
> > follow the hardware spec correctly.
> >
> > I think this would require only a small modification to this function:
> >
> > static void imx_eth_update(IMXFECState *s)
> > {
> >
> > if (s->regs[ENET_EIR] & s->regs[ENET_EIMR] & ENET_INT_TS_TIMER) {
> >
> > qemu_set_irq(s->irq[1], 1);
> >
> > } else {
> >
> > qemu_set_irq(s->irq[1], 0);
> >
> > }
> >
> > if (s->regs[ENET_EIR] & s->regs[ENET_EIMR] & ENET_INT_MAC) {
> >
> > qemu_set_irq(s->irq[0], 1);
> >
> > } else {
> >
> > qemu_set_irq(s->irq[0], 0);
> >
> > }
> >
> > }
> >
> > (For ENET_INT_MAC events, just signal both s->irq[0] and s->irq[1]).
>
> Now this is an excellent idea.
>
> > This of course means signaling spurious events on vector 151, but you're
> > doing that now anyway. :)
>
> Actually, it doesn't. It looks like the first interrupt is handled,
> resetting the interrupt status, and the second interrupt is never even
> executed. I tested this with all kernel releases back to v3.16.
I just did a quick test with your patch and I can confirm that VxWorks 7 also
works correctly.
-Bill
> I'll send out patch v2 with this change and let Peter decide which version
> to apply.
>
> Thanks,
> Guenter
--
=
-Bill Paul(510) 749-2329 | Senior Member of Technical Staff,
wp...@windriver.com | Master of Unix-Fu - Wind River Systems
=
"I put a dollar in a change machine. Nothing changed." - George Carlin
=
Re: [Qemu-devel] [PATCH] fsl-imx6: Swap Ethernet interrupt defines
Of all the gin joints in all the towns in all the world, Bill Paul had to walk
into mine at 10:53 on Friday 09 March 2018 and say:
> Of all the gin joints in all the towns in all the world, Guenter Roeck had
> to
>
> walk into mine at 10:20 on Friday 09 March 2018 and say:
> > On Fri, Mar 09, 2018 at 05:47:16PM +, Peter Maydell wrote:
> > > On 8 March 2018 at 18:28, Bill Paul <wp...@windriver.com> wrote:
> > > > Anyway, this means that the only reason older Linux kernels worked in
> > > > QEMU with the broken interrupt configuration is that they also
> > > > registered a handler on vector 151 (119). Even though QEMU could not
> > > > send events via GPIO6, it was mistakenly sending them via vector 151,
> > > > so it looked like things worked. On real hardware, the older kernels
> > > > would have gotten their interrupts via GPIO6 and also worked. The
> > > > older kernels would _not_ work if you fix QEMU because now they would
> > > > never get interrupts on either vector, unless you fudge things so
> > > > that the ENET module triggers both vector 150 and the vector for
> > > > GPIO6 in the GIC or patch them to back out the erratum 6678
> > > > workaround as later kernels do.
> > >
> > > Thanks for that really useful writeup. So if I understand correctly
> > >
> > > we have several choices here:
> > > (1) we could implement a model of the IOMUX block that is at least
> > > sufficient to support guests that configure it to route the ENET
> > > interrupt line to a GPIO pin. Then we could apply this patch that
> > > fixes the ENET line definitions. Old kernels would continue to work
> > > (for the same reason they worked on hardware), and new ones would
> > > work now too. This is in some ways the preferred option, but it's
> > > possibly a lot of code and we're nearly in freeze for 2.12.
> > >
> > > (2) we could leave everything as it is for 2.12. This would mean that
> > > at least we don't regress setups that used to work on older QEMU
> > > versions. Downside is that we wouldn't be able to run Linux v4.15+, or
> > > other guest OSes that don't have the bug that older Linux kernels do.
> > > (Presumably we'd only do this on the understanding that we were going
> > > to go down route (1) for 2.13.)
> > >
> > > (3) we could apply this patch for 2.12. Linux v4.15+ now works, as
> > > do other guest OSes that use the ENET interrupt. v4.1 and older Linux
> > > guests that used to boot in QEMU stop doing so, and 4.2-4.9 boot but
> > > lose the ethernet device support. Perhaps for 2.13 we might
> > > take route (1) to make those older guests start working again.
> > >
> > > Do I have that right?
> >
> > Pretty much.
>
> There may be a 4th option.
>
> Since older kernels work because they were looking at vector 151, you could
> patch the imx_fec.c module so that when it signals an event for vector 150,
> it also signals vector 151 too. This would keep newer versions of QEMU
> "bug- compatible" with older versions while also fixing support for newer
> Linux kernels and other guests (e.g. VxWorks) that follow the hardware
> spec correctly.
Oops, just to be clear: I mean that the vector definitions should be fixed and
this backward-compatibility change should be applied at the same time.
-Bill
> I think this would require only a small modification to this function:
>
> static void imx_eth_update(IMXFECState *s)
> {
> if (s->regs[ENET_EIR] & s->regs[ENET_EIMR] & ENET_INT_TS_TIMER) {
> qemu_set_irq(s->irq[1], 1);
> } else {
> qemu_set_irq(s->irq[1], 0);
> }
>
> if (s->regs[ENET_EIR] & s->regs[ENET_EIMR] & ENET_INT_MAC) {
> qemu_set_irq(s->irq[0], 1);
> } else {
> qemu_set_irq(s->irq[0], 0);
> }
> }
>
> (For ENET_INT_MAC events, just signal both s->irq[0] and s->irq[1]).
>
> This of course means signaling spurious events on vector 151, but you're
> doing that now anyway. :)
>
> This is much less work than implementing an IOMUX module. Creating an IOMUX
> module for QEMU just for this one fixup would probably be the most elegant
> solution, but adding IOMUX support to QEMU also doesn't make much sense
> since QEMU has no actual pins.
>
> -Bill
>
> > > None of these options seems especially palatable to me, so we're
> > > choosing the lesser evil, I think... (unless somebody wants to say
> > > that option (1) would be 2
Re: [Qemu-devel] [PATCH] fsl-imx6: Swap Ethernet interrupt defines
Of all the gin joints in all the towns in all the world, Guenter Roeck had to
walk into mine at 10:20 on Friday 09 March 2018 and say:
> On Fri, Mar 09, 2018 at 05:47:16PM +, Peter Maydell wrote:
> > On 8 March 2018 at 18:28, Bill Paul <wp...@windriver.com> wrote:
> > > Anyway, this means that the only reason older Linux kernels worked in
> > > QEMU with the broken interrupt configuration is that they also
> > > registered a handler on vector 151 (119). Even though QEMU could not
> > > send events via GPIO6, it was mistakenly sending them via vector 151,
> > > so it looked like things worked. On real hardware, the older kernels
> > > would have gotten their interrupts via GPIO6 and also worked. The
> > > older kernels would _not_ work if you fix QEMU because now they would
> > > never get interrupts on either vector, unless you fudge things so that
> > > the ENET module triggers both vector 150 and the vector for GPIO6 in
> > > the GIC or patch them to back out the erratum 6678 workaround as later
> > > kernels do.
> >
> > Thanks for that really useful writeup. So if I understand correctly
> >
> > we have several choices here:
> > (1) we could implement a model of the IOMUX block that is at least
> > sufficient to support guests that configure it to route the ENET
> > interrupt line to a GPIO pin. Then we could apply this patch that fixes
> > the ENET line definitions. Old kernels would continue to work (for the
> > same reason they worked on hardware), and new ones would work now too.
> > This is in some ways the preferred option, but it's possibly a lot of
> > code and we're nearly in freeze for 2.12.
> >
> > (2) we could leave everything as it is for 2.12. This would mean that
> > at least we don't regress setups that used to work on older QEMU
> > versions. Downside is that we wouldn't be able to run Linux v4.15+, or
> > other guest OSes that don't have the bug that older Linux kernels do.
> > (Presumably we'd only do this on the understanding that we were going
> > to go down route (1) for 2.13.)
> >
> > (3) we could apply this patch for 2.12. Linux v4.15+ now works, as
> > do other guest OSes that use the ENET interrupt. v4.1 and older Linux
> > guests that used to boot in QEMU stop doing so, and 4.2-4.9 boot but
> > lose the ethernet device support. Perhaps for 2.13 we might
> > take route (1) to make those older guests start working again.
> >
> > Do I have that right?
>
> Pretty much.
There may be a 4th option.
Since older kernels work because they were looking at vector 151, you could
patch the imx_fec.c module so that when it signals an event for vector 150, it
also signals vector 151 too. This would keep newer versions of QEMU "bug-
compatible" with older versions while also fixing support for newer Linux
kernels and other guests (e.g. VxWorks) that follow the hardware spec
correctly.
I think this would require only a small modification to this function:
static void imx_eth_update(IMXFECState *s)
{
if (s->regs[ENET_EIR] & s->regs[ENET_EIMR] & ENET_INT_TS_TIMER) {
qemu_set_irq(s->irq[1], 1);
} else {
qemu_set_irq(s->irq[1], 0);
}
if (s->regs[ENET_EIR] & s->regs[ENET_EIMR] & ENET_INT_MAC) {
qemu_set_irq(s->irq[0], 1);
} else {
qemu_set_irq(s->irq[0], 0);
}
}
(For ENET_INT_MAC events, just signal both s->irq[0] and s->irq[1]).
This of course means signaling spurious events on vector 151, but you're doing
that now anyway. :)
This is much less work than implementing an IOMUX module. Creating an IOMUX
module for QEMU just for this one fixup would probably be the most elegant
solution, but adding IOMUX support to QEMU also doesn't make much sense since
QEMU has no actual pins.
-Bill
> > None of these options seems especially palatable to me, so we're
> > choosing the lesser evil, I think... (unless somebody wants to say
> > that option (1) would be 20 lines of code and here's the patch :-))
> > I guess in the absence of (1) that (3) is better than (2) ?
>
> I would prefer (2). This is what I decided to use in my "local"
> version of qemu. Older versions of Linux can be fixed by applying one
> (4.2..4.9) or two (4.1 and older) upstream patches; anyone interested
> running those kernels in qemu with Ethernet working should apply those
> patches (or, alternatively, provide a patch adding IOMUX support to
> qemu).
>
> Thanks,
> Guenter
--
=
-Bill Paul(510) 749-2329 | Senior Member of Technical Staff,
wp...@windriver.com | Master of Unix-Fu - Wind River Systems
=
"I put a dollar in a change machine. Nothing changed." - George Carlin
=
Re: [Qemu-devel] [PATCH] fsl-imx6: Swap Ethernet interrupt defines
h the broken interrupt configuration is that they also registered a handler on vector 151 (119). Even though QEMU could not send events via GPIO6, it was mistakenly sending them via vector 151, so it looked like things worked. On real hardware, the older kernels would have gotten their interrupts via GPIO6 and also worked. The older kernels would _not_ work if you fix QEMU because now they would never get interrupts on either vector, unless you fudge things so that the ENET module triggers both vector 150 and the vector for GPIO6 in the GIC or patch them to back out the erratum 6678 workaround as later kernels do. Later kernels that register vectors 150 and 151 would work with both broken and fixed QEMU and real hardware. But you should fix the incorrect interrupt configuration regardless. :) -Bill > Any idea how to fix this ? > > Guenter -- = -Bill Paul(510) 749-2329 | Senior Member of Technical Staff, wp...@windriver.com | Master of Unix-Fu - Wind River Systems = "I put a dollar in a change machine. Nothing changed." - George Carlin =
[Qemu-devel] [Bug 1753309] Re: Ethernet interrupt vectors for sabrelite machine are defined backwards
"4.14+: Both versions of qemu (as-is and interrupts reverted) work fine" Hm. I really wonder how it can be possible that Linux works with the interrupt vectors reversed, though to be fair I have not looked at the Linux i.MX6 ENET driver code. I suppose it's possible that the driver is binding the same interrupt service routine to both interrupt vectors. If so, then it works by accident. :) I think U-Boot uses polling so it wouldn't care if the interrupt vectors are wrong. We have several SabreLite boards in house. We also have NXP Sabre SD reference boards which use the same i.MX6Q SoC and the exact same ethernet driver with the same interrupt configuration. I have always used VxWorks with them rather than Linux, and I can say for a fact that the VxWorks ENET driver only binds an ISR to vector 150 (118) (VxWorks doesn't currently support the IEEE 1588 feature with this interface so it never uses vector 151) and it works as expected -- network interrupt events are indeed received via vector 150. The same VxWorks image that works with real hardware does not work with QEMU unless I fix the vectors in fsl-imx6.h. In short, both the hardware and the manual seem to agree. QEMU is doing it wrong. :) Also, the errata sheet for the i.MX6 is here: https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf Apparently erratum 6687 is related to power management and wakeup events. I'm not sure how that factors in to how Linux behaves. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1753309 Title: Ethernet interrupt vectors for sabrelite machine are defined backwards Status in QEMU: New Bug description: The sabrelite machine model used by qemu-system-arm is based on the Freescale/NXP i.MX6Q processor. This SoC has an on-board ethernet controller which is supported in QEMU using the imx_fec.c module (actually called imx.enet for this model.) The include/hw/arm/fsm-imx6.h file defines the interrupt vectors for the imx.enet device like this: #define FSL_IMX6_ENET_MAC_1588_IRQ 118 #define FSL_IMX6_ENET_MAC_IRQ 119 However, this is backwards. The reference manual for the i.MX6D/Q devices can be found here: https://www.nxp.com/docs/en/reference-manual/IMX6DQRM.pdf On page 225, in Table 3-1. ARM Cortex A9 domain interrupt summary, it shows the following: 150 ENET MAC 0 IRQ, Logical OR of: MAC 0 Periodic Timer Overflow MAC 0 Time Stamp Available MAC 0 Time Stamp Available MAC 0 Time Stamp Available MAC 0 Payload Receive Error MAC 0 Transmit FIFO Underrun MAC 0 Collision Retry Limit MAC 0 Late Collision MAC 0 Ethernet Bus Error MAC 0 MII Data Transfer Done MAC 0 Receive Buffer Done MAC 0 Receive Frame Done MAC 0 Transmit Buffer Done MAC 0 Transmit Frame Done MAC 0 Graceful Stop MAC 0 Babbling Transmit Error MAC 0 Babbling Receive Error MAC 0 Wakeup Request [synchronous] 151 ENET MAC 0 1588 Timer interrupt [synchronous] request Note: 150 - 32 == 118 151 - 32 == 119 In other words, the vector definitions in the fsl-imx6.h file are reversed. The correct definition is: #define FSL_IMX6_ENET_MAC_IRQ 118 #define FSL_IMX6_ENET_MAC_1588_IRQ 119 I tested the sabrelite simulation using VxWorks 7 (which supports the SabreLite board) and found that while I was able to send and receive packet data via the simulated ethernet interface, the VxWorks i.MX6 ethernet driver failed to receive any interrupts. When I corrected the interrupt vector definitions as shown above and recompiled QEMU, everything worked as expected. I was able to exchange ICMP packets with the simulated target and telnet to/from the VxWorks instance running in the virtual machine. I used the tap interface for this. As a workaround I was also able to make the ethernet work by modifying the VxWorks imx6q-sabrelite.dts file to change the ethernet interrupt property from 150 to 151. This problem was observed with the following environment: Host: FreeBSD/amd64 11.1-RELEASE QEMU version: 2.11.0 and 2.11.1 built from source code To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1753309/+subscriptions
[Qemu-devel] [Bug 1753314] [NEW] UART in sabrelite machine simulation doesn't work with VxWorks 7
Public bug reported: The imx_serial.c driver currently implements only partial support for the i.MX6 UART hardware. (I understand it's a work in progress and that's fine.) dIn particular, it does not implement support for the Transmit Complete Interrupt Enable bit in the UCR4 register. The VxWorks 7 i.MX6 serial driver depends on the behavior of this bit in actual hardware in order to send characters through the UART correctly. The result is that with the current machine model, VxWorks will boot and run in QEMU but it's unable to print any characters to the console serial port. I have produced a small patch for the imx_serial.c module to make it nominally functional with VxWorks 7. It works well enough to allow the boot banner to appear and for the user to interact with the target shell. I'm not submitting this as a patch to the development list as I'm not fully certain it complies with the hardware spec and doesn't break any other functionality. I would prefer if the maintainer (or someone) reviewed it for any issues/refinements first. I'm attaching the patch to this bug report. A copy can also be obtained from: http://people.freebsd.org/~wpaul/qemu/imx_serial.zip This patch was generated against QEMU 2.11.0 but also works with QEMU 2.11.1. My host environment is FreeBSD/amd64 11.1-RELEASE with QEMU 2.11.0/2.11.11 built from source. ** Affects: qemu Importance: Undecided Status: New ** Tags: arm sabrelite uart ** Patch added: "Patch for imx_serial.c/h to make it work with VxWorks" https://bugs.launchpad.net/bugs/1753314/+attachment/5069246/+files/imx_serial.zip ** Summary changed: - UART in sabrelite machine simulation doesn't work wth VxWorks + UART in sabrelite machine simulation doesn't work with VxWorks 7 ** Description changed: The imx_serial.c driver currently implements only partial support for - the i.MX6 UART hardware. In particular, it does not implement support - for the Transmit Complete Interrupt Enable bit in the UCR4 register. The - VxWorks 7 i.MX6 serial driver depends on the behavior of this bit in - actual hardware in order to send characters through the UART correctly. - The result is that with the current machine model, VxWorks will boot and - run in QEMU but it's unable to print any characters to the console - serial port. + the i.MX6 UART hardware. (I understand it's a work in progress and + that's fine.) In particular, it does not implement support for the + Transmit Complete Interrupt Enable bit in the UCR4 register. The VxWorks + 7 i.MX6 serial driver depends on the behavior of this bit in actual + hardware in order to send characters through the UART correctly. The + result is that with the current machine model, VxWorks will boot and run + in QEMU but it's unable to print any characters to the console serial + port. I have produced a small patch for the imx_serial.c module to make it nominally functional with VxWorks 7. It works well enough to allow the boot banner to appear and for the user to interact with the target shell. I'm not submitting this as a patch to the development list as I'm not fully certain it complies with the hardware spec and doesn't break any other functionality. I would prefer if the maintainer (or someone) reviewed it for any issues/refinements first. I'm attaching the patch to this bug report. A copy can also be obtained from: http://people.freebsd.org/~wpaul/qemu/imx_serial.zip This patch was generated against QEMU 2.11.0 but also works with QEMU 2.11.1. ** Description changed: The imx_serial.c driver currently implements only partial support for the i.MX6 UART hardware. (I understand it's a work in progress and - that's fine.) In particular, it does not implement support for the + that's fine.) dIn particular, it does not implement support for the Transmit Complete Interrupt Enable bit in the UCR4 register. The VxWorks 7 i.MX6 serial driver depends on the behavior of this bit in actual hardware in order to send characters through the UART correctly. The result is that with the current machine model, VxWorks will boot and run in QEMU but it's unable to print any characters to the console serial port. I have produced a small patch for the imx_serial.c module to make it nominally functional with VxWorks 7. It works well enough to allow the boot banner to appear and for the user to interact with the target shell. I'm not submitting this as a patch to the development list as I'm not fully certain it complies with the hardware spec and doesn't break any other functionality. I would prefer if the maintainer (or someone) reviewed it for any issues/refinements first. I'm attaching the patch to this bug report. A copy can also be obtained from: http://people.freebsd.org/~wpaul/qemu/imx_serial.zip This patch was generated against QEMU 2.11.0 but also works with QEMU 2.11.1. + + My host environment is FreeBSD/amd64 11.1-RELEASE
[Qemu-devel] [Bug 1753309] [NEW] Ethernet interrupt vectors for sabrelite machine are defined backwards
Public bug reported: The sabrelite machine model used by qemu-system-arm is based on the Freescale/NXP i.MX6Q processor. This SoC has an on-board ethernet controller which is supported in QEMU using the imx_fec.c module (actually called imx.enet for this model.) The include/hw/arm/fsm-imx6.h file defines the interrupt vectors for the imx.enet device like this: #define FSL_IMX6_ENET_MAC_1588_IRQ 118 #define FSL_IMX6_ENET_MAC_IRQ 119 However, this is backwards. The reference manual for the i.MX6D/Q devices can be found here: https://www.nxp.com/docs/en/reference-manual/IMX6DQRM.pdf On page 225, in Table 3-1. ARM Cortex A9 domain interrupt summary, it shows the following: 150 ENET MAC 0 IRQ, Logical OR of: MAC 0 Periodic Timer Overflow MAC 0 Time Stamp Available MAC 0 Time Stamp Available MAC 0 Time Stamp Available MAC 0 Payload Receive Error MAC 0 Transmit FIFO Underrun MAC 0 Collision Retry Limit MAC 0 Late Collision MAC 0 Ethernet Bus Error MAC 0 MII Data Transfer Done MAC 0 Receive Buffer Done MAC 0 Receive Frame Done MAC 0 Transmit Buffer Done MAC 0 Transmit Frame Done MAC 0 Graceful Stop MAC 0 Babbling Transmit Error MAC 0 Babbling Receive Error MAC 0 Wakeup Request [synchronous] 151 ENET MAC 0 1588 Timer interrupt [synchronous] request Note: 150 - 32 == 118 151 - 32 == 119 In other words, the vector definitions in the fsl-imx6.h file are reversed. The correct definition is: #define FSL_IMX6_ENET_MAC_IRQ 118 #define FSL_IMX6_ENET_MAC_1588_IRQ 119 I tested the sabrelite simulation using VxWorks 7 (which supports the SabreLite board) and found that while I was able to send and receive packet data via the simulated ethernet interface, the VxWorks i.MX6 ethernet driver failed to receive any interrupts. When I corrected the interrupt vector definitions as shown above and recompiled QEMU, everything worked as expected. I was able to exchange ICMP packets with the simulated target and telnet to/from the VxWorks instance running in the virtual machine. I used the tap interface for this. As a workaround I was also able to make the ethernet work by modifying the VxWorks imx6q-sabrelite.dts file to change the ethernet interrupt property from 150 to 151. This problem was observed with the following environment: Host: FreeBSD/amd64 11.1-RELEASE QEMU version: 2.11.0 and 2.11.1 built from source code ** Affects: qemu Importance: Undecided Status: New ** Tags: arm imx.enet sabrelite -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1753309 Title: Ethernet interrupt vectors for sabrelite machine are defined backwards Status in QEMU: New Bug description: The sabrelite machine model used by qemu-system-arm is based on the Freescale/NXP i.MX6Q processor. This SoC has an on-board ethernet controller which is supported in QEMU using the imx_fec.c module (actually called imx.enet for this model.) The include/hw/arm/fsm-imx6.h file defines the interrupt vectors for the imx.enet device like this: #define FSL_IMX6_ENET_MAC_1588_IRQ 118 #define FSL_IMX6_ENET_MAC_IRQ 119 However, this is backwards. The reference manual for the i.MX6D/Q devices can be found here: https://www.nxp.com/docs/en/reference-manual/IMX6DQRM.pdf On page 225, in Table 3-1. ARM Cortex A9 domain interrupt summary, it shows the following: 150 ENET MAC 0 IRQ, Logical OR of: MAC 0 Periodic Timer Overflow MAC 0 Time Stamp Available MAC 0 Time Stamp Available MAC 0 Time Stamp Available MAC 0 Payload Receive Error MAC 0 Transmit FIFO Underrun MAC 0 Collision Retry Limit MAC 0 Late Collision MAC 0 Ethernet Bus Error MAC 0 MII Data Transfer Done MAC 0 Receive Buffer Done MAC 0 Receive Frame Done MAC 0 Transmit Buffer Done MAC 0 Transmit Frame Done MAC 0 Graceful Stop MAC 0 Babbling Transmit Error MAC 0 Babbling Receive Error MAC 0 Wakeup Request [synchronous] 151 ENET MAC 0 1588 Timer interrupt [synchronous] request Note: 150 - 32 == 118 151 - 32 == 119 In other words, the vector definitions in the fsl-imx6.h file are reversed. The correct definition is: #define FSL_IMX6_ENET_MAC_IRQ 118 #define FSL_IMX6_ENET_MAC_1588_IRQ 119 I tested the sabrelite simulation using VxWorks 7 (which supports the SabreLite board) and found that while I was able to send and receive packet data via the simulated ethernet interface, the VxWorks i.MX6 ethernet driver failed to receive any interrupts. When I corrected the interrupt vector definitions as shown above and recompiled QEMU, everything worked as expected. I was able to exchange ICMP packets with the simulated target and telnet to/from the VxWorks instance running in the virtual machine. I used the tap interface for this. As a workaround I was also able to make the ethernet work by modifying the VxWorks imx6q-sabrelite.dts file to change the ethernet interrupt property from 150 to
[Qemu-devel] [PATCH] Fix TXE/TXEIE support in the STM32 USART model
This commit attempts to fix the behavior of the TX FIFO Empty bit in
the STM32 USART driver. The two changes are:
1) After wrtiting to the data register, don't clear the TXE
bit in the status register. The way this model works, the FIFO is
effectively always empty. This is because we dump each character to
qemu_chr_fe_write_all() right away and unlike real hardware we don't
have to wait for the appropriate bits to be written to the I/O pins.
2) Implement support for the TXEIE (TXE interrupt enable) bit in CR1. When
OS driver code unmasks this bit and TXE is set, this should trigger an
interrupt to let the OS know the channel is now empty and it can send again.
ChibiOS depends on the correct behavior of the TXE and the TXEIE interrupt.
It checks to see if the TXE bit is set before trying to write any
characters. however at the outset TXE is clear, so it blocks forever
(or until you press a key at the serial console, which causes an RX
interrupt that unjams in).
Also once a character has been written, it waits for a TXEIE interrupt
before sending the next character in a string. With these two fixes, it can
now write to the serial port sucessfully.
Signed-off-by: Bill Paul <wp...@windriver.com>
Cc: Paolo Bonzini <pbonz...@redhat.com>
---
hw/char/stm32f2xx_usart.c | 7 ++-
include/hw/char/stm32f2xx_usart.h | 1 +
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/char/stm32f2xx_usart.c b/hw/char/stm32f2xx_usart.c
index 15657ab..5671a7d 100644
--- a/hw/char/stm32f2xx_usart.c
+++ b/hw/char/stm32f2xx_usart.c
@@ -156,7 +156,6 @@ static void stm32f2xx_usart_write(void *opaque, hwaddr addr,
qemu_chr_fe_write_all(s->chr, , 1);
}
s->usart_sr |= USART_SR_TC;
-s->usart_sr &= ~USART_SR_TXE;
}
return;
case USART_BRR:
@@ -168,6 +167,12 @@ static void stm32f2xx_usart_write(void *opaque, hwaddr
addr,
s->usart_sr & USART_SR_RXNE) {
qemu_set_irq(s->irq, 1);
}
+
+if (s->usart_cr1 & USART_CR1_TXEIE &&
+s->usart_sr & USART_SR_TXE) {
+qemu_set_irq(s->irq, 1);
+}
+
return;
case USART_CR2:
s->usart_cr2 = value;
diff --git a/include/hw/char/stm32f2xx_usart.h
b/include/hw/char/stm32f2xx_usart.h
index b97f192..2820209 100644
--- a/include/hw/char/stm32f2xx_usart.h
+++ b/include/hw/char/stm32f2xx_usart.h
@@ -44,6 +44,7 @@
#define USART_SR_RXNE (1 << 5)
#define USART_CR1_UE (1 << 13)
+#define USART_CR1_TXEIE (1 << 7)
#define USART_CR1_RXNEIE (1 << 5)
#define USART_CR1_TE (1 << 3)
#define USART_CR1_RE (1 << 2)
--
2.9.0
Re: [Qemu-devel] ARM Cortex-M issues
Of all the gin joints in all the towns in all the world, Peter Maydell had to walk into mine at 12:51:04 on Monday 29 August 2016 and say: > On 29 August 2016 at 13:59, Bill Paul <wp...@windriver.com> wrote: > > Unfortunately it's been a frustrating experience because there seem to be > > several key places where QEMU's hardware emulation diverges from reality. > > The ChibiOS examples often seem to depend on behavior that is valid for > > actual hardware but which is either broken or just missing in QEMU. Some > > of these issues are board-specific, but the last one seems a bit more > > general. > > Yes, our Cortex-M support is a bit undermaintained at the moment. > If you'd like to write patches to fix some of the bugs you're > encountering I'd be happy to review them, but I'm not aware of anybody > actively working on M profile right now. We could really use a > contributor who cares about it and has time to tackle improving it. > (A-profile ARM emulation is in much better shape.) I had a feeling you were going to say that. But I already fell for this trick once when I started using FreeBSD, and then I ended up being a developer for about 10 years. I'm older and wiser now. (Also I have a day job that consumes most of my time.) The best I might be able to do is patch the STM32 SUART driver so that it supports the TX fifo empty interrupt. I'm really not sure how to fix the STM32 timer driver (like I said, the ST Micro documentation is really hard to follow) and I'm not sure that any attempt to get the NMI to work would be any less of a hack then what's there now. [...] > The reason for this kind of thing is that the original support was > done to support a specific RTOS, and so bugs which resulted in that > RTOS not working were found and fixed. Bugs which weren't exercised > by that RTOS remain lurking in the code, and if you try to use a > different RTOS guest then you can run into them. (This is less > obvious on the A profile cores because to a first approximation > nobody runs anything but Linux on them, but in the embedded world > there's still a fairly rich diversity of RTOSes which take different > approaches to how they prod the hardware.) In other words it's half-baked. :( -Bill > thanks > -- PMM -- = -Bill Paul(510) 749-2329 | Senior Member of Technical Staff, wp...@windriver.com | Master of Unix-Fu - Wind River Systems = "I put a dollar in a change machine. Nothing changed." - George Carlin =
Re: [Qemu-devel] ARM Cortex-M issues
Of all the gin joints in all the towns in all the world, Liviu Ionescu had to
walk into mine at 12:19:42 on Monday 29 August 2016 and say:
> > On 29 Aug 2016, at 20:59, Bill Paul <wp...@windriver.com> wrote:
> >
> > I recently started tinkering with ChibiOS as part of a small personal
> > project ...
>
> I did most of the development for the µOS++/CMSIS++
> (http://micro-os-plus.github.io) on STM32F4DISCOVERY board, emulated by
> GNU ARM Eclipse QEMU, which implements even animated LEDs on a graphical
> image of the board.
>
> FreeRTOS also works properly on the emulator, both the M0 and M3 ports.
>
> As for Cortex-M implementation, there are many improvements in the GNU ARM
> Eclipse QEMU fork (http://gnuarmeclipse.github.io/qemu/), including an
> Eclipse debug plug-in to start it; it may be worth giving it a try for
> ChibiOS too.
I think I've been down this road already with Xilinx ARM support. ("We have
our own fork of QEMU For the MPSoC parts!" "It seems to have diverged a lot of
from the mainline. Also you've only been testing the ARM builds and those only
on Linux hosts, and now the code has bitrotted." "Yeah... but... we're going
to submit our changes to upstream Real Soon Now (tm).")
Note that I'm not suggesting the ARM Eclipse code suffers from bitrot. I'll
give it a try. I just wish all of this was in once place.
Also, from a cursory look at the code, it doesn't look like this fork handles
the NMI interrupt any better than the mainline.
The Cortex-M model has an explicit NMI vector in its vector table (#2). It's
possible to trigger this interrupt via software by writing a 1 to the
NMIPENDSET bit in the ICSR register in the System Control Block (which seems
to be a Cortex-M-specific thing).
Currently this vector is treated just like any other IRQ. The problem is that
means it is also subject to the case where CPSR_I is masked off in the CPU,
which for the NMI is wrong. (How can you mask that which is unmaskabkle?)
From looking at how things are structured, I think the only way to make it
work is to give the target-arm/cpu.c code a separate external NMI pin (e.g.
CPU_INTERRUPT_NMI) and make the arm_gic.c smart enough to trigger that pin
instead of the IRQ or FIQ pins when the NMI is triggered. The handling for
that pin could then be special-cased not to ignore the state of CPSR_I.
But that was just from a quick look last night while I was experimenting. I
don't know if maybe there's a better way. This is why I'm here asking
questions. :)
-Bill
>
> Regards,
>
> Liviu
--
=
-Bill Paul(510) 749-2329 | Senior Member of Technical Staff,
wp...@windriver.com | Master of Unix-Fu - Wind River Systems
=
"I put a dollar in a change machine. Nothing changed." - George Carlin
=
[Qemu-devel] ARM Cortex-M issues
MI just work? -Bill -- ===== -Bill Paul(510) 749-2329 | Senior Member of Technical Staff, wp...@windriver.com | Master of Unix-Fu - Wind River Systems = "I put a dollar in a change machine. Nothing changed." - George Carlin =
[Qemu-devel] [PATCH] hw/timer: Revert "hpet: inverse polarity when pin above ISA_NUM_IRQS"
This reverts commit 0d63b2dd31464cfccc80bbeedc24e3863fe4c895.
This change was originally intended to correct the HPET behavior
in conjunction with Linux, however the behavior that it actually creates
doesn't match what happens with real hardware, the logic doesn't seem
compatible with the ioapic.c implementation either.
Signed-off-by: Bill Paul <wp...@windriver.com>
CC: Paolo Bonzini <pbonz...@redhat.com>
CC: Richard Henderson <r...@twiddle.net>
CC: Eduardo Habkost <ehabk...@redhat.com>
---
hw/timer/hpet.c | 14 ++
1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 78140e6..a2c18b3 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -201,12 +201,7 @@ static void update_irq(struct HPETTimer *timer, int set)
if (!set || !timer_enabled(timer) || !hpet_enabled(timer->state)) {
s->isr &= ~mask;
if (!timer_fsb_route(timer)) {
-/* fold the ICH PIRQ# pin's internal inversion logic into hpet */
-if (route >= ISA_NUM_IRQS) {
-qemu_irq_raise(s->irqs[route]);
-} else {
-qemu_irq_lower(s->irqs[route]);
-}
+qemu_irq_lower(s->irqs[route]);
}
} else if (timer_fsb_route(timer)) {
address_space_stl_le(_space_memory, timer->fsb >> 32,
@@ -214,12 +209,7 @@ static void update_irq(struct HPETTimer *timer, int set)
NULL);
} else if (timer->config & HPET_TN_TYPE_LEVEL) {
s->isr |= mask;
-/* fold the ICH PIRQ# pin's internal inversion logic into hpet */
-if (route >= ISA_NUM_IRQS) {
-qemu_irq_lower(s->irqs[route]);
-} else {
-qemu_irq_raise(s->irqs[route]);
-}
+qemu_irq_raise(s->irqs[route]);
} else {
s->isr &= ~mask;
qemu_irq_pulse(s->irqs[route]);
--
2.4.6
Re: [Qemu-devel] Question about hw/timer/hpet.c, hw/intc/ioapic.c and polarity
Of all the gin joints in all the towns in all the world, Paolo Bonzini had to
walk into mine at 06:20:05 on Tuesday 05 April 2016 and say:
> On 04/04/2016 23:42, Bill Paul wrote:
> > I'm testing some of the HPET handling code in VxWorks using QEMU 2.4.1
> > and I've encountered something which looks a little odd that I'm hoping
> > someone can clarify for me. First, some background:
> >
> > The HPET timer supports three kinds of interrupt delivery:
> >
> > Legacy: HPET can use the same IRQs as the old 8254 timer (IRQ2, IRQ8)
> > I/O APIC: HPET can use any of the first 32 I/O APIC IRQs in the system
> > FSB: HPET uses "front-side bus" mode, i.e interrupts are routed right to
> > the local APIC (I/O APIC is bypassed)
> >
> > By default, VxWorks uses front-side bus mode, and that seems to work
> > fine. However I wanted to try I/O APIC mode too, and that seems to
> > behave in a funny
> >
> > way. In particular, the following code in hw/timer/hpet.c puzzles me:
> > if (!set || !timer_enabled(timer) || !hpet_enabled(timer->state)) {
> >
> > s->isr &= ~mask;
> > if (!timer_fsb_route(timer)) {
> >
> > /* fold the ICH PIRQ# pin's internal inversion logic into
> > hpet */ if (route >= ISA_NUM_IRQS) {
> >
> > qemu_irq_raise(s->irqs[route]);
> >
> > } else {
> >
> > qemu_irq_lower(s->irqs[route]);
> >
> > }
> >
> > }
> >
> > } else if (timer_fsb_route(timer)) {
> >
> > address_space_stl_le(_space_memory, timer->fsb >> 32,
> >
> > timer->fsb & 0x,
> > MEMTXATTRS_UNSPECIFIED, NULL);
> >
> > } else if (timer->config & HPET_TN_TYPE_LEVEL) {
> >
> > s->isr |= mask;
> > /* fold the ICH PIRQ# pin's internal inversion logic into hpet */
> > if (route >= ISA_NUM_IRQS) {
> >
> > qemu_irq_lower(s->irqs[route]);
> >
> > } else {
> >
> > qemu_irq_raise(s->irqs[route]);
> >
> > }
> >
> > } else {
> >
> > s->isr &= ~mask;
> > qemu_irq_pulse(s->irqs[route]);
> >
> > }
> >
> > Note the part that inverts the interrupt handling logic for ICH PIRQ
> > pins. I don't understand how this is supposed to work.
>
> I think t should be removed.
>
> If I use level triggered IRQ2
>
> > or IRQ8 in VxWorks, then things work as expected. But if I use IRQ21, the
> > HPET timer interrupt service routine is immediately called, even though
> > the timer hasn't expired yet. The ISR reads 0 from the HPET status
> > register, indicating that no timers have events pending, so it just
> > returns. The first qemu_irq_raise() call is triggered because
> > hpet_enabled() returns true, but timer_enabled() returns false.
> >
> > Researching the code history, I see that the inversion logic was added in
> > 2013 in order to fix a problem with HPET usage in Linux. However
> > something about the way this was done looks wrong to me. In the case
> > where we actually want to signal an interrupt because the timer has
> > expired, and the IRQ is larger than 15, the code calls qemu_irq_lower()
> > instead of qemu_irq_raise(). Eventually this results in ioapic_set_irq()
> > being called with level = 0. The problem is, ioapic_set_irq() will only
> > call ioapic_service() to notify the quest of an interrupt if level == 1.
> >
> > Given this, I can't understand how this is supposed to work. The hpet.c
> > code seems to treat qemu_irq_raise() and qemu_irq_lower() as though they
> > can trigger active high or active low interrupts, but the ioapic.c code
> > doesn't support any polarity settings. The only way to actually trigger
> > an interrupt to the guest is to "raise" (assert) the interrupt by
> > calling qemu_irq_raise().
>
> I think that commit 0d63b2d ("hpet: inverse polarity when pin above
> ISA_NUM_IRQS", 2013-12-11) can be reverted. The code was probably
> tested on KVM only, but KVM now is *also* ignoring the IOAPIC polarity
> (commit 100943c54e09, "kvm: x86: ignore ioapic polarity", 2014-03-13).
I wonder how it worked on KVM.
> Does that work for you? If so, can you
[Qemu-devel] Question about hw/timer/hpet.c, hw/intc/ioapic.c and polarity
I'm testing some of the HPET handling code in VxWorks using QEMU 2.4.1 and
I've encountered something which looks a little odd that I'm hoping someone
can clarify for me. First, some background:
The HPET timer supports three kinds of interrupt delivery:
Legacy: HPET can use the same IRQs as the old 8254 timer (IRQ2, IRQ8)
I/O APIC: HPET can use any of the first 32 I/O APIC IRQs in the system
FSB: HPET uses "front-side bus" mode, i.e interrupts are routed right to the
local APIC (I/O APIC is bypassed)
By default, VxWorks uses front-side bus mode, and that seems to work fine.
However I wanted to try I/O APIC mode too, and that seems to behave in a funny
way. In particular, the following code in hw/timer/hpet.c puzzles me:
if (!set || !timer_enabled(timer) || !hpet_enabled(timer->state)) {
s->isr &= ~mask;
if (!timer_fsb_route(timer)) {
/* fold the ICH PIRQ# pin's internal inversion logic into hpet */
if (route >= ISA_NUM_IRQS) {
qemu_irq_raise(s->irqs[route]);
} else {
qemu_irq_lower(s->irqs[route]);
}
}
} else if (timer_fsb_route(timer)) {
address_space_stl_le(_space_memory, timer->fsb >> 32,
timer->fsb & 0x, MEMTXATTRS_UNSPECIFIED,
NULL);
} else if (timer->config & HPET_TN_TYPE_LEVEL) {
s->isr |= mask;
/* fold the ICH PIRQ# pin's internal inversion logic into hpet */
if (route >= ISA_NUM_IRQS) {
qemu_irq_lower(s->irqs[route]);
} else {
qemu_irq_raise(s->irqs[route]);
}
} else {
s->isr &= ~mask;
qemu_irq_pulse(s->irqs[route]);
}
Note the part that inverts the interrupt handling logic for ICH PIRQ pins. I
don't understand how this is supposed to work. If I use level triggered IRQ2
or IRQ8 in VxWorks, then things work as expected. But if I use IRQ21, the HPET
timer interrupt service routine is immediately called, even though the timer
hasn't expired yet. The ISR reads 0 from the HPET status register, indicating
that no timers have events pending, so it just returns. The first
qemu_irq_raise() call is triggered because hpet_enabled() returns true, but
timer_enabled() returns false.
Researching the code history, I see that the inversion logic was added in 2013
in order to fix a problem with HPET usage in Linux. However something about
the way this was done looks wrong to me. In the case where we actually want to
signal an interrupt because the timer has expired, and the IRQ is larger than
15, the code calls qemu_irq_lower() instead of qemu_irq_raise(). Eventually
this results in ioapic_set_irq() being called with level = 0. The problem is,
ioapic_set_irq() will only call ioapic_service() to notify the quest of an
interrupt if level == 1.
Given this, I can't understand how this is supposed to work. The hpet.c code
seems to treat qemu_irq_raise() and qemu_irq_lower() as though they can
trigger active high or active low interrupts, but the ioapic.c code doesn't
support any polarity settings. The only way to actually trigger an interrupt
to the guest is to "raise" (assert) the interrupt by calling qemu_irq_raise().
Right now all I know is that VxWorks' usage of the HPET seems to work on real
hardware, but not on QEMU. I suspect that the changes made to hpet.c here may
have only "fixed" the problem with Linux by introducing some non-standard
behavior that happens to pacify Linux's particular usage model.
Can someone comment on whether or not this inversion logic is really still
necessary in Linux? Is there maybe a better way to handle this?
-Bill
--
=
-Bill Paul(510) 749-2329 | Senior Member of Technical Staff,
wp...@windriver.com | Master of Unix-Fu - Wind River Systems
=
"I put a dollar in a change machine. Nothing changed." - George Carlin
=
Re: [Qemu-devel] Possible bug in target-i386/helper.c:do_cpu_init()?
Ping?
> Consider the following circumstances:
>
> - An x86-64 multicore system is running with all cores set for long mode
> (EFER.LME and EFER.LMA set)
> - The OS decides to re-launch one of the AP CPUs using an INIT IPI
>
> According to the Intel architecture manual, an INIT IPI should reset the
> CPU state (with a few small exceptions):
>
> [...]
> 10.4.7.3 Local APIC State After an INIT Reset ("Wait-for-SIPI" State)
>
> An INIT reset of the processor can be initiated in either of two ways:
> ·By asserting the processor's INIT# pin.
> ·By sending the processor an INIT IPI (an IPI with the delivery mode
> set to INIT).
>
> Upon receiving an INIT through either of these mechanisms, the processor
> responds by beginning the initialization process of the processor core and
> the local APIC. The state of the local APIC following an INIT reset is the
> same as it is after a power-up or hardware reset, except that the APIC ID
> and arbitration ID registers are not affected. This state is also referred
> to at the "wait-for-SIPI" state (see also: Section 8.4.2, "MP
> Initialization Protocol Requirements and Restrictions").
> [...]
>
> Note however that do_cpu_init() does this:
>
> 1225 void do_cpu_init(X86CPU *cpu)
> 1226 {
> 1227 CPUState *cs = CPU(cpu);
> 1228 CPUX86State *env = >env;
> 1229 CPUX86State *save = g_new(CPUX86State, 1);
> 1230 int sipi = cs->interrupt_request & CPU_INTERRUPT_SIPI;
> 1231
> 1232 *save = *env;
> 1233
> 1234 cpu_reset(cs);
> 1235 cs->interrupt_request = sipi;
> 1236 memcpy(>start_init_save, >start_init_save,
> 1237offsetof(CPUX86State, end_init_save) -
> 1238offsetof(CPUX86State, start_init_save));
> 1239 g_free(save);
> 1240
> 1241 if (kvm_enabled()) {
> 1242 kvm_arch_do_init_vcpu(cpu);
> 1243 }
> 1244 apic_init_reset(cpu->apic_state);
> 1245 }
>
> The CPU environment, which in this case includes the EFER state, is saved
> and restored when calling cpu_reset(). The x86_cpu_reset() function
> actually does clear all of the CPU environment, but this function puts it
> all back.
>
> The result of this is that if the CPU was in long mode and you do an INIT
> IPI, the CPU still has the EFER.LMA and EFER.LME bits set, even though
> it's not actually running in long mode anymore. It doesn't seem possible
> for the guest to get the CPU out of this state, and one nasty side-effect
> is that trying to set the CR0 to enable paging never succeeds.
>
> I added the following code at line 1240 above as a workaround:
>
> #ifdef TARGET_X86_64
> /*
> * The initial state of the CPU is not 64-bit mode. This being
> * the case, don't leave the EFER.LME or EFER.LME bits set.
> */
>
> cpu_load_efer(env, 0);
> #endif
>
> This seemed to fix the problem I was having, however I'm not certain this
> is the correct fix.
>
> As background, I ran across this problem testing VxWorks with QEMU 2.3.0
> and OVMF firmware. The VxWorks BOOTX64.EFI loader is able to load and run
> 32-bit VxWorks images on 64-bit hardware by forcing the CPU back to 32-bit
> mode before handing control to the OS. However it only does this for the
> BSP (CPU 0). It turns out that the UEFI firmware puts the AP cores into
> long mode too. (This may be new in recent UEFI/OVMF versions, because I'm
> pretty sure tested this path before and didn't see a problem.) Everything
> works ok with uniprocessor images, but with SMP images, launching the
> first AP CPU fails due to the above condition (the CPU starts up, but is
> unable to enable paging and dies screaming in short order).
>
> Booting with the 32-bit OVMF build and the VxWorks BOOTIA32.EFI loader
> works ok. The same VxWorks loader and kernel code also seems to run ok on
> real hardware.
>
> I'm using QEMU 2.3.0 on FreeBSD/amd64 9.2-RELEASE. I'm not using KVM. It
> looks like the code is still the same in the git repo. Am I correct that
> do_cpu_init() should be clearing the EFER contents?
>
> -Bill
--
=
-Bill Paul(510) 749-2329 | Senior Member of Technical Staff,
wp...@windriver.com | Master of Unix-Fu - Wind River Systems
=
"I put a dollar in a change machine. Nothing changed." - George Carlin
=
[Qemu-devel] [PATCH] Correctly re-init EFER state during INIT IPI
When doing a re-initialization of a CPU core, the default state is to _not_
have 64-bit long mode enabled. This means the LME (long mode enable) and LMA
(long mode active) bits in the EFER model-specific register should be cleared.
However, the EFER state is part of the CPU environment which is
preserved by do_cpu_init(), so if EFER.LME and EFER.LMA were set at the
time an INIT IPI was received, they will remain set after the init completes.
This is contrary to what the Intel architecture manual describes and what
happens on real hardware, and it leaves the CPU in a weird state that the
guest can't clear.
To fix this, the 'efer' member of the CPUX86State structure has been moved
to an area outside the region preserved by do_cpu_init(), so that it can
be properly re-initialized by x86_cpu_reset().
Signed-off-by: Bill Paul <wp...@windriver.com>
CC: Paolo Bonzini <pbonz...@redhat.com>
CC: Richard Henderson <r...@twiddle.net>
CC: Eduardo Habkost <ehabk...@redhat.com>
---
target-i386/cpu.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 034fab6..fac773c 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -833,6 +833,7 @@ typedef struct CPUX86State {
BNDReg bnd_regs[4];
BNDCSReg bndcs_regs;
uint64_t msr_bndcfgs;
+uint64_t efer;
/* Beginning of state preserved by INIT (dummy marker). */
struct {} start_init_save;
@@ -865,7 +866,6 @@ typedef struct CPUX86State {
uint32_t sysenter_cs;
target_ulong sysenter_esp;
target_ulong sysenter_eip;
-uint64_t efer;
uint64_t star;
uint64_t vm_hsave;
--
1.8.0
[Qemu-devel] Possible bug in target-i386/helper.c:do_cpu_init()?
Consider the following circumstances:
- An x86-64 multicore system is running with all cores set for long mode
(EFER.LME and EFER.LMA set)
- The OS decides to re-launch one of the AP CPUs using an INIT IPI
According to the Intel architecture manual, an INIT IPI should reset the CPU
state (with a few small exceptions):
[...]
10.4.7.3 Local APIC State After an INIT Reset ("Wait-for-SIPI" State)
An INIT reset of the processor can be initiated in either of two ways:
·By asserting the processor's INIT# pin.
·By sending the processor an INIT IPI (an IPI with the delivery mode set
to INIT).
Upon receiving an INIT through either of these mechanisms, the processor
responds by beginning the initialization process of the processor core and the
local APIC. The state of the local APIC following an INIT reset is the same as
it is after a power-up or hardware reset, except that the APIC ID and
arbitration ID registers are not affected. This state is also referred to at
the "wait-for-SIPI" state (see also: Section 8.4.2, "MP Initialization
Protocol Requirements and Restrictions").
[...]
Note however that do_cpu_init() does this:
1225 void do_cpu_init(X86CPU *cpu)
1226 {
1227 CPUState *cs = CPU(cpu);
1228 CPUX86State *env = >env;
1229 CPUX86State *save = g_new(CPUX86State, 1);
1230 int sipi = cs->interrupt_request & CPU_INTERRUPT_SIPI;
1231
1232 *save = *env;
1233
1234 cpu_reset(cs);
1235 cs->interrupt_request = sipi;
1236 memcpy(>start_init_save, >start_init_save,
1237offsetof(CPUX86State, end_init_save) -
1238offsetof(CPUX86State, start_init_save));
1239 g_free(save);
1240
1241 if (kvm_enabled()) {
1242 kvm_arch_do_init_vcpu(cpu);
1243 }
1244 apic_init_reset(cpu->apic_state);
1245 }
The CPU environment, which in this case includes the EFER state, is saved and
restored when calling cpu_reset(). The x86_cpu_reset() function actually does
clear all of the CPU environment, but this function puts it all back.
The result of this is that if the CPU was in long mode and you do an INIT IPI,
the CPU still has the EFER.LMA and EFER.LME bits set, even though it's not
actually running in long mode anymore. It doesn't seem possible for the guest
to get the CPU out of this state, and one nasty side-effect is that trying to
set the CR0 to enable paging never succeeds.
I added the following code at line 1240 above as a workaround:
#ifdef TARGET_X86_64
/*
* The initial state of the CPU is not 64-bit mode. This being
* the case, don't leave the EFER.LME or EFER.LME bits set.
*/
cpu_load_efer(env, 0);
#endif
This seemed to fix the problem I was having, however I'm not certain this is
the correct fix.
As background, I ran across this problem testing VxWorks with QEMU 2.3.0 and
OVMF firmware. The VxWorks BOOTX64.EFI loader is able to load and run 32-bit
VxWorks images on 64-bit hardware by forcing the CPU back to 32-bit mode
before handing control to the OS. However it only does this for the BSP (CPU
0). It turns out that the UEFI firmware puts the AP cores into long mode too.
(This may be new in recent UEFI/OVMF versions, because I'm pretty sure tested
this path before and didn't see a problem.) Everything works ok with
uniprocessor images, but with SMP images, launching the first AP CPU fails due
to the above condition (the CPU starts up, but is unable to enable paging and
dies screaming in short order).
Booting with the 32-bit OVMF build and the VxWorks BOOTIA32.EFI loader works
ok. The same VxWorks loader and kernel code also seems to run ok on real
hardware.
I'm using QEMU 2.3.0 on FreeBSD/amd64 9.2-RELEASE. I'm not using KVM. It looks
like the code is still the same in the git repo. Am I correct that
do_cpu_init() should be clearing the EFER contents?
-Bill
--
=====
-Bill Paul(510) 749-2329 | Senior Member of Technical Staff,
wp...@windriver.com | Master of Unix-Fu - Wind River Systems
=
"I put a dollar in a change machine. Nothing changed." - George Carlin
=
Re: [Qemu-devel] [edk2] Windows does not support DataTableRegion at all [was: docs: describe QEMU's VMGenID design]
Of all the gin joints in all the towns in all the world, Laszlo Ersek had to walk into mine at 11:20:28 on Monday 14 September 2015 and say: > On 09/14/15 18:53, Bill Paul wrote: > > Of all the gin joints in all the towns in all the world, Laszlo Ersek had > > to > > > > walk into mine at 03:24:42 on Monday 14 September 2015 and say: > >> On 09/14/15 10:24, Igor Mammedov wrote: > >>> On Sun, 13 Sep 2015 15:34:51 +0300 > >>> > >>> "Michael S. Tsirkin" <m...@redhat.com> wrote: > >>>> On Sun, Sep 13, 2015 at 01:56:44PM +0200, Laszlo Ersek wrote: > >>>>> As the subject suggests, I have terrible news. > >>>>> > >>>>> I'll preserve the full context here, so that it's easy to scroll back > >>>>> to the ASL for reference. > >>>>> > >>>>> I'm also CC'ing edk2-devel, because a number of BIOS developers > >>>>> should be congregating there. > >>>> > >>>> Wow, bravo! It does look like we need to go back to > >>>> the drawing board. > > > > I read your original post on this with great interest, and I applaud your > > determination in tracking this down. Nice job. > > Thank you! > > > Sadly, it seems you too have > > fallen victim to the "If It Works With Windows, It Must Be Ok" syndrome. > > Well, I'd put it like this: we've fallen victim to a publicly > undocumented feature gap / divergence from the ACPI spec in Windows' > ACPI.SYS. > > > Now, I realize that as far as this particular situation is concerned, > > even if Microsoft decided to add support for DataTableRegion() tomorrow, > > it wouldn't really help because there are too many different versions of > > Windows in the field and there's no way to retroactively patch them all. > > (Gee, that sounds familiar...) > > Correct. > > > Nevertheless, am I correct in saying that this is in fact a bug in > > Microsoft's ACPI implementation (both in their ASL compiler and in the > > AML parser)? > > Absolutely. You are absolutely right. > > We implemented the VMGENID spec with some undeniable creativity, but it > broke down because the AML interpreter in Windows does not support an > ACPI 2.0 feature. > > (That interpreter is supposed to be ACPI 4.0 compliant, minimally; at > least if we can judge it after the "matching" AML.exe's stated > compatibility level, which is ACPI 4.0 in the standalone download, and > 5.0 if you get it as part of the WDK.) > > > Unless > > DataTableRegion() is specified to be optional in some way (I don't know > > if it is or not, but I doubt it), > > It's not, to the best of my knowledge. > > > this sounds like an clear cut case of non- > > compliance with the ACPI spec. > > Yes, it's precisely that. > > > And if that's true, isn't there any way to get > > Microsoft to fix it? > > I don't know. Is there? You would think that someone at Intel would know someone at Microsoft that could put some wheels in motion. (All this technology and still we have trouble communicating. :P ) > Microsoft continue to release updates (KB*) for Windows 7, Windows 8, > Windows 10, and I think rolling a fix out for those would cover our > needs quite okay. > > But: > - This would force QEMU/KVM host users to upgrade their Windows guest. > Maybe not such a big hurdle, but I reckon Windows sysadmins are > really conservative about installing updates. Perhaps we could solve > this issue but documentation. I agree with you that it's a hassle, but so is patching any other Windows bug. And while this particular use of DataTableRegion() affects VMs, it has bearing on bare metal installations too. > - More importantly, how do I even *report* this bug? How do I convince > Microsoft to implement a whole missing feature in their ACPI compiler > and interpreter? Can I demonstrate business justification? > > I'm doubtful especially because DataTableRegion's usefulness is quite > apparent to the ACPI developer in search for parametrization options. > DataTableRegion was published as part of ACPI 2.0, on July 27, 2000 > (more than fifteen years ago). I simply cannot imagine that in all > that time *no* physical platform's firmware developer tried to use > DataTableRegion. > > Therefore I can't help but assume that some big BIOS developer > company has already reported this to Microsoft, and the feature > request has been rejected. So what chance would I have? I understand what you're saying. But, there has to be some way to deal with these sorts of
Re: [Qemu-devel] [edk2] Windows does not support DataTableRegion at all [was: docs: describe QEMU's VMGenID design]
Of all the gin joints in all the towns in all the world, Laszlo Ersek had to walk into mine at 03:24:42 on Monday 14 September 2015 and say: > On 09/14/15 10:24, Igor Mammedov wrote: > > On Sun, 13 Sep 2015 15:34:51 +0300 > > > > "Michael S. Tsirkin" <m...@redhat.com> wrote: > >> On Sun, Sep 13, 2015 at 01:56:44PM +0200, Laszlo Ersek wrote: > >>> As the subject suggests, I have terrible news. > >>> > >>> I'll preserve the full context here, so that it's easy to scroll back > >>> to the ASL for reference. > >>> > >>> I'm also CC'ing edk2-devel, because a number of BIOS developers should > >>> be congregating there. > >> > >> Wow, bravo! It does look like we need to go back to > >> the drawing board. I read your original post on this with great interest, and I applaud your determination in tracking this down. Nice job. Sadly, it seems you too have fallen victim to the "If It Works With Windows, It Must Be Ok" syndrome. Now, I realize that as far as this particular situation is concerned, even if Microsoft decided to add support for DataTableRegion() tomorrow, it wouldn't really help because there are too many different versions of Windows in the field and there's no way to retroactively patch them all. (Gee, that sounds familiar...) Nevertheless, am I correct in saying that this is in fact a bug in Microsoft's ACPI implementation (both in their ASL compiler and in the AML parser)? Unless DataTableRegion() is specified to be optional in some way (I don't know if it is or not, but I doubt it), this sounds like an clear cut case of non- compliance with the ACPI spec. And if that's true, isn't there any way to get Microsoft to fix it? -Bill > > I suggest we go back to the last Gal's series > > which is though not universal but a simple and > > straightforward solution. > > That series with comments addressed probably > > is what we need in the end. > > I agree (I commented the same on the RHBZ too). The only one requirement > we might not satisfy with that is that the page containing the > generation ID must always be mapped as cacheable. In practice it doesn't > seem to cause issues. > > We tried to play nice, but given that (a) the vmgenid doc doesn't > mention a real requirement about the _CRS -- ie. no IO descriptors are > allowed to be in it --, (b) Windows doesn't support DataTableRegion(), I > doubt we could cover our bases 100% anyway. There can be any number of > further hidden requirements, and hidden gaps in ACPI support too, so > it's just business as usual with Windows: whatever works, works, don't > ask why. > > Just my opinion of course. > > Laszlo > > >> The only crazy thing you didn't try is to use > >> an XSDT instead of the DSDT. > >> I find it unlikely that this will help ... > > ___ > edk2-devel mailing list > edk2-de...@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel -- = -Bill Paul(510) 749-2329 | Senior Member of Technical Staff, wp...@windriver.com | Master of Unix-Fu - Wind River Systems = "I put a dollar in a change machine. Nothing changed." - George Carlin =
[Qemu-devel] Fix for incorrect SYSRET instruction implementation -- anyone looked at this yet?
Nobody has commented on this yet. According to my reading of the Intel documentation, the SYSRET instruction is supposed to force the RPL bits of the %ss register to 3 when returning to user mode. The actual sequence is: SS.Selector -- (IA32_STAR[63:48]+8) OR 3; (* RPL forced to 3 *) However, the code in helper_sysret() leaves them at 0 (in other words, the OR 3 part of the above sequence is missing). It does set the privilege level bits of %cs correctly though. This has caused me trouble with some of my VxWorks development: code that runs okay on real hardware will crash on QEMU, unless I apply the patch below. Can someone confirm that this is in fact a real bug? The Intel architecture manual seems quite clear about the SYSRET behavior. The bug seems to have been around as far back as QEMU 0.10.5. I am using QEMU 2.2.0 on FreeBSD/amd64 9.1-RELEASE. -Bill Signed-off-by: Bill Paul wp...@windriver.com --- target-i386/seg_helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c index fa374d0..2bc757a 100644 --- a/target-i386/seg_helper.c +++ b/target-i386/seg_helper.c @@ -1043,7 +1043,7 @@ void helper_sysret(CPUX86State *env, int dflag) DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK); env-eip = (uint32_t)env-regs[R_ECX]; } -cpu_x86_load_seg_cache(env, R_SS, selector + 8, +cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3, 0, 0x, DESC_G_MASK | DESC_B_MASK | DESC_P_MASK | DESC_S_MASK | (3 DESC_DPL_SHIFT) | @@ -1056,7 +1056,7 @@ void helper_sysret(CPUX86State *env, int dflag) DESC_S_MASK | (3 DESC_DPL_SHIFT) | DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK); env-eip = (uint32_t)env-regs[R_ECX]; -cpu_x86_load_seg_cache(env, R_SS, selector + 8, +cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3, 0, 0x, DESC_G_MASK | DESC_B_MASK | DESC_P_MASK | DESC_S_MASK | (3 DESC_DPL_SHIFT) | -- 1.8.0 -- = -Bill Paul(510) 749-2329 | Senior Member of Technical Staff, wp...@windriver.com | Master of Unix-Fu - Wind River Systems = I put a dollar in a change machine. Nothing changed. - George Carlin =
Re: [Qemu-devel] Fix for incorrect SYSRET instruction implementation -- anyone looked at this yet?
Of all the gin joints in all the towns in all the world, Stefan Weil had to walk into mine at 15:23:36 on Monday 09 March 2015 and say: Hi Bill, sending text only e-mails might help. Usually git send-email is better than using KMail or other mail software. You know, I thought I was sending in plain text. Somewhere along the line KMail lied to me. Use tools like git gui to get a correct signature. Any optional personal comments should come directly after the line with ---. After? Or before? Could you please re-send your patch? Check it before sending with scripts/checkpatch.pl. Maybe this looks like much regulations to fix a bug, but some rules are necessary to keep a project like QEMU alive. I'm going to send it once more because it was entirely my fault that my stupid mailer kept sending in HTML mode, but that's it. -Bill -- = -Bill Paul(510) 749-2329 | Senior Member of Technical Staff, wp...@windriver.com | Master of Unix-Fu - Wind River Systems = I put a dollar in a change machine. Nothing changed. - George Carlin =
[Qemu-devel] Fix for incorrect SYSRET instruction implementation -- anyone looked at this yet?
I'm certain I'm sending this in plain text mode this time. According to my reading of the Intel documentation, the SYSRET instruction is supposed to force the RPL bits of the %ss register to 3 when returning to user mode. The actual sequence is: SS.Selector -- (IA32_STAR[63:48]+8) OR 3; (* RPL forced to 3 *) However, the code in helper_sysret() leaves them at 0 (in other words, the OR 3 part of the above sequence is missing). It does set the privilege level bits of %cs correctly though. This has caused me trouble with some of my VxWorks development: code that runs okay on real hardware will crash on QEMU, unless I apply the patch below. Can someone confirm that this is in fact a real bug? The Intel architecture manual seems quite clear about the SYSRET behavior. The bug seems to have been around as far back as QEMU 0.10.5. I am using QEMU 2.2.0 on FreeBSD/amd64 9.1-RELEASE. -Bill Signed-off-by: Bill Paul wp...@windriver.com --- target-i386/seg_helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c index fa374d0..2bc757a 100644 --- a/target-i386/seg_helper.c +++ b/target-i386/seg_helper.c @@ -1043,7 +1043,7 @@ void helper_sysret(CPUX86State *env, int dflag) DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK); env-eip = (uint32_t)env-regs[R_ECX]; } -cpu_x86_load_seg_cache(env, R_SS, selector + 8, +cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3, 0, 0x, DESC_G_MASK | DESC_B_MASK | DESC_P_MASK | DESC_S_MASK | (3 DESC_DPL_SHIFT) | @@ -1056,7 +1056,7 @@ void helper_sysret(CPUX86State *env, int dflag) DESC_S_MASK | (3 DESC_DPL_SHIFT) | DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK); env-eip = (uint32_t)env-regs[R_ECX]; -cpu_x86_load_seg_cache(env, R_SS, selector + 8, +cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3, 0, 0x, DESC_G_MASK | DESC_B_MASK | DESC_P_MASK | DESC_S_MASK | (3 DESC_DPL_SHIFT) | -- 1.8.0
[Qemu-devel] Fix for bug in implementation of SYSRET instruction for x86-64
(I tried sending this once earlier but it seems to have disappeared into the void. Apologies if it ends up showing up twice.) Hi guys. I seem to have found a bug in the helper_systet() function in target-i386/seg_helper.c. I downloaded the Intel architecture manual from here: http://www.intel.com/content/www/us/en/processors/architectures-software- developer-manuals.html And it describes the behavior of SYSRET with regards to the stack selector (SS) as follows: SS.Selector -- (IA32_STAR[63:48]+8) OR 3; (* RPL forced to 3 *) In other words, the value of the SS register is supposed to be loaded from bits 63:48 of the IA32_STAR model-specific register (MSR), incremented by 8, _AND_ ORed with 3. ORing in the 3 sets the privilege level to 3 (user). This is done since SYSRET returns to user mode after a system call. The code in helper_sysret() performs the increment by 8 step, but _NOT_ the OR with 3 step. Here's another description of the SYSRET instruction which says basically the same thing, though in slightly different format: http://tptp.cc/mirrors/siyobik.info/instruction/SYSRET.html [...] SS(SEL) = IA32_STAR[63:48] + 8; SS(PL) = 0x3; [...] The effect of this bug is that when x86-64 code uses the SYSCALL instruction to enter kernel mode, the SYSRET instruction will return the CPU to user mode with the SS register incorrectly set (the privilege level bits will be 0). In my case, the original SS value for user mode was 0x2B, but after the return from SYSRET, it was changed to 0x28. It took me quite some time to realize this was due to a bug in QEMU rather than my own code. This caused a problem later when the user-mode code was preempted by an interrupt. At the end of the interrupt handling, an IRET instruction was used to exit the interrupt context, and the IRET instruction would generate a general protection fault because it would attempt to validate the stack selector value and notice that it was set for PL 0 (supervisor) instead of PL 3 (user). From my reading, the code is quite clearly in error with respect to the Intel documentation, and fixing the bug in my local sources results in my test code (which runs correctly on real hardware) working correctly in QEMU. It seems this bug has been there for a long time -- I happened to have the sources for QEMU 0.10.5 and the bug is there too (in target-i386/op_helper.c). I'm puzzled as to how it's gone unnoticed for so long, which has me thinking that maybe I'm missing something. I'm pretty sure this is wrong though. I'm attaching a patch for this below. It's a pretty simple fix. Comments/questions welcome. -Bill -- = -Bill Paul(510) 749-2329 | Senior Member of Technical Staff, wp...@windriver.com | Master of Unix-Fu - Wind River Systems = I put a dollar in a change machine. Nothing changed. - George Carlin = Signed-off-by: Bill Paul wp...@windriver.com --- target-i386/seg_helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c index fa374d0..2bc757a 100644 --- a/target-i386/seg_helper.c +++ b/target-i386/seg_helper.c @@ -1043,7 +1043,7 @@ void helper_sysret(CPUX86State *env, int dflag) DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK); env-eip = (uint32_t)env-regs[R_ECX]; } -cpu_x86_load_seg_cache(env, R_SS, selector + 8, +cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3, 0, 0x, DESC_G_MASK | DESC_B_MASK | DESC_P_MASK | DESC_S_MASK | (3 DESC_DPL_SHIFT) | @@ -1056,7 +1056,7 @@ void helper_sysret(CPUX86State *env, int dflag) DESC_S_MASK | (3 DESC_DPL_SHIFT) | DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK); env-eip = (uint32_t)env-regs[R_ECX]; -cpu_x86_load_seg_cache(env, R_SS, selector + 8, +cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3, 0, 0x, DESC_G_MASK | DESC_B_MASK | DESC_P_MASK | DESC_S_MASK | (3 DESC_DPL_SHIFT) | -- 1.8.0 From e85d3d374594f6c31e1bf72f8912334194be8654 Mon Sep 17 00:00:00 2001 From: Bill Paul wp...@windriver.com Date: Wed, 4 Mar 2015 09:19:03 -0800 Subject: [PATCH] This checkin corrects a bug in the implementation of the sysret instruction. Per the Intel architecture manual, the stack selector (SS) is to be loaded from the IA32_STAR register, incremented by 8, _AND_ ORed with 3 (this forces the privilege level to 3). The latter step (ORing in the 3) is missing. This breaks
[Qemu-devel] [PATCH] Fix bug in implementation of SYSRET instruction for x86-64
Hi guys. I seem to have found a bug in the helper_systet() function in target-i386/seg_helper.c. I downloaded the Intel architecture manual from here: http://www.intel.com/content/www/us/en/processors/architectures-software- developer-manuals.html And it describes the behavior of SYSRET with regards to the stack selector (SS) as follows: SS.Selector -- (IA32_STAR[63:48]+8) OR 3; (* RPL forced to 3 *) In other words, the value of the SS register is supposed to be loaded from bits 63:48 of the IA32_STAR model-specific register (MSR), incremented by 8, _AND_ ORed with 3. ORing in the 3 sets the privilege level to 3 (user). This is done since SYSRET returns to user mode after a system call. The code in helper_sysret() performs the increment by 8 step, but _NOT_ the OR with 3 step. Here's another description of the SYSRET instruction which says basically the same thing, though in slightly different format: http://tptp.cc/mirrors/siyobik.info/instruction/SYSRET.html [...] SS(SEL) = IA32_STAR[63:48] + 8; SS(PL) = 0x3; [...] The effect of this bug is that when x86-64 code uses the SYSCALL instruction to enter kernel mode, the SYSRET instruction will return the CPU to user mode with the SS register incorrectly set (the privilege level bits will be 0). In my case, the original SS value for user mode was 0x2B, but after the return from SYSRET, it was changed to 0x28. It took me quite some time to realize this was due to a bug in QEMU rather than my own code. This caused a problem later when the user-mode code was preempted by an interrupt. At the end of the interrupt handling, an IRET instruction was used to exit the interrupt context, and the IRET instruction would generate a general protection fault because it would attempt to validate the stack selector value and notice that it was set for PL 0 (supervisor) instead of PL 3 (user). From my reading, the code is quite clearly in error with respect to the Intel documentation, and fixing the bug in my local sources results in my test code (which runs correctly on real hardware) working correctly in QEMU. It seems this bug has been there for a long time -- I happened to have the sources for QEMU 0.10.5 and the bug is there too (in target-i386/op_helper.c). I'm puzzled as to how it's gone unnoticed for so long, which has me thinking that maybe I'm missing something. I'm pretty sure this is wrong though. I'm including a patch to fix this below. It seems to fix the problem quite nicely on my QEMU 2.2.0 installation. I'm also attaching a separate copy in case my mail client mangles the formatting on the inline copy. -Bill -- = -Bill Paul(510) 749-2329 | Senior Member of Technical Staff, wp...@windriver.com | Master of Unix-Fu - Wind River Systems = I put a dollar in a change machine. Nothing changed. - George Carlin = Signed-off-by: Bill Paul wp...@windriver.com --- target-i386/seg_helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c index fa374d0..2bc757a 100644 --- a/target-i386/seg_helper.c +++ b/target-i386/seg_helper.c @@ -1043,7 +1043,7 @@ void helper_sysret(CPUX86State *env, int dflag) DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK); env-eip = (uint32_t)env-regs[R_ECX]; } -cpu_x86_load_seg_cache(env, R_SS, selector + 8, +cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3, 0, 0x, DESC_G_MASK | DESC_B_MASK | DESC_P_MASK | DESC_S_MASK | (3 DESC_DPL_SHIFT) | @@ -1056,7 +1056,7 @@ void helper_sysret(CPUX86State *env, int dflag) DESC_S_MASK | (3 DESC_DPL_SHIFT) | DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK); env-eip = (uint32_t)env-regs[R_ECX]; -cpu_x86_load_seg_cache(env, R_SS, selector + 8, +cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3, 0, 0x, DESC_G_MASK | DESC_B_MASK | DESC_P_MASK | DESC_S_MASK | (3 DESC_DPL_SHIFT) | -- 1.8.0 From e85d3d374594f6c31e1bf72f8912334194be8654 Mon Sep 17 00:00:00 2001 From: Bill Paul wp...@windriver.com Date: Wed, 4 Mar 2015 09:19:03 -0800 Subject: [PATCH] This checkin corrects a bug in the implementation of the sysret instruction. Per the Intel architecture manual, the stack selector (SS) is to be loaded from the IA32_STAR register, incremented by 8, _AND_ ORed with 3 (this forces the privilege level to 3). The latter step (ORing in the 3) is missing. This breaks the machine
[Qemu-devel] [Bug 1428352] [NEW] SYSRET instruction incorrectly implemented
Public bug reported: The Intel architecture manual states that when returning to user mode, the SYSRET instruction will re-load the stack selector (%ss) from the IA32_STAR model specific register using the following logic: SS.Selector -- (IA32_STAR[63:48]+8) OR 3; (* RPL forced to 3 *) Another description of the instruction behavior which shows the same logic in a slightly different form can also be found here: http://tptp.cc/mirrors/siyobik.info/instruction/SYSRET.html [...] SS(SEL) = IA32_STAR[63:48] + 8; SS(PL) = 0x3; [...] In other words, the value of the %ss register is supposed to be loaded from bits 63:48 of the IA32_STAR model-specific register, incremented by 8, and then ORed with 3. ORing in the 3 sets the privilege level to 3 (user). This is done since SYSRET returns to user mode after a system call. However, helper_sysret() in target-i386/seg_helper.c does not do the OR 3 step. The code looks like this: cpu_x86_load_seg_cache(env, R_SS, selector + 8, 0, 0x, DESC_G_MASK | DESC_B_MASK | DESC_P_MASK | DESC_S_MASK | (3 DESC_DPL_SHIFT) | DESC_W_MASK | DESC_A_MASK); It should look like this: cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3, 0, 0x, DESC_G_MASK | DESC_B_MASK | DESC_P_MASK | DESC_S_MASK | (3 DESC_DPL_SHIFT) | DESC_W_MASK | DESC_A_MASK); The code does correctly set the privilege level bits for the code selector register (%cs) but not for the stack selector (%ss). The effect of this is that when SYSRET returns control to the user-mode caller, %ss will be have the privilege level bits cleared. In my case, it went from 0x2b to 0x28. This caused a crash later: when the user-mode code was preempted by an interrupt, and the interrupt handler would do an IRET, a general protection fault would occur because the %ss value being loaded from the exception frame was not valid for user mode. (At least, I think that's what happened.) This behavior seems inconsistent with real hardware, and also appears to be wrong with respect to the Intel documentation, so I'm pretty confident in calling this a bug. :) Note that this issue seems to have been around for a long time. I discovered it while using QEMU 2.2.0, but I happened to have the sources for QEMU 0.10.5, and the problem is there too (in os_helper.c). I am using FreeBSD/amd64 9.1-RELEASE as my host system, without KVM. The fix is fairly simple. I'm attaching a patch which worked for me. Using this fix, the code that I'm testing now behaves the same on the QEMU virtual machine as on real hardware. - Bill (wp...@windriver.com) ** Affects: qemu Importance: Undecided Status: New ** Attachment added: Patch to correct helper_sysret() https://bugs.launchpad.net/bugs/1428352/+attachment/4334723/+files/0001-This-checkin-corrects-a-bug-in-the-implementation-of.patch -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1428352 Title: SYSRET instruction incorrectly implemented Status in QEMU: New Bug description: The Intel architecture manual states that when returning to user mode, the SYSRET instruction will re-load the stack selector (%ss) from the IA32_STAR model specific register using the following logic: SS.Selector -- (IA32_STAR[63:48]+8) OR 3; (* RPL forced to 3 *) Another description of the instruction behavior which shows the same logic in a slightly different form can also be found here: http://tptp.cc/mirrors/siyobik.info/instruction/SYSRET.html [...] SS(SEL) = IA32_STAR[63:48] + 8; SS(PL) = 0x3; [...] In other words, the value of the %ss register is supposed to be loaded from bits 63:48 of the IA32_STAR model-specific register, incremented by 8, and then ORed with 3. ORing in the 3 sets the privilege level to 3 (user). This is done since SYSRET returns to user mode after a system call. However, helper_sysret() in target-i386/seg_helper.c does not do the OR 3 step. The code looks like this: cpu_x86_load_seg_cache(env, R_SS, selector + 8, 0, 0x, DESC_G_MASK | DESC_B_MASK | DESC_P_MASK | DESC_S_MASK | (3 DESC_DPL_SHIFT) | DESC_W_MASK | DESC_A_MASK); It should look like this: cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3, 0, 0x, DESC_G_MASK | DESC_B_MASK | DESC_P_MASK | DESC_S_MASK | (3 DESC_DPL_SHIFT) | DESC_W_MASK | DESC_A_MASK); The code does
Re: [Qemu-devel] [PATCH] e1000: make ICS write-only
Of all the gin joints in all the towns in all the world, Michael S. Tsirkin had to walk into mine at 02:51:00 on Wednesday 09 January 2013 and say: Since commit b1332393cdd7d023de8f1f8aa136ee7866a18968, qemu started updating ICS register when interrupt is sent, with the intent to match spec better (guests do not actually read this register). However, the function set_interrupt_cause where ICS is updated is often called internally by device emulation so reading it does not produce the last value written by driver. Looking closer at the spec, it documents ICS as write-only, so there's no need to update it at all. I conclude that while harmless this line is useless code so removing it is a bit cleaner than keeping it in. You are wrong. I know what the spec says. The spec lies (or at the very least, it doesn't agree with reality). With actual PRO/1000 hardware, the ICS register is readable. It provides a way to obtain the currently pending interrupt bits without the auto-clear behavior of the ICR register (which in some cases is actually detrimental). The Intel 10GbE NICs (82598, 82599, x540) are the same way (they're very similar in design to the PRO/1000s, particularly the 82575, 82576, 82580 and later devices). The spes for the 10GbE devices _also_ say that ICS is read only. But if you look at the Intel drivers for those chips, you'll see they have actually implemented a workaround for a device errata (I think the for the 82598) that actually requires reading the ICS register. If you had implemented a PRO/10GbE virtual device based on the same chip and obeyed the spec the same way, I suspect Intel's driver would break. I actually have in my possession real PRO/1000 silicon going all the way back to the 82543 and have tested every single one of them, and they all work like this. I've also asked the Intel LAN access division people about it and they said that in spite of it not being documented as readable, there's nothing particularly wrong with doing this. The problem with using ICR is that the auto-clear behavior can sometimes result in some awkward interrupt handling code. You need to test ICR in interrupt context to see if there are events pending, and then schedule some other thread to handle them. But since reading ICR clears the bits, you need to save the events somewhere so that the other thread knows what events need servicing. Keeping the saved copy of pending events properly synchronized so that you don't lose any is actually big challenge. The VxWorks PRO/1000 driver used to have some very ugly code in it to deal with it, all of which became much simpler when using the ICS register instead. I know what the spec says. But this is a case where the spec only says that because Intel decided not to disclose what the hardware actually does. They also don't tell you about all the hidden debug registers in the hardware either, but that doesn't mean they don't exist. So pretty please, with sugar on top, leave this code alone. -Bill Tested with windows and linux guests. Cc: Bill Paul wp...@windriver.com Reported-by: Yan Vugenfirer y...@daynix.com Signed-off-by: Michael S. Tsirkin m...@redhat.com --- hw/e1000.c | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/e1000.c b/hw/e1000.c index 92fb00a..928d804 100644 --- a/hw/e1000.c +++ b/hw/e1000.c @@ -230,7 +230,6 @@ set_interrupt_cause(E1000State *s, int index, uint32_t val) 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); } -- = -Bill Paul(510) 749-2329 | Member of Technical Staff, wp...@windriver.com | Master of Unix-Fu - Wind River Systems = I put a dollar in a change machine. Nothing changed. - George Carlin =
Re: [Qemu-devel] [PATCH] e1000: make ICS write-only
Of all the gin joints in all the towns in all the world, Michael S. Tsirkin had to walk into mine at 14:07:53 on Wednesday 09 January 2013 and say: On Wed, Jan 09, 2013 at 01:50:39PM -0600, Anthony Liguori wrote: I figured this would be the case but this is where a comment in the code or commit message would have helped a lot in avoiding future confusion. Regards, Anthony Liguori I just sent a documentation patch, please take a look. Thanks! Well, strictly speaking, VxWorks doesn't rely on the ICS behavior anymore, but it prefers it. I ran into problems with VMware and Simics too, as they also don't emulate the ICS behavior, and getting that fixed was more of an uphill battle. So as a compromise I added a check to the driver to see if the ICS register has 'real hardware behavior' or 'emulated behavior' and made it use an alternate interrupt handling scheme in the emulated case. The alternate scheme is a little less efficient than the ICS scheme in some cases (mainly when the PRO/1000 device's interrupt is shared), but it still handles interrupts reliably. That's a minor nit though. I'm fine with the comments as is. I just didn't want you do think VxWorks was completely brain damaged. :) Note that I don't think VMware emulates the 'flexible RX mode' descriptor mechanism for the PRO/100 either, because the non-NDAed PRO/100 manual doesn't bother to mention it exists. I think this is something that was fixed in QEMU but I haven't had a chance to test it in a while. There's a similar problem with simulated the AMD PCnet/PCI devices (they only support the 'configuration block' setup method -- for the older non-PCI LANCE chips that was the only way to configure them but PCI devices starting with the am97c970 can be configured just by setting up registers). Honestly I'm surprised I still have all my hair and that it's still the same color. -Bill -- = -Bill Paul(510) 749-2329 | Member of Technical Staff, wp...@windriver.com | Master of Unix-Fu - Wind River Systems = I put a dollar in a change machine. Nothing changed. - George Carlin =
Re: [Qemu-devel] [PATCH] e1000: make ICS write-only
Of all the gin joints in all the towns in all the world, Michael S. Tsirkin had to walk into mine at 13:44:38 on Wednesday 09 January 2013 and say: On Wed, Jan 09, 2013 at 09:30:43AM -0800, Bill Paul wrote: Of all the gin joints in all the towns in all the world, Michael S. Tsirkin had to walk into mine at 02:51:00 on Wednesday 09 January 2013 and say: Since commit b1332393cdd7d023de8f1f8aa136ee7866a18968, qemu started updating ICS register when interrupt is sent, with the intent to match spec better (guests do not actually read this register). However, the function set_interrupt_cause where ICS is updated is often called internally by device emulation so reading it does not produce the last value written by driver. Looking closer at the spec, it documents ICS as write-only, so there's no need to update it at all. I conclude that while harmless this line is useless code so removing it is a bit cleaner than keeping it in. You are wrong. I know what the spec says. The spec lies (or at the very least, it doesn't agree with reality). With actual PRO/1000 hardware, the ICS register is readable. It provides a way to obtain the currently pending interrupt bits without the auto-clear behavior of the ICR register (which in some cases is actually detrimental). The Intel 10GbE NICs (82598, 82599, x540) are the same way (they're very similar in design to the PRO/1000s, particularly the 82575, 82576, 82580 and later devices). The spes for the 10GbE devices _also_ say that ICS is read only. But if you look at the Intel drivers for those chips, you'll see they have actually implemented a workaround for a device errata (I think the for the 82598) that actually requires reading the ICS register. If you had implemented a PRO/10GbE virtual device based on the same chip and obeyed the spec the same way, I suspect Intel's driver would break. I actually have in my possession real PRO/1000 silicon going all the way back to the 82543 and have tested every single one of them, and they all work like this. I've also asked the Intel LAN access division people about it and they said that in spite of it not being documented as readable, there's nothing particularly wrong with doing this. The problem with using ICR is that the auto-clear behavior can sometimes result in some awkward interrupt handling code. You need to test ICR in interrupt context to see if there are events pending, and then schedule some other thread to handle them. But since reading ICR clears the bits, you need to save the events somewhere so that the other thread knows what events need servicing. Keeping the saved copy of pending events properly synchronized so that you don't lose any is actually big challenge. The VxWorks PRO/1000 driver used to have some very ugly code in it to deal with it, all of which became much simpler when using the ICS register instead. I know what the spec says. But this is a case where the spec only says that because Intel decided not to disclose what the hardware actually does. They also don't tell you about all the hidden debug registers in the hardware either, but that doesn't mean they don't exist. So pretty please, with sugar on top, leave this code alone. -Bill OK now since there's no spec, I'd like to find out how does the register behave. Let's assume I read ICR. This clears ICR - does it also clear ICS? Yes. ICS mirrors ICR: reading ICS lets you peek at the contents of ICR without auto-clearing them. If you read ICR, you're acknowledging any events that may be pending. Let's say the LSC (link state change) becomes set, because you unplugged the cable. The LSC bit becomes set in both ICR and in ICS. If you read ICS first, then you'll see the LSC bit is set, and it'll stay set (the event stays unacked). If you read ICR first, then you'll see the LSC bit is set, and you'll clear it (the event is acked). If you read ICS _after_ you read ICR, then LSC will be clear (because reading ICR first acked it). -Bill Thanks, MST Tested with windows and linux guests. Cc: Bill Paul wp...@windriver.com Reported-by: Yan Vugenfirer y...@daynix.com Signed-off-by: Michael S. Tsirkin m...@redhat.com --- hw/e1000.c | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/e1000.c b/hw/e1000.c index 92fb00a..928d804 100644 --- a/hw/e1000.c +++ b/hw/e1000.c @@ -230,7 +230,6 @@ set_interrupt_cause(E1000State *s, int index, uint32_t val) 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); } -- = -Bill Paul(510) 749-2329 | Member of Technical Staff, wp...@windriver.com | Master of Unix-Fu
