hw/intc/arm_gicv3_its: downgrade error_report to warn_report in kvm_arm_its_reset

[Qemu-devel] [PATCH] hw/intc/arm_gicv3_its: downgrade error_report to warn_report in kvm_arm_its_reset

Posted by Jia He 5 years, 9 months ago

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)) {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] hw/intc/arm_gicv3_its: downgrade error_report to warn_report in kvm_arm_its_reset

Posted by Peter Maydell 5 years, 9 months ago

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

Re: [Qemu-devel] [PATCH] hw/intc/arm_gicv3_its: downgrade error_report to warn_report in kvm_arm_its_reset

Posted by Jia He 5 years, 9 months ago

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.

-- 
Cheers,
Jia

Re: [Qemu-devel] [PATCH] hw/intc/arm_gicv3_its: downgrade error_report to warn_report in kvm_arm_its_reset

Posted by Peter Maydell 5 years, 9 months ago

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

Re: [Qemu-devel] [PATCH] hw/intc/arm_gicv3_its: downgrade error_report to warn_report in kvm_arm_its_reset

Posted by Peter Maydell 5 years, 8 months ago

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