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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3239 bytes --]

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)

> @@ -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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]