[kvm-unit-tests PATCH 0/3] check multiple init for vm/smp

[kvm-unit-tests PATCH 0/3] check multiple init for vm/smp

[Peter Xu]

The first two patches check whether there are multiple init for
vm/smp, assertion fail will be triggered if so.

Patch 3 is to cleanup existing setup_igt() when smp_init() is called
(suggested by Drew).

Peter Xu (3):
  x86: vm: add assert to avoid multiple vm init
  x86: smp: assert to avoid multiple init of smp
  tests: don't call setup_idt() if with smp_init()

 lib/x86/smp.c             | 1 +
 lib/x86/vm.c              | 1 +
 x86/apic.c                | 1 -
 x86/hyperv_stimer.c       | 1 -
 x86/hyperv_synic.c        | 1 -
 x86/ioapic.c              | 1 -
 x86/tscdeadline_latency.c | 1 -
 7 files changed, 2 insertions(+), 5 deletions(-)

-- 
2.7.4

[kvm-unit-tests PATCH 1/3] x86: vm: add assert to avoid multiple vm init

[Peter Xu]

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 lib/x86/vm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/x86/vm.c b/lib/x86/vm.c
index 906fbf2..f7e778b 100644
--- a/lib/x86/vm.c
+++ b/lib/x86/vm.c
@@ -151,6 +151,7 @@ static void setup_mmu(unsigned long len)
 
 void setup_vm()
 {
+    assert(!end_of_memory);
     end_of_memory = fwcfg_get_u64(FW_CFG_RAM_SIZE);
     free_memory(&edata, end_of_memory - (unsigned long)&edata);
     setup_mmu(end_of_memory);
-- 
2.7.4

[kvm-unit-tests PATCH 2/3] x86: smp: assert to avoid multiple init of smp

[Peter Xu]

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 lib/x86/smp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/x86/smp.c b/lib/x86/smp.c
index 1eb49f2..1a05d0f 100644
--- a/lib/x86/smp.c
+++ b/lib/x86/smp.c
@@ -113,6 +113,7 @@ void smp_init(void)
     int i;
     void ipi_entry(void);
 
+    assert(_cpu_count == 0);
     _cpu_count = fwcfg_get_nb_cpus();
 
     setup_idt();
-- 
2.7.4

[kvm-unit-tests PATCH 3/3] tests: don't call setup_idt() if with smp_init()

[Peter Xu]

smp_init() will setup idt as default. No need to call it twice.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 x86/apic.c                | 1 -
 x86/hyperv_stimer.c       | 1 -
 x86/hyperv_synic.c        | 1 -
 x86/ioapic.c              | 1 -
 x86/tscdeadline_latency.c | 1 -
 5 files changed, 5 deletions(-)

diff --git a/x86/apic.c b/x86/apic.c
index fefb584..eff9a11 100644
--- a/x86/apic.c
+++ b/x86/apic.c
@@ -393,7 +393,6 @@ int main()
 {
     setup_vm();
     smp_init();
-    setup_idt();
 
     test_lapic_existence();
 
diff --git a/x86/hyperv_stimer.c b/x86/hyperv_stimer.c
index 6382938..5292523 100644
--- a/x86/hyperv_stimer.c
+++ b/x86/hyperv_stimer.c
@@ -330,7 +330,6 @@ static void stimer_test_all(void)
 
     setup_vm();
     smp_init();
-    setup_idt();
     enable_apic();
 
     handle_irq(SINT1_VEC, stimer_isr);
diff --git a/x86/hyperv_synic.c b/x86/hyperv_synic.c
index 3f98c42..74bbd58 100644
--- a/x86/hyperv_synic.c
+++ b/x86/hyperv_synic.c
@@ -154,7 +154,6 @@ int main(int ac, char **av)
 
         setup_vm();
         smp_init();
-        setup_idt();
         enable_apic();
 
         synic_prepare_sint_vecs();
diff --git a/x86/ioapic.c b/x86/ioapic.c
index a554e43..314c9bb 100644
--- a/x86/ioapic.c
+++ b/x86/ioapic.c
@@ -398,7 +398,6 @@ int main(void)
 {
 	setup_vm();
 	smp_init();
-	setup_idt();
 
 	mask_pic_interrupts();
 
diff --git a/x86/tscdeadline_latency.c b/x86/tscdeadline_latency.c
index 0592802..0617a1b 100644
--- a/x86/tscdeadline_latency.c
+++ b/x86/tscdeadline_latency.c
@@ -106,7 +106,6 @@ int main(int argc, char **argv)
 
     setup_vm();
     smp_init();
-    setup_idt();
 
     test_lapic_existence();
 
-- 
2.7.4

Re: [kvm-unit-tests PATCH 3/3] tests: don't call setup_idt() if with smp_init()

[Andrew Jones]

On Tue, Oct 25, 2016 at 05:00:30PM +0800, Peter Xu wrote:
> smp_init() will setup idt as default. No need to call it twice.

This patch should probably be squashed into the previous patch
to maintain bisectability.

> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  x86/apic.c                | 1 -
>  x86/hyperv_stimer.c       | 1 -
>  x86/hyperv_synic.c        | 1 -
>  x86/ioapic.c              | 1 -
>  x86/tscdeadline_latency.c | 1 -
>  5 files changed, 5 deletions(-)
> 
> diff --git a/x86/apic.c b/x86/apic.c
> index fefb584..eff9a11 100644
> --- a/x86/apic.c
> +++ b/x86/apic.c
> @@ -393,7 +393,6 @@ int main()
>  {
>      setup_vm();
>      smp_init();
> -    setup_idt();
>  
>      test_lapic_existence();
>  
> diff --git a/x86/hyperv_stimer.c b/x86/hyperv_stimer.c
> index 6382938..5292523 100644
> --- a/x86/hyperv_stimer.c
> +++ b/x86/hyperv_stimer.c
> @@ -330,7 +330,6 @@ static void stimer_test_all(void)
>  
>      setup_vm();
>      smp_init();
> -    setup_idt();
>      enable_apic();
>  
>      handle_irq(SINT1_VEC, stimer_isr);
> diff --git a/x86/hyperv_synic.c b/x86/hyperv_synic.c
> index 3f98c42..74bbd58 100644
> --- a/x86/hyperv_synic.c
> +++ b/x86/hyperv_synic.c
> @@ -154,7 +154,6 @@ int main(int ac, char **av)
>  
>          setup_vm();
>          smp_init();
> -        setup_idt();
>          enable_apic();
>  
>          synic_prepare_sint_vecs();
> diff --git a/x86/ioapic.c b/x86/ioapic.c
> index a554e43..314c9bb 100644
> --- a/x86/ioapic.c
> +++ b/x86/ioapic.c
> @@ -398,7 +398,6 @@ int main(void)
>  {
>  	setup_vm();
>  	smp_init();
> -	setup_idt();
>  
>  	mask_pic_interrupts();
>  
> diff --git a/x86/tscdeadline_latency.c b/x86/tscdeadline_latency.c
> index 0592802..0617a1b 100644
> --- a/x86/tscdeadline_latency.c
> +++ b/x86/tscdeadline_latency.c
> @@ -106,7 +106,6 @@ int main(int argc, char **argv)
>  
>      setup_vm();
>      smp_init();
> -    setup_idt();
>  
>      test_lapic_existence();
>  
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [kvm-unit-tests PATCH 0/3] check multiple init for vm/smp

[Andrew Jones]

On Tue, Oct 25, 2016 at 05:00:27PM +0800, Peter Xu wrote:
> The first two patches check whether there are multiple init for
> vm/smp, assertion fail will be triggered if so.
> 
> Patch 3 is to cleanup existing setup_igt() when smp_init() is called
> (suggested by Drew).
> 
> Peter Xu (3):
>   x86: vm: add assert to avoid multiple vm init
>   x86: smp: assert to avoid multiple init of smp
>   tests: don't call setup_idt() if with smp_init()
> 
>  lib/x86/smp.c             | 1 +
>  lib/x86/vm.c              | 1 +
>  x86/apic.c                | 1 -
>  x86/hyperv_stimer.c       | 1 -
>  x86/hyperv_synic.c        | 1 -
>  x86/ioapic.c              | 1 -
>  x86/tscdeadline_latency.c | 1 -
>  7 files changed, 2 insertions(+), 5 deletions(-)
> 
> -- 
> 2.7.4
>

I actually looked at the code this time around and think that only
patch 1/3 is needed. While it would be pointless to call smp_init
multiple times, it doesn't hurt (that means 2/3 is OK, but not
necessary). And, now I see 3/3 is just a cleanup. No assert would
fire without it. So it's not necessary either.

Actually, wrt to setup_idt, I'd say the only problems with it is
that there's the 'if (idt_initialized) return' and that it isn't
named something like setup_idt_defaults. Isn't it conceivable that
a unit test may want to revert its handlers back to the defaults
after installing custom handlers by simply calling this again?

Thanks,
drew

Re: [kvm-unit-tests PATCH 3/3] tests: don't call setup_idt() if with smp_init()

[Andrew Jones]

On Tue, Oct 25, 2016 at 11:53:27AM +0200, Andrew Jones wrote:
> On Tue, Oct 25, 2016 at 05:00:30PM +0800, Peter Xu wrote:
> > smp_init() will setup idt as default. No need to call it twice.
> 
> This patch should probably be squashed into the previous patch
> to maintain bisectability.

Nevermind this. I just looked at setup_idt; no asserts or
harmful side-effects would occur with multiple calls of
setup_idt. So, it doesn't matter if they're there or not,
and thus this cleanup can be a separate patch, as it is,
if we want to bother with it at all.

drew

> 
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  x86/apic.c                | 1 -
> >  x86/hyperv_stimer.c       | 1 -
> >  x86/hyperv_synic.c        | 1 -
> >  x86/ioapic.c              | 1 -
> >  x86/tscdeadline_latency.c | 1 -
> >  5 files changed, 5 deletions(-)
> > 
> > diff --git a/x86/apic.c b/x86/apic.c
> > index fefb584..eff9a11 100644
> > --- a/x86/apic.c
> > +++ b/x86/apic.c
> > @@ -393,7 +393,6 @@ int main()
> >  {
> >      setup_vm();
> >      smp_init();
> > -    setup_idt();
> >  
> >      test_lapic_existence();
> >  
> > diff --git a/x86/hyperv_stimer.c b/x86/hyperv_stimer.c
> > index 6382938..5292523 100644
> > --- a/x86/hyperv_stimer.c
> > +++ b/x86/hyperv_stimer.c
> > @@ -330,7 +330,6 @@ static void stimer_test_all(void)
> >  
> >      setup_vm();
> >      smp_init();
> > -    setup_idt();
> >      enable_apic();
> >  
> >      handle_irq(SINT1_VEC, stimer_isr);
> > diff --git a/x86/hyperv_synic.c b/x86/hyperv_synic.c
> > index 3f98c42..74bbd58 100644
> > --- a/x86/hyperv_synic.c
> > +++ b/x86/hyperv_synic.c
> > @@ -154,7 +154,6 @@ int main(int ac, char **av)
> >  
> >          setup_vm();
> >          smp_init();
> > -        setup_idt();
> >          enable_apic();
> >  
> >          synic_prepare_sint_vecs();
> > diff --git a/x86/ioapic.c b/x86/ioapic.c
> > index a554e43..314c9bb 100644
> > --- a/x86/ioapic.c
> > +++ b/x86/ioapic.c
> > @@ -398,7 +398,6 @@ int main(void)
> >  {
> >  	setup_vm();
> >  	smp_init();
> > -	setup_idt();
> >  
> >  	mask_pic_interrupts();
> >  
> > diff --git a/x86/tscdeadline_latency.c b/x86/tscdeadline_latency.c
> > index 0592802..0617a1b 100644
> > --- a/x86/tscdeadline_latency.c
> > +++ b/x86/tscdeadline_latency.c
> > @@ -106,7 +106,6 @@ int main(int argc, char **argv)
> >  
> >      setup_vm();
> >      smp_init();
> > -    setup_idt();
> >  
> >      test_lapic_existence();
> >  
> > -- 
> > 2.7.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [kvm-unit-tests PATCH 0/3] check multiple init for vm/smp

[Peter Xu]

On Tue, Oct 25, 2016 at 12:12:47PM +0200, Andrew Jones wrote:
> On Tue, Oct 25, 2016 at 05:00:27PM +0800, Peter Xu wrote:
> > The first two patches check whether there are multiple init for
> > vm/smp, assertion fail will be triggered if so.
> > 
> > Patch 3 is to cleanup existing setup_igt() when smp_init() is called
> > (suggested by Drew).
> > 
> > Peter Xu (3):
> >   x86: vm: add assert to avoid multiple vm init
> >   x86: smp: assert to avoid multiple init of smp
> >   tests: don't call setup_idt() if with smp_init()
> > 
> >  lib/x86/smp.c             | 1 +
> >  lib/x86/vm.c              | 1 +
> >  x86/apic.c                | 1 -
> >  x86/hyperv_stimer.c       | 1 -
> >  x86/hyperv_synic.c        | 1 -
> >  x86/ioapic.c              | 1 -
> >  x86/tscdeadline_latency.c | 1 -
> >  7 files changed, 2 insertions(+), 5 deletions(-)
> > 
> > -- 
> > 2.7.4
> >
> 
> I actually looked at the code this time around and think that only
> patch 1/3 is needed. While it would be pointless to call smp_init
> multiple times, it doesn't hurt (that means 2/3 is OK, but not
> necessary). And, now I see 3/3 is just a cleanup. No assert would
> fire without it. So it's not necessary either.
> 
> Actually, wrt to setup_idt, I'd say the only problems with it is
> that there's the 'if (idt_initialized) return' and that it isn't
> named something like setup_idt_defaults. Isn't it conceivable that
> a unit test may want to revert its handlers back to the defaults
> after installing custom handlers by simply calling this again?

Yeah, that's possible. Maybe we can let anyone who needs this first to
introduce the setup_idt_defaults().

For this series: I have no strong willingness to push this in, and yes
all three patches are not essential at all. I just picked them out
from vt-d unit test series, in case any of us would like to pick it.

Thanks,

-- peterx

Re: [kvm-unit-tests PATCH 1/3] x86: vm: add assert to avoid multiple vm init

[Andrew Jones]

On Tue, Oct 25, 2016 at 05:00:28PM +0800, Peter Xu wrote:
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  lib/x86/vm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/x86/vm.c b/lib/x86/vm.c
> index 906fbf2..f7e778b 100644
> --- a/lib/x86/vm.c
> +++ b/lib/x86/vm.c
> @@ -151,6 +151,7 @@ static void setup_mmu(unsigned long len)
>  
>  void setup_vm()
>  {
> +    assert(!end_of_memory);
>      end_of_memory = fwcfg_get_u64(FW_CFG_RAM_SIZE);
>      free_memory(&edata, end_of_memory - (unsigned long)&edata);
>      setup_mmu(end_of_memory);
> -- 
> 2.7.4
>

Reviewed-by: Andrew Jones <drjones@redhat.com> 

Re: [kvm-unit-tests PATCH 0/3] check multiple init for vm/smp

[Paolo Bonzini]

On 25/10/2016 13:56, Peter Xu wrote:
> 
> For this series: I have no strong willingness to push this in, and yes
> all three patches are not essential at all. I just picked them out
> from vt-d unit test series, in case any of us would like to pick it.

I applied patch 1.

Paolo

Re: [kvm-unit-tests PATCH 3/3] tests: don't call setup_idt() if with smp_init()

[Paolo Bonzini]

On 25/10/2016 11:00, Peter Xu wrote:
> smp_init() will setup idt as default. No need to call it twice.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  x86/apic.c                | 1 -
>  x86/hyperv_stimer.c       | 1 -
>  x86/hyperv_synic.c        | 1 -
>  x86/ioapic.c              | 1 -
>  x86/tscdeadline_latency.c | 1 -
>  5 files changed, 5 deletions(-)
> 
> diff --git a/x86/apic.c b/x86/apic.c
> index fefb584..eff9a11 100644
> --- a/x86/apic.c
> +++ b/x86/apic.c
> @@ -393,7 +393,6 @@ int main()
>  {
>      setup_vm();
>      smp_init();
> -    setup_idt();
>  
>      test_lapic_existence();
>  
> diff --git a/x86/hyperv_stimer.c b/x86/hyperv_stimer.c
> index 6382938..5292523 100644
> --- a/x86/hyperv_stimer.c
> +++ b/x86/hyperv_stimer.c
> @@ -330,7 +330,6 @@ static void stimer_test_all(void)
>  
>      setup_vm();
>      smp_init();
> -    setup_idt();
>      enable_apic();
>  
>      handle_irq(SINT1_VEC, stimer_isr);
> diff --git a/x86/hyperv_synic.c b/x86/hyperv_synic.c
> index 3f98c42..74bbd58 100644
> --- a/x86/hyperv_synic.c
> +++ b/x86/hyperv_synic.c
> @@ -154,7 +154,6 @@ int main(int ac, char **av)
>  
>          setup_vm();
>          smp_init();
> -        setup_idt();
>          enable_apic();
>  
>          synic_prepare_sint_vecs();
> diff --git a/x86/ioapic.c b/x86/ioapic.c
> index a554e43..314c9bb 100644
> --- a/x86/ioapic.c
> +++ b/x86/ioapic.c
> @@ -398,7 +398,6 @@ int main(void)
>  {
>  	setup_vm();
>  	smp_init();
> -	setup_idt();
>  
>  	mask_pic_interrupts();
>  
> diff --git a/x86/tscdeadline_latency.c b/x86/tscdeadline_latency.c
> index 0592802..0617a1b 100644
> --- a/x86/tscdeadline_latency.c
> +++ b/x86/tscdeadline_latency.c
> @@ -106,7 +106,6 @@ int main(int argc, char **argv)
>  
>      setup_vm();
>      smp_init();
> -    setup_idt();
>  
>      test_lapic_existence();
>  
> 

... and patch 3 too.

Paolo

Re: [kvm-unit-tests PATCH 0/3] check multiple init for vm/smp

[Peter Xu]

On Tue, Oct 25, 2016 at 06:00:24PM +0200, Paolo Bonzini wrote:
> 
> 
> On 25/10/2016 13:56, Peter Xu wrote:
> > 
> > For this series: I have no strong willingness to push this in, and yes
> > all three patches are not essential at all. I just picked them out
> > from vt-d unit test series, in case any of us would like to pick it.
> 
> I applied patch 1.

Thanks. :)

-- peterx