[PATCH 0/2] mips: Minor simplifications for KVM use

[PATCH 0/2] mips: Minor simplifications for KVM use

[Philippe Mathieu-Daudé]

A pair of trivial patches while reviewing Huacai's
"KVM target support for MIPS64" series.

Philippe Mathieu-Daudé (2):
  hw/mips/mips_int: De-duplicate KVM interrupt delivery
  target/mips/kvm: Assert unreachable code is not used

 hw/mips/mips_int.c | 11 +++--------
 target/mips/kvm.c  |  8 ++------
 2 files changed, 5 insertions(+), 14 deletions(-)

-- 
2.21.1

[PATCH 1/2] hw/mips/mips_int: De-duplicate KVM interrupt delivery

[Philippe Mathieu-Daudé]

Refactor duplicated code in a single place.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/mips/mips_int.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
index 796730b11d..4a1bf846da 100644
--- a/hw/mips/mips_int.c
+++ b/hw/mips/mips_int.c
@@ -47,17 +47,12 @@ static void cpu_mips_irq_request(void *opaque, int irq, int level)
 
     if (level) {
         env->CP0_Cause |= 1 << (irq + CP0Ca_IP);
-
-        if (kvm_enabled() && irq == 2) {
-            kvm_mips_set_interrupt(cpu, irq, level);
-        }
-
     } else {
         env->CP0_Cause &= ~(1 << (irq + CP0Ca_IP));
+    }
 
-        if (kvm_enabled() && irq == 2) {
-            kvm_mips_set_interrupt(cpu, irq, level);
-        }
+    if (kvm_enabled() && irq == 2) {
+        kvm_mips_set_interrupt(cpu, irq, level);
     }
 
     if (env->CP0_Cause & CP0Ca_IP_mask) {
-- 
2.21.1

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

[Philippe Mathieu-Daudé]

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 1/2] hw/mips/mips_int: De-duplicate KVM interrupt delivery

[chen huacai]

Hi, Philippe,

On Wed, Apr 29, 2020 at 4:30 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Refactor duplicated code in a single place.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/mips/mips_int.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
> index 796730b11d..4a1bf846da 100644
> --- a/hw/mips/mips_int.c
> +++ b/hw/mips/mips_int.c
> @@ -47,17 +47,12 @@ static void cpu_mips_irq_request(void *opaque, int irq, int level)
>
>      if (level) {
>          env->CP0_Cause |= 1 << (irq + CP0Ca_IP);
> -
> -        if (kvm_enabled() && irq == 2) {
> -            kvm_mips_set_interrupt(cpu, irq, level);
> -        }
> -
>      } else {
>          env->CP0_Cause &= ~(1 << (irq + CP0Ca_IP));
> +    }
Since the if-else has become one line, so can we remove { and } here?

