Re: [Qemu-devel] [RFC 2/3] target-i386: add -smp X,apics=0x option

[Chen Fan]

On Mon, 2014-02-17 at 11:37 -0700, Eric Blake wrote:
> On 01/14/2014 02:27 AM, Chen Fan wrote:
> > This option provides the infrastructure for specifying apicids when
> > boot VM, For example:
> > 
> >  #boot with apicid 0 and 2:
> >  -smp 2,apics=0xA,maxcpus=4  /* 1010 */
> >  #boot with apicid 1 and 7:
> >  -smp 2,apics=0x41,maxcpus=8 /* 0100 0001 */
> 
> This syntax feels a bit odd when maxcpus is not a multiple of 8, and
> even harder when not a multiple of 4.  I think part of my confusion
> stems from you treating the lsb as the left-most bit, but expect me to
> write in hex where I'm used to the right-most bit being lsb   Wouldn't
> it be easier to express:
> 
> msb .... lsb
> 
> with leading 0s implied as needed, as in:
> 
> 0x5 => 0101 => id 0 (lsb) and id 2 are enabled, regardless of whether
> maxcpus=4 or maxcpus=8
> 0x82 => 1000 0010 => id 1 and id 7 are enabled, regardless of whether
> maxcpus=8 or maxcpus=256
> 
> 0x100000000 => id 32 is enabled
> 
> Or even better, why not reuse existing parsers that take cpu ids
> directly as numbers instead of making me compute a bitmap (as in
> maxcpus=4,id=0,id=2 - although I don't quite know QemuOpts well enough
> to know if you can repeat id= for forming a list of disjoint id numbers)
> 
Thanks for your review, but this form was deprecated. Igor proposed
using -device /-device-add to specify the disjoint id numbers.

Thanks,
Chen 

> > @@ -92,6 +93,14 @@ of @var{threads} per cores and the total number of 
> > @var{sockets} can be
> >  specified. Missing values will be computed. If any on the three values is
> >  given, the total number of CPUs @var{n} can be omitted. @var{maxcpus}
> >  specifies the maximum number of hotpluggable CPUs.
> > address@hidden specifies the boot bitmap of existed apicid.
> 
> s/existed/existing/
> 
> > +
> > address@hidden
> > +#specify the boot bitmap of apicid with 0 and 2:
> > +qemu-system-i386 -smp 2,apics=0xA,maxcpus=4  /* 1010 */
> > +#specify the boot bitmap of apicid with 1 and 7:
> > +qemu-system-i386 -smp 2,apics=0x41,maxcpus=8 /* 0100 0001 */
> > address@hidden example
> 
> These examples would need updating to match my concerns.
> 
> > @@ -1379,6 +1382,9 @@ static QemuOptsList qemu_smp_opts = {
> >          }, {
> >              .name = "maxcpus",
> >              .type = QEMU_OPT_NUMBER,
> > +        }, {
> > +            .name = "apics",
> > +            .type = QEMU_OPT_STRING,
> 
> Why a string with your own ad-hoc parser?  Can't we reuse some of the
> existing parsers that already know how to handle (possibly-disjoint)
> lists of cpu numbers?
> 
> > +        if (apics) {
> > +            if (strstart(apics, "0x", &apics)) {
> 
> Why not also allow 0X?
> 
> > +                if (*apics != '\0') {
> > +                    int i, count;
> > +                    int64_t max_apicid = 0;
> > +                    uint32_t val;
> > +                    char tmp[2];
> > +
> > +                    count = strlen(apics);
> > +
> > +                    for (i = 0; i < count; i++) {
> > +                        tmp[0] = apics[i];
> > +                        tmp[1] = '\0';
> > +                        sscanf(tmp, "%x", &val);
> 
> sscanf is evil.  It has undefined behavior on input overflow (that is,
> if I say 0x10000000000000000, there is no guarantee what sscanf will
> stick into val).  All the more reason you should be using an existing
> parser which gracefully handles overflow.
>