Re: [libvirt] [PATCH v2 08/12] qemu: Add length for bps/iops throttling parameters to driver

[John Ferlan]

On 10/25/2016 01:30 PM, Erik Skultety wrote:
> On Thu, Oct 06, 2016 at 06:38:56PM -0400, John Ferlan wrote:
>> Add support for a duration/length for the bps/iops and friends.
>>
>> Modify the API in order to add the "blkdeviotune." specific definitions
>> for the iotune throttling duration/length options
>>
>> total_bytes_sec_max_length
>> write_bytes_sec_max_length
>> read_bytes_sec_max_length
>> total_iops_sec_max_length
>> write_iops_sec_max_length
>> read_iops_sec_max_length
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  include/libvirt/libvirt-domain.h |  54 +
>>  src/conf/domain_conf.h   |   6 +++
>>  src/qemu/qemu_driver.c   | 100 
>> +--
>>  src/qemu/qemu_monitor.c  |   7 ++-
>>  src/qemu/qemu_monitor.h  |   3 +-
>>  src/qemu/qemu_monitor_json.c |  25 +-
>>  src/qemu/qemu_monitor_json.h |   3 +-
>>  tests/qemumonitorjsontest.c  |  17 ++-
>>  8 files changed, 202 insertions(+), 13 deletions(-)
>>
> 
> [...]
> 
>> @@ -17296,7 +17297,9 @@ 
>> qemuDomainSetBlockIoTuneSetDefaults(virDomainBlockIoTuneInfoPtr newinfo,
> 
> just a nitpick, are both the 'Set's necessary in the name, unless one of them
> is a verb and the other a noun, I think the first one could be dropped.
> 

D'oh... The name was so long I missed the double set!

>>  bool set_iops,
>>  bool set_bytes_max,
>>  bool set_iops_max,
>> -bool set_size_iops)
>> +bool set_size_iops,
>> +bool set_bytes_max_length,
>> +bool set_iops_max_length)
>>  {
>>  if (!set_bytes) {
>>  newinfo->total_bytes_sec = oldinfo->total_bytes_sec;
>> @@ -17320,6 +17323,36 @@ 
>> qemuDomainSetBlockIoTuneSetDefaults(virDomainBlockIoTuneInfoPtr newinfo,
>>  }
>>  if (!set_size_iops)
>>  newinfo->size_iops_sec = oldinfo->size_iops_sec;
>> +
>> +/* The length field is handled a bit differently. If not defined/set,
>> + * QEMU will default these to 0 or 1 depending on whether something in
>> + * the same family is set or not.
>> + *
>> + * Similar to other values, if nothing in the family is defined/set,
>> + * then take whatever is in the oldinfo.
>> + *
>> + * To clear an existing limit, a 0 is provided; however, passing that
>> + * 0 onto QEMU if there's a family value defined/set (or defaulted)
>> + * will cause an error. So, to mimic that, if our oldinfo was set and
>> + * our newinfo is clearing, then set max_length based on whether we
>> + * have a value in the family set/defined. */
> 
> Hmm. You can disregard my comment in 4/12 about replacing the whole function
> with a macro, instead I suggest keeping the function and making the macro 
> below
> also work for all the settings above, otherwise it just doesn't look right, 
> one
> part of the function being expanded while the other covered by a macro because
> the behaviour is slightly different (I have to admit though, it was kinda
> painful to comprehend what's going on on the qemu side)
> 

Yes, quite painful - especially when I realized it after the initial
series...  The difference is I went with a function.

I create a macro for the setting of the defaults

>> +#define SET_MAX_LENGTH(BOOL, FIELD) 
>>\
>> +if (!BOOL)  
>>\
>> +newinfo->FIELD##_max_length = oldinfo->FIELD##_max_length;  
>>\
>> +else if (BOOL && oldinfo->FIELD##_max_length && 
>>\
>> + !newinfo->FIELD##_max_length)  
>>\
>> +newinfo->FIELD##_max_length = (newinfo->FIELD ||
>>\
>> +   newinfo->FIELD##_max) ? 1 : 0;
>> +
>> +SET_MAX_LENGTH(set_bytes_max_length, total_bytes_sec);
>> +SET_MAX_LENGTH(set_bytes_max_length, read_bytes_sec);
>> +SET_MAX_LENGTH(set_bytes_max_length, write_bytes_sec);
>> +SET_MAX_LENGTH(set_iops_max_length, total_iops_sec);
>> +SET_MAX_LENGTH(set_iops_max_length, read_iops_sec);
>> +SET_MAX_LENGTH(set_iops_max_length, write_iops_sec);
>> +
>> +#undef SET_MAX_LENGTH
>> +
>>  }
>>  
>>  
>> @@ -17346,7 +17379,10 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
>>  bool set_bytes_max = false;
>>  bool set_iops_max = false;
>>  bool set_size_iops = false;
>> +bool set_bytes_max_length = false;
>> +bool set_iops_max_length = false;
>>  bool supportMaxOptions = true;
>> +bool supportMaxLengthOptions = true;
>>  virQEMUDriverConfigPtr cfg = NULL;
>>  virObjectEventPtr event = NULL;
>>  

Re: [libvirt] [PATCH v2 11/12] virsh: Create a macro to add IOTUNE values

[John Ferlan]

On 10/25/2016 01:44 PM, Erik Skultety wrote:
> On Thu, Oct 06, 2016 at 06:38:59PM -0400, John Ferlan wrote:
>> Rework the repetitive lines to add iotune values into easier to read macro
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  tools/virsh-domain.c | 141 
>> +--
>>  1 file changed, 25 insertions(+), 116 deletions(-)
>>
> 
> [...]
> 
>> +#define VSH_ADD_IOTUNE(PARAM, CONST)
>>\
>> +if ((rv = vshCommandOptScaledInt(ctl, cmd, #PARAM, ,  
>>\
>> + 1, ULLONG_MAX)) < 0) { 
>>\
> 
> Again a nitpick, I guess the -length setting should not get scaled when one

Upon closer inspect, good catch and it's not just -length... The iops
values aren't scaled either!

There's now two macros... I renamed the current one to add a _SCALED and
created a new one that uses vshCommandOptULongLong (I don't want to mess
with another SCALE argument)


> tries e.g. 'kib' to pass along with the setting. In terms of a cleanup, yes,
> it's very welcome, so maybe add a 'SCALE' argument - your call.

and the -length values will all be part of the non scaled version..

Thanks for the review - the series is now pushed.

John
> 
> ACK
> 
> Erik
> 
>> +goto interror;  
>>\
>> +} else if (rv > 0) {
>>\
>> +if (virTypedParamsAddULLong(, , ,  
>>\
>> +VIR_DOMAIN_BLOCK_IOTUNE_##CONST,
>>\
>> +value) < 0) 
>>\
>> +goto save_error;
>>\
>> +}   
>>\
>> +
>> +VSH_ADD_IOTUNE(total-bytes-sec, TOTAL_BYTES_SEC);
>> +VSH_ADD_IOTUNE(read-bytes-sec, READ_BYTES_SEC);
>> +VSH_ADD_IOTUNE(write-bytes-sec, WRITE_BYTES_SEC);
>> +VSH_ADD_IOTUNE(total-iops-sec, TOTAL_IOPS_SEC);
>> +VSH_ADD_IOTUNE(read-iops-sec, READ_IOPS_SEC);
>> +VSH_ADD_IOTUNE(write-iops-sec, WRITE_IOPS_SEC);
>> +
>> +VSH_ADD_IOTUNE(total-bytes-sec-max, TOTAL_BYTES_SEC_MAX);
>> +VSH_ADD_IOTUNE(read-bytes-sec-max, READ_BYTES_SEC_MAX);
>> +VSH_ADD_IOTUNE(write-bytes-sec-max, WRITE_BYTES_SEC_MAX);
>> +VSH_ADD_IOTUNE(total-iops-sec-max, TOTAL_IOPS_SEC_MAX);
>> +VSH_ADD_IOTUNE(read-iops-sec-max, READ_IOPS_SEC_MAX);
>> +VSH_ADD_IOTUNE(write-iops-sec-max, WRITE_IOPS_SEC_MAX);
>> +VSH_ADD_IOTUNE(size-iops-sec, SIZE_IOPS_SEC);
>>  
>>  if (nparams == 0) {
>>  if (virDomainGetBlockIoTune(dom, NULL, NULL, , flags) != 0) 
>> {
>> -- 
>> 2.7.4
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v11 2/5] qemu: Swap order of RNG hot unplug removal

[Pavel Hrdina]

On Tue, Oct 25, 2016 at 03:57:34PM -0400, John Ferlan wrote:
> 
> 
> On 10/25/2016 08:48 AM, Pavel Hrdina wrote:
> > On Mon, Oct 24, 2016 at 06:46:18PM -0400, John Ferlan wrote:
> >> Rather than remove the frontend first and then the backend, let's swap the
> >> order. The order of addition is add the chardev backend if EGD being used,
> >> then add the RNG object. So there's a dependency there. When we remove,
> >> unplug the backend first, then remove the object would seem to be a more
> >> logical step. This would then follow similar processing for disk removal
> >> where dependent objects (secret and encryption) of the drive are removed
> >> before the drive is removed.
> > 
> > Nice catch, qemuDomainRemoveChrDevice needs the same fix.
> > 
> 
> Going through my pencil/paper exercise, I think I was wrong...  Too many
> objects and similar names.
> 
> During the attach processing we have (up to this point at least)
> 
>  1. Add TLS Object (if necessary)
>  2. Attach chardev for EGD Backend (if necessary)
> -> NB: Could depend on 1 via 'tlsProps'
>  3. Add frontend object
> -> NB: Could depend on 2 via 'props'
>  4. Add device
> 
> So the corollary then becomes:
> 
>  1. Detach Device  (in DetachRNGDevice)
>  2. Delete frontend object (in RemoveRNGDevice)
>  3. Detach chardev for EGD Backend (if necessary)
>  4. Delete TLS object
> 
> When the secobj is added later it's added before TLS and removed after TLS.
> 
> Since the existing code has the above order, this patch I think is
> dropped... That does effect the next one, but only insomuch as placement
> of the delete TLS object per the above order.

OK I've checked it again and properly this time and I've check qemu command
line too:

  -chardev socket,id=charrng0,host=localhost,port=4466,server,nowait
  -object rng-egd,id=objrng0,chardev=charrng0
  -device virtio-rng-pci,rng=objrng0,id=rng0,bus=pci.2,addr=0x2

So the dependency is clear and yes this patch was making it wrong.  I've for
some reason thought that the objAlias means a different object, sigh, shame
on me :)

Pavel


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v11 3/5] qemu: Need to remove TLS object in RemoveRNGDevice

[John Ferlan]

On 10/25/2016 12:04 PM, Pavel Hrdina wrote:
> On Tue, Oct 25, 2016 at 09:19:58AM -0400, John Ferlan wrote:
>>
>>
>> On 10/25/2016 08:53 AM, Pavel Hrdina wrote:
>>> On Mon, Oct 24, 2016 at 06:46:19PM -0400, John Ferlan wrote:
 Commit id '6e6b4bfc' added the object, but forgot the other end.

 Signed-off-by: John Ferlan 
 ---
  src/qemu/qemu_hotplug.c | 12 +++-
  1 file changed, 11 insertions(+), 1 deletion(-)
>>>
>>> ACK if you agree with review on the first patch that the order of 
>>> qemuMonitorDelObject and qemuMonitorDetachCharDev should be swapped.
>>>
>>> Pavel
>>>
>>
>> I do agree, but I hadn't fully convinced myself - I needed the extra set
>> of eyes to help with that... Of course, I botched that again here by
>> removing the tlsobj before the chardev.   A problem which naturally
>> persists in patch 5, but is easily adjusted..
>>
>> I looked at and thought about qemuDomainRemoveRNGDevice long enough and
>> compared to qemuDomainRemoveDiskDevice, but couldn't quite make up my
>> mind and really didn't want to be creating "extra" patches for perhaps
>> all the erroneous removals.
>>
>> If my comparison is {Attach|Remove}Disk, which has...
>>
>> Attach: Add secobj, Add encobj, Add Drive, and Add Device where drive
>> can rely on secobj and/or encobj.  The encobj and secobj do not rely on
>> each other. The secobj is the possible secret for the iSCSI/RBD server,
>> while encobj is the possible secret for
>>
>> On attach error the removal is Del Drive, Del secobj, Del encobj (order
>> of secobj/encobj doesn't matter).
>>
>> Detach: Del Device and call RemoveDisk which Del secobj, Del encobj, and
>> Del Drive.
>>
>> Then, the RemoveDisk is probably wrong (Del Drive should be first), but
>> I didn't want to continue to bog down the chardev changes if my thoughts
>> in patch 2 were wrong.
> 
> I think that the RemoveDisk is wrong and that the Del Drive should be first.
> As you've wrote, the drive can rely on secobj and/or encobj so we should not
> remove the object while they can be still used.
> 

RemoveDisk is a different/separate issue - I sent a separate patch.

As for the rest of this - I think in my latest response to 2/5 - the
pencil and paper exercise shows the delete TLS object goes after the
Detach chardev below.

That then just leaves fixing up the order for the secret object in patch
5 of this series.

John

> Pavel
> 
>>
>> Long way of saying, I want to get it right for this path and then if I'm
>> right generate a RemoveDisk patch to swap order.
>>
>> John
>>

 diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
 index e287aaf..f401b48 100644
 --- a/src/qemu/qemu_hotplug.c
 +++ b/src/qemu/qemu_hotplug.c
 @@ -3608,6 +3608,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver,
  virObjectEventPtr event;
  char *charAlias = NULL;
  char *objAlias = NULL;
 +char *tlsAlias = NULL;
  qemuDomainObjPrivatePtr priv = vm->privateData;
  ssize_t idx;
  int ret = -1;
 @@ -3616,15 +3617,23 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver,
  VIR_DEBUG("Removing RNG device %s from domain %p %s",
rng->info.alias, vm, vm->def->name);
  
 +
  if (virAsprintf(, "obj%s", rng->info.alias) < 0)
  goto cleanup;
  
  if (!(charAlias = qemuAliasChardevFromDevAlias(rng->info.alias)))
  goto cleanup;
  
 +if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD &&
 +!(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
 +goto cleanup;
 +
  qemuDomainObjEnterMonitor(driver, vm);
 -if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD)
 +if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) {
 +if (tlsAlias)
 +ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias));
  ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias));
 +}
  
  rc = qemuMonitorDelObject(priv->mon, objAlias);
  
 @@ -3648,6 +3657,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver,
   cleanup:
  VIR_FREE(charAlias);
  VIR_FREE(objAlias);
 +VIR_FREE(tlsAlias);
  return ret;
  }
  
 -- 
 2.7.4

 --
 libvir-list mailing list
 libvir-list@redhat.com
 https://www.redhat.com/mailman/listinfo/libvir-list
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] qemu: Fix depedency order in qemuRemoveDiskDevice

[John Ferlan]

Need to remove the drive first, then the secobj and/or encobj if they exist.
This is because the drive has a dependency on secobj (or the secret for
the networked storage server) and/or the encobj (or the secret for the
LUKS encrypted volume).  Deleting either object first leaves an drive
without it's respective objects.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_hotplug.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 7a21dc6..30565d5 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3176,6 +3176,9 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
 
 qemuDomainObjEnterMonitor(driver, vm);
 
+qemuMonitorDriveDel(priv->mon, drivestr);
+VIR_FREE(drivestr);
+
 /* If it fails, then so be it - it was a best shot */
 if (objAlias)
 ignore_value(qemuMonitorDelObject(priv->mon, objAlias));
@@ -3186,8 +3189,6 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
 ignore_value(qemuMonitorDelObject(priv->mon, encAlias));
 VIR_FREE(encAlias);
 
-qemuMonitorDriveDel(priv->mon, drivestr);
-VIR_FREE(drivestr);
 if (qemuDomainObjExitMonitor(driver, vm) < 0)
 return -1;
 
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v11 2/5] qemu: Swap order of RNG hot unplug removal

[John Ferlan]

On 10/25/2016 08:48 AM, Pavel Hrdina wrote:
> On Mon, Oct 24, 2016 at 06:46:18PM -0400, John Ferlan wrote:
>> Rather than remove the frontend first and then the backend, let's swap the
>> order. The order of addition is add the chardev backend if EGD being used,
>> then add the RNG object. So there's a dependency there. When we remove,
>> unplug the backend first, then remove the object would seem to be a more
>> logical step. This would then follow similar processing for disk removal
>> where dependent objects (secret and encryption) of the drive are removed
>> before the drive is removed.
> 
> Nice catch, qemuDomainRemoveChrDevice needs the same fix.
> 

Going through my pencil/paper exercise, I think I was wrong...  Too many
objects and similar names.

During the attach processing we have (up to this point at least)

 1. Add TLS Object (if necessary)
 2. Attach chardev for EGD Backend (if necessary)
-> NB: Could depend on 1 via 'tlsProps'
 3. Add frontend object
-> NB: Could depend on 2 via 'props'
 4. Add device

So the corollary then becomes:

 1. Detach Device  (in DetachRNGDevice)
 2. Delete frontend object (in RemoveRNGDevice)
 3. Detach chardev for EGD Backend (if necessary)
 4. Delete TLS object

When the secobj is added later it's added before TLS and removed after TLS.

Since the existing code has the above order, this patch I think is
dropped... That does effect the next one, but only insomuch as placement
of the delete TLS object per the above order.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v11 1/5] qemu: Move TLS object remove from DetachChr to RemoveChr

[John Ferlan]

