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.
> > +@var{apics} specifies the boot bitmap of existed apicid.
>
> s/existed/existing/
>
> > +
> > +@example
> > +#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 */
> > +@end 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.
>
