| Message ID | 20191021134357.14266-1-peter.maydell@linaro.org |
|---|---|
| Headers | show |
| Series | Convert sparc devices to new ptimer API | expand |
On 10/21/19 3:43 PM, Peter Maydell wrote: > This patchset converts the devices used by sparc machines to the new > ptimer API. > > Currently the ptimer design uses a QEMU bottom-half as its mechanism > for calling back into the device model using the ptimer when the > timer has expired. Unfortunately this design is fatally flawed, > because it means that there is a lag between the ptimer updating its > own state and the device callback function updating device state, and > guest accesses to device registers between the two can return > inconsistent device state. This was reported as a bug in a specific > timer device but it's a problem with the generic ptimer code: > https://bugs.launchpad.net/qemu/+bug/1777777 > > The updates to the individual ptimer devices are straightforward: > we need to add begin/commit calls around the various places that > modify the ptimer state, and use the new ptimer_init() function > to create the timer. > > Changes v1->v2: > * patches 2 and 3 are the old 1 and 2 and have been reviewed > * patch 1 is new and removes a pointless NULL check; without > this we'd probably have got Coverity errors when patch 3 > added a use of t->timer before the check for it being NULL > > thanks > --PMM > > > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > *** BLURB HERE *** > > Peter Maydell (3): > hw/timer/slavio_timer: Remove useless check for NULL t->timer > hw/timer/grlib_gptimer.c: Switch to transaction-based ptimer API > hw/timer/slavio_timer.c: Switch to transaction-based ptimer API Nitpicking, maybe reorder the grlib_gptimer patch last: hw/timer/slavio_timer: Remove useless check for NULL t->timer hw/timer/slavio_timer.c: Switch to transaction-based ptimer API hw/timer/grlib_gptimer.c: Switch to transaction-based ptimer API
On Mon, 21 Oct 2019 at 14:43, Peter Maydell <peter.maydell@linaro.org> wrote: > > This patchset converts the devices used by sparc machines to the new > ptimer API. > > Currently the ptimer design uses a QEMU bottom-half as its mechanism > for calling back into the device model using the ptimer when the > timer has expired. Unfortunately this design is fatally flawed, > because it means that there is a lag between the ptimer updating its > own state and the device callback function updating device state, and > guest accesses to device registers between the two can return > inconsistent device state. This was reported as a bug in a specific > timer device but it's a problem with the generic ptimer code: > https://bugs.launchpad.net/qemu/+bug/1777777 > > The updates to the individual ptimer devices are straightforward: > we need to add begin/commit calls around the various places that > modify the ptimer state, and use the new ptimer_init() function > to create the timer. > > Changes v1->v2: > * patches 2 and 3 are the old 1 and 2 and have been reviewed > * patch 1 is new and removes a pointless NULL check; without > this we'd probably have got Coverity errors when patch 3 > added a use of t->timer before the check for it being NULL I'm going to apply these to target-arm.next; I know they haven't been on list long but the change since v1 is only minor and they've all been reviewed. thanks -- PMM
On 24/10/2019 13:19, Peter Maydell wrote: > On Mon, 21 Oct 2019 at 14:43, Peter Maydell <peter.maydell@linaro.org> wrote: >> >> This patchset converts the devices used by sparc machines to the new >> ptimer API. >> >> Currently the ptimer design uses a QEMU bottom-half as its mechanism >> for calling back into the device model using the ptimer when the >> timer has expired. Unfortunately this design is fatally flawed, >> because it means that there is a lag between the ptimer updating its >> own state and the device callback function updating device state, and >> guest accesses to device registers between the two can return >> inconsistent device state. This was reported as a bug in a specific >> timer device but it's a problem with the generic ptimer code: >> https://bugs.launchpad.net/qemu/+bug/1777777 >> >> The updates to the individual ptimer devices are straightforward: >> we need to add begin/commit calls around the various places that >> modify the ptimer state, and use the new ptimer_init() function >> to create the timer. >> >> Changes v1->v2: >> * patches 2 and 3 are the old 1 and 2 and have been reviewed >> * patch 1 is new and removes a pointless NULL check; without >> this we'd probably have got Coverity errors when patch 3 >> added a use of t->timer before the check for it being NULL > > I'm going to apply these to target-arm.next; I know they haven't > been on list long but the change since v1 is only minor and > they've all been reviewed. Thanks Peter! Not sure if you saw my Tested-by tag last week for the slavio (sun4m) parts, but there were no obvious regressions that I could see under qemu-system-sparc. ATB, Mark.
On 10/24/19 8:04 PM, Mark Cave-Ayland wrote: > On 24/10/2019 13:19, Peter Maydell wrote: > >> On Mon, 21 Oct 2019 at 14:43, Peter Maydell <peter.maydell@linaro.org> wrote: >>> >>> This patchset converts the devices used by sparc machines to the new >>> ptimer API. >>> >>> Currently the ptimer design uses a QEMU bottom-half as its mechanism >>> for calling back into the device model using the ptimer when the >>> timer has expired. Unfortunately this design is fatally flawed, >>> because it means that there is a lag between the ptimer updating its >>> own state and the device callback function updating device state, and >>> guest accesses to device registers between the two can return >>> inconsistent device state. This was reported as a bug in a specific >>> timer device but it's a problem with the generic ptimer code: >>> https://bugs.launchpad.net/qemu/+bug/1777777 >>> >>> The updates to the individual ptimer devices are straightforward: >>> we need to add begin/commit calls around the various places that >>> modify the ptimer state, and use the new ptimer_init() function >>> to create the timer. >>> >>> Changes v1->v2: >>> * patches 2 and 3 are the old 1 and 2 and have been reviewed >>> * patch 1 is new and removes a pointless NULL check; without >>> this we'd probably have got Coverity errors when patch 3 >>> added a use of t->timer before the check for it being NULL >> >> I'm going to apply these to target-arm.next; I know they haven't >> been on list long but the change since v1 is only minor and >> they've all been reviewed. > > Thanks Peter! Not sure if you saw my Tested-by tag last week for the slavio (sun4m) > parts, but there were no obvious regressions that I could see under qemu-system-sparc. This was on v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg653861.html
On Thu, 24 Oct 2019 at 19:10, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > > On 24/10/2019 13:19, Peter Maydell wrote: > > I'm going to apply these to target-arm.next; I know they haven't > > been on list long but the change since v1 is only minor and > > they've all been reviewed. > > Thanks Peter! Not sure if you saw my Tested-by tag last week for the slavio (sun4m) > parts, but there were no obvious regressions that I could see under qemu-system-sparc. Yeah, I saw that, thanks for the testing. I decided that since I'd added patch 1 I didn't quite feel comfortable carrying the tested-by tag across. thanks -- PMM