On 10/25/2016 08:42 AM, Pavel Hrdina wrote:
> On Mon, Oct 24, 2016 at 06:46:17PM -0400, John Ferlan wrote:
>> Commit id '2c32237' added the TLS object removal to the DetachChrDevice
>> all when it should have been added to the RemoveChrDevice since that's
>> the norm for similar processing (e.g. disk)
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/qemu/qemu_hotplug.c | 44 +---
>>  1 file changed, 21 insertions(+), 23 deletions(-)
>>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index cf69945..31b5876 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -3549,7 +3549,9 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
>>virDomainChrDefPtr chr)
>>  {
>>  virObjectEventPtr event;
>> +virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>>  char *charAlias = NULL;
>> +char *tlsAlias = NULL;
>>  qemuDomainObjPrivatePtr priv = vm->privateData;
>>  int ret = -1;
>>  int rc;
>> @@ -3560,7 +3562,16 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
>>  if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias)))
>>  goto cleanup;
>>  
>> +if (chr->source->type == VIR_DOMAIN_CHR_TYPE_TCP &&
>> +chr->source->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES &&
>> +!(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
>> +goto cleanup;
>> +
>>  qemuDomainObjEnterMonitor(driver, vm);
>> +
>> +if (tlsAlias && qemuMonitorDelObject(priv->mon, tlsAlias) < 0)
>> +goto exit_monitor;
>> +
> 
> Those two qemu commands should be swapped as well.  This is similar to the
> issues that the next patch fixes.  The chardev uses tls object so the chardev
> should be detached before the tls object.

Took out my trusty pen and paper today and drew a large chart of attach
order, error order, and detach/remove order.

Long story short, for this one yes, moving the TLS deletion after the
chardev deletion is proper due to dependencies.

I've pushed this change.

John
> 
> ACK with that fixed.
> 
> Pavel
> 
>>  rc = qemuMonitorDetachCharDev(priv->mon, charAlias);
>>  if (qemuDomainObjExitMonitor(driver, vm) < 0)
>>  goto cleanup;
>> @@ -3579,7 +3590,13 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
>>  
>>   cleanup:
>>  VIR_FREE(charAlias);
>> +VIR_FREE(tlsAlias);
>> +virObjectUnref(cfg);
>>  return ret;
>> +
>> + exit_monitor:
>> +ignore_value(qemuDomainObjExitMonitor(driver, vm));
>> +goto cleanup;
>>  }
>>  
>>  
>> @@ -4461,13 +4478,10 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr 
>> driver,
>>virDomainChrDefPtr chr)
>>  {
>>  int ret = -1;
>> -virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>>  qemuDomainObjPrivatePtr priv = vm->privateData;
>>  virDomainDefPtr vmdef = vm->def;
>>  virDomainChrDefPtr tmpChr;
>> -char *objAlias = NULL;
>>  char *devstr = NULL;
>> -char *charAlias = NULL;
>>  
>>  if (!(tmpChr = virDomainChrFind(vmdef, chr))) {
>>  virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>> @@ -4480,26 +4494,16 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr 
>> driver,
>>  
>>  sa_assert(tmpChr->info.alias);
>>  
>> -if (!(charAlias = qemuAliasChardevFromDevAlias(tmpChr->info.alias)))
>> -goto cleanup;
>> -
>> -if (tmpChr->source->type == VIR_DOMAIN_CHR_TYPE_TCP &&
>> -tmpChr->source->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES &&
>> -!(objAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
>> -goto cleanup;
>> -
>>  if (qemuBuildChrDeviceStr(, vmdef, chr, priv->qemuCaps) < 0)
>>  goto cleanup;
>>  
>>  qemuDomainMarkDeviceForRemoval(vm, >info);
>>  
>>  qemuDomainObjEnterMonitor(driver, vm);
>> -if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0)
>> -goto exit_monitor;
>> -
>> -if (objAlias && qemuMonitorDelObject(priv->mon, objAlias) < 0)
>> -goto exit_monitor;
>> -
>> +if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) {
>> +ignore_value(qemuDomainObjExitMonitor(driver, vm));
>> +goto cleanup;
>> +}
>>  if (qemuDomainObjExitMonitor(driver, vm) < 0)
>>  goto cleanup;
>>  
>> @@ -4511,13 +4515,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
>>   cleanup:
>>  qemuDomainResetDeviceRemoval(vm);
>>  VIR_FREE(devstr);
>> -VIR_FREE(charAlias);
>> -virObjectUnref(cfg);
>>  return ret;
>> -
>> - exit_monitor:
>> -ignore_value(qemuDomainObjExitMonitor(driver, vm));
>> -goto cleanup;
>>  }
>>  
>>  
>> -- 
>> 2.7.4
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] question about rdma migration

[Roy Shterman]

Hi Jiri,

Sorry about the late response, only now I managed to push iSER into QEMU.
Because ISER is registered as a different protocol than iSCSI with the
prefix of iser:// I want to add support for it in libvirt.

Libvirt code is pretty new for me, wondered if adding
VIR_STORAGE_NET_PROTOCOL_ISER as another virStorageNetProtocol should be
enough?
Of course I added ISER in all necessary switch-case in code also.

Thanks,
Roy


On Tue, Mar 22, 2016 at 2:56 PM, Jiri Denemark  wrote:

> On Tue, Mar 22, 2016 at 14:21:52 +0200, Roy Shterman wrote:
> > Correct me if I'm wrong but locked option is pinning all VM memory in
> host
> > RAM,
> >
> > for example if I have a VM with 4G memory, and I want to run some QEMU
> code
> > which needs to pin 500M,
> >
> > I will need to lock all 4G in host memory instead of locking only 500M.
>
> So the question is which code wants to lock part of the memory, why, and
> if it's something that can be influenced by user.
>
> For example, we know that if you ask for all memory to by locked, we
> need to set the limit. The same applies when RDMA migration is started.
> On PPC we know some amount of memory will always need to be locked, we
> compute the amount and set the limit accordingly. We can't really expect
> user to have deep knowledge of QEMU and know what limit needs to be set
> when they use a specific device, QMP command, or whatever. So if the
> limit is something predictable and deterministic, we can automatically
> compute the amount of memory and use it when starting QEMU. Forcing
> users to set the limit when all memory needs to be locked is already bad
> enough that I don't think we should add a new option to explicitly set
> arbitrary lock limit.
>
> Jirka
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v5 03/15] qemu: set/use info->pciConnectFlags when validating/assigning PCI addresses

[Laine Stump]

On 10/25/2016 12:53 PM, Andrea Bolognani wrote:

On Tue, 2016-10-25 at 09:49 -0400, Laine Stump wrote:

Set pciConnectFlags in each device's DeviceInfo and then use those
flags later when validating existing addresses in
qemuDomainCollectPCIAddress() and when assigning new addresses with
qemuDomainPCIAddressReserveNextAddr() (rather than scattering the
logic about which devices need which type of slot all over the place).
  
Note that the exact flags set by

qemuDomainDeviceCalculatePCIConnectFlags() are different from the
flags previously set manually in qemuDomainCollectPCIAddress(), but
this doesn't matter because all validation of addresses in that case
ignores the setting of the HOTPLUGGABLE flag, and treats PCIE_DEVICE
and PCI_DEVICE the same (this lax checking was done on purpose,
because there are some things that we want to allow the user to
specify manually, e.g. assigning a PCIe device to a PCI slot, that we
*don't* ever want libvirt to do automatically. The flag settings that
we *really* want to match are 1) the old flag settings in
qemuDomainAssignDevicePCISlots() (which is HOTPLUGGABLE | PCI_DEVICE
for everything except PCI controllers) and the new flag settings done

[...] and 2) the new [...]


by qemuDomainDeviceCalculatePCIConnectFlags() (which are currently
exactly that - HOTPLUGGABLE | PCI_DEVICE for everything except PCI
controllers).
---
   src/qemu/qemu_domain_address.c | 205 
+++--
   1 file changed, 74 insertions(+), 131 deletions(-)
  
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c

