[Qemu-devel] Meaning of "-smp threads" on mips_malta

[Qemu-devel] Meaning of "-smp threads" on mips_malta

[Eduardo Habkost]

Hi,

I'm trying to refactor the SMP topology code in QEMU and I found
some suspicious code on mips_malta.c:

static void malta_mips_config(MIPSCPU *cpu)
{
    CPUMIPSState *env = &cpu->env;
    CPUState *cs = CPU(cpu);

    env->mvp->CP0_MVPConf0 |= ((smp_cpus - 1) << CP0MVPC0_PVPE) |
                         ((smp_cpus * cs->nr_threads - 1) << CP0MVPC0_PTC);
}


The (smp_cpus * cs->nr_threads) expression here doesn't make
sense to me (because smp_cpus is already supposed to be a
multiple of smp_threads), and seems to indicate that the code has
some unusual assumptions about the semantics of the -smp option.

So, I'd like to know: do all the examples below make sense for
Malta?

 -smp 1
 -smp 2
 -smp 2,threads=1
 -smp 2,threads=2
 -smp 1,threads=2 [*]
 -smp 2,threads=3 [*]

The generic -smp parsing code considers the last 2 entries
above[*] to be invalid.  If they make sense for Malta, we need to
find a way to fix that.  Replacing "-smp threads=..." with a
"-cpu" or "-machine" option seems like the best alternative.

-- 
Eduardo

Re: [Qemu-devel] Meaning of "-smp threads" on mips_malta

[Aleksandar Markovic]

On Monday, January 14, 2019, Eduardo Habkost <ehabkost@redhat.com> wrote:

> Hi,
>
> I'm trying to refactor the SMP topology code in QEMU


>
Eduardo, I truly appreciate your interest in details of Malta
implementations, but before I answer your questions, could you please
outline the motivation and the current concept of your envisioned
refactoring of SMP, so that I have some picture of the whole context?

Sincerely, Aleksandar



>
> and I found
> some suspicious code on mips_malta.c:
>
> static void malta_mips_config(MIPSCPU *cpu)
> {
>     CPUMIPSState *env = &cpu->env;
>     CPUState *cs = CPU(cpu);
>
>     env->mvp->CP0_MVPConf0 |= ((smp_cpus - 1) << CP0MVPC0_PVPE) |
>                          ((smp_cpus * cs->nr_threads - 1) << CP0MVPC0_PTC);
> }
>
>
> The (smp_cpus * cs->nr_threads) expression here doesn't make
> sense to me (because smp_cpus is already supposed to be a
> multiple of smp_threads), and seems to indicate that the code has
> some unusual assumptions about the semantics of the -smp option.
>
> So, I'd like to know: do all the examples below make sense for
> Malta?
>
>  -smp 1
>  -smp 2
>  -smp 2,threads=1
>  -smp 2,threads=2
>  -smp 1,threads=2 [*]
>  -smp 2,threads=3 [*]
>
> The generic -smp parsing code considers the last 2 entries
> above[*] to be invalid.  If they make sense for Malta, we need to
> find a way to fix that.  Replacing "-smp threads=..." with a
> "-cpu" or "-machine" option seems like the best alternative.
>
> --
> Eduardo
>
>

Re: [Qemu-devel] Meaning of "-smp threads" on mips_malta

[Eduardo Habkost]

On Tue, Jan 15, 2019 at 03:28:50AM +0100, Aleksandar Markovic wrote:
> On Monday, January 14, 2019, Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > Hi,
> >
> > I'm trying to refactor the SMP topology code in QEMU
> 
> 
> >
> Eduardo, I truly appreciate your interest in details of Malta
> implementations, but before I answer your questions, could you please
> outline the motivation and the current concept of your envisioned
> refactoring of SMP, so that I have some picture of the whole context?

I don't have any concrete plans right now.  I'm trying to
understand how existing code uses the -smp options, before
deciding what to do.

My goal is to make features like
  "[PATCH v1 0/5] Introduce cpu die topology and enable CPUID.1F for i386"