>
> -        if (kvm_enabled() && irq == 2) {
> -            kvm_mips_set_interrupt(cpu, irq, level);
> -        }
> +    if (kvm_enabled() && irq == 2) {
> +        kvm_mips_set_interrupt(cpu, irq, level);
>      }
>
>      if (env->CP0_Cause & CP0Ca_IP_mask) {
> --
> 2.21.1
>
>


-- 
Huacai Chen

Re: [PATCH 1/2] hw/mips/mips_int: De-duplicate KVM interrupt delivery

[Philippe Mathieu-Daudé]

On 4/29/20 10:48 AM, chen huacai wrote:
> Hi, Philippe,
> 
> On Wed, Apr 29, 2020 at 4:30 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Refactor duplicated code in a single place.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/mips/mips_int.c | 11 +++--------
>>   1 file changed, 3 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
>> index 796730b11d..4a1bf846da 100644
>> --- a/hw/mips/mips_int.c
>> +++ b/hw/mips/mips_int.c
>> @@ -47,17 +47,12 @@ static void cpu_mips_irq_request(void *opaque, int irq, int level)
>>
>>       if (level) {
>>           env->CP0_Cause |= 1 << (irq + CP0Ca_IP);
>> -
>> -        if (kvm_enabled() && irq == 2) {
>> -            kvm_mips_set_interrupt(cpu, irq, level);
>> -        }
>> -
>>       } else {
>>           env->CP0_Cause &= ~(1 << (irq + CP0Ca_IP));
>> +    }
> Since the if-else has become one line, so can we remove { and } here?

This is the QEMU coding style, see CODING_STYLE.rst:

Block structure
===============

Every indented statement is braced; even if the block contains just one
statement.  The opening brace is on the line that contains the control
flow statement that introduces the new block; the closing brace is on the
same line as the else keyword, or on a line by itself if there is no else
keyword.  Example:

.. code-block:: c

     if (a == 5) {
         printf("a was 5.\n");
     } else if (a == 6) {
         printf("a was 6.\n");
     } else {
         printf("a was something else entirely.\n");
     }

Rationale: a consistent (except for functions...) bracing style reduces
ambiguity and avoids needless churn when lines are added or removed.
Furthermore, it is the QEMU coding style.

> 
>>
>> -        if (kvm_enabled() && irq == 2) {
>> -            kvm_mips_set_interrupt(cpu, irq, level);
>> -        }
>> +    if (kvm_enabled() && irq == 2) {
>> +        kvm_mips_set_interrupt(cpu, irq, level);
>>       }
>>
>>       if (env->CP0_Cause & CP0Ca_IP_mask) {
>> --
>> 2.21.1
>>
>>
> 
> 

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

[Philippe Mathieu-Daudé]

+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 0/2] mips: Minor simplifications for KVM use

[Philippe Mathieu-Daudé]

ping...

On 4/29/20 10:29 AM, Philippe Mathieu-Daudé wrote:
> A pair of trivial patches while reviewing Huacai's
> "KVM target support for MIPS64" series.
> 
> Philippe Mathieu-Daudé (2):
>   hw/mips/mips_int: De-duplicate KVM interrupt delivery
>   target/mips/kvm: Assert unreachable code is not used
> 
>  hw/mips/mips_int.c | 11 +++--------
>  target/mips/kvm.c  |  8 ++------
>  2 files changed, 5 insertions(+), 14 deletions(-)
> 

Re: [PATCH 1/2] hw/mips/mips_int: De-duplicate KVM interrupt delivery

[Thomas Huth]

On 12/05/2020 09.08, Philippe Mathieu-Daudé wrote:
> On 4/29/20 10:48 AM, chen huacai wrote:
>> Hi, Philippe,
>>
>> On Wed, Apr 29, 2020 at 4:30 PM Philippe Mathieu-Daudé
>> <f4bug@amsat.org> wrote:
>>>
>>> Refactor duplicated code in a single place.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>   hw/mips/mips_int.c | 11 +++--------
>>>   1 file changed, 3 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
>>> index 796730b11d..4a1bf846da 100644
>>> --- a/hw/mips/mips_int.c
>>> +++ b/hw/mips/mips_int.c
>>> @@ -47,17 +47,12 @@ static void cpu_mips_irq_request(void *opaque,
>>> int irq, int level)
>>>
>>>       if (level) {
>>>           env->CP0_Cause |= 1 << (irq + CP0Ca_IP);
>>> -
>>> -        if (kvm_enabled() && irq == 2) {
>>> -            kvm_mips_set_interrupt(cpu, irq, level);
>>> -        }
>>> -
>>>       } else {
>>>           env->CP0_Cause &= ~(1 << (irq + CP0Ca_IP));
>>> +    }
>>>
>>> -        if (kvm_enabled() && irq == 2) {
>>> -            kvm_mips_set_interrupt(cpu, irq, level);
>>> -        }
>>> +    if (kvm_enabled() && irq == 2) {
>>> +        kvm_mips_set_interrupt(cpu, irq, level);
>>>       }
>>>
>>>       if (env->CP0_Cause & CP0Ca_IP_mask) {
>>> -- 
>>> 2.21.1

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 1/2] hw/mips/mips_int: De-duplicate KVM interrupt delivery

[Philippe Mathieu-Daudé]

On 5/26/20 9:57 AM, Thomas Huth wrote:
> On 12/05/2020 09.08, Philippe Mathieu-Daudé wrote:
>> On 4/29/20 10:48 AM, chen huacai wrote:
>>> Hi, Philippe,
>>>
>>> On Wed, Apr 29, 2020 at 4:30 PM Philippe Mathieu-Daudé
>>> <f4bug@amsat.org> wrote:
>>>>
>>>> Refactor duplicated code in a single place.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> ---
>>>>   hw/mips/mips_int.c | 11 +++--------
>>>>   1 file changed, 3 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
>>>> index 796730b11d..4a1bf846da 100644
>>>> --- a/hw/mips/mips_int.c
>>>> +++ b/hw/mips/mips_int.c
>>>> @@ -47,17 +47,12 @@ static void cpu_mips_irq_request(void *opaque,
>>>> int irq, int level)
>>>>
>>>>       if (level) {
>>>>           env->CP0_Cause |= 1 << (irq + CP0Ca_IP);
>>>> -
>>>> -        if (kvm_enabled() && irq == 2) {
>>>> -            kvm_mips_set_interrupt(cpu, irq, level);
>>>> -        }
>>>> -
>>>>       } else {
>>>>           env->CP0_Cause &= ~(1 << (irq + CP0Ca_IP));
>>>> +    }
>>>>
>>>> -        if (kvm_enabled() && irq == 2) {
>>>> -            kvm_mips_set_interrupt(cpu, irq, level);
>>>> -        }
>>>> +    if (kvm_enabled() && irq == 2) {
>>>> +        kvm_mips_set_interrupt(cpu, irq, level);
>>>>       }
>>>>
>>>>       if (env->CP0_Cause & CP0Ca_IP_mask) {
>>>> -- 
>>>> 2.21.1
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Thanks, queued to mips-next.

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

[Philippe Mathieu-Daudé]

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

[Paolo Bonzini]

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

[Huacai Chen]

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;
> >>
> >

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

[Philippe Mathieu-Daudé]

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.