[libvirt] [PATCH 1/1] Assign spapr-vio address value to VIO devices without hardcode

[Li Zhang]

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