index 602179c..d731b19 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -721,7 +721,7 @@ qemuDomainFillDevicePCIConnectFlagsIter(virDomainDefPtr def 
ATTRIBUTE_UNUSED,
* failure would be some internal problem with
* virDomainDeviceInfoIterate())
*/
-static int ATTRIBUTE_UNUSED
+static int
   qemuDomainFillAllPCIConnectFlags(virDomainDefPtr def,
virQEMUCapsPtr qemuCaps)
   {

You should remove ATTRIBUTE_UNUSED from
qemuDomainFillDevicePCIConnectFlags() as well.

[...]

@@ -926,7 +857,7 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def 
ATTRIBUTE_UNUSED,
   entireSlot = (addr->function == 0 &&
 addr->multi != VIR_TRISTATE_SWITCH_ON);
   
-if (virDomainPCIAddressReserveAddr(addrs, addr, flags,

+if (virDomainPCIAddressReserveAddr(addrs, addr, info->pciConnectFlags,
  entireSlot, true) < 0)

Would it be cleaner to have a qemuDomainPCIAddressReserveAddr()
function that takes @info directly?


Actually in a later series (the one that cleans up the *Slot() vs 
*Addr() naming), I eliminated all but one of the 
qemuDomainPCIAddressReserve*() functions anyway. After that series, 
there are only two *PCIAddressReserve*() functions used in this file: 
qemuDomainPCIAddressReserveNextAddr() (21 times), and 
virDomainPCIAddressReserveAddr() (12 times). The latter can't have a 
nice flags-removing wrapper added in qemu_domain_address.c (like the 
former does) because it often is called with a bare address - no DeviceInfo


(Well, I don't know, maybe it could be done by reorganizing some of the 
calls, I'll have to look at it).







It would be used only a single time, so it could very well be
argued that it would be overkill... On the other hand, it would
be neat not to use virDomainPCIAddressReserve*() functions at
all in the qemu driver and rely solely on the wrappers instead.

Speaking of which, even with the full series applied there
are still a bunch of uses of virDomainPCIAddressReserveAddr()
and virDomainPCIAddressReserveSlot(), mostly in
qemuDomainValidateDevicePCISlots{PIIX3,Q35}().


Yeah, my later series turns all of those into 
virDomainPCIAddressReserveAddr().




The attached patch makes all of them go away, except a few
that actually make sense because they set aside addresses for
potential future devices and as such don't have access to
a virDomainDeviceInfo yet.

I'm not saying we should deal with this right away: I'd
rather go back later to tidy up the whole thing than hold up
the series or go through another round of reviews for what
is ultimately a cosmetic issue.

[...]

@@ -1878,24 +1795,50 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
*/
   if (!buses_reserved &&
   !qemuDomainMachineIsVirt(def) &&
-qemuDomainPCIAddressReserveNextSlot(addrs, , flags) < 0)
+qemuDomainPCIAddressReserveNextSlot(addrs, ) < 0)
   goto cleanup;
   
   if (qemuDomainAssignDevicePCISlots(def, qemuCaps, addrs) < 0)

   goto cleanup;
   
   for (i = 1; i < addrs->nbuses; i++) {

+virDomainDeviceDef dev;
+int contIndex;
   virDomainPCIAddressBusPtr bus = >buses[i];
   
   if ((rv = virDomainDefMaybeAddController(

def, 

Re: [libvirt] [PATCH v2 12/12] virsh: Add _length parameters to virsh output

[Erik Skultety]

On Thu, Oct 06, 2016 at 06:39:00PM -0400, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1349898
> 
> Add the duration parameters to the virsh input/output for blkdeviotune
> command and describe them in the pod file.
> 
> Signed-off-by: John Ferlan 
> ---
>  tools/virsh-domain.c | 55 
> 
>  tools/virsh.pod  | 21 
>  2 files changed, 76 insertions(+)
> 

ACK

Erik


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 11/12] virsh: Create a macro to add IOTUNE values

[Erik Skultety]

On Thu, Oct 06, 2016 at 06:38:59PM -0400, John Ferlan wrote:
> Rework the repetitive lines to add iotune values into easier to read macro
> 
> Signed-off-by: John Ferlan 
> ---
>  tools/virsh-domain.c | 141 
> +--
>  1 file changed, 25 insertions(+), 116 deletions(-)
> 

[...]

> +#define VSH_ADD_IOTUNE(PARAM, CONST) 
>   \
> +if ((rv = vshCommandOptScaledInt(ctl, cmd, #PARAM, ,   
>   \
> + 1, ULLONG_MAX)) < 0) {  
>   \

Again a nitpick, I guess the -length setting should not get scaled when one
tries e.g. 'kib' to pass along with the setting. In terms of a cleanup, yes,
it's very welcome, so maybe add a 'SCALE' argument - your call.

ACK

Erik

> +goto interror;   
>   \
> +} else if (rv > 0) { 
>   \
> +if (virTypedParamsAddULLong(, , ,   
>   \
> +VIR_DOMAIN_BLOCK_IOTUNE_##CONST, 
>   \
> +value) < 0)  
>   \
> +goto save_error; 
>   \
> +}
>   \
> +
> +VSH_ADD_IOTUNE(total-bytes-sec, TOTAL_BYTES_SEC);
> +VSH_ADD_IOTUNE(read-bytes-sec, READ_BYTES_SEC);
> +VSH_ADD_IOTUNE(write-bytes-sec, WRITE_BYTES_SEC);
> +VSH_ADD_IOTUNE(total-iops-sec, TOTAL_IOPS_SEC);
> +VSH_ADD_IOTUNE(read-iops-sec, READ_IOPS_SEC);
> +VSH_ADD_IOTUNE(write-iops-sec, WRITE_IOPS_SEC);
> +
> +VSH_ADD_IOTUNE(total-bytes-sec-max, TOTAL_BYTES_SEC_MAX);
> +VSH_ADD_IOTUNE(read-bytes-sec-max, READ_BYTES_SEC_MAX);
> +VSH_ADD_IOTUNE(write-bytes-sec-max, WRITE_BYTES_SEC_MAX);
> +VSH_ADD_IOTUNE(total-iops-sec-max, TOTAL_IOPS_SEC_MAX);
> +VSH_ADD_IOTUNE(read-iops-sec-max, READ_IOPS_SEC_MAX);
> +VSH_ADD_IOTUNE(write-iops-sec-max, WRITE_IOPS_SEC_MAX);
> +VSH_ADD_IOTUNE(size-iops-sec, SIZE_IOPS_SEC);
>  
>  if (nparams == 0) {
>  if (virDomainGetBlockIoTune(dom, NULL, NULL, , flags) != 0) {
> -- 
> 2.7.4
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 10/12] qemu: Add the length options to the iotune command line

[Erik Skultety]

On Thu, Oct 06, 2016 at 06:38:58PM -0400, John Ferlan wrote:
> Add in the block I/O throttling length/duration parameter to the command
> line if supported. If not supported, fail command creation.
> 
> Add the xml2argvtest for testing.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/qemu/qemu_command.c| 21 +
>  .../qemuxml2argv-blkdeviotune-max-length.args  | 34 
> ++
>  tests/qemuxml2argvtest.c   |  4 +++
>  3 files changed, 59 insertions(+)
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.args

ACK

Erik


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 09/12] conf: Add support for blkiotune "_length" options

[Erik Skultety]

On Thu, Oct 06, 2016 at 06:38:57PM -0400, John Ferlan wrote:
> Modify _virDomainBlockIoTuneInfo and rng schema to support the _length
> options for bps/iops throttling values. Document the new values.
> 
> Signed-off-by: John Ferlan 
> ---
>  docs/formatdomain.html.in  | 40 -
>  docs/schemas/domaincommon.rng  | 38 +
>  src/conf/domain_conf.c | 24 +++-
>  .../qemuxml2argv-blkdeviotune-max-length.xml   | 65 
> ++
>  .../qemuxml2xmlout-blkdeviotune-max-length.xml |  1 +
>  tests/qemuxml2xmltest.c|  1 +
>  6 files changed, 166 insertions(+), 3 deletions(-)
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.xml
>  create mode 12 
> tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max-length.xml
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 1266e9d..274b9bb 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -2617,7 +2617,45 @@
>  maximum write I/O operations per second.
>size_iops_sec
>The optional size_iops_sec element is the
> -size of I/O operations per second.
> +size of I/O operations per second.
> +  
> +Throughput limits since 1.2.11 and QEMU 
> 1.7
> +  
> +  
> +  total_bytes_sec_max_length
> +  The optional total_bytes_sec_max_length
> +element is the maximum duration in seconds for the
> +total_bytes_sec_max burst period. Only valid
> +when the total_bytes_sec_max is set.
> +  read_bytes_sec_max_length
> +  The optional read_bytes_sec_max_length
> +element is the maximum duration in seconds for the
> +read_bytes_sec_max burst period. Only valid
> +when the read_bytes_sec_max is set.
> +  write_bytes_sec_max
> +  The optional write_bytes_sec_max_length
> +element is the maximum duration in seconds for the
> +write_bytes_sec_max burst period. Only valid
> +when the if write_bytes_sec_max is set.

s/if //

ACK

Erik


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 08/12] qemu: Add length for bps/iops throttling parameters to driver

[Erik Skultety]

On Thu, Oct 06, 2016 at 06:38:56PM -0400, John Ferlan wrote:
> Add support for a duration/length for the bps/iops and friends.
> 
> Modify the API in order to add the "blkdeviotune." specific definitions
> for the iotune throttling duration/length options
> 
> total_bytes_sec_max_length
> write_bytes_sec_max_length
> read_bytes_sec_max_length
> total_iops_sec_max_length
> write_iops_sec_max_length
> read_iops_sec_max_length
> 
> Signed-off-by: John Ferlan 
> ---
>  include/libvirt/libvirt-domain.h |  54 +
>  src/conf/domain_conf.h   |   6 +++
>  src/qemu/qemu_driver.c   | 100 
> +--
>  src/qemu/qemu_monitor.c  |   7 ++-
>  src/qemu/qemu_monitor.h  |   3 +-
>  src/qemu/qemu_monitor_json.c |  25 +-
>  src/qemu/qemu_monitor_json.h |   3 +-
>  tests/qemumonitorjsontest.c  |  17 ++-
>  8 files changed, 202 insertions(+), 13 deletions(-)
> 

[...]

> @@ -17296,7 +17297,9 @@ 
> qemuDomainSetBlockIoTuneSetDefaults(virDomainBlockIoTuneInfoPtr newinfo,

just a nitpick, are both the 'Set's necessary in the name, unless one of them
is a verb and the other a noun, I think the first one could be dropped.

>  bool set_iops,
>  bool set_bytes_max,
>  bool set_iops_max,
> -bool set_size_iops)
> +bool set_size_iops,
> +bool set_bytes_max_length,
> +bool set_iops_max_length)
>  {
>  if (!set_bytes) {
>  newinfo->total_bytes_sec = oldinfo->total_bytes_sec;
> @@ -17320,6 +17323,36 @@ 
> qemuDomainSetBlockIoTuneSetDefaults(virDomainBlockIoTuneInfoPtr newinfo,
>  }
>  if (!set_size_iops)
>  newinfo->size_iops_sec = oldinfo->size_iops_sec;
> +
> +/* The length field is handled a bit differently. If not defined/set,
> + * QEMU will default these to 0 or 1 depending on whether something in
> + * the same family is set or not.
> + *
> + * Similar to other values, if nothing in the family is defined/set,
> + * then take whatever is in the oldinfo.
> + *
> + * To clear an existing limit, a 0 is provided; however, passing that
> + * 0 onto QEMU if there's a family value defined/set (or defaulted)
> + * will cause an error. So, to mimic that, if our oldinfo was set and
> + * our newinfo is clearing, then set max_length based on whether we
> + * have a value in the family set/defined. */

Hmm. You can disregard my comment in 4/12 about replacing the whole function
with a macro, instead I suggest keeping the function and making the macro below
also work for all the settings above, otherwise it just doesn't look right, one
part of the function being expanded while the other covered by a macro because
the behaviour is slightly different (I have to admit though, it was kinda
painful to comprehend what's going on on the qemu side)

> +#define SET_MAX_LENGTH(BOOL, FIELD)  
>   \
> +if (!BOOL)   
>   \
> +newinfo->FIELD##_max_length = oldinfo->FIELD##_max_length;   
>   \
> +else if (BOOL && oldinfo->FIELD##_max_length &&  
>   \
> + !newinfo->FIELD##_max_length)   
>   \
> +newinfo->FIELD##_max_length = (newinfo->FIELD || 
>   \
> +   newinfo->FIELD##_max) ? 1 : 0;
> +
> +SET_MAX_LENGTH(set_bytes_max_length, total_bytes_sec);
> +SET_MAX_LENGTH(set_bytes_max_length, read_bytes_sec);
> +SET_MAX_LENGTH(set_bytes_max_length, write_bytes_sec);
> +SET_MAX_LENGTH(set_iops_max_length, total_iops_sec);
> +SET_MAX_LENGTH(set_iops_max_length, read_iops_sec);
> +SET_MAX_LENGTH(set_iops_max_length, write_iops_sec);
> +
> +#undef SET_MAX_LENGTH
> +
>  }
>  
>  
> @@ -17346,7 +17379,10 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
>  bool set_bytes_max = false;
>  bool set_iops_max = false;
>  bool set_size_iops = false;
> +bool set_bytes_max_length = false;
> +bool set_iops_max_length = false;
>  bool supportMaxOptions = true;
> +bool supportMaxLengthOptions = true;
>  virQEMUDriverConfigPtr cfg = NULL;
>  virObjectEventPtr event = NULL;
>  virTypedParameterPtr eventParams = NULL;
> @@ -17382,6 +17418,18 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
> VIR_TYPED_PARAM_ULLONG,
> VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC,
> VIR_TYPED_PARAM_ULLONG,
> +   
> VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH,
> +

Re: [libvirt] [PATCH v5 03/15] qemu: set/use info->pciConnectFlags when validating/assigning PCI addresses

[Andrea Bolognani]

On Tue, 2016-10-25 at 09:49 -0400, Laine Stump wrote:
> Set pciConnectFlags in each device's DeviceInfo and then use those
> flags later when validating existing addresses in
> qemuDomainCollectPCIAddress() and when assigning new addresses with
> qemuDomainPCIAddressReserveNextAddr() (rather than scattering the
> logic about which devices need which type of slot all over the place).
> 
> Note that the exact flags set by
> qemuDomainDeviceCalculatePCIConnectFlags() are different from the
> flags previously set manually in qemuDomainCollectPCIAddress(), but
> this doesn't matter because all validation of addresses in that case
> ignores the setting of the HOTPLUGGABLE flag, and treats PCIE_DEVICE
> and PCI_DEVICE the same (this lax checking was done on purpose,
> because there are some things that we want to allow the user to
> specify manually, e.g. assigning a PCIe device to a PCI slot, that we
> *don't* ever want libvirt to do automatically. The flag settings that
> we *really* want to match are 1) the old flag settings in
> qemuDomainAssignDevicePCISlots() (which is HOTPLUGGABLE | PCI_DEVICE
> for everything except PCI controllers) and the new flag settings done

[...] and 2) the new [...]

> by qemuDomainDeviceCalculatePCIConnectFlags() (which are currently
> exactly that - HOTPLUGGABLE | PCI_DEVICE for everything except PCI
> controllers).
> ---
>  src/qemu/qemu_domain_address.c | 205 
>+++--
>  1 file changed, 74 insertions(+), 131 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index 602179c..d731b19 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -721,7 +721,7 @@ qemuDomainFillDevicePCIConnectFlagsIter(virDomainDefPtr 
> def ATTRIBUTE_UNUSED,
>   * failure would be some internal problem with
>   * virDomainDeviceInfoIterate())
>   */
> -static int ATTRIBUTE_UNUSED
> +static int
>  qemuDomainFillAllPCIConnectFlags(virDomainDefPtr def,
>   virQEMUCapsPtr qemuCaps)
>  {

You should remove ATTRIBUTE_UNUSED from
qemuDomainFillDevicePCIConnectFlags() as well.

[...]
> @@ -926,7 +857,7 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def 
> ATTRIBUTE_UNUSED,
>  entireSlot = (addr->function == 0 &&
>addr->multi != VIR_TRISTATE_SWITCH_ON);
>  
> -if (virDomainPCIAddressReserveAddr(addrs, addr, flags,
> +if (virDomainPCIAddressReserveAddr(addrs, addr, info->pciConnectFlags,
> entireSlot, true) < 0)

Would it be cleaner to have a qemuDomainPCIAddressReserveAddr()
function that takes @info directly?

It would be used only a single time, so it could very well be
argued that it would be overkill... On the other hand, it would
be neat not to use virDomainPCIAddressReserve*() functions at
all in the qemu driver and rely solely on the wrappers instead.

Speaking of which, even with the full series applied there
are still a bunch of uses of virDomainPCIAddressReserveAddr()
and virDomainPCIAddressReserveSlot(), mostly in
qemuDomainValidateDevicePCISlots{PIIX3,Q35}().

The attached patch makes all of them go away, except a few
that actually make sense because they set aside addresses for
potential future devices and as such don't have access to
a virDomainDeviceInfo yet.

I'm not saying we should deal with this right away: I'd
rather go back later to tidy up the whole thing than hold up
the series or go through another round of reviews for what
is ultimately a cosmetic issue.

[...]
> @@ -1878,24 +1795,50 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
>   */
>  if (!buses_reserved &&
>  !qemuDomainMachineIsVirt(def) &&
> -qemuDomainPCIAddressReserveNextSlot(addrs, , flags) < 0)
> +qemuDomainPCIAddressReserveNextSlot(addrs, ) < 0)
>  goto cleanup;
>  
>  if (qemuDomainAssignDevicePCISlots(def, qemuCaps, addrs) < 0)
>  goto cleanup;
>  
>  for (i = 1; i < addrs->nbuses; i++) {
> +virDomainDeviceDef dev;
> +int contIndex;
>  virDomainPCIAddressBusPtr bus = >buses[i];
>  
>  if ((rv = virDomainDefMaybeAddController(
>   def, VIR_DOMAIN_CONTROLLER_TYPE_PCI,
>   i, bus->model)) < 0)
>  goto cleanup;
> -/* If we added a new bridge, we will need one more address */
> -if (rv > 0 &&
> -qemuDomainPCIAddressReserveNextSlot(addrs, , flags) < 0)
> +
> +if (rv == 0)
> +continue; /* no new controller added */

Alternatively, you could turn this into

  if (rv > 0) {
  virDomainDeviceDef dev;
  int contIndex;

  /* The code below */
  }

but I leave up to you whether to go one way or the other.

> +
> +/* We did add a new controller, so we will need one more
> + * address (and we need to set the new 

Re: [libvirt] [PATCH v11 3/5] qemu: Need to remove TLS object in RemoveRNGDevice

[Pavel Hrdina]

On Tue, Oct 25, 2016 at 09:19:58AM -0400, John Ferlan wrote:
> 
> 
> On 10/25/2016 08:53 AM, Pavel Hrdina wrote:
> > On Mon, Oct 24, 2016 at 06:46:19PM -0400, John Ferlan wrote:
> >> Commit id '6e6b4bfc' added the object, but forgot the other end.
> >>
> >> Signed-off-by: John Ferlan 
> >> ---
> >>  src/qemu/qemu_hotplug.c | 12 +++-
> >>  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > ACK if you agree with review on the first patch that the order of 
> > qemuMonitorDelObject and qemuMonitorDetachCharDev should be swapped.
> > 
> > Pavel
> > 
> 
> I do agree, but I hadn't fully convinced myself - I needed the extra set
> of eyes to help with that... Of course, I botched that again here by
> removing the tlsobj before the chardev.   A problem which naturally
> persists in patch 5, but is easily adjusted..
> 
> I looked at and thought about qemuDomainRemoveRNGDevice long enough and
> compared to qemuDomainRemoveDiskDevice, but couldn't quite make up my
> mind and really didn't want to be creating "extra" patches for perhaps
> all the erroneous removals.
> 
> If my comparison is {Attach|Remove}Disk, which has...
> 
> Attach: Add secobj, Add encobj, Add Drive, and Add Device where drive
> can rely on secobj and/or encobj.  The encobj and secobj do not rely on
> each other. The secobj is the possible secret for the iSCSI/RBD server,
> while encobj is the possible secret for
> 
> On attach error the removal is Del Drive, Del secobj, Del encobj (order
> of secobj/encobj doesn't matter).
> 
> Detach: Del Device and call RemoveDisk which Del secobj, Del encobj, and
> Del Drive.
> 
> Then, the RemoveDisk is probably wrong (Del Drive should be first), but
> I didn't want to continue to bog down the chardev changes if my thoughts
> in patch 2 were wrong.

I think that the RemoveDisk is wrong and that the Del Drive should be first.
As you've wrote, the drive can rely on secobj and/or encobj so we should not
remove the object while they can be still used.

Pavel

> 
> Long way of saying, I want to get it right for this path and then if I'm
> right generate a RemoveDisk patch to swap order.
> 
> John
> 
> >>
> >> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> >> index e287aaf..f401b48 100644
> >> --- a/src/qemu/qemu_hotplug.c
> >> +++ b/src/qemu/qemu_hotplug.c
> >> @@ -3608,6 +3608,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver,
> >>  virObjectEventPtr event;
> >>  char *charAlias = NULL;
> >>  char *objAlias = NULL;
> >> +char *tlsAlias = NULL;
> >>  qemuDomainObjPrivatePtr priv = vm->privateData;
> >>  ssize_t idx;
> >>  int ret = -1;
> >> @@ -3616,15 +3617,23 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver,
> >>  VIR_DEBUG("Removing RNG device %s from domain %p %s",
> >>rng->info.alias, vm, vm->def->name);
> >>  
> >> +
> >>  if (virAsprintf(, "obj%s", rng->info.alias) < 0)
> >>  goto cleanup;
> >>  
> >>  if (!(charAlias = qemuAliasChardevFromDevAlias(rng->info.alias)))
> >>  goto cleanup;
> >>  
> >> +if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD &&
> >> +!(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
> >> +goto cleanup;
> >> +
> >>  qemuDomainObjEnterMonitor(driver, vm);
> >> -if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD)
> >> +if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) {
> >> +if (tlsAlias)
> >> +ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias));
> >>  ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias));
> >> +}
> >>  
> >>  rc = qemuMonitorDelObject(priv->mon, objAlias);
> >>  
> >> @@ -3648,6 +3657,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver,
> >>   cleanup:
> >>  VIR_FREE(charAlias);
> >>  VIR_FREE(objAlias);
> >> +VIR_FREE(tlsAlias);
> >>  return ret;
> >>  }
> >>  
> >> -- 
> >> 2.7.4
> >>
> >> --
> >> libvir-list mailing list
> >> libvir-list@redhat.com
> >> https://www.redhat.com/mailman/listinfo/libvir-list
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH python 1/1] fix crash in libvirt_virDomainPin*

[Martin Kletzander]

On Tue, Oct 25, 2016 at 12:42:23PM +0300, Konstantin Neumoin wrote:

If we pass large(more than cpunum) cpu mask to any libvirt_virDomainPin*
methods, it could leads to crash. So we have to check tuple size and
ignore extra tuple members.

Signed-off-by: Konstantin Neumoin 
---
libvirt-override.c | 8 
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/libvirt-override.c b/libvirt-override.c
index fa3e2ca..83b760b 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -1327,7 +1327,7 @@ libvirt_virDomainPinVcpu(PyObject *self ATTRIBUTE_UNUSED,
if (VIR_ALLOC_N(cpumap, cpumaplen) < 0)
return PyErr_NoMemory();

-for (i = 0; i < tuple_size; i++) {
+for (i = 0; i < MIN(cpunum, tuple_size); i++) {


I don't like it being called every single iteration of the loop.
Temporary variable outside the loop would be nicer.

Also since all these functions do the same thing, why not make a
function for this and call it from all the places?  It would also make a
nice clean-up.

Martin


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v5 03/15] qemu: set/use info->pciConnectFlags when validating/assigning PCI addresses

[Laine Stump]

Set pciConnectFlags in each device's DeviceInfo and then use those
flags later when validating existing addresses in
qemuDomainCollectPCIAddress() and when assigning new addresses with
qemuDomainPCIAddressReserveNextAddr() (rather than scattering the
logic about which devices need which type of slot all over the place).

Note that the exact flags set by
qemuDomainDeviceCalculatePCIConnectFlags() are different from the
flags previously set manually in qemuDomainCollectPCIAddress(), but
this doesn't matter because all validation of addresses in that case
ignores the setting of the HOTPLUGGABLE flag, and treats PCIE_DEVICE
and PCI_DEVICE the same (this lax checking was done on purpose,
because there are some things that we want to allow the user to
specify manually, e.g. assigning a PCIe device to a PCI slot, that we
*don't* ever want libvirt to do automatically. The flag settings that
we *really* want to match are 1) the old flag settings in
qemuDomainAssignDevicePCISlots() (which is HOTPLUGGABLE | PCI_DEVICE
for everything except PCI controllers) and the new flag settings done
by qemuDomainDeviceCalculatePCIConnectFlags() (which are currently
exactly that - HOTPLUGGABLE | PCI_DEVICE for everything except PCI
controllers).
---
 src/qemu/qemu_domain_address.c | 205 +++--
 1 file changed, 74 insertions(+), 131 deletions(-)

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 602179c..d731b19 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -721,7 +721,7 @@ qemuDomainFillDevicePCIConnectFlagsIter(virDomainDefPtr def 
ATTRIBUTE_UNUSED,
  * failure would be some internal problem with
  * virDomainDeviceInfoIterate())
  */
-static int ATTRIBUTE_UNUSED
+static int
 qemuDomainFillAllPCIConnectFlags(virDomainDefPtr def,
  virQEMUCapsPtr qemuCaps)
 {
@@ -777,21 +777,20 @@ qemuDomainFillDevicePCIConnectFlags(virDomainDefPtr def,
 static int
 qemuDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,
 virDomainDeviceInfoPtr dev,
-virDomainPCIConnectFlags flags,
 unsigned int function,
 bool reserveEntireSlot)
 {
-return virDomainPCIAddressReserveNextAddr(addrs, dev, flags,
+return virDomainPCIAddressReserveNextAddr(addrs, dev,
+  dev->pciConnectFlags,
   function, reserveEntireSlot);
 }
 
 
 static int
 qemuDomainPCIAddressReserveNextSlot(virDomainPCIAddressSetPtr addrs,
-virDomainDeviceInfoPtr dev,
-virDomainPCIConnectFlags flags)
+virDomainDeviceInfoPtr dev)
 {
-return qemuDomainPCIAddressReserveNextAddr(addrs, dev, flags, 0, true);
+return qemuDomainPCIAddressReserveNextAddr(addrs, dev, 0, true);
 }
 
 
@@ -805,9 +804,6 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def 
ATTRIBUTE_UNUSED,
 int ret = -1;
 virPCIDeviceAddressPtr addr = >addr.pci;
 bool entireSlot;
-/* flags may be changed from default below */
-virDomainPCIConnectFlags flags = (VIR_PCI_CONNECT_HOTPLUGGABLE |
-  VIR_PCI_CONNECT_TYPE_PCI_DEVICE);
 
 if (!virDeviceInfoPCIAddressPresent(info) ||
 ((device->type == VIR_DOMAIN_DEVICE_HOSTDEV) &&
@@ -819,71 +815,6 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def 
ATTRIBUTE_UNUSED,
 return 0;
 }
 
-/* Change flags according to differing requirements of different
- * devices.
- */
-switch (device->type) {
-case VIR_DOMAIN_DEVICE_CONTROLLER:
-switch (device->data.controller->type) {
-case  VIR_DOMAIN_CONTROLLER_TYPE_PCI:
-   flags = 
virDomainPCIControllerModelToConnectType(device->data.controller->model);
-break;
-
-case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
-/* SATA controllers aren't hot-plugged, and can be put in
- * either a PCI or PCIe slot
- */
-flags = (VIR_PCI_CONNECT_TYPE_PCI_DEVICE
- | VIR_PCI_CONNECT_TYPE_PCIE_DEVICE);
-break;
-
-case VIR_DOMAIN_CONTROLLER_TYPE_USB:
-/* allow UHCI and EHCI controllers to be manually placed on
- * the PCIe bus (but don't put them there automatically)
- */
-switch (device->data.controller->model) {
-case VIR_DOMAIN_CONTROLLER_MODEL_USB_EHCI:
-case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1:
-case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1:
-case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2:
-case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3:
-case VIR_DOMAIN_CONTROLLER_MODEL_USB_VT82C686B_UHCI:
-flags = 

[libvirt] [PATCH v5 09/15] [ACKED] qemu: only force an available legacy-PCI slot on domains with pci-root

[Laine Stump]

Andrea had the right idea when he disabled the "reserve an extra
unused slot" bit for aarch64/virt. For *any* PCI Express-based
machine, it is pointless since 1) an extra legacy-PCI slot can't be
used for hotplug, since hotplug into legacy PCI slots doesn't work on
PCI Express machinetypes, and 2) even for "coldplug" expansion,
everybody will want to expand using Express controllers, not legacy
PCI.

This patch eliminates the extra slot reserve unless the system has a
pci-root (i.e. legacy PCI)
---
 src/qemu/qemu_domain_address.c | 44 ++
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index f1f46b7..eb754f1 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -1805,8 +1805,6 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
  */
 virDomainDeviceInfo info;
 
-info.pciConnectFlags = VIR_PCI_CONNECT_TYPE_PCI_DEVICE;
-
 /* 1st pass to figure out how many PCI bridges we need */
 if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, true)))
 goto cleanup;
@@ -1815,23 +1813,35 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
  addrs) < 0)
 goto cleanup;
 
-for (i = 0; i < addrs->nbuses; i++) {
-if (!qemuDomainPCIBusFullyReserved(>buses[i]))
-buses_reserved = false;
-}
-
-/* Reserve 1 extra slot for a (potential) bridge only if buses
- * are not fully reserved yet.
+/* For domains that have pci-root, reserve 1 extra slot for a
+ * (potential) bridge (for future expansion) only if buses are
+ * not fully reserved yet (if all buses are fully reserved
+ * with manually/previously assigned addresses, any attempt to
+ * reserve an extra slot would fail anyway. But if all buses
+ * are *not* fully reserved, this extra reservation might push
+ * the config to add a new pci-bridge to plug into the final
+ * available slot, thus preserving the ability to expand)
  *
- * We don't reserve the extra slot for aarch64 mach-virt guests
- * either because we want to be able to have pure virtio-mmio
- * guests, and reserving this slot would force us to add at least
- * a dmi-to-pci-bridge to an otherwise PCI-free topology
+ * We only do this for those domains that have pci-root, since
+ * those with pcie-root will usually want to expand using PCIe
+ * controllers, which we will do after assigning addresses for
+ * all *actual* devices.
  */
-if (!buses_reserved &&
-!qemuDomainMachineIsVirt(def) &&
-qemuDomainPCIAddressReserveNextSlot(addrs, ) < 0)
-goto cleanup;
+
+if (qemuDomainMachineHasPCIRoot(def)) {
+info.pciConnectFlags = (VIR_PCI_CONNECT_HOTPLUGGABLE |
+VIR_PCI_CONNECT_TYPE_PCI_DEVICE);
+
+for (i = 0; i < addrs->nbuses; i++) {
+if (!qemuDomainPCIBusFullyReserved(>buses[i])) {
+buses_reserved = false;
+break;
+}
+}
+if (!buses_reserved &&
+qemuDomainPCIAddressReserveNextSlot(addrs, ) < 0)
+goto cleanup;
+}
 
 if (qemuDomainAssignDevicePCISlots(def, qemuCaps, addrs) < 0)
 goto cleanup;
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v5 15/15] qemu: initially reserve one open pcie-root-port for hotplug

[Laine Stump]

For machinetypes with a pci-root bus (all legacy PCI), libvirt will
make a "fake" reservation for one extra slot prior to assigning
addresses to unaddressed PCI endpoint devices in the domain. This will
trigger auto-adding of a pci-bridge for the final device to be
assigned an address *if that device would have otherwise instead been
the last device on the last available pci-bridge*; thus it assures
that there will always be at least one slot left open in the domain's
bus topology for expansion (which is important both for hotplug (since
a new pci-bridge can't be added while the guest is running) as well as
for offline additions to the config (since adding a new device might
otherwise in some cases require re-addressing existing devices, which
we want to avoid)).

It's important to note that for the above case (legacy PCI), we must
check for the special case of all slots on all buses being occupied
*prior to assigning any addresses*, and avoid attempting to reserve
the extra address in that case, because there is no free address in
the existing topology, so no place to auto-add a pci-bridge for
expansion (i.e. it would always fail anyway). Since that condition can
only be reached by manual intervention, this is acceptable.

For machinetypes with pcie-root (Q35, aarch64 virt), libvirt's
methodology for automatically expanding the bus topology is different
- pcie-root-ports are plugged into slots (soon to be functions) of
pcie-root as needed, and the new endpoint devices are assigned to the
single slot in each pcie-root-port. This is done so that the devices
are, by default, hotpluggable (the slots of pcie-root don't support
hotplug, but the single slot of the pcie-root-port does). Since
pcie-root-ports can only be plugged into pcie-root, and we don't
auto-assign endpoint devices to the pcie-root slots, this means
topology expansion doesn't compete with endpoint devices for slots, so
we don't need to worry about checking for all "useful" slots being
free *prior* to assigning addresses to new endpoint devices - as a
matter of fact, if we attempt to reserve the open slots before the
used slots, it can lead to errors.

Instead this patch just reserves one slot for a "future potential"
PCIe device after doing the assignment for actual devices, but only
if the only PCI controller defined prior to starting address
assignment was pcie-root, and only if we auto-added at least one PCI
controller during address assignment.  This assures two things:

1) that reserving the open slots will only be done when the domain is
   initially defined, never at any time after, and

2) that if the user understands enough about PCI controllers that they
   are adding them manually, that we don't mess up their plan by
   adding extras - if they know enough to add one pcie-root-port, or
   to manually assign addresses such that no pcie-root-ports are
   needed, they know enough to add extra pcie-root-ports if they want
   them (this could be called the "libguestfs clause", since
   libguestfs needs to be able to create domains with as few
   devices/controllers as possible).

This is set to reserve a single free port for now, but could be
increased in the future if public sentiment goes in that direction
(it's easy to increase later, but essential impossible to decrease)
---
 src/qemu/qemu_domain_address.c | 30 
 .../qemuxml2argv-bios-nvram-secure.args|  1 +
 .../qemuxml2argv-machine-smm-opt.args  |  1 +
 .../qemuxml2argv-q35-default-devices-only.args |  1 +
 .../qemuxml2argv-q35-pcie-autoadd.args |  1 +
 .../qemuxml2argv-q35-pm-disable-fallback.args  |  1 +
 .../qemuxml2argv-q35-pm-disable.args   |  1 +
 .../qemuxml2argv-q35-virt-manager-basic.args   |  1 +
 tests/qemuxml2argvtest.c   | 33 ++
 .../qemuxml2xmlout-q35-default-devices-only.xml|  5 
 .../qemuxml2xmlout-q35-pcie-autoadd.xml|  5 
 .../qemuxml2xmlout-q35-virt-manager-basic.xml  |  5 
 tests/qemuxml2xmltest.c| 19 +++--
 13 files changed, 96 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 773e707..42af435 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -1927,6 +1927,36 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
 if (qemuDomainAssignDevicePCISlots(def, qemuCaps, addrs) < 0)
 goto cleanup;
 
+/* Only for *new* domains with pcie-root (and no other
+ * manually specified PCI controllers in the definition): If,
+ * after assigning addresses/reserving slots for all devices,
+ * we see that any extra buses have been auto-added, we
+ * understand that the application has left management of PCI
+ * addresses and controllers up to libvirt. In order to allow
+ * such applications to easily 

[libvirt] [PATCH v5 08/15] qemu: assign nec-xhci (USB3) controller to a PCIe address when appropriate

[Laine Stump]

The nec-usb-xhci device (which is a USB3 controller) has always
presented itself as a PCI device when plugged into a legacy PCI slot,
and a PCIe device when plugged into a PCIe slot, but libvirt has
always auto-assigned it to a legacy PCI slot.

This patch changes that behavior to auto-assign to a PCIe slot on
systems that have pcie-root (e.g. Q35 and aarch64/virt).

Since we don't yet auto-create pcie-*-port controllers on demand, this
means a config with an nec-xhci USB controller that has no PCI address
assigned will also need to have an otherwise-unused pcie-*-port
controller specified:

   
   

(this assumes there is an otherwise-unused slot on pcie-root to accept
the pcie-root-port)
---
 src/qemu/qemu_domain_address.c |  2 +-
 tests/qemuxml2argvdata/qemuxml2argv-autoindex.args | 10 +++
 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.args  | 21 ++---
 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.xml   |  2 ++
 .../qemuxml2argv-q35-virtio-pci.args   |  7 ++---
 tests/qemuxml2argvtest.c   |  2 ++
 .../qemuxml2xmlout-autoindex.xml   | 10 +++
 .../qemuxml2xmloutdata/qemuxml2xmlout-q35-pcie.xml | 35 +-
 .../qemuxml2xmlout-q35-virtio-pci.xml  | 21 +
 tests/qemuxml2xmltest.c|  2 ++
 10 files changed, 49 insertions(+), 63 deletions(-)

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 9d00825..f1f46b7 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -446,7 +446,7 @@ 
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
 case VIR_DOMAIN_CONTROLLER_TYPE_USB:
 switch ((virDomainControllerModelUSB)cont->model) {
 case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI:
-return pciFlags;
+return pcieFlags;
 
 case VIR_DOMAIN_CONTROLLER_MODEL_USB_EHCI:
 case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1:
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-autoindex.args 
b/tests/qemuxml2argvdata/qemuxml2argv-autoindex.args
index 43b9661..fc14dc4 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-autoindex.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-autoindex.args
@@ -43,11 +43,11 @@ addr=0x1 \
 -device ich9-usb-uhci2,masterbus=usb2.0,firstport=2,bus=pci.2,addr=0x1.0x1 \
 -device ich9-usb-uhci3,masterbus=usb2.0,firstport=4,bus=pci.2,addr=0x1.0x2 \
 -device ich9-usb-ehci1,id=usb2,bus=pci.2,addr=0x1.0x7 \
--device nec-usb-xhci,id=usb3,bus=pci.2,addr=0x2 \
+-device nec-usb-xhci,id=usb3,bus=pci.5,addr=0x0 \
 -device 
ich9-usb-uhci1,masterbus=usb4.0,firstport=0,bus=pci.2,multifunction=on,\
-addr=0x3 \
--device ich9-usb-uhci2,masterbus=usb4.0,firstport=2,bus=pci.2,addr=0x3.0x1 \
--device ich9-usb-uhci3,masterbus=usb4.0,firstport=4,bus=pci.2,addr=0x3.0x2 \
--device ich9-usb-ehci1,id=usb4,bus=pci.2,addr=0x3.0x7 \
+addr=0x2 \
+-device ich9-usb-uhci2,masterbus=usb4.0,firstport=2,bus=pci.2,addr=0x2.0x1 \
+-device ich9-usb-uhci3,masterbus=usb4.0,firstport=4,bus=pci.2,addr=0x2.0x2 \
+-device ich9-usb-ehci1,id=usb4,bus=pci.2,addr=0x2.0x7 \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-sata0-0-0 \
 -device ide-drive,bus=ide.0,drive=drive-sata0-0-0,id=sata0-0-0
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.args 
b/tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.args
index d92e0b8..2738749 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.args
@@ -30,15 +30,12 @@ QEMU_AUDIO_DRV=none \
 -device ioh3420,port=0x58,chassis=12,id=pci.12,bus=pcie.0,addr=0xb \
 -device ioh3420,port=0x60,chassis=13,id=pci.13,bus=pcie.0,addr=0xc \
 -device ioh3420,port=0x68,chassis=14,id=pci.14,bus=pcie.0,addr=0xd \
--device ich9-usb-ehci1,id=usb,bus=pcie.0,addr=0x1d.0x7 \
--device 
ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pcie.0,multifunction=on,\
-addr=0x1d \
--device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pcie.0,addr=0x1d.0x1 \
--device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pcie.0,addr=0x1d.0x2 \
+-device ioh3420,port=0x70,chassis=15,id=pci.15,bus=pcie.0,addr=0xe \
+-device nec-usb-xhci,id=usb,bus=pci.8,addr=0x0 \
 -device virtio-scsi-pci,id=scsi0,bus=pci.7,addr=0x0 \
 -device virtio-serial-pci,id=virtio-serial0,bus=pci.6,addr=0x0 \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk1 \
--device virtio-blk-pci,bus=pci.8,addr=0x0,drive=drive-virtio-disk1,\
+-device virtio-blk-pci,bus=pci.9,addr=0x0,drive=drive-virtio-disk1,\
 id=virtio-disk1 \
 -fsdev local,security_model=passthrough,id=fsdev-fs0,path=/export/to/guest \
 -device virtio-9p-pci,id=fs0,fsdev=fsdev-fs0,mount_tag=/import/from/host,\
@@ -49,13 +46,13 @@ addr=0x0 \
 -netdev user,id=hostnet1 \
 -device e1000e,netdev=hostnet1,id=net1,mac=00:11:22:33:44:66,bus=pci.5,\
 addr=0x0 \
--device 

[libvirt] [PATCH v5 11/15] [RFC] qemu: if pci-bridge is in PCIe config w/o dmi-to-pci-bridge, add it

[Laine Stump]

I'm undecided if it is worthwhile to add this...

Up until now it has been legal to have something like this in the xml:

  
  

(for example, see the existing test case
"usb-controller-default-q35").  This is handled in
qemuDomainPCIAddressSetCreate() when it's adding in controllers to
fill holes in the indexes.)

Since we have always added the following to every config:

  

there has always been a proper place for the unaddressed pci-bridge to
plug in.

A upcoming commit will eliminate the forced adding of a
dmi-to-pci-bridge to *every* Q35 domain config, though. This would
mean the above-mentioned test case (and one other) would begin to
fail. There are two possible solutions to this problem:

1) change the test cases, and made the above construct an error (note
that in the future it will work just fine to not list *any* pci
controller, and they will all be auto-added as needed).

or

2) This patch, which specifically looks for the case where we have a
pci-bridge (but no dmi-to-pci-bridge) in a PCIe domain config and
auto-adds the necessary dmi-to-pci-bridge. This can only work in the
case that the pci-bridge has been given an index such that there is an
unused index with a lower value for the dmi-to-pci-bridge to occupy.

This seems like a kind of odd corner case that nobody should need to
do in the future anyway. On the other hand, I already wrote the code
and it works, so I might as well send it and see what opinions (if
any) it generates.
---
 src/qemu/qemu_domain_address.c | 42 +-
 1 file changed, 37 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 9e71ed5..e1e1f77 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -909,6 +909,9 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
 virDomainPCIAddressSetPtr addrs;
 size_t i;
 bool hasPCIeRoot = false;
+unsigned int lowestDMIToPCIBridge = nbuses;
+unsigned int lowestUnaddressedPCIBridge = nbuses;
+unsigned int lowestAddressedPCIBridge = nbuses;
 virDomainControllerModelPCI defaultModel;
 
 if ((addrs = virDomainPCIAddressSetAlloc(nbuses)) == NULL)
@@ -933,8 +936,24 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
 if (virDomainPCIAddressBusSetModel(>buses[idx], cont->model) < 
0)
 goto error;
 
-if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
+/* we'll use all this info later to determine if we need
+ * to add a dmi-to-pci-bridge due to unaddressed pci-bridge controllers
+ */
+if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) {
 hasPCIeRoot = true;
+} else if (cont->model ==
+   VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE) {
+if (lowestDMIToPCIBridge > idx)
+lowestDMIToPCIBridge = idx;
+} else if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE) {
+if (virDeviceInfoPCIAddressWanted(>info)) {
+if (lowestUnaddressedPCIBridge > idx)
+lowestUnaddressedPCIBridge = idx;
+} else {
+if (lowestAddressedPCIBridge > idx)
+lowestAddressedPCIBridge = idx;
+}
+}
 }
 
 if (nbuses > 0 && !addrs->buses[0].model) {
@@ -950,13 +969,21 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
 
 /* Now fill in a reasonable model for all the buses in the set
  * that don't yet have a corresponding controller in the domain
- * config.
+ * config.  In the rare (and ... strange, but still allowed) case
+ * that a domain has 1) a pcie-root at index 0, 2) *no*
+ * dmi-to-pci-bridge (or pci-bridge that was manually addressed to
+ * sit directly on pcie-root), and 3) does have an unaddressed
+ * pci-bridge at an index > 1, then we need to add a
+ * dmi-to-pci-bridge.
  */
 
-if (hasPCIeRoot)
-defaultModel = VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT;
-else
+if (!hasPCIeRoot)
 defaultModel = VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE;
+else if (lowestUnaddressedPCIBridge < MIN(lowestAddressedPCIBridge,
+  lowestDMIToPCIBridge))
+defaultModel = VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE;
+else
+defaultModel = VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT;
 
 for (i = 1; i < addrs->nbuses; i++) {
 
@@ -968,6 +995,11 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
 
 VIR_DEBUG("Auto-adding ",
   virDomainControllerModelPCITypeToString(defaultModel), i);
+/* only add a single dmi-to-pci-bridge, then add pcie-root-port
+ * for any other unspecified controller indexes.
+ */
+if (hasPCIeRoot)
+defaultModel = VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT;
 }
 
 if (virDomainDeviceInfoIterate(def, qemuDomainCollectPCIAddress, 

[libvirt] [PATCH v5 12/15] [ACKED] qemu: don't force-add a dmi-to-pci-bridge just on principle

[Laine Stump]

Now the a dmi-to-pci-bridge is automatically added just as it's needed
(when a pci-bridge is being added), we no longer have any need to
force-add one to every single Q35 domain.
---
 src/qemu/qemu_domain.c | 12 
 tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args |  3 +-
 .../qemuxml2argv-q35-pcie-autoadd.args | 55 +++
 .../qemuxml2xmlout-pcie-root.xml   |  4 --
 .../qemuxml2xmlout-q35-pcie-autoadd.xml| 82 ++
 5 files changed, 67 insertions(+), 89 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 310bfa6..5327add 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2125,18 +2125,6 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
  
VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)) {
 goto cleanup;
 }
-/* Add a dmi-to-pci-bridge bridge if there are no PCI controllers
- * other than the pcie-root. This is so that there will be 
hot-pluggable
- * PCI slots available.
- *
- * We skip this step for aarch64 mach-virt guests, where we want to
- * be able to have a pure virtio-mmio topology
- */
-if (virDomainControllerFind(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 1) < 
0 &&
-!qemuDomainMachineIsVirt(def) &&
-!virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 1,
-   
VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE))
-goto cleanup;
 }
 
 if (addDefaultMemballoon && !def->memballoon) {
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args 
b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args
index 7ef03d3..59a849f 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args
@@ -15,5 +15,4 @@ QEMU_AUDIO_DRV=none \
 -nodefaults \
 -monitor unix:/tmp/lib/domain--1-q35-test/monitor.sock,server,nowait \
 -no-acpi \
--boot c \
--device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e
+-boot c
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-pcie-autoadd.args 
b/tests/qemuxml2argvdata/qemuxml2argv-q35-pcie-autoadd.args
index 70c759e..daecf96 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-q35-pcie-autoadd.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-pcie-autoadd.args
@@ -16,42 +16,41 @@ QEMU_AUDIO_DRV=none \
 -monitor unix:/tmp/lib/domain--1-q35-test/monitor.sock,server,nowait \
 -no-acpi \
 -boot c \
--device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \
--device ioh3420,port=0x10,chassis=2,id=pci.2,bus=pcie.0,addr=0x2 \
--device ioh3420,port=0x18,chassis=3,id=pci.3,bus=pcie.0,addr=0x3 \
--device ioh3420,port=0x20,chassis=4,id=pci.4,bus=pcie.0,addr=0x4 \
--device ioh3420,port=0x28,chassis=5,id=pci.5,bus=pcie.0,addr=0x5 \
--device ioh3420,port=0x30,chassis=6,id=pci.6,bus=pcie.0,addr=0x6 \
--device ioh3420,port=0x38,chassis=7,id=pci.7,bus=pcie.0,addr=0x7 \
--device ioh3420,port=0x40,chassis=8,id=pci.8,bus=pcie.0,addr=0x8 \
--device ioh3420,port=0x48,chassis=9,id=pci.9,bus=pcie.0,addr=0x9 \
--device ioh3420,port=0x50,chassis=10,id=pci.10,bus=pcie.0,addr=0xa \
--device ioh3420,port=0x58,chassis=11,id=pci.11,bus=pcie.0,addr=0xb \
--device ioh3420,port=0x60,chassis=12,id=pci.12,bus=pcie.0,addr=0xc \
--device ioh3420,port=0x68,chassis=13,id=pci.13,bus=pcie.0,addr=0xd \
--device ioh3420,port=0x70,chassis=14,id=pci.14,bus=pcie.0,addr=0xe \
--device nec-usb-xhci,id=usb,bus=pci.7,addr=0x0 \
--device virtio-scsi-pci,id=scsi0,bus=pci.6,addr=0x0 \
--device virtio-serial-pci,id=virtio-serial0,bus=pci.5,addr=0x0 \
+-device ioh3420,port=0x10,chassis=1,id=pci.1,bus=pcie.0,addr=0x2 \
+-device ioh3420,port=0x18,chassis=2,id=pci.2,bus=pcie.0,addr=0x3 \
+-device ioh3420,port=0x20,chassis=3,id=pci.3,bus=pcie.0,addr=0x4 \
+-device ioh3420,port=0x28,chassis=4,id=pci.4,bus=pcie.0,addr=0x5 \
+-device ioh3420,port=0x30,chassis=5,id=pci.5,bus=pcie.0,addr=0x6 \
+-device ioh3420,port=0x38,chassis=6,id=pci.6,bus=pcie.0,addr=0x7 \
+-device ioh3420,port=0x40,chassis=7,id=pci.7,bus=pcie.0,addr=0x8 \
+-device ioh3420,port=0x48,chassis=8,id=pci.8,bus=pcie.0,addr=0x9 \
+-device ioh3420,port=0x50,chassis=9,id=pci.9,bus=pcie.0,addr=0xa \
+-device ioh3420,port=0x58,chassis=10,id=pci.10,bus=pcie.0,addr=0xb \
+-device ioh3420,port=0x60,chassis=11,id=pci.11,bus=pcie.0,addr=0xc \
+-device ioh3420,port=0x68,chassis=12,id=pci.12,bus=pcie.0,addr=0xd \
+-device ioh3420,port=0x70,chassis=13,id=pci.13,bus=pcie.0,addr=0xe \
+-device nec-usb-xhci,id=usb,bus=pci.6,addr=0x0 \
+-device virtio-scsi-pci,id=scsi0,bus=pci.5,addr=0x0 \
+-device virtio-serial-pci,id=virtio-serial0,bus=pci.4,addr=0x0 \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk1 \
--device virtio-blk-pci,bus=pci.8,addr=0x0,drive=drive-virtio-disk1,\
+-device virtio-blk-pci,bus=pci.7,addr=0x0,drive=drive-virtio-disk1,\
 id=virtio-disk1 \
 -fsdev 

[libvirt] [PATCH v5 04/15] qemu: set/use proper pciConnectFlags during hotplug

[Laine Stump]

Before now, all the qemu hotplug functions assumed that all devices to
be hotplugged were legacy PCI endpoint devices
(VIR_PCI_CONNECT_TYPE_PCI_DEVICE). This worked out "okay", because all
devices *are* legacy PCI endpoint devices on x86/440fx machinetypes,
and hotplug didn't work properly on machinetypes using PCIe anyway
(hotplugging onto a legacy PCI slot doesn't work, and until commit
b87703cf any attempt to manually specify a PCIe address for a
hotplugged device would be erroneously rejected).

This patch makes all qemu hotplug operations honor the pciConnectFlags
set by the single all-knowing function
qemuDomainDeviceCalculatePCIConnectFlags(). This is done in 3 steps,
but in a single commit since we would have to touch the other points
at each step anyway:

1) add a flags argument to the hypervisor-agnostic
virDomainPCIAddressEnsureAddr() (previously it hardcoded
..._PCI_DEVICE)

2) add a new qemu-specific function qemuDomainEnsurePCIAddress() which
gets the correct pciConnectFlags for the device from
qemuDomainDeviceConnectFlags(), then calls
virDomainPCIAddressEnsureAddr().

3) in qemu_hotplug.c replace all calls to
virDomainPCIAddressEnsureAddr() with calls to
qemuDomainEnsurePCIAddress()