easier, less intrusive, and less risky.

My current impression is that things will be better if we move
the complex PC-specific topology rules from vl.c to PC-specific
code.  Most of the other architectures seem to have different
assumptions and expectations about -smp/cores/threads.


> 
> Sincerely, Aleksandar
> 
> 
> 
> >
> > and I found
> > some suspicious code on mips_malta.c:
> >
> > static void malta_mips_config(MIPSCPU *cpu)
> > {
> >     CPUMIPSState *env = &cpu->env;
> >     CPUState *cs = CPU(cpu);
> >
> >     env->mvp->CP0_MVPConf0 |= ((smp_cpus - 1) << CP0MVPC0_PVPE) |
> >                          ((smp_cpus * cs->nr_threads - 1) << CP0MVPC0_PTC);
> > }
> >
> >
> > The (smp_cpus * cs->nr_threads) expression here doesn't make
> > sense to me (because smp_cpus is already supposed to be a
> > multiple of smp_threads), and seems to indicate that the code has
> > some unusual assumptions about the semantics of the -smp option.
> >
> > So, I'd like to know: do all the examples below make sense for
> > Malta?
> >
> >  -smp 1
> >  -smp 2
> >  -smp 2,threads=1
> >  -smp 2,threads=2
> >  -smp 1,threads=2 [*]
> >  -smp 2,threads=3 [*]
> >
> > The generic -smp parsing code considers the last 2 entries
> > above[*] to be invalid.  If they make sense for Malta, we need to
> > find a way to fix that.  Replacing "-smp threads=..." with a
> > "-cpu" or "-machine" option seems like the best alternative.
> >
> > --
> > Eduardo
> >
> >

-- 
Eduardo

Re: [Qemu-devel] Meaning of "-smp threads" on mips_malta

[Aleksandar Markovic]

> From: Eduardo Habkost <ehabkost@redhat.com>
> Sent: Wednesday, January 16, 2019 4:30 PM
> To: Aleksandar Markovic
> Cc: qemu-devel@nongnu.org; Aleksandar Rikalo; Stefan Markovic; Aleksandar Markovic; Aurelien > Jarno; Igor Mammedov
> Subject: Re: [Qemu-devel] Meaning of "-smp threads" on mips_malta
> 
> On Tue, Jan 15, 2019 at 03:28:50AM +0100, Aleksandar Markovic wrote:
> > On Monday, January 14, 2019, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > > Hi,
> > >
> > > I'm trying to refactor the SMP topology code in QEMU
> >
> >
> > >
> > Eduardo, I truly appreciate your interest in details of Malta
> > implementations, but before I answer your questions, could you please
> > outline the motivation and the current concept of your envisioned
> > refactoring of SMP, so that I have some picture of the whole context?
> 
> I don't have any concrete plans right now.  I'm trying to
> understand how existing code uses the -smp options, before
> deciding what to do.
> 
> My goal is to make features like
>   "[PATCH v1 0/5] Introduce cpu die topology and enable CPUID.1F for i386"
> easier, less intrusive, and less risky.
> 
> My current impression is that things will be better if we move
> the complex PC-specific topology rules from vl.c to PC-specific
> code.  Most of the other architectures seem to have different
> assumptions and expectations about -smp/cores/threads.
> 

Eduardo, thanks for giving me this background info and details.

I completely understand your concerns, and also frustration with the
current situation in the multi-core area of your (and many other)
platforms. What appears to be a simple thing suddenly becomes
a major issue - very annoying, isn't it?

Your observation on different assumptions and expectations
between targets in this area is so true.

I feel this topic deserves wider discussion, and I think we need better
possibilities and solutions for future endeavors across the board for
all targets, not only in your or mine case.

