Re-check for ast after ast handler
ClosedPublic
Actions

Authored by kib on Sep 14 2015, 10:23 PM.

Details

Reviewers

andrew

Commits

rS288115: Re-check for new ast after ast was handled. We should not return to

Summary

MD code must recheck for new ast scheduled after the existing ast was handled. It seems that the branch was missed as result of typo, the code is correctly structured to reenter the loop.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

kib updated this revision to Diff 8760.Sep 14 2015, 10:23 PM

kib retitled this revision from to Re-check for ast after ast handler.

kib updated this object.

kib edited the test plan for this revision. (Show Details)

kib added a reviewer: andrew.

kib set the repository for this revision to rS FreeBSD src repository - subversion.

kib added a project: arm64.

Herald added subscribers: emaste, imp. · View Herald TranscriptSep 14 2015, 10:23 PM

Seems like it ought to have a comment
Lines 123-125 are the same as 101-103, no?

Indeed, it seems that placing the 1: label earlier would allow to remove the daif read duplication.

Remove duplication; add comment.

andrew added inline comments.Sep 18 2015, 1:08 PM

sys/arm64/arm64/exception.S
101	I think you could move this to the `daifset` instruction below. Unless there is a case where interrupts are left dis/enabled after calling ast this shouldn't change. Along with this the restore interrupts code below can also be removed, the next instruction after do_ast disables interrupts as we will be modifying `x18`, which may be in an inconsistent state.

kib added inline comments.Sep 18 2015, 5:54 PM

sys/arm64/arm64/exception.S
101	I moved the '1' label to msr instruction. x19 is callee-saved. Do you propose to not restore the the interrupts state around the call to ast ? If yes, I do not think this is correct. Ast handling should be done with interrupts enabled. This way, I even think that we should not restore the interrupts, but explicitely enable them. But the patch is probably fine for now.