So in effect, we're putting a "shim" on top of all calls to
virDomainPCIAddressEnsureAddr() that sets the right pciConnectFlags.
---
 src/conf/domain_addr.c | 10 ++
 src/conf/domain_addr.h |  3 ++-
 src/qemu/qemu_domain_address.c | 27 +++
 src/qemu/qemu_domain_address.h |  4 
 src/qemu/qemu_hotplug.c| 20 ++--
 5 files changed, 49 insertions(+), 15 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 3db02f3..2195a95 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -487,17 +487,11 @@ virDomainPCIAddressReserveSlot(virDomainPCIAddressSetPtr 
addrs,
 
 int
 virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr addrs,
-  virDomainDeviceInfoPtr dev)
+  virDomainDeviceInfoPtr dev,
+  virDomainPCIConnectFlags flags)
 {
 int ret = -1;
 char *addrStr = NULL;
-/* Flags should be set according to the particular device,
- * but only the caller knows the type of device. Currently this
- * function is only used for hot-plug, though, and hot-plug is
- * only supported for standard PCI devices, so we can safely use
- * the setting below */
-virDomainPCIConnectFlags flags = (VIR_PCI_CONNECT_HOTPLUGGABLE |
-  VIR_PCI_CONNECT_TYPE_PCI_DEVICE);
 
 if (!(addrStr = virDomainPCIAddressAsString(>addr.pci)))
 goto cleanup;
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index 4d6d12a..92ee04c 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -144,7 +144,8 @@ int 
virDomainPCIAddressReserveSlot(virDomainPCIAddressSetPtr addrs,
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 
 int virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr addrs,
-  virDomainDeviceInfoPtr dev)
+  virDomainDeviceInfoPtr dev,
+  virDomainPCIConnectFlags flags)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 
 int virDomainPCIAddressReleaseAddr(virDomainPCIAddressSetPtr addrs,
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index d731b19..aa54d6b 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -2126,6 +2126,33 @@ qemuDomainAssignAddresses(virDomainDefPtr def,
 return 0;
 }
 
+/**
+ * qemuDomainEnsurePCIAddress:
+ *
+ * if @dev should have a PCI address but doesn't, assign an address on
+ * a compatible PCI bus, and set it in @dev->...info. If there is an
+ * address already, validate that it is on a compatible bus, based on
+ * @dev->...info.pciConnectFlags.
+ *
+ * @obj: the virDomainObjPtr for the domain. This will include
+ *   qemuCaps and address cache (if there is one)
+ *
+ * @dev: the device that we need to ensure has a PCI address
+ *
+ * returns 0 on success -1 on failure.
+ */
+int
+qemuDomainEnsurePCIAddress(virDomainObjPtr obj,
+   virDomainDeviceDefPtr dev)
+{
+qemuDomainObjPrivatePtr priv = obj->privateData;
+virDomainDeviceInfoPtr info = virDomainDeviceGetInfo(dev);
+
+qemuDomainFillDevicePCIConnectFlags(obj->def, dev, priv->qemuCaps);
+
+return virDomainPCIAddressEnsureAddr(priv->pciaddrs, info,
+ info->pciConnectFlags);
+}
 
 void
 qemuDomainReleaseDeviceAddress(virDomainObjPtr vm,
diff --git a/src/qemu/qemu_domain_address.h b/src/qemu/qemu_domain_address.h
index 11d6e92..800859c 100644
--- a/src/qemu/qemu_domain_address.h
+++ b/src/qemu/qemu_domain_address.h
@@ -37,6 +37,10 @@ int qemuDomainAssignAddresses(virDomainDefPtr def,
   bool newDomain)
 

[libvirt] [PATCH v5 13/15] [ACKED] qemu: add a USB3 controller to Q35 domains by default

[Laine Stump]

Previously we added a set of EHCI+UHCI controllers to Q35 machines to
mimic real hardware as closely as possible, but recent discussions
have pointed out that the nec-usb-xhci (USB3) controller is much more
virtualization-friendly (uses less CPU), so this patch switches the
default for Q35 machinetypes to add an XHCI instead (if it's
supported, which it of course *will* be).

Since none of the existing test cases left out USB controllers in the
input XML, a new Q35 test case was added which has *no* devices, so
ends up with only the defaults always put in by qemu, plus those added
by libvirt.
---
 src/qemu/qemu_domain.c | 10 --
 .../qemuxml2argv-q35-default-devices-only.args | 22 
 .../qemuxml2argv-q35-default-devices-only.xml  | 18 ++
 tests/qemuxml2argvtest.c   | 23 +
 .../qemuxml2xmlout-q35-default-devices-only.xml| 40 ++
 tests/qemuxml2xmltest.c| 23 +
 6 files changed, 133 insertions(+), 3 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-q35-default-devices-only.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-q35-default-devices-only.xml
 create mode 100644 
tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-default-devices-only.xml

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 5327add..cddb91f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2017,10 +2017,14 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
 addPCIeRoot = true;
 addImplicitSATA = true;
 
-/* add a USB2 controller set, but only if the
- * ich9-usb-ehci1 device is supported
+/* Prefer adding USB3 controller if supported
+ * (nec-usb-xhci). Failing that, add a USB2 controller set
+ * if the ich9-usb-ehci1 device is supported. Otherwise
+ * don't add anything.
  */
-if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_USB_EHCI1))
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
+usbModel = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
+else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_USB_EHCI1))
 usbModel = VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1;
 else
 addDefaultUSB = false;
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-default-devices-only.args 
b/tests/qemuxml2argvdata/qemuxml2argv-q35-default-devices-only.args
new file mode 100644
index 000..9ac30dd
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-default-devices-only.args
@@ -0,0 +1,22 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/libexec/qemu-kvm \
+-name q35-test \
+-S \
+-M q35 \
+-m 2048 \
+-smp 2,sockets=2,cores=1,threads=1 \
+-uuid 11dbdcdd-4c3b-482b-8903-9bdb8c0a2774 \
+-nographic \
+-nodefaults \
+-monitor unix:/tmp/lib/domain--1-q35-test/monitor.sock,server,nowait \
+-no-acpi \
+-boot c \
+-device ioh3420,port=0x8,chassis=1,id=pci.1,bus=pcie.0,addr=0x1 \
+-device ioh3420,port=0x10,chassis=2,id=pci.2,bus=pcie.0,addr=0x2 \
+-device nec-usb-xhci,id=usb,bus=pci.1,addr=0x0 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x0
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-default-devices-only.xml 
b/tests/qemuxml2argvdata/qemuxml2argv-q35-default-devices-only.xml
new file mode 100644
index 000..7436583
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-default-devices-only.xml
@@ -0,0 +1,18 @@
+
+  q35-test
+  11dbdcdd-4c3b-482b-8903-9bdb8c0a2774
+  2097152
+  2097152
+  2
+  
+hvm
+
+  
+  
+  destroy
+  restart
+  destroy
+  
+/usr/libexec/qemu-kvm
+  
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 6d9d101..3e6537f 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1822,6 +1822,29 @@ mymain(void)
 QEMU_CAPS_ICH9_USB_EHCI1,
 QEMU_CAPS_NEC_USB_XHCI,
 QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