As for MIPS smp/threads combinations, they are valid, although truly
unusual, and this is because of certain MIPS-specific features of
so-called hardware multithreading and core virtualization, where a
core can present itself as multiple cores - even theoretically run
different OSs on such cores. The code snippet is very old by itself
(almost obsolete), since MIPS architecture meanwhile evolved, so
the code snippet is actually ripe for rewriting, but this is what we
have now, and I am against just throwing it away. I gather the
authors of the code did not have any better choice at the time
of writing.

Again, I am with you regarding your concerns. However, let's not
rush, let's not jump to quick conclusions. Let me think for a while,
try to come up with some suggestions that will make life easier
for both of us and hopefully for majority of the community.

Speak to you later, I'll be back soon, give me some time, and
thanks for bringing this to my (and everybody else's) attention.

Regards,
Aleksandar

> 
> >
> > Sincerely, Aleksandar
> >
> >
> >
> > >
> > > and I found
> > > some suspicious code on mips_malta.c:
> > >
> > > static void malta_mips_config(MIPSCPU *cpu)
> > > {
> > >     CPUMIPSState *env = &cpu->env;
> > >     CPUState *cs = CPU(cpu);
> > >
> > >     env->mvp->CP0_MVPConf0 |= ((smp_cpus - 1) << CP0MVPC0_PVPE) |
> > >                          ((smp_cpus * cs->nr_threads - 1) << CP0MVPC0_PTC);
> > > }
> > >
> > >
> > > The (smp_cpus * cs->nr_threads) expression here doesn't make
> > > sense to me (because smp_cpus is already supposed to be a
> > > multiple of smp_threads), and seems to indicate that the code has
> > > some unusual assumptions about the semantics of the -smp option.
> > >
> > > So, I'd like to know: do all the examples below make sense for
> > > Malta?
> > >
> > >  -smp 1
> > >  -smp 2
> > >  -smp 2,threads=1
> > >  -smp 2,threads=2
> > >  -smp 1,threads=2 [*]
> > >  -smp 2,threads=3 [*]
> > >
> > > The generic -smp parsing code considers the last 2 entries
> > > above[*] to be invalid.  If they make sense for Malta, we need to
> > > find a way to fix that.  Replacing "-smp threads=..." with a
> > > "-cpu" or "-machine" option seems like the best alternative.
> > >
> > > --
> > > Eduardo
> > >
> > >
> 
> --
> Eduardo
> 

Re: [Qemu-devel] Meaning of "-smp threads" on mips_malta

[Andrew Jones]

On Wed, Jan 16, 2019 at 01:30:11PM -0200, Eduardo Habkost wrote:
> On Tue, Jan 15, 2019 at 03:28:50AM +0100, Aleksandar Markovic wrote:
> > On Monday, January 14, 2019, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > Hi,
> > >
> > > I'm trying to refactor the SMP topology code in QEMU
> > 
> > 
> > >
> > Eduardo, I truly appreciate your interest in details of Malta
> > implementations, but before I answer your questions, could you please
> > outline the motivation and the current concept of your envisioned
> > refactoring of SMP, so that I have some picture of the whole context?
> 
> I don't have any concrete plans right now.  I'm trying to
> understand how existing code uses the -smp options, before
> deciding what to do.
> 
> My goal is to make features like
>   "[PATCH v1 0/5] Introduce cpu die topology and enable CPUID.1F for i386"
> easier, less intrusive, and less risky.
> 
> My current impression is that things will be better if we move
> the complex PC-specific topology rules from vl.c to PC-specific
> code.  Most of the other architectures seem to have different
> assumptions and expectations about -smp/cores/threads.
>

Yes. For example, if we start describing cpu topology for mach-virt
then, in order to maintain compatibility, we'll need these semantics

  -smp <n>   : sockets=1,cores=<n>,threads=1
 
And any other -smp incantation, attempting to describe the topology,
should enforce explicitly providing a value for each parameter. As
that's different behavior than what x86 does and what the common
-smp parsing code does, then we really need to refactor things and
push more down into machine code.

Also, if we could make these parameters machine and cpu properties,
instead of global variables, that would be a huge improvement.

Thanks for doing this :-)

drew