Re: [libvirt] [PATCH 1/1] Assign spapr-vio address value to VIO devices without hardcode
On 05/18/2012 06:41 AM, Eric Blake wrote:
On 05/17/2012 12:16 AM, Li Zhang wrote:
> Hardcode address will cause conflicts when there are a lot of VIO
> devices.
>
> This patch is to remove the harcode of the address, and assign
> a variable to it, which is cnt * 0x1000UL. And assign spapr-vio
> address to VIO devices, such as spapr-vlan and spapr-vty. Several
> test cases are modified.
> /* Default values match QEMU. See spapr_(llan|vscsi|vty).c */
>
> for (i = 0 ; i< def->nnets; i++) {
> - rc = qemuAssignSpaprVIOAddress(def,&def->nets[i]->info,
> - 0x1000ul);
Isn't the point of qemuAssignSpaprVIOAddress to start at 0x1000 and
increment until it finds a gap? If so, then the caller shouldn't also
be incrementing.
Yes, I didn't realize that. Sorry.
My way is to resolve the same problem with that.
Now it is not necessary to do this. :)
> - if (rc)
> - return rc;
> + if (!def->nets[i]->model&&
This says def->nets[i]->model is NULL...
> + STREQ(def->os.arch, "ppc64")&&
> + STREQ(def->os.machine, "pseries"))
> + strcpy(def->nets[i]->model, "spapr-vlan");
and this tells strcpy() to dereference it. You can't have possibly
tested this line of code, since it will crash. Did you mean to do a
strdup() instead?
Sorry for my stupid mistake. It needs to malloc memory to the
string.
or use strdup() to do that.
> + if (def->nets[i]->model&&
> + STREQ(def->nets[i]->model, "spapr-vlan")) {
> + cnt ++;
Style nit - no spacing between ++ or -- operator and the variable it
modifies.
Got it. Thanks for your comments.
> + def->nets[i]->info.type =
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO;
> + rc = qemuAssignSpaprVIOAddress(def,&def->nets[i]->info,
> + cnt * 0x1000ul);
Indentation is off.
> + if (rc)
> + return rc;
> + }
> +
> }
>
> for (i = 0 ; i< def->ncontrollers; i++) {
> @@ -797,19 +808,28 @@ int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def)
> model = qemuDefaultScsiControllerModel(def);
> if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI&&
> def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
> + cnt ++;
> def->controllers[i]->info.type =
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO;
> rc =
qemuAssignSpaprVIOAddress(def,&def->controllers[i]->info,
> - 0x2000ul);
> + cnt * 0x1000ul);
About the only difference I can see is that if you skipped the first
loop entirely, this would let the second loop start at 0x1000 instead of
0x2000. But again, since qemuAssignSpaprVIOAddress is supposed to
auto-increment until it finds a gap, wouldn't that mean you can just
change this to start at 0x1000 without needing 'cnt'?
You are right.
> if (rc)
> return rc;
> }
> }
>
> for (i = 0 ; i< def->nserials; i++) {
> - rc = qemuAssignSpaprVIOAddress(def,&def->serials[i]->info,
> - 0x30000000ul);
> - if (rc)
> - return rc;
> + if (STREQ(def->os.arch, "ppc64")&&
> + STREQ(def->os.machine, "pseries"))
> + def->serials[i]->info.type =
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO;
> +
> + if (def->serials[i]->info.type ==
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) {
> + cnt ++;
> + rc = qemuAssignSpaprVIOAddress(def,&def->serials[i]->info,
> + cnt * 0x1000ul);
This changes the default value from 0x30000000 to the first gap after
0x1000. Can you please justify this change by pointing to the qemu
commit that shows how qemu assigns addresses, to make sure libvirt is
picking the same defaults as qemu?
I will check this in Qemu. Thanks. :)
--
Best Regards
Li
IBM LTC, China System&Technology Lab, Beijing