| Message ID | 1531969910-32843-1-git-send-email-jia.he@hxt-semitech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19 July 2018 at 04:11, Jia He <hejianet@gmail.com> wrote: > In scripts/arch-run.bash of kvm-unit-tests, it will check the qemu > output log with: > if [ -z "$(echo "$errors" | grep -vi warning)" ]; then > > Thus without the warning prefix, all of the test fail. > > Since it is not unrecoverable error in kvm_arm_its_reset for > current implementation, downgrading the report from error to > warn makes sense. I think the counterargument would be that this should report an error, because you just asked the device to do something that it doesn't support (ie to do a clean reset). Since the device isn't going to behave correctly, the tests should fail, and the way to make them pass is to upgrade to a kernel that implements the device correctly (by implementing the necessary feature). But we could maybe move to warn_report() here -- I'm not sure what our rules are for what counts as an error and what counts as a warning. thanks -- PMM
Hi Peter。 Thanks for the comments On 7/19/2018 8:41 PM, Peter Maydell Wrote: > On 19 July 2018 at 04:11, Jia He <hejianet@gmail.com> wrote: >> In scripts/arch-run.bash of kvm-unit-tests, it will check the qemu >> output log with: >> if [ -z "$(echo "$errors" | grep -vi warning)" ]; then >> >> Thus without the warning prefix, all of the test fail. >> >> Since it is not unrecoverable error in kvm_arm_its_reset for >> current implementation, downgrading the report from error to >> warn makes sense. > > I think the counterargument would be that this should report > an error, because you just asked the device to do something > that it doesn't support (ie to do a clean reset). Since the > device isn't going to behave correctly, the tests should fail, > and the way to make them pass is to upgrade to a kernel that > implements the device correctly (by implementing the necessary > feature). > > But we could maybe move to warn_report() here -- I'm not > sure what our rules are for what counts as an error and > what counts as a warning. I reviewed the other error_report in qemu. Some of them are followed by exit(), but the rest are not. So it seems there is no definite rules for error_report. IMO, the best resolution is to change the output grep search criteria. But it is difficult for kvm-unit-tests to identify whether it is an error without exit() or a warning. The fastest resolution is moving error_report to warn_report.
On 20 July 2018 at 02:22, Jia He <hejianet@gmail.com> wrote: > Hi Peter。 Thanks for the comments > > On 7/19/2018 8:41 PM, Peter Maydell Wrote: >> On 19 July 2018 at 04:11, Jia He <hejianet@gmail.com> wrote: >>> In scripts/arch-run.bash of kvm-unit-tests, it will check the qemu >>> output log with: >>> if [ -z "$(echo "$errors" | grep -vi warning)" ]; then >>> >>> Thus without the warning prefix, all of the test fail. >>> >>> Since it is not unrecoverable error in kvm_arm_its_reset for >>> current implementation, downgrading the report from error to >>> warn makes sense. >> >> I think the counterargument would be that this should report >> an error, because you just asked the device to do something >> that it doesn't support (ie to do a clean reset). Since the >> device isn't going to behave correctly, the tests should fail, >> and the way to make them pass is to upgrade to a kernel that >> implements the device correctly (by implementing the necessary >> feature). >> >> But we could maybe move to warn_report() here -- I'm not >> sure what our rules are for what counts as an error and >> what counts as a warning. > I reviewed the other error_report in qemu. Some of them are followed > by exit(), but the rest are not. So it seems there is no definite > rules for error_report. > > IMO, the best resolution is to change the output grep search criteria. > But it is difficult for kvm-unit-tests to identify whether it is an > error without exit() or a warning. The fastest resolution is moving > error_report to warn_report. Well, as I say, this implementation of the ITS *should* fail tests -- it doesn't behave to spec. The fastest resolution is for you to upgrade your host kernel to one that has the bugfix :-) thanks -- PMM
On 19 July 2018 at 04:11, Jia He <hejianet@gmail.com> wrote: > In scripts/arch-run.bash of kvm-unit-tests, it will check the qemu > output log with: > if [ -z "$(echo "$errors" | grep -vi warning)" ]; then > > Thus without the warning prefix, all of the test fail. > > Since it is not unrecoverable error in kvm_arm_its_reset for > current implementation, downgrading the report from error to > warn makes sense. > > Signed-off-by: Jia He <jia.he@hxt-semitech.com> > --- > hw/intc/arm_gicv3_its_kvm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c > index 271ebe4..01573ab 100644 > --- a/hw/intc/arm_gicv3_its_kvm.c > +++ b/hw/intc/arm_gicv3_its_kvm.c > @@ -211,7 +211,7 @@ static void kvm_arm_its_reset(DeviceState *dev) > return; > } > > - error_report("ITS KVM: full reset is not supported by the host kernel"); > + warn_report("ITS KVM: full reset is not supported by the host kernel"); > > if (!kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS, > GITS_CTLR)) { Applied to target-arm.next, thanks. (I'm still not entirely sure of our warn-vs-error criteria, but I guess since we don't actually bail out of QEMU here it's more of a warning than an error.) -- PMM
diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c index 271ebe4..01573ab 100644 --- a/hw/intc/arm_gicv3_its_kvm.c +++ b/hw/intc/arm_gicv3_its_kvm.c @@ -211,7 +211,7 @@ static void kvm_arm_its_reset(DeviceState *dev) return; } - error_report("ITS KVM: full reset is not supported by the host kernel"); + warn_report("ITS KVM: full reset is not supported by the host kernel"); if (!kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS, GITS_CTLR)) {
In scripts/arch-run.bash of kvm-unit-tests, it will check the qemu output log with: if [ -z "$(echo "$errors" | grep -vi warning)" ]; then Thus without the warning prefix, all of the test fail. Since it is not unrecoverable error in kvm_arm_its_reset for current implementation, downgrading the report from error to warn makes sense. Signed-off-by: Jia He <jia.he@hxt-semitech.com> --- hw/intc/arm_gicv3_its_kvm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)