+DO_TEST("q35-default-devices-only",
+QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY,
+QEMU_CAPS_DEVICE_VIRTIO_RNG,
+QEMU_CAPS_OBJECT_RNG_RANDOM,
+QEMU_CAPS_NETDEV,
+QEMU_CAPS_DEVICE_VIRTIO_NET,
+QEMU_CAPS_DEVICE_VIRTIO_GPU,
+QEMU_CAPS_VIRTIO_GPU_VIRGL,
+QEMU_CAPS_VIRTIO_KEYBOARD,
+QEMU_CAPS_VIRTIO_MOUSE,
+QEMU_CAPS_VIRTIO_TABLET,
+QEMU_CAPS_VIRTIO_INPUT_HOST,
+QEMU_CAPS_VIRTIO_SCSI,
+QEMU_CAPS_FSDEV,
+QEMU_CAPS_FSDEV_WRITEOUT,
+QEMU_CAPS_DEVICE_PCI_BRIDGE,
+QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
+QEMU_CAPS_DEVICE_IOH3420,
+QEMU_CAPS_ICH9_AHCI,
+QEMU_CAPS_PCI_MULTIFUNCTION,
+QEMU_CAPS_ICH9_USB_EHCI1,
+QEMU_CAPS_NEC_USB_XHCI,
+   

[libvirt] [PATCH v5 07/15] qemu: assign e1000e network devices to PCIe slots when appropriate

[Laine Stump]

The e1000e is an emulated network device based on the Intel 82574,
present in qemu 2.7.0 and later. Among other differences from the
e1000, it presents itself as a PCIe device rather than legacy PCI. In
order to get it assigned to a PCIe controller, this patch updates the
flags setting for network devices when the model name is "e1000e".

(Note that for some reason libvirt has never validated the network
device model names other than to check that there are no dangerous
characters in them. That should probably change, but is the subject of
another patch.)

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1343094
---
 src/qemu/qemu_domain_address.c |  6 --
 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.args  | 23 --
 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.xml   |  4 
 .../qemuxml2argv-q35-virtio-pci.args   |  3 +++
 .../qemuxml2xmloutdata/qemuxml2xmlout-q35-pcie.xml | 23 +-
 .../qemuxml2xmlout-q35-virtio-pci.xml  |  5 +
 6 files changed, 43 insertions(+), 21 deletions(-)

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index b90e4d1..9d00825 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -426,8 +426,7 @@ qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr def,
  */
 static virDomainPCIConnectFlags
 qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
- virDomainPCIConnectFlags pcieFlags
- ATTRIBUTE_UNUSED,
+ virDomainPCIConnectFlags pcieFlags,
  virDomainPCIConnectFlags virtioFlags)
 {
 virDomainPCIConnectFlags pciFlags =  (VIR_PCI_CONNECT_TYPE_PCI_DEVICE |
@@ -518,6 +517,9 @@ 
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
 if (STREQ(net->model, "virtio"))
 return  virtioFlags;
 
+if (STREQ(net->model, "e1000e"))
+return pcieFlags;
+
 return pciFlags;
 }
 
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.args 
b/tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.args
index f07c085..d92e0b8 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.args
@@ -35,10 +35,10 @@ QEMU_AUDIO_DRV=none \
 addr=0x1d \
 -device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pcie.0,addr=0x1d.0x1 \
 -device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pcie.0,addr=0x1d.0x2 \
--device virtio-scsi-pci,id=scsi0,bus=pci.6,addr=0x0 \
--device virtio-serial-pci,id=virtio-serial0,bus=pci.5,addr=0x0 \
+-device virtio-scsi-pci,id=scsi0,bus=pci.7,addr=0x0 \
+-device virtio-serial-pci,id=virtio-serial0,bus=pci.6,addr=0x0 \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk1 \
--device virtio-blk-pci,bus=pci.7,addr=0x0,drive=drive-virtio-disk1,\
+-device virtio-blk-pci,bus=pci.8,addr=0x0,drive=drive-virtio-disk1,\
 id=virtio-disk1 \
 -fsdev local,security_model=passthrough,id=fsdev-fs0,path=/export/to/guest \
 -device virtio-9p-pci,id=fs0,fsdev=fsdev-fs0,mount_tag=/import/from/host,\
@@ -46,13 +46,16 @@ bus=pci.3,addr=0x0 \
 -netdev user,id=hostnet0 \
 -device 
virtio-net-pci,netdev=hostnet0,id=net0,mac=00:11:22:33:44:55,bus=pci.4,\
 addr=0x0 \
--device virtio-input-host-pci,id=input0,evdev=/dev/input/event1234,bus=pci.10,\
+-netdev user,id=hostnet1 \
+-device e1000e,netdev=hostnet1,id=net1,mac=00:11:22:33:44:66,bus=pci.5,\
 addr=0x0 \
--device virtio-mouse-pci,id=input1,bus=pci.11,addr=0x0 \
--device virtio-keyboard-pci,id=input2,bus=pci.12,addr=0x0 \
--device virtio-tablet-pci,id=input3,bus=pci.13,addr=0x0 \
+-device virtio-input-host-pci,id=input0,evdev=/dev/input/event1234,bus=pci.11,\
+addr=0x0 \
+-device virtio-mouse-pci,id=input1,bus=pci.12,addr=0x0 \
+-device virtio-keyboard-pci,id=input2,bus=pci.13,addr=0x0 \
+-device virtio-tablet-pci,id=input3,bus=pci.14,addr=0x0 \
 -device virtio-gpu-pci,id=video0,bus=pcie.0,addr=0x1 \
--device virtio-balloon-pci,id=balloon0,bus=pci.8,addr=0x0 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.9,addr=0x0 \
 -object rng-random,id=objrng0,filename=/dev/urandom \
--device 
virtio-rng-pci,rng=objrng0,id=rng0,max-bytes=123,period=1234,bus=pci.9,\
-addr=0x0
+-device virtio-rng-pci,rng=objrng0,id=rng0,max-bytes=123,period=1234,\
+bus=pci.10,addr=0x0
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.xml 
b/tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.xml
index be2439e..39db5f0 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.xml
@@ -46,6 +46,10 @@
   
   
 
+
+  
+  
+
 
 
   
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-virtio-pci.args 
b/tests/qemuxml2argvdata/qemuxml2argv-q35-virtio-pci.args
index 98284a2..68d19ca 100644
--- 

[libvirt] [PATCH v5 14/15] [ACKED] qemu: try to put ich9 sound device at 00:1B.0

[Laine Stump]

Real Q35 hardware has an ICH9 chip that includes several integrated
devices at particular addresses (see the file docs/q35-chipset.cfg in
the qemu source). libvirt already attempts to put the first two sets
of ich9 USB2 controllers it finds at 00:1D.* and 00:1A.* to match the
real hardware. This patch does the same for the ich9 "HD audio"
device.

The main inspiration for this patch is that currently the *only*
device in a reasonable "workstation" type virtual machine config that
requires a legacy PCI slot is the audio device, Without this patch,
the standard Q35 machine created by virt-manager will have a
dmi-to-pci-bridge and a pci-bridge just for the sound device; with the
patch (and if you change the sound device model from the default
"ich6" to "ich9"), the machine definition constructed by virt-manager
has absolutely no legacy PCI controllers - any legacy PCI devices
(e.g. video and sound) are on pcie-root as integrated devices.
---
 src/qemu/qemu_domain_address.c |  26 +
 .../qemuxml2argv-q35-virt-manager-basic.args   |  55 ++
 .../qemuxml2argv-q35-virt-manager-basic.xml|  76 ++
 tests/qemuxml2argvtest.c   |  34 ++
 .../qemuxml2xmlout-q35-virt-manager-basic.xml  | 116 +
 tests/qemuxml2xmltest.c|  24 +
 6 files changed, 331 insertions(+)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-q35-virt-manager-basic.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-q35-virt-manager-basic.xml
 create mode 100644 
tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-virt-manager-basic.xml

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index e1e1f77..773e707 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -1332,6 +1332,32 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def,
 goto cleanup;
 }
 }
+
+memset(_addr, 0, sizeof(tmp_addr));
+tmp_addr.slot = 0x1B;
+if (!virDomainPCIAddressSlotInUse(addrs, _addr)) {
+/* Since real Q35 hardware has an ICH9 chip that has an
+ * integrated HD audio device at :00:1B.0 put any
+ * unaddressed ICH9 audio device at that address if it's not
+ * already taken. If there's something already there, let the
+ * normal device addressing assign something later.
+ */
+for (i = 0; i < def->nsounds; i++) {
+virDomainSoundDefPtr sound = def->sounds[i];
+
+if (sound->model != VIR_DOMAIN_SOUND_MODEL_ICH9 ||
+!virDeviceInfoPCIAddressWanted(>info)) {
+continue;
+}
+if (virDomainPCIAddressReserveSlot(addrs, _addr, flags) < 0)
+goto cleanup;
+
+sound->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
+sound->info.addr.pci = tmp_addr;
+break;
+}
+}
+
 ret = 0;
  cleanup:
 VIR_FREE(addrStr);
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-virt-manager-basic.args 
b/tests/qemuxml2argvdata/qemuxml2argv-q35-virt-manager-basic.args
new file mode 100644
index 000..d3217d5
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-virt-manager-basic.args
@@ -0,0 +1,55 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=spice \
+/usr/bin/qemu-system_x86_64 \
+-name virt-manager-basic \
+-S \
+-M pc-q35-2.7 \
+-m 4096 \
+-smp 2,sockets=2,cores=1,threads=1 \
+-uuid 1b826c23-8767-47ad-a6b5-c83a88277f71 \
+-nodefaults \
+-monitor unix:/tmp/lib/domain--1-virt-manager-basic/monitor.sock,server,nowait 
\
+-rtc base=utc,driftfix=slew \
+-no-kvm-pit-reinjection \
+-global ICH9-LPC.disable_s3=1 \
+-global ICH9-LPC.disable_s4=1 \
+-boot c \
+-device ioh3420,port=0x10,chassis=1,id=pci.1,bus=pcie.0,addr=0x2 \
+-device ioh3420,port=0x18,chassis=2,id=pci.2,bus=pcie.0,addr=0x3 \
+-device ioh3420,port=0x20,chassis=3,id=pci.3,bus=pcie.0,addr=0x4 \
+-device ioh3420,port=0x28,chassis=4,id=pci.4,bus=pcie.0,addr=0x5 \
+-device ioh3420,port=0x30,chassis=5,id=pci.5,bus=pcie.0,addr=0x6 \
+-device nec-usb-xhci,id=usb,bus=pci.2,addr=0x0 \
+-device virtio-serial-pci,id=virtio-serial0,bus=pci.3,addr=0x0 \
+-drive file=/var/lib/libvirt/images/basic.qcow2,format=qcow2,if=none,\
+id=drive-virtio-disk0 \
+-device virtio-blk-pci,bus=pci.4,addr=0x0,drive=drive-virtio-disk0,\
+id=virtio-disk0 \
+-netdev user,id=hostnet0 \
+-device 
virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:9a:e6:c6,bus=pci.1,\
+addr=0x0 \
+-serial pty \
+-chardev socket,id=charchannel0,\
+path=/tmp/channel/domain--1-virt-manager-basic/org.qemu.guest_agent.0,server,\
+nowait \
+-device virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,\
+id=channel0,name=org.qemu.guest_agent.0 \
+-chardev spicevmc,id=charchannel1,name=vdagent \
+-device virtserialport,bus=virtio-serial0.0,nr=2,chardev=charchannel1,\
+id=channel1,name=com.redhat.spice.0 \

[libvirt] [PATCH v5 01/15] qemu: new functions qemuDomainMachineHasPCI[e]Root()

[Laine Stump]

These functions provide a simple one line method of learning if the
current domain has a pci-root or pcie-root bus.
---
 src/qemu/qemu_domain.c | 30 ++
 src/qemu/qemu_domain.h |  2 ++
 2 files changed, 32 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 41ac52d..310bfa6 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5393,6 +5393,36 @@ qemuDomainMachineIsI440FX(const virDomainDef *def)
 
 
 bool
+qemuDomainMachineHasPCIRoot(const virDomainDef *def)
+{
+int root = virDomainControllerFind(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0);
+
+if (root < 0)
+return false;
+
+if (def->controllers[root]->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)
+return false;
+
+return true;
+}
+
+
+bool
+qemuDomainMachineHasPCIeRoot(const virDomainDef *def)
+{
+int root = virDomainControllerFind(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0);
+
+if (root < 0)
+return false;
+
+if (def->controllers[root]->model != VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
+return false;
+
+return true;
+}
+
+
+bool
 qemuDomainMachineNeedsFDC(const virDomainDef *def)
 {
 char *p = STRSKIP(def->os.machine, "pc-q35-");
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 4f9bf82..f896756 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -654,6 +654,8 @@ virDomainChrDefPtr qemuFindAgentConfig(virDomainDefPtr def);
 
 bool qemuDomainMachineIsQ35(const virDomainDef *def);
 bool qemuDomainMachineIsI440FX(const virDomainDef *def);
+bool qemuDomainMachineHasPCIRoot(const virDomainDef *def);
+bool qemuDomainMachineHasPCIeRoot(const virDomainDef *def);
 bool qemuDomainMachineNeedsFDC(const virDomainDef *def);
 bool qemuDomainMachineIsS390CCW(const virDomainDef *def);
 bool qemuDomainMachineIsVirt(const virDomainDef *def);
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v5 10/15] qemu: auto-add pcie-root-port/dmi-to-pci-bridge controllers as needed

[Laine Stump]

Previously libvirt would only add pci-bridge devices automatically
when an address was requested for a device that required a legacy PCI
slot and none was available. This patch expands that support to
dmi-to-pci-bridge (which is needed in order to add a pci-bridge on a
machine with a pcie-root), and pcie-root-port (which is needed to add
a hotpluggable PCIe device). It does *not* automatically add
pcie-switch-upstream-ports or pcie-switch-downstream-ports (and
currently there are no plans for that).

Given the existing code to auto-add pci-bridge devices, automatically
adding pcie-root-ports is fairly straightforward. The
dmi-to-pci-bridge support is a bit tricky though, for a few reasons:

1) Although the only reason to add a dmi-to-pci-bridge is so that
   there is a reasonable place to plug in a pci-bridge controller,
   most of the time it's not the presence of a pci-bridge *in the
   config* that triggers the requirement to add a dmi-to-pci-bridge.
   Rather, it is the presence of a legacy-PCI device in the config,
   which triggers auto-add of a pci-bridge, which triggers auto-add of
   a dmi-to-pci-bridge (this is handled in
   virDomainPCIAddressSetGrow() - if there's a request to add a
   pci-bridge we'll check if there is a suitable bus to plug it into;
   if not, we first add a dmi-to-pci-bridge).

2) Once there is already a single dmi-to-pci-bridge on the system,
   there won't be a need for any more, even if it's full, as long as
   there is a pci-bridge with an open slot - you can also plug
   pci-bridges into existing pci-bridges. So we have to make sure we
   don't add a dmi-to-pci-bridge unless there aren't any
   dmi-to-pci-bridges *or* any pci-bridges.

3) Although it is strongly discouraged, it is legal for a pci-bridge
   to be directly plugged into pcie-root, and we don't want to
   auto-add a dmi-to-pci-bridge if there is already a pci-bridge
   that's been forced directly into pcie-root.

Although libvirt will now automatically create a dmi-to-pci-bridge
when it's needed, the code still remains for now that forces a
dmi-to-pci-bridge on all domains with pcie-root (in
qemuDomainDefAddDefaultDevices()). That will be removed in the next
patch.

For now, the pcie-root-ports are added one to a slot, which is a bit
wasteful and means it will fail after 31 total PCIe devices (30 if
there are also some PCI devices), but helps keep the changeset down
for this patch. A future patch will have 8 pcie-root-ports sharing the
functions on a single slot.
---
 src/conf/domain_addr.c |  96 +++---
 src/qemu/qemu_domain_address.c |  55 +---
 .../qemuxml2argv-q35-pcie-autoadd.args |  57 
 .../qemuxml2argv-q35-pcie-autoadd.xml  |  51 +++
 tests/qemuxml2argvtest.c   |  24 
 .../qemuxml2xmlout-q35-pcie-autoadd.xml| 147 +
 tests/qemuxml2xmltest.c|  25 +++-
 7 files changed, 421 insertions(+), 34 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie-autoadd.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie-autoadd.xml
 create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-pcie-autoadd.xml

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 48e9872..718fde7 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -82,6 +82,30 @@ 
virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI model)
 return 0;
 }
 
+
+static int
+virDomainPCIControllerConnectTypeToModel(virDomainPCIConnectFlags flags)
+{
+if (flags & VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT)
+return VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT;
+if (flags & VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT)
+return VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT;
+if (flags & VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_DOWNSTREAM_PORT)
+return VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT;
+if (flags & VIR_PCI_CONNECT_TYPE_DMI_TO_PCI_BRIDGE)
+return VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE;
+if (flags & VIR_PCI_CONNECT_TYPE_PCI_EXPANDER_BUS)
+return VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS;
+if (flags & VIR_PCI_CONNECT_TYPE_PCIE_EXPANDER_BUS)
+return VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS;
+if (flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE)
+return VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE;
+
+/* some connect types don't correspond to a controller model */
+return -1;
+}
+
+
 bool
 virDomainPCIAddressFlagsCompatible(virPCIDeviceAddressPtr addr,
const char *addrStr,
@@ -334,10 +358,7 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr 
bus,
 }
 
 
-/* Ensure addr fits in the address set, by expanding it if needed.
- * This will only grow if the flags say that we need a normal
- * hot-pluggable PCI slot. If we need a different type of slot, 

[libvirt] [PATCH v5 05/15] qemu: set pciConnectFlags to 0 instead of PCI|HOTPLUGGABLE if device isn't PCI

[Laine Stump]

This patch cleans up the connect flags for certain types/models of
devices that aren't PCI to return 0. In the future that may be used as
an indicator to the caller about whether or not a device needs a PCI
address. For now it's just ignored, except for in
virDomainPCIAddressEnsureAddr() - called during device hotplug - (and
in some cases actually needs to be re-set to PCI|HOTPLUGGABLE just in
case someone (in some old config) has manually set a PCI address for a
device that isn't PCI.
---
 src/conf/domain_addr.c |  6 +
 src/qemu/qemu_domain_address.c | 54 +-
 2 files changed, 38 insertions(+), 22 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 2195a95..48e9872 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -493,6 +493,12 @@ virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr 
addrs,
 int ret = -1;
 char *addrStr = NULL;
 
+/* if flags is 0, the particular model of this device on this
+ * machinetype doesn't need a PCI address, so we're done.
+ */
+if (!flags)
+   return 0;
+
 if (!(addrStr = virDomainPCIAddressAsString(>addr.pci)))
 goto cleanup;
 
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index aa54d6b..ccfea4d 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -465,8 +465,7 @@ 
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
 case VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB2: /* xen only */
 case VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE:
 case VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST:
-/* should be 0 */
-return pciFlags;
+return 0;
 }
 
 case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
@@ -495,8 +494,7 @@ 
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
  */
 if (net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV ||
 STREQ(net->model, "usb-net")) {
-/* should be 0 */
-return pciFlags;
+return 0;
 }
 return pciFlags;
 }
@@ -513,8 +511,7 @@ 
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
 case VIR_DOMAIN_SOUND_MODEL_PCSPK:
 case VIR_DOMAIN_SOUND_MODEL_USB:
 case VIR_DOMAIN_SOUND_MODEL_LAST:
-/* should be 0 */
-return pciFlags;
+return 0;
 }
 
 case VIR_DOMAIN_DEVICE_DISK:
@@ -531,8 +528,7 @@ 
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
 case VIR_DOMAIN_DISK_BUS_SATA:
 case VIR_DOMAIN_DISK_BUS_SD:
 case VIR_DOMAIN_DISK_BUS_LAST:
-/* should be 0 */
-return pciFlags;
+return 0;
 }
 
 case VIR_DOMAIN_DEVICE_HOSTDEV:
@@ -546,8 +542,7 @@ 
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
 case VIR_DOMAIN_MEMBALLOON_MODEL_XEN:
 case VIR_DOMAIN_MEMBALLOON_MODEL_NONE:
 case VIR_DOMAIN_MEMBALLOON_MODEL_LAST:
-/* should be 0 (not PCI) */
-return pciFlags;
+return 0;
 }
 
 case VIR_DOMAIN_DEVICE_RNG:
@@ -556,8 +551,7 @@ 
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
 return pciFlags;
 
 case VIR_DOMAIN_RNG_MODEL_LAST:
-/* should be 0 */
-return pciFlags;
+return 0;
 }
 
 case VIR_DOMAIN_DEVICE_WATCHDOG:
@@ -569,8 +563,7 @@ 
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
 case VIR_DOMAIN_WATCHDOG_MODEL_IB700:
 case VIR_DOMAIN_WATCHDOG_MODEL_DIAG288:
 case VIR_DOMAIN_WATCHDOG_MODEL_LAST:
-/* should be 0 */
-return pciFlags;
+return 0;
 }
 
 case VIR_DOMAIN_DEVICE_VIDEO:
@@ -586,8 +579,7 @@ 
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
 return pciFlags;
 
 case VIR_DOMAIN_VIDEO_TYPE_LAST:
-/* should be 0 */
-return pciFlags;
+return 0;
 }
 
 
@@ -604,8 +596,7 @@ 
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
 case VIR_DOMAIN_INPUT_BUS_XEN:
 case VIR_DOMAIN_INPUT_BUS_PARALLELS:
 case VIR_DOMAIN_INPUT_BUS_LAST:
-/* should be 0 */
-return pciFlags;
+return 0;
 }
 
 case VIR_DOMAIN_DEVICE_CHR:
@@ -616,8 +607,7 @@ 
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
 case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA:
 case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB:
 case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST:
-/* should be 0 */
-return pciFlags;
+return 0;
 }
 
 /* These devices don't ever connect with PCI */
@@ -634,8 +624,7 @@ 
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
 case 

[libvirt] [PATCH v5 00/15] Use more PCIe less legacy PCI

[Laine Stump]

These are just the rebased versions of the patches from v4 that I
haven't yet pushed, to make review easier. Patch 1 and 2 (and several
others) are already ACKed, but included for completeness.

Laine Stump (15):
  qemu: new functions qemuDomainMachineHasPCI[e]Root()
  qemu: new functions to calculate/set device pciConnectFlags
  qemu: set/use info->pciConnectFlags when validating/assigning PCI
addresses
  qemu: set/use proper pciConnectFlags during hotplug
  qemu: set pciConnectFlags to 0 instead of PCI|HOTPLUGGABLE if device
isn't PCI
  qemu: assign virtio devices to PCIe slot when appropriate
  qemu: assign e1000e network devices to PCIe slots when appropriate
  qemu: assign nec-xhci (USB3) controller to a PCIe address when
appropriate
  qemu: only force an available legacy-PCI slot on domains with pci-root
  qemu: auto-add pcie-root-port/dmi-to-pci-bridge controllers as needed
  [RFC] qemu: if pci-bridge is in PCIe config w/o dmi-to-pci-bridge, add
it
  qemu: don't force-add a dmi-to-pci-bridge just on principle
  qemu: add a USB3 controller to Q35 domains by default
  qemu: try to put ich9 sound device at 00:1B.0
  qemu: initially reserve one open pcie-root-port for hotplug

 src/conf/device_conf.h |   5 +
 src/conf/domain_addr.c | 112 ++-
 src/conf/domain_addr.h |   3 +-
 src/qemu/qemu_domain.c |  52 +-
 src/qemu/qemu_domain.h |   2 +
 src/qemu/qemu_domain_address.c | 805 +
 src/qemu/qemu_domain_address.h |   4 +
 src/qemu/qemu_hotplug.c|  20 +-
 tests/qemuxml2argvdata/qemuxml2argv-autoindex.args |  10 +-
 .../qemuxml2argv-bios-nvram-secure.args|   1 +
 .../qemuxml2argv-machine-smm-opt.args  |   1 +
 tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args |   3 +-
 .../qemuxml2argv-q35-default-devices-only.args |  23 +
 .../qemuxml2argv-q35-default-devices-only.xml  |  18 +
 .../qemuxml2argv-q35-pcie-autoadd.args |  57 ++
 .../qemuxml2argv-q35-pcie-autoadd.xml  |  51 ++
 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.args  |  58 ++
 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.xml   |  67 ++
 .../qemuxml2argv-q35-pm-disable-fallback.args  |   1 +
 .../qemuxml2argv-q35-pm-disable.args   |   1 +
 .../qemuxml2argv-q35-virt-manager-basic.args   |  56 ++
 .../qemuxml2argv-q35-virt-manager-basic.xml|  76 ++
 .../qemuxml2argv-q35-virtio-pci.args   |  58 ++
 .../qemuxml2argv-q35-virtio-pci.xml|   1 +
 tests/qemuxml2argvtest.c   | 164 -
 .../qemuxml2xmlout-autoindex.xml   |  10 +-
 .../qemuxml2xmlout-pcie-root.xml   |   4 -
 .../qemuxml2xmlout-q35-default-devices-only.xml|  45 ++
 .../qemuxml2xmlout-q35-pcie-autoadd.xml| 148 
 .../qemuxml2xmloutdata/qemuxml2xmlout-q35-pcie.xml | 152 
 .../qemuxml2xmlout-q35-virt-manager-basic.xml  | 121 
 .../qemuxml2xmlout-q35-virtio-pci.xml  | 152 
 tests/qemuxml2xmltest.c| 136 +++-
 33 files changed, 2189 insertions(+), 228 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-q35-default-devices-only.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-q35-default-devices-only.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie-autoadd.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie-autoadd.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.xml
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-q35-virt-manager-basic.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-q35-virt-manager-basic.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-virtio-pci.args
 create mode 12 tests/qemuxml2argvdata/qemuxml2argv-q35-virtio-pci.xml
 create mode 100644 
tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-default-devices-only.xml
 create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-pcie-autoadd.xml
 create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-pcie.xml
 create mode 100644 
tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-virt-manager-basic.xml
 create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-virtio-pci.xml

-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v5 06/15] qemu: assign virtio devices to PCIe slot when appropriate

[Laine Stump]

libvirt previously assigned nearly all devices to a "hotpluggable"
legacy PCI slot even on machines with a PCIe root bus (and even though
most such machines don't even support hotplug on legacy PCI slots!)
Forcing all devices onto legacy PCI slots means that the domain will
need a dmi-to-pci-bridge (to convert from PCIe to legacy PCI) and a
pci-bridge (to provide hotpluggable legacy PCI slots which, again,
usually aren't hotpluggable anyway).

To help reduce the need for these legacy controllers, this patch tries
to assign virtio-1.0-capable devices to PCIe slots whenever possible,
by setting appropriate connectFlags in
virDomainCalculateDevicePCIConnectFlags(). Happily, when that function
was written (just a few commits ago) it was created with a
"virtioFlags" argument, set by both of its callers, which is the
proper connectFlags to set for any virtio-*-pci device - depending on
the arch/machinetype of the domain, and whether or not the qemu binary
supports virtio-1.0, that flag will have either been set to PCI or
PCIE. This patch merely enables the functionality by setting the flags
for the device to whatever is in virtioFlags if the device is a
virtio-*-pci device.

NB: the first virtio video device will be placed directly on bus 0
slot 1 rather than on a pcie-root-port due to the override for primary
video devices in qemuDomainValidateDevicePCISlotsQ35(). Whether or not
to change that is a topic of discussion, but this patch doesn't change
that particular behavior.

NB2: since the slot must be hotpluggable, and pcie-root (the PCIe root
complex) does *not* support hotplug, this means that suitable
controllers must also be in the config (i.e. either pcie-root-port, or
pcie-downstream-port). For now, libvirt doesn't add those
automatically, so if you put virtio devices in a config for a qemu
that has PCIe-capable virtio devices, you'll need to add extra
pcie-root-ports yourself. That requirement will be eliminated in a
future patch, but for now, it's simple to do this:

   
   
   
   ...

Partially Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1330024
---
 src/qemu/qemu_domain_address.c |  41 --
 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.args  |  58 
 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.xml   |  61 
 .../qemuxml2argv-q35-virtio-pci.args   |  58 
 .../qemuxml2argv-q35-virtio-pci.xml|   1 +
 tests/qemuxml2argvtest.c   |  48 +++
 .../qemuxml2xmloutdata/qemuxml2xmlout-q35-pcie.xml | 154 +
 .../qemuxml2xmlout-q35-virtio-pci.xml  | 154 +
 tests/qemuxml2xmltest.c|  43 ++
 9 files changed, 609 insertions(+), 9 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-virtio-pci.args
 create mode 12 tests/qemuxml2argvdata/qemuxml2argv-q35-virtio-pci.xml
 create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-pcie.xml
 create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-virtio-pci.xml

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index ccfea4d..b90e4d1 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -428,8 +428,7 @@ static virDomainPCIConnectFlags
 qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
  virDomainPCIConnectFlags pcieFlags
  ATTRIBUTE_UNUSED,
- virDomainPCIConnectFlags virtioFlags
- ATTRIBUTE_UNUSED)
+ virDomainPCIConnectFlags virtioFlags)
 {
 virDomainPCIConnectFlags pciFlags =  (VIR_PCI_CONNECT_TYPE_PCI_DEVICE |
   VIR_PCI_CONNECT_HOTPLUGGABLE);
@@ -469,9 +468,28 @@ 
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
 }
 
 case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
+return pciFlags;
+
 case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
+switch ((virDomainControllerModelSCSI)cont->model) {
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
+return virtioFlags;
+
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO:
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC:
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068:
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI:
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI:
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078:
+return pciFlags;
+
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST:
+return 0;
+}
+

[libvirt] [PATCH v5 02/15] qemu: new functions to calculate/set device pciConnectFlags

[Laine Stump]

The lowest level function of this trio
(qemuDomainDeviceCalculatePCIConnectFlags()) aims to be the single
authority for the virDomainPCIConnectFlags to use for any given device
using a particular arch/machinetype/qemu-binary.

qemuDomainFillDevicePCIConnectFlags() sets info->pciConnectFlags in a
single device (unless it has no virDomainDeviceInfo, in which case
it's a NOP).

qemuDomainFillAllPCIConnectFlags() sets info->pciConnectFlags in all
devices that have a virDomainDeviceInfo

The latter two functions aren't called anywhere yet. This commit is
just making them available. Later patches will replace all the current
hodge-podge of flag settings with calls to this single authority.
---
 src/conf/device_conf.h |   5 +
 src/qemu/qemu_domain_address.c | 367 +
 2 files changed, 372 insertions(+)

diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index 8443de6..f435fb5 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -142,6 +142,11 @@ typedef struct _virDomainDeviceInfo {
 /* bootIndex is only used for disk, network interface, hostdev
  * and redirdev devices */
 unsigned int bootIndex;
+
+/* pciConnectFlags is only used internally during address
+ * assignment, never saved and never reported.
+ */
+int pciConnectFlags; /* enum virDomainPCIConnectFlags */
 } virDomainDeviceInfo, *virDomainDeviceInfoPtr;
 
 
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index e206b9f..602179c 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -407,6 +407,373 @@ qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr 
def,
 }
 
 
+/**
+ * qemuDomainDeviceCalculatePCIConnectFlags:
+ *
+ * @dev: The device to be checked
+ * @pcieFlags: flags to use for a known PCI Express device
+ * @virtioFlags: flags to use for a virtio device (properly vetted
+ *   for the current qemu binary and arch/machinetype)
+ *
+ * Lowest level function to determine PCI connectFlags for a
+ * device. This function relies on the next higher-level function
+ * determining the value for pcieFlags and virtioFlags in advance -
+ * this is to make it more efficient to call multiple times.
+ *
+ * Returns appropriate virDomainPCIConnectFlags for this device in
+ * this domain, or 0 if the device doesn't connect using PCI. There
+ * is no failure.
+ */
+static virDomainPCIConnectFlags
+qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
+ virDomainPCIConnectFlags pcieFlags
+ ATTRIBUTE_UNUSED,
+ virDomainPCIConnectFlags virtioFlags
+ ATTRIBUTE_UNUSED)
+{
+virDomainPCIConnectFlags pciFlags =  (VIR_PCI_CONNECT_TYPE_PCI_DEVICE |
+  VIR_PCI_CONNECT_HOTPLUGGABLE);
+
+switch ((virDomainDeviceType)dev->type) {
+case VIR_DOMAIN_DEVICE_CONTROLLER: {
+virDomainControllerDefPtr cont = dev->data.controller;
+
+switch ((virDomainControllerType)cont->type) {
+case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
+return virDomainPCIControllerModelToConnectType(cont->model);
+
+case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
+return pciFlags;
+
+case VIR_DOMAIN_CONTROLLER_TYPE_USB:
+switch ((virDomainControllerModelUSB)cont->model) {
+case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI:
+return pciFlags;
+
+case VIR_DOMAIN_CONTROLLER_MODEL_USB_EHCI:
+case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1:
+case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1:
+case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2:
+case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3:
+case VIR_DOMAIN_CONTROLLER_MODEL_USB_VT82C686B_UHCI:
+case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI:
+case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX4_UHCI:
+case VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI:
+return pciFlags;
+
+case VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB1: /* xen only */
+case VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB2: /* xen only */
+case VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE:
+case VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST:
+/* should be 0 */
+return pciFlags;
+}
+
+case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
+case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
+case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
+return pciFlags;
+
+case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
+case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
+case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
+/* should be 0 */
+return pciFlags;
+}
+}
+
+case VIR_DOMAIN_DEVICE_FS:
+/* the only type of filesystem so far is virtio-9p-pci */
+ 

Re: [libvirt] [PATCH v2 07/12] caps: Add new capability for the bps/iops throttling length

[Erik Skultety]

On Thu, Oct 06, 2016 at 06:38:55PM -0400, John Ferlan wrote:
> Add the capability to detect if the qemu binary can support the feature
> to use bps-max-length and friends.
> 
> Signed-off-by: John Ferlan 
> ---

this patch caused a rebase conflict but you'd come across that for sure.

ACK (when I read the cover letter it was already too late as at that time I'd
already sent an ACK to 6/12, so I'll do it here as well to stay consistent)

Erik


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 04/12] qemu: Introduce qemuDomainSetBlockIoTuneSetDefaults

[John Ferlan]

On 10/25/2016 09:05 AM, Erik Skultety wrote:
> On Thu, Oct 06, 2016 at 06:38:52PM -0400, John Ferlan wrote:
>> Create a helper to set the bytes/iops iotune default values based on
>> the current qemu setting
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/qemu/qemu_driver.c | 66 
>> ++
>>  1 file changed, 40 insertions(+), 26 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 78e917e..fcce3f0 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -17286,6 +17286,43 @@ qemuDomainOpenGraphicsFD(virDomainPtr dom,
>>  return ret;
>>  }
>>  
>> +
>> +/* If the user didn't specify bytes limits, inherit previous values;
>> + * likewise if the user didn't specify iops limits.  */
>> +static void
>> +qemuDomainSetBlockIoTuneSetDefaults(virDomainBlockIoTuneInfoPtr newinfo,
>> +virDomainBlockIoTuneInfoPtr oldinfo,
>> +bool set_bytes,
>> +bool set_iops,
>> +bool set_bytes_max,
>> +bool set_iops_max,
>> +bool set_size_iops)
>> +{
> 
> Could this be a macro instead? 5 booleans, it just seems a tiny bit odd.
> 

Probably - I can try...  Obvious impact later when _length is added.

>> +if (!set_bytes) {
>> +newinfo->total_bytes_sec = oldinfo->total_bytes_sec;
>> +newinfo->read_bytes_sec = oldinfo->read_bytes_sec;
>> +newinfo->write_bytes_sec = oldinfo->write_bytes_sec;
>> +}
> 
> And then you'll end up just with snippet like ^^this. Or you could use
> bitwise OR'd flags instead of the booleans, as it scales better (in terms of
> number of arguments), though I'm not very fond of that even though it works.
> 
> ACK with the adjustment, but see 5/12 as well before pushing.

There is no official bug report - it was one of those visual inspection
and oh sh*t moments during testing when I didn't release my domain
wasn't started and my max settings were zero'd out.

I can merge the two together as well - not a problem.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v11 3/5] qemu: Need to remove TLS object in RemoveRNGDevice

[John Ferlan]

On 10/25/2016 08:53 AM, Pavel Hrdina wrote:
> On Mon, Oct 24, 2016 at 06:46:19PM -0400, John Ferlan wrote:
>> Commit id '6e6b4bfc' added the object, but forgot the other end.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/qemu/qemu_hotplug.c | 12 +++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> ACK if you agree with review on the first patch that the order of 
> qemuMonitorDelObject and qemuMonitorDetachCharDev should be swapped.
> 
> Pavel
> 

I do agree, but I hadn't fully convinced myself - I needed the extra set
of eyes to help with that... Of course, I botched that again here by
removing the tlsobj before the chardev.   A problem which naturally
persists in patch 5, but is easily adjusted..

I looked at and thought about qemuDomainRemoveRNGDevice long enough and
compared to qemuDomainRemoveDiskDevice, but couldn't quite make up my
mind and really didn't want to be creating "extra" patches for perhaps
all the erroneous removals.

If my comparison is {Attach|Remove}Disk, which has...

Attach: Add secobj, Add encobj, Add Drive, and Add Device where drive
can rely on secobj and/or encobj.  The encobj and secobj do not rely on
each other. The secobj is the possible secret for the iSCSI/RBD server,
while encobj is the possible secret for

On attach error the removal is Del Drive, Del secobj, Del encobj (order
of secobj/encobj doesn't matter).

Detach: Del Device and call RemoveDisk which Del secobj, Del encobj, and
Del Drive.

Then, the RemoveDisk is probably wrong (Del Drive should be first), but
I didn't want to continue to bog down the chardev changes if my thoughts
in patch 2 were wrong.

Long way of saying, I want to get it right for this path and then if I'm
right generate a RemoveDisk patch to swap order.

John

>>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index e287aaf..f401b48 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -3608,6 +3608,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver,
>>  virObjectEventPtr event;
>>  char *charAlias = NULL;
>>  char *objAlias = NULL;
>> +char *tlsAlias = NULL;
>>  qemuDomainObjPrivatePtr priv = vm->privateData;
>>  ssize_t idx;
>>  int ret = -1;
>> @@ -3616,15 +3617,23 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver,
>>  VIR_DEBUG("Removing RNG device %s from domain %p %s",
>>rng->info.alias, vm, vm->def->name);
>>  
>> +
>>  if (virAsprintf(, "obj%s", rng->info.alias) < 0)
>>  goto cleanup;
>>  
>>  if (!(charAlias = qemuAliasChardevFromDevAlias(rng->info.alias)))
>>  goto cleanup;
>>  
>> +if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD &&
>> +!(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
>> +goto cleanup;
>> +
>>  qemuDomainObjEnterMonitor(driver, vm);
>> -if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD)
>> +if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) {
>> +if (tlsAlias)
>> +ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias));
>>  ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias));
>> +}
>>  
>>  rc = qemuMonitorDelObject(priv->mon, objAlias);
>>  
>> @@ -3648,6 +3657,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver,
>>   cleanup:
>>  VIR_FREE(charAlias);
>>  VIR_FREE(objAlias);
>> +VIR_FREE(tlsAlias);
>>  return ret;
>>  }
>>  
>> -- 
>> 2.7.4
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 12/25] qemu: new functions to calculate/set device pciConnectFlags

[Andrea Bolognani]

On Fri, 2016-10-14 at 15:54 -0400, Laine Stump wrote:
> The lowest level function of this trio
> (qemuDomainDeviceCalculatePCIConnectFlags()) aims to be the single
> authority for the virDomainPCIConnectFlags to use for any given device
> using a particular arch/machinetype/qemu-binary.
> 
> qemuDomainFillDevicePCIConnectFlags() sets info->pciConnectFlags in a
> single device (unless it has no virDomainDeviceInfo, in which case
> it's a NOP).
> 
> qemuDomainFillAllPCIConnectFlags() sets info->pciConnectFlags in all
> devices that have a virDomainDeviceInfo
> 
> The latter two functions aren't called anywhere yet. This commit is
> just making them available. Later patches will replace all the current
> hodge-podge of flag settings with calls to this single authority.
> ---
> 
> Change: re-implemented as a giant two-level compound switch statement
> with no default cases, as suggested by Andrea. Also start off
> with the function setting the flags for *all* devices to
> PCI_DEVICE|HOTPLUGGABLE, which is what's currently used when
> assigning addresses (as opposed to merely validating addresses,
> which is much less strict).
> 
>  src/conf/device_conf.h |   5 +
>  src/qemu/qemu_domain_address.c | 379 
>+
>  2 files changed, 384 insertions(+)
> 
> diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
> index 8443de6..f435fb5 100644
> --- a/src/conf/device_conf.h
> +++ b/src/conf/device_conf.h
> @@ -142,6 +142,11 @@ typedef struct _virDomainDeviceInfo {
>  /* bootIndex is only used for disk, network interface, hostdev
>   * and redirdev devices */
>  unsigned int bootIndex;
> +
> +/* pciConnectFlags is only used internally during address
> + * assignment, never saved and never reported.
> + */
> +int pciConnectFlags; /* enum virDomainPCIConnectFlags */
>  } virDomainDeviceInfo, *virDomainDeviceInfoPtr;
>  
>  
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index b4ea906..457eae6 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -405,6 +405,385 @@ qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr 
> def,
>  }
>  
>  
> +/**
> + * qemuDomainDeviceCalculatePCIConnectFlags:
> + *
> + * Lowest level function to determine PCI connectFlags for a
> + * device. This function relies on the next higher-level function
> + * determining the value for pcieFlags and virtioFlags in advance -
> + * this is to make it more efficient to call multiple times.
> + *
> + * @dev: The device to be checked
> + *
> + * @pcieFlags: flags to use for a known PCI Express device
> + *
> + * @virtioFlags: flags to use for a virtio device (properly vetted
> + *   for the current qemu binary and arch/machinetype)
> + *
> + * returns appropriate virDomainPCIConnectFlags for this device in
> + * this domain, or 0 if the device doesn't connect using PCI. There
> + * is no failure.
> + */

Please adjust the formatting of this and later comments to
match the guidelines we discussed for earlier patches.

> +static virDomainPCIConnectFlags
> +qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
> + virDomainPCIConnectFlags pcieFlags
> + ATTRIBUTE_UNUSED,
> + virDomainPCIConnectFlags virtioFlags
> + ATTRIBUTE_UNUSED)
> +{
> +virDomainPCIConnectFlags pciFlags =  (VIR_PCI_CONNECT_TYPE_PCI_DEVICE |

Double space.

> +  VIR_PCI_CONNECT_HOTPLUGGABLE);
> +
> +switch ((virDomainDeviceType)dev->type) {

We use both '(type)expr' and '(type) expr', but the latter
seems to be more popular. Up to you whether or not you want
to conform :)

> +case VIR_DOMAIN_DEVICE_CONTROLLER: {
> +virDomainControllerDefPtr cont = dev->data.controller;
> +
> +switch ((virDomainControllerType)cont->type) {
> +case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
> +return virDomainPCIControllerModelToConnectType(cont->model);
> +
> +case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
> +return pciFlags;
> +
> +case VIR_DOMAIN_CONTROLLER_TYPE_USB:
> +switch ((virDomainControllerModelUSB)cont->model) {
> +case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI:
> +return pciFlags;
> +
> +case VIR_DOMAIN_CONTROLLER_MODEL_USB_EHCI:
> +case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1:
> +case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1:
> +case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2:
> +case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3:
> +case VIR_DOMAIN_CONTROLLER_MODEL_USB_VT82C686B_UHCI:
> +case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI:
> +case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX4_UHCI:
> +   

Re: [libvirt] [PATCH v2 04/12] qemu: Introduce qemuDomainSetBlockIoTuneSetDefaults

[Erik Skultety]

On Thu, Oct 06, 2016 at 06:38:52PM -0400, John Ferlan wrote:
> Create a helper to set the bytes/iops iotune default values based on
> the current qemu setting
> 
> Signed-off-by: John Ferlan 
> ---
>  src/qemu/qemu_driver.c | 66 
> ++
>  1 file changed, 40 insertions(+), 26 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 78e917e..fcce3f0 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -17286,6 +17286,43 @@ qemuDomainOpenGraphicsFD(virDomainPtr dom,
>  return ret;
>  }
>  
> +
> +/* If the user didn't specify bytes limits, inherit previous values;
> + * likewise if the user didn't specify iops limits.  */
> +static void
> +qemuDomainSetBlockIoTuneSetDefaults(virDomainBlockIoTuneInfoPtr newinfo,
> +virDomainBlockIoTuneInfoPtr oldinfo,
> +bool set_bytes,
> +bool set_iops,
> +bool set_bytes_max,
> +bool set_iops_max,
> +bool set_size_iops)
> +{

Could this be a macro instead? 5 booleans, it just seems a tiny bit odd.

> +if (!set_bytes) {
> +newinfo->total_bytes_sec = oldinfo->total_bytes_sec;
> +newinfo->read_bytes_sec = oldinfo->read_bytes_sec;
> +newinfo->write_bytes_sec = oldinfo->write_bytes_sec;
> +}

And then you'll end up just with snippet like ^^this. Or you could use
bitwise OR'd flags instead of the booleans, as it scales better (in terms of
number of arguments), though I'm not very fond of that even though it works.

ACK with the adjustment, but see 5/12 as well before pushing.

Erik


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 05/12] qemu: Use qemuDomainsetBlockIoTuneSetDefaults for config

[Erik Skultety]

On Thu, Oct 06, 2016 at 06:38:53PM -0400, John Ferlan wrote:
> Rather than repeat the lines in the persistent def, use the same helper
> to set config values.
> 
> This also fixes a bug where config values for *_max and size_iops_sec would
> be set back to 0 if a config value was set.

Is there a BZ for this^^, if yes, then please add it here, if not, then
I think this can be easily squashed to 4/12.

ACK anyway.

Erik


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v11 3/5] qemu: Need to remove TLS object in RemoveRNGDevice

[Pavel Hrdina]

On Mon, Oct 24, 2016 at 06:46:19PM -0400, John Ferlan wrote:
> Commit id '6e6b4bfc' added the object, but forgot the other end.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/qemu/qemu_hotplug.c | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)

ACK if you agree with review on the first patch that the order of 
qemuMonitorDelObject and qemuMonitorDetachCharDev should be swapped.

Pavel

> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index e287aaf..f401b48 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -3608,6 +3608,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver,
>  virObjectEventPtr event;
>  char *charAlias = NULL;
>  char *objAlias = NULL;
> +char *tlsAlias = NULL;
>  qemuDomainObjPrivatePtr priv = vm->privateData;
>  ssize_t idx;
>  int ret = -1;
> @@ -3616,15 +3617,23 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver,
>  VIR_DEBUG("Removing RNG device %s from domain %p %s",
>rng->info.alias, vm, vm->def->name);
>  
> +
>  if (virAsprintf(, "obj%s", rng->info.alias) < 0)
>  goto cleanup;
>  
>  if (!(charAlias = qemuAliasChardevFromDevAlias(rng->info.alias)))
>  goto cleanup;
>  
> +if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD &&
> +!(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
> +goto cleanup;
> +
>  qemuDomainObjEnterMonitor(driver, vm);
> -if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD)
> +if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) {
> +if (tlsAlias)
> +ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias));
>  ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias));
> +}
>  
>  rc = qemuMonitorDelObject(priv->mon, objAlias);
>  
> @@ -3648,6 +3657,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver,
>   cleanup:
>  VIR_FREE(charAlias);
>  VIR_FREE(objAlias);
> +VIR_FREE(tlsAlias);
>  return ret;
>  }
>  
> -- 
> 2.7.4
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v11 2/5] qemu: Swap order of RNG hot unplug removal

[Pavel Hrdina]

On Mon, Oct 24, 2016 at 06:46:18PM -0400, John Ferlan wrote:
> Rather than remove the frontend first and then the backend, let's swap the
> order. The order of addition is add the chardev backend if EGD being used,
> then add the RNG object. So there's a dependency there. When we remove,
> unplug the backend first, then remove the object would seem to be a more
> logical step. This would then follow similar processing for disk removal
> where dependent objects (secret and encryption) of the drive are removed
> before the drive is removed.

Nice catch, qemuDomainRemoveChrDevice needs the same fix.

ACK

Pavel


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v11 1/5] qemu: Move TLS object remove from DetachChr to RemoveChr

[Pavel Hrdina]

On Mon, Oct 24, 2016 at 06:46:17PM -0400, John Ferlan wrote:
> Commit id '2c32237' added the TLS object removal to the DetachChrDevice
> all when it should have been added to the RemoveChrDevice since that's
> the norm for similar processing (e.g. disk)
> 
> Signed-off-by: John Ferlan 
> ---
>  src/qemu/qemu_hotplug.c | 44 +---
>  1 file changed, 21 insertions(+), 23 deletions(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index cf69945..31b5876 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -3549,7 +3549,9 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
>virDomainChrDefPtr chr)
>  {
>  virObjectEventPtr event;
> +virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>  char *charAlias = NULL;
> +char *tlsAlias = NULL;
>  qemuDomainObjPrivatePtr priv = vm->privateData;
>  int ret = -1;
>  int rc;
> @@ -3560,7 +3562,16 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
>  if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias)))
>  goto cleanup;
>  
> +if (chr->source->type == VIR_DOMAIN_CHR_TYPE_TCP &&
> +chr->source->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES &&
> +!(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
> +goto cleanup;
> +
>  qemuDomainObjEnterMonitor(driver, vm);
> +
> +if (tlsAlias && qemuMonitorDelObject(priv->mon, tlsAlias) < 0)
> +goto exit_monitor;
> +

Those two qemu commands should be swapped as well.  This is similar to the
issues that the next patch fixes.  The chardev uses tls object so the chardev
should be detached before the tls object.

ACK with that fixed.

Pavel

>  rc = qemuMonitorDetachCharDev(priv->mon, charAlias);
>  if (qemuDomainObjExitMonitor(driver, vm) < 0)
>  goto cleanup;
> @@ -3579,7 +3590,13 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
>  
>   cleanup:
>  VIR_FREE(charAlias);
> +VIR_FREE(tlsAlias);
> +virObjectUnref(cfg);
>  return ret;
> +
> + exit_monitor:
> +ignore_value(qemuDomainObjExitMonitor(driver, vm));
> +goto cleanup;
>  }
>  
>  
> @@ -4461,13 +4478,10 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
>virDomainChrDefPtr chr)
>  {
>  int ret = -1;
> -virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>  qemuDomainObjPrivatePtr priv = vm->privateData;
>  virDomainDefPtr vmdef = vm->def;
>  virDomainChrDefPtr tmpChr;
> -char *objAlias = NULL;
>  char *devstr = NULL;
> -char *charAlias = NULL;
>  
>  if (!(tmpChr = virDomainChrFind(vmdef, chr))) {
>  virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> @@ -4480,26 +4494,16 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
>  
>  sa_assert(tmpChr->info.alias);
>  
> -if (!(charAlias = qemuAliasChardevFromDevAlias(tmpChr->info.alias)))
> -goto cleanup;
> -
> -if (tmpChr->source->type == VIR_DOMAIN_CHR_TYPE_TCP &&
> -tmpChr->source->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES &&
> -!(objAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
> -goto cleanup;
> -
>  if (qemuBuildChrDeviceStr(, vmdef, chr, priv->qemuCaps) < 0)
>  goto cleanup;
>  
>  qemuDomainMarkDeviceForRemoval(vm, >info);
>  
>  qemuDomainObjEnterMonitor(driver, vm);
> -if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0)
> -goto exit_monitor;
> -
> -if (objAlias && qemuMonitorDelObject(priv->mon, objAlias) < 0)
> -goto exit_monitor;
> -
> +if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) {
> +ignore_value(qemuDomainObjExitMonitor(driver, vm));
> +goto cleanup;
> +}
>  if (qemuDomainObjExitMonitor(driver, vm) < 0)
>  goto cleanup;
>  
> @@ -4511,13 +4515,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
>   cleanup:
>  qemuDomainResetDeviceRemoval(vm);
>  VIR_FREE(devstr);
> -VIR_FREE(charAlias);
> -virObjectUnref(cfg);
>  return ret;
> -
> - exit_monitor:
> -ignore_value(qemuDomainObjExitMonitor(driver, vm));
> -goto cleanup;
>  }
>  
>  
> -- 
> 2.7.4
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC] qemu: Use virtio-pci by default for mach-virt guests

[Pavel Hrdina]

On Tue, Oct 25, 2016 at 02:10:37PM +0200, Martin Kletzander wrote:
> On Mon, Oct 24, 2016 at 06:24:07PM +0200, Andrea Bolognani wrote:
> >On Mon, 2016-10-24 at 17:47 +0200, Pavel Hrdina wrote:
> >> > > I like that this makes pci truly the default in a simple manner, but 
> >> > > still allows switching back to mmio if necessary. On the other hand, 
> >> > > it 
> >> > > puts the potential "switch" to decide whether or not to use mmio for 
> >> > > all 
> >> > > devices down into the config of a single device, which is a bit weird 
> >> > > to 
> >> > > explain. (On the other hand, how often will mmio be used in the 
> >> > > future? 
> >> > > Maybe it doesn't matter if it's weird to explain...)
> >> > 
> >> > We really want to push for virtio-pci going forward as it has
> >> > a number of advantages, but there are still legacy guest OSs
> >> > out there that don't support virtio-pci at all yet we still
> >> > want to be able to run.
> >> 
> >> Changing the default is usually a tricky part and may break some users.
> >> I'm not sure that this will save the need to adapt management applications
> >> and users.  They will have to adapt in both cases to support legacy and
> >> new OSes based on libosinfo or another tool/database.  If we make 
> >> virtio-pci
> >> the default one, which I think is the way it should be used, they will have
> >> to make sure that with new libvirt for the same OS they will fallback
> >> to virtio-mmio.
> >> 
> >> From what I can remember we've never done such change of default device
> >> model or default address and we always left it no management application
> >> or user to change the default to better model.  In case of management
> >> applications it's not an issue because they will make sure that the best
> >> device models and device address are used.
> >> 
> >> I'm not against changing the default to virtio-pci, but we may break
> >> things for some users and management tools like virt-manager unless they
> >> will adapt to this change.
> >
> >You raise very good points, thanks for your input! :)
> >
> >AFAICT the only use case that we'd risk breaking is installing
> >a legacy guest OS that doesn't support virtio-pci without
> >requiring the user to explicitly ask for virtio-mmio addresses.
> >
> 
> And that is only for new installs (just if someone misses that you said
> "installs").

Yes it will break only new installs.

> >Once libosinfo has learned about this, and virt-manager has
> >been updated to query libosinfo and switch to virtio-mmio
> >automatically if required, would you be okay with this change?
> >
> >I think for aarch64 we're still in a phase where we can afford
> >to take some tradeoffs when it comes to compatibility, if
> >they're properly motivated: in this specific case, seeing as
> >basic stuff like device hotplug has simply never worked for
> >virtio-mmio, I'm fairly confident nobody will want to stick
> >with virtio-mmio for very long now that virtio-pci is finally
> >viable.
> >
> 
> And I feel like we can change defaults like that, especially with new
> installs, that's why we save the generated info in the XMLs.  If we were
> not able to change the defaults, we would not be able to add *anything*
> to the command line by default.  And, yes, aarch64 is still in its
> diapers, so I, too, feel like we have even more leeway in such scenarios.

I agree that we can change it.  It was just to make a not that the management
application will have to update itself in any case to adapt to new libvirt.

Pavel


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/1] qemu: host NUMA hugepage policy without guest NUMA

[Martin Kletzander]

On Tue, Oct 25, 2016 at 01:10:23PM +1100, Sam Bobroff wrote:

On Tue, Oct 18, 2016 at 10:43:31PM +0200, Martin Kletzander wrote:

On Mon, Oct 17, 2016 at 03:45:09PM +1100, Sam Bobroff wrote:
>On Fri, Oct 14, 2016 at 10:19:42AM +0200, Martin Kletzander wrote:
>>On Fri, Oct 14, 2016 at 11:52:22AM +1100, Sam Bobroff wrote:
>>>I did look at the libnuma and cgroups approaches, but I was concerned they
>>>wouldn't work in this case, because of the way QEMU allocates memory when
>>>mem-prealloc is used: the memory is allocated in the main process, before the
>>>CPU threads are created. (This is based only on a bit of hacking and 
debugging
>>>in QEMU, but it does seem explain the behaviour I've seen so far.)
>>>
>>
>>But we use numactl before QEMU is exec()'d.
>
>Sorry, I jumped ahead a bit. I'll try to explain what I mean:
>
>I think the problem with using this method would be that the NUMA policy is
>applied to all allocations by QEMU, not just ones related to the memory
>backing. I'm not sure if that would cause a serious problem but it seems 
untidy,
>and it doesn't happen in other situations (i.e. with separate memory backend
>objects, QEMU sets up the policy specifically for each one and other
>allocations aren't affected, AFAIK).  Presumably, if memory were very
>restricted it could prevent the guest from starting.
>

Yes, it is, that's what  does if you don't have any
other () specifics set.

>>>I think QEMU could be altered to move the preallocations into the VCPU
>>>threads but it didn't seem trivial and I suspected the QEMU community would
>>>point out that there was already a way to do it using backend objects.  
Another
>>>option would be to add a -host-nodes parameter to QEMU so that the policy can
>>>be given without adding a memory backend object. (That seems like a more
>>>reasonable change to QEMU.)
>>>
>>
>>I think upstream won't like that, mostly because there is already a
>>way.  And that is using memory-backend object.  I think we could just
>>use that and disable changing it live.  But upstream will probably want
>>that to be configurable or something.
>
>Right, but isn't this already an issue in the cases where libvirt is already
>using memory backend objects and NUMA policy? (Or does libvirt already disable
>changing it live in those situations?)
>

It is.  I'm not trying to say libvirt is perfect.  There are bugs,
e.g. like this one.  The problem is that we tried to do *everything*,
but it's not currently possible.  I'm trying to explain how stuff works
now.  It definitely needs some fixing, though.


OK :-)

Well, given our discussion, do you think it's worth a v2 of my original patch
or would it be better to drop it in favour of some broader change?



Honestly, I thought about the approaches so much I'm now not sure I'll
make a good decision.  RFC could do.  If I were to pick, I would go with
a new setting that would control whether we want the binding to be
changeable throughout the domain's lifetime or not so that we can make
better decisions (and don't feel bad about the bad ones).


Cheers,
Sam.



signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC] qemu: Use virtio-pci by default for mach-virt guests

[Martin Kletzander]

On Mon, Oct 24, 2016 at 06:24:07PM +0200, Andrea Bolognani wrote:

On Mon, 2016-10-24 at 17:47 +0200, Pavel Hrdina wrote:

> > I like that this makes pci truly the default in a simple manner, but 
> > still allows switching back to mmio if necessary. On the other hand, it 
> > puts the potential "switch" to decide whether or not to use mmio for all 
> > devices down into the config of a single device, which is a bit weird to 
> > explain. (On the other hand, how often will mmio be used in the future? 
> > Maybe it doesn't matter if it's weird to explain...)
> 
> We really want to push for virtio-pci going forward as it has
> a number of advantages, but there are still legacy guest OSs
> out there that don't support virtio-pci at all yet we still
> want to be able to run.
 
Changing the default is usually a tricky part and may break some users.
I'm not sure that this will save the need to adapt management applications
and users.  They will have to adapt in both cases to support legacy and
new OSes based on libosinfo or another tool/database.  If we make virtio-pci
the default one, which I think is the way it should be used, they will have
to make sure that with new libvirt for the same OS they will fallback
to virtio-mmio.
 
From what I can remember we've never done such change of default device
model or default address and we always left it no management application
or user to change the default to better model.  In case of management
applications it's not an issue because they will make sure that the best
device models and device address are used.
 
I'm not against changing the default to virtio-pci, but we may break
things for some users and management tools like virt-manager unless they
will adapt to this change.


You raise very good points, thanks for your input! :)

AFAICT the only use case that we'd risk breaking is installing
a legacy guest OS that doesn't support virtio-pci without
requiring the user to explicitly ask for virtio-mmio addresses.



And that is only for new installs (just if someone misses that you said
"installs").


Once libosinfo has learned about this, and virt-manager has
been updated to query libosinfo and switch to virtio-mmio
automatically if required, would you be okay with this change?

I think for aarch64 we're still in a phase where we can afford
to take some tradeoffs when it comes to compatibility, if
they're properly motivated: in this specific case, seeing as
basic stuff like device hotplug has simply never worked for
virtio-mmio, I'm fairly confident nobody will want to stick
with virtio-mmio for very long now that virtio-pci is finally
viable.



And I feel like we can change defaults like that, especially with new
installs, that's why we save the generated info in the XMLs.  If we were
not able to change the defaults, we would not be able to add *anything*
to the command line by default.  And, yes, aarch64 is still in its
diapers, so I, too, feel like we have even more leeway in such scenarios.


-- 
Andrea Bolognani / Red Hat / Virtualization


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 5/6] Fix crash on usb-serial hotplug

[John Ferlan]

On 10/21/2016 09:58 AM, Ján Tomko wrote:
> For domains with no USB address cache, we should not attempt
> to generate a USB address.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1387665
> ---
>  src/qemu/qemu_hotplug.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

ACK

John

Yet another case where ATTRIBUTE_NONNULL(#arg) doesn't really work if
the passed argument is assigned to NULL  (as opposed to the checks
made if someone passed 'NULL' in the argument.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 6/6] Do not try to release virtio serial addresses

[John Ferlan]

On 10/21/2016 09:58 AM, Ján Tomko wrote:
> Return 0 instead of 1, so that qemuDomainAttachChrDevice does not
> assume the address neeeds to be released on error.
> 
> No functional change, since qemuDomainReleaseDeviceAddress has been a noop
> for virtio serial addresses since the address cache was removed.

s/./ in commit id '19a148b7'.


ACK,

John

> ---
>  src/qemu/qemu_hotplug.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 9e9073d..d432616 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1635,6 +1635,10 @@ qemuDomainChrRemove(virDomainDefPtr vmdef,
>  return ret;
>  }
>  
> +/* Returns  1 if the address will need to be released later,
> + * -1 on error
> + *  0 otherwise
> + */

Something that should have been there from the start!

>  static int
>  qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr def,
>  qemuDomainObjPrivatePtr priv,
> @@ -1644,7 +1648,7 @@ qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr def,
>  chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) {
>  if (virDomainVirtioSerialAddrAutoAssign(def, >info, true) < 0)
>  return -1;
> -return 1;
> +return 0;
>  
>  } else if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
> chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI) {
> @@ -1663,7 +1667,7 @@ qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr def,
> chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO) 
> {
>  if (virDomainVirtioSerialAddrAutoAssign(def, >info, false) < 0)
>  return -1;
> -return 1;
> +return 0;
>  }
>  
>  if (chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL ||
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 4/6] Also create the USB address cache for domains with all the USB addresses

[John Ferlan]

$SUBJ...

The "Also" just seems misplaced.

especially with the "Also generate the..." below...

How about "At Reconnect, recreate the USB address cache"

?

On 10/21/2016 09:58 AM, Ján Tomko wrote:
> When starting a new domain, we allocate the USB addresses and keep
> an address cache in the domain object's private data.
> 
> However this data is lost on libvirtd restart.

The qemuProcessReconnect will call qemuDomainAssignAddresses w/
newDomain = false, so this code needs to "repopulate" the usbaddr cache
properly, but can only properly do so if all the USB addresses for the
domain were specified in the domain XML.

In this path are we getting the status XML file which "should" have all
the addresses, right?

> 
> Also generate the address cache if all the addresses have been
> specified, so that devices hotplugged after libvirtd restart
> also get theirs assigned.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1387666
> ---
>  src/conf/domain_addr.c | 12 
>  src/conf/domain_addr.h |  4 
>  src/libvirt_private.syms   |  1 +
>  src/qemu/qemu_domain_address.c | 16 ++--
>  4 files changed, 31 insertions(+), 2 deletions(-)
> 

ACK for the patch, just validating/checking my assumptions on the commit
message.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/6] Add 'FromCache' to virDomainVirtioSerialAddrAutoAssign

[John Ferlan]

On 10/21/2016 09:58 AM, Ján Tomko wrote:
> We dropped the cache from QEMU's private domain object.

s/We dropped the cache/Commit id '?' dropped the cache

Is it commit id '19a148b7c8'?

> Assume the callers do not have the cache by default and use
> a longer name for the internal ones that do.
> 
> This makes the shorter 'virDomainVirtioSerialAddrAutoAssign'
> name availabe for a function that will not require the cache.
> ---
>  src/conf/domain_addr.c | 8 
>  src/conf/domain_addr.h | 8 
>  src/libvirt_private.syms   | 2 +-
>  src/qemu/qemu_domain_address.c | 6 --
>  src/qemu/qemu_hotplug.c| 8 
>  5 files changed, 17 insertions(+), 15 deletions(-)
> 

ACK with the reference...

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/6] Return directly from qemuDomainAttachChrDeviceAssignAddr

[John Ferlan]

On 10/21/2016 09:58 AM, Ján Tomko wrote:
> This function should never need a cleanup section.
> ---
>  src/qemu/qemu_hotplug.c | 28 ++--
>  1 file changed, 10 insertions(+), 18 deletions(-)
> 

ACK

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/6] Introduce virDomainVirtioSerialAddrAutoAssign again

[John Ferlan]

On 10/21/2016 09:58 AM, Ján Tomko wrote:
> This time do not require an address cache as a parameter.

Essentially reworking commit id '925fa4b90', right?  Provide the
reference...

> 
> Simplify qemuDomainAttachChrDeviceAssignAddr to not generate
> the virtio serial address cache for devices of other types.
> ---
>  src/conf/domain_addr.c   | 21 +
>  src/conf/domain_addr.h   |  6 ++
>  src/libvirt_private.syms |  1 +
>  src/qemu/qemu_hotplug.c  | 11 ++-
>  4 files changed, 30 insertions(+), 9 deletions(-)
> 

ACK w/ the reference

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] how to install and remove Libvirt cleanly

[zhun...@gmail.com]

I  just want to know how to uninstall Libvirt which installed by source code 
cleanly.because I found make uninstall can not remove all the installed file.so 
that I install it later failed.



zhun...@gmail.com
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH python 1/1] fix crash in libvirt_virDomainPin*

[Konstantin Neumoin]

If we pass large(more than cpunum) cpu mask to any libvirt_virDomainPin*
methods, it could leads to crash. So we have to check tuple size and
ignore extra tuple members.

Signed-off-by: Konstantin Neumoin 
---
 libvirt-override.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/libvirt-override.c b/libvirt-override.c
index fa3e2ca..83b760b 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -1327,7 +1327,7 @@ libvirt_virDomainPinVcpu(PyObject *self ATTRIBUTE_UNUSED,
 if (VIR_ALLOC_N(cpumap, cpumaplen) < 0)
 return PyErr_NoMemory();
 
-for (i = 0; i < tuple_size; i++) {
+for (i = 0; i < MIN(cpunum, tuple_size); i++) {
 PyObject *flag = PyTuple_GetItem(pycpumap, i);
 bool b;
 
@@ -1392,7 +1392,7 @@ libvirt_virDomainPinVcpuFlags(PyObject *self 
ATTRIBUTE_UNUSED,
 if (VIR_ALLOC_N(cpumap, cpumaplen) < 0)
 return PyErr_NoMemory();
 
-for (i = 0; i < tuple_size; i++) {
+for (i = 0; i < MIN(cpunum, tuple_size); i++) {
 PyObject *flag = PyTuple_GetItem(pycpumap, i);
 bool b;
 
@@ -1532,7 +1532,7 @@ libvirt_virDomainPinEmulator(PyObject *self 
ATTRIBUTE_UNUSED,
 if (VIR_ALLOC_N(cpumap, cpumaplen) < 0)
 return PyErr_NoMemory();
 
-for (i = 0; i < tuple_size; i++) {
+for (i = 0; i < MIN(cpunum, tuple_size); i++) {
 PyObject *flag = PyTuple_GetItem(pycpumap, i);
 bool b;
 
@@ -1738,7 +1738,7 @@ libvirt_virDomainPinIOThread(PyObject *self 
ATTRIBUTE_UNUSED,
 if (VIR_ALLOC_N(cpumap, cpumaplen) < 0)
 return PyErr_NoMemory();
 
-for (i = 0; i < tuple_size; i++) {
+for (i = 0; i < MIN(cpunum, tuple_size); i++) {
 PyObject *flag = PyTuple_GetItem(pycpumap, i);
 bool b;
 
-- 
2.5.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 0/4] Add VCPU halted to domain statistics

[Viktor Mihajlovski]

On 25.10.2016 00:56, John Ferlan wrote:
> 
[...]
> Now pushed
> 
> John
> 
Thanks!

-- 

Mit freundlichen Grüßen/Kind Regards
   Viktor Mihajlovski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [jenkins-ci PATCH] jobs: change maintainer email address

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina 
---

Pushed as trivial.

 jobs/defaults.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/jobs/defaults.yaml b/jobs/defaults.yaml
index 23d9ae7..b21bff3 100644
--- a/jobs/defaults.yaml
+++ b/jobs/defaults.yaml
@@ -9,4 +9,4 @@
   MAKE='gmake'
   fi
 smp: 3
-spam: shaj...@redhat.com libvirt...@redhat.com
+spam: yman...@redhat.com libvirt...@redhat.com
-- 
2.10.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list