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
signature.asc
Description: OpenPGP digital signature
