[PATCH 2/2] target/mips/kvm: Assert unreachable code is not used

Philippe Mathieu-Daudé posted 2 patches 5 years, 9 months ago
[PATCH 2/2] target/mips/kvm: Assert unreachable code is not used
Posted by Philippe Mathieu-Daudé 5 years, 9 months ago
This code must not be used outside of KVM. Abort if it is.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/kvm.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/target/mips/kvm.c b/target/mips/kvm.c
index de3e26ef1f..050bfbd7fa 100644
--- a/target/mips/kvm.c
+++ b/target/mips/kvm.c
@@ -196,9 +196,7 @@ int kvm_mips_set_interrupt(MIPSCPU *cpu, int irq, int level)
     CPUState *cs = CPU(cpu);
     struct kvm_mips_interrupt intr;
 
-    if (!kvm_enabled()) {
-        return 0;
-    }
+    assert(kvm_enabled());
 
     intr.cpu = -1;
 
@@ -219,9 +217,7 @@ int kvm_mips_set_ipi_interrupt(MIPSCPU *cpu, int irq, int level)
     CPUState *dest_cs = CPU(cpu);
     struct kvm_mips_interrupt intr;
 
-    if (!kvm_enabled()) {
-        return 0;
-    }
+    assert(kvm_enabled());
 
     intr.cpu = dest_cs->cpu_index;
 
-- 
2.21.1


Re: [PATCH 2/2] target/mips/kvm: Assert unreachable code is not used
Posted by Philippe Mathieu-Daudé 5 years, 9 months ago
+Paolo

On 4/29/20 10:29 AM, Philippe Mathieu-Daudé wrote:
> This code must not be used outside of KVM. Abort if it is.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   target/mips/kvm.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/target/mips/kvm.c b/target/mips/kvm.c
> index de3e26ef1f..050bfbd7fa 100644
> --- a/target/mips/kvm.c
> +++ b/target/mips/kvm.c
> @@ -196,9 +196,7 @@ int kvm_mips_set_interrupt(MIPSCPU *cpu, int irq, int level)
>       CPUState *cs = CPU(cpu);
>       struct kvm_mips_interrupt intr;
>   
> -    if (!kvm_enabled()) {
> -        return 0;
> -    }
> +    assert(kvm_enabled());
>   
>       intr.cpu = -1;
>   
> @@ -219,9 +217,7 @@ int kvm_mips_set_ipi_interrupt(MIPSCPU *cpu, int irq, int level)
>       CPUState *dest_cs = CPU(cpu);
>       struct kvm_mips_interrupt intr;
>   
> -    if (!kvm_enabled()) {
> -        return 0;
> -    }
> +    assert(kvm_enabled());
>   
>       intr.cpu = dest_cs->cpu_index;
>   
> 

Re: [PATCH 2/2] target/mips/kvm: Assert unreachable code is not used
Posted by Philippe Mathieu-Daudé 5 years, 2 months ago
Huacai, ping?

On 5/12/20 9:09 AM, Philippe Mathieu-Daudé wrote:
> +Paolo
> 
> On 4/29/20 10:29 AM, Philippe Mathieu-Daudé wrote:
>> This code must not be used outside of KVM. Abort if it is.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   target/mips/kvm.c | 8 ++------
>>   1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/target/mips/kvm.c b/target/mips/kvm.c
>> index de3e26ef1f..050bfbd7fa 100644
>> --- a/target/mips/kvm.c
>> +++ b/target/mips/kvm.c
>> @@ -196,9 +196,7 @@ int kvm_mips_set_interrupt(MIPSCPU *cpu, int irq,
>> int level)
>>       CPUState *cs = CPU(cpu);
>>       struct kvm_mips_interrupt intr;
>>   -    if (!kvm_enabled()) {
>> -        return 0;
>> -    }
>> +    assert(kvm_enabled());
>>         intr.cpu = -1;
>>   @@ -219,9 +217,7 @@ int kvm_mips_set_ipi_interrupt(MIPSCPU *cpu, int
>> irq, int level)
>>       CPUState *dest_cs = CPU(cpu);
>>       struct kvm_mips_interrupt intr;
>>   -    if (!kvm_enabled()) {
>> -        return 0;
>> -    }
>> +    assert(kvm_enabled());
>>         intr.cpu = dest_cs->cpu_index;
>>  
> 

Re: [PATCH 2/2] target/mips/kvm: Assert unreachable code is not used
Posted by Paolo Bonzini 5 years, 2 months ago
On 24/11/20 11:41, Philippe Mathieu-Daudé wrote:
> Huacai, ping?
> 
> On 5/12/20 9:09 AM, Philippe Mathieu-Daudé wrote:
>> +Paolo
>>
>> On 4/29/20 10:29 AM, Philippe Mathieu-Daudé wrote:
>>> This code must not be used outside of KVM. Abort if it is.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>    target/mips/kvm.c | 8 ++------
>>>    1 file changed, 2 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/target/mips/kvm.c b/target/mips/kvm.c
>>> index de3e26ef1f..050bfbd7fa 100644
>>> --- a/target/mips/kvm.c
>>> +++ b/target/mips/kvm.c
>>> @@ -196,9 +196,7 @@ int kvm_mips_set_interrupt(MIPSCPU *cpu, int irq,
>>> int level)
>>>        CPUState *cs = CPU(cpu);
>>>        struct kvm_mips_interrupt intr;
>>>    -    if (!kvm_enabled()) {
>>> -        return 0;
>>> -    }
>>> +    assert(kvm_enabled());
>>>          intr.cpu = -1;
>>>    @@ -219,9 +217,7 @@ int kvm_mips_set_ipi_interrupt(MIPSCPU *cpu, int
>>> irq, int level)
>>>        CPUState *dest_cs = CPU(cpu);
>>>        struct kvm_mips_interrupt intr;
>>>    -    if (!kvm_enabled()) {
>>> -        return 0;
>>> -    }
>>> +    assert(kvm_enabled());
>>>          intr.cpu = dest_cs->cpu_index;
>>>   
>>
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

For kvm_mips_set_ipi_interrupt, however, it would be nicer if 
hw/intc/mips_gic.c always used gic->vps[vp].env->irq[], and the qemu_irq 
handler took care of calling kvm_mips_set_ipi_interrupt.

Likewise, there is some duplication between kvm_mips_interrupt's caller 
and kvm_arch_pre_run.  I'm not sure if kvm_arch_pre_run is needed at all.

Paolo


Re: [PATCH 2/2] target/mips/kvm: Assert unreachable code is not used
Posted by Philippe Mathieu-Daudé 5 years, 2 months ago
On 11/24/20 12:02 PM, Paolo Bonzini wrote:
> On 24/11/20 11:41, Philippe Mathieu-Daudé wrote:
>> Huacai, ping?
>>
>> On 5/12/20 9:09 AM, Philippe Mathieu-Daudé wrote:
>>> +Paolo
>>>
>>> On 4/29/20 10:29 AM, Philippe Mathieu-Daudé wrote:
>>>> This code must not be used outside of KVM. Abort if it is.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> ---
>>>>    target/mips/kvm.c | 8 ++------
>>>>    1 file changed, 2 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/target/mips/kvm.c b/target/mips/kvm.c
>>>> index de3e26ef1f..050bfbd7fa 100644
>>>> --- a/target/mips/kvm.c
>>>> +++ b/target/mips/kvm.c
>>>> @@ -196,9 +196,7 @@ int kvm_mips_set_interrupt(MIPSCPU *cpu, int irq,
>>>> int level)
>>>>        CPUState *cs = CPU(cpu);
>>>>        struct kvm_mips_interrupt intr;
>>>>    -    if (!kvm_enabled()) {
>>>> -        return 0;
>>>> -    }
>>>> +    assert(kvm_enabled());
>>>>          intr.cpu = -1;
>>>>    @@ -219,9 +217,7 @@ int kvm_mips_set_ipi_interrupt(MIPSCPU *cpu, int
>>>> irq, int level)
>>>>        CPUState *dest_cs = CPU(cpu);
>>>>        struct kvm_mips_interrupt intr;
>>>>    -    if (!kvm_enabled()) {
>>>> -        return 0;
>>>> -    }
>>>> +    assert(kvm_enabled());
>>>>          intr.cpu = dest_cs->cpu_index;
>>>>   
>>>
>>
> 
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> For kvm_mips_set_ipi_interrupt, however, it would be nicer if
> hw/intc/mips_gic.c always used gic->vps[vp].env->irq[], and the qemu_irq
> handler took care of calling kvm_mips_set_ipi_interrupt.
> 
> Likewise, there is some duplication between kvm_mips_interrupt's caller
> and kvm_arch_pre_run.  I'm not sure if kvm_arch_pre_run is needed at all.

OK, I'll have a look and ask Huacai to test.

Meanwhile, patch applied to mips-next.

Thanks,

Phil.

Re: [PATCH 2/2] target/mips/kvm: Assert unreachable code is not used
Posted by Huacai Chen 5 years, 2 months ago
Reviewed-by: Huacai Chen <chenhc@lemote.com>

On Tue, Nov 24, 2020 at 6:42 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Huacai, ping?
>
> On 5/12/20 9:09 AM, Philippe Mathieu-Daudé wrote:
> > +Paolo
> >
> > On 4/29/20 10:29 AM, Philippe Mathieu-Daudé wrote:
> >> This code must not be used outside of KVM. Abort if it is.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >> ---
> >>   target/mips/kvm.c | 8 ++------
> >>   1 file changed, 2 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/target/mips/kvm.c b/target/mips/kvm.c
> >> index de3e26ef1f..050bfbd7fa 100644
> >> --- a/target/mips/kvm.c
> >> +++ b/target/mips/kvm.c
> >> @@ -196,9 +196,7 @@ int kvm_mips_set_interrupt(MIPSCPU *cpu, int irq,
> >> int level)
> >>       CPUState *cs = CPU(cpu);
> >>       struct kvm_mips_interrupt intr;
> >>   -    if (!kvm_enabled()) {
> >> -        return 0;
> >> -    }
> >> +    assert(kvm_enabled());
> >>         intr.cpu = -1;
> >>   @@ -219,9 +217,7 @@ int kvm_mips_set_ipi_interrupt(MIPSCPU *cpu, int
> >> irq, int level)
> >>       CPUState *dest_cs = CPU(cpu);
> >>       struct kvm_mips_interrupt intr;
> >>   -    if (!kvm_enabled()) {
> >> -        return 0;
> >> -    }
> >> +    assert(kvm_enabled());
> >>         intr.cpu = dest_cs->cpu_index;
> >>
> >