Re: [libvirt] [Qemu-devel] [RFC v2] libvirt vGPU QEMU integration

[Kirti Wankhede]

On 9/29/2016 8:12 PM, Tian, Kevin wrote:
>> From: Daniel P. Berrange [mailto:berra...@redhat.com]
>> Sent: Thursday, September 29, 2016 10:39 PM
>>
>> On Thu, Sep 29, 2016 at 02:35:48PM +, Tian, Kevin wrote:
 From: Daniel P. Berrange [mailto:berra...@redhat.com]
 Sent: Thursday, September 29, 2016 4:06 PM

 On Wed, Sep 28, 2016 at 12:48:33PM -0700, Neo Jia wrote:
> On Tue, Sep 20, 2016 at 10:47:53AM +0100, Daniel P. Berrange wrote:
>> On Tue, Sep 20, 2016 at 02:05:52AM +0530, Kirti Wankhede wrote:
>>>
>>> Hi libvirt experts,
>>>
>>> Thanks for valuable input on v1 version of RFC.
>>>
>>> Quick brief, VFIO based mediated device framework provides a way to
>>> virtualize their devices without SR-IOV, like NVIDIA vGPU, Intel KVMGT
>>> and IBM's channel IO. This framework reuses VFIO APIs for all the
>>> functionalities for mediated devices which are currently being used for
>>> pass through devices. This framework introduces a set of new sysfs files
>>> for device creation and its life cycle management.
>>>
>>> Here is the summary of discussion on v1:
>>> 1. Discover mediated device:
>>> As part of physical device initialization process, vendor driver will
>>> register their physical devices, which will be used to create virtual
>>> device (mediated device, aka mdev) to the mediated framework.
>>>
>>> Vendor driver should specify mdev_supported_types in directory format.
>>> This format is class based, for example, display class directory format
>>> should be as below. We need to define such set for each class of devices
>>> which would be supported by mediated device framework.
>>>
>>>  --- mdev_destroy
>>>  --- mdev_supported_types
>>>  |-- 11
>>>  |   |-- create
>>>  |   |-- name
>>>  |   |-- fb_length
>>>  |   |-- resolution
>>>  |   |-- heads
>>>  |   |-- max_instances
>>>  |   |-- params
>>>  |   |-- requires_group
>>>  |-- 12
>>>  |   |-- create
>>>  |   |-- name
>>>  |   |-- fb_length
>>>  |   |-- resolution
>>>  |   |-- heads
>>>  |   |-- max_instances
>>>  |   |-- params
>>>  |   |-- requires_group
>>>  |-- 13
>>>  |-- create
>>>  |-- name
>>>  |-- fb_length
>>>  |-- resolution
>>>  |-- heads
>>>  |-- max_instances
>>>  |-- params
>>>  |-- requires_group
>>>
>>>
>>> In the above example directory '11' represents a type id of mdev device.
>>> 'name', 'fb_length', 'resolution', 'heads', 'max_instance' and
>>> 'requires_group' would be Read-Only files that vendor would provide to
>>> describe about that type.
>>>
>>> 'create':
>>> Write-only file. Mandatory.
>>> Accepts string to create mediated device.
>>>
>>> 'name':
>>> Read-Only file. Mandatory.
>>> Returns string, the name of that type id.
>>
>> Presumably this is a human-targetted title/description of
>> the device.
>>
>>>
>>> 'fb_length':
>>> Read-only file. Mandatory.
>>> Returns {K,M,G}, size of framebuffer.
>>>
>>> 'resolution':
>>> Read-Only file. Mandatory.
>>> Returns 'hres x vres' format. Maximum supported resolution.
>>>
>>> 'heads':
>>> Read-Only file. Mandatory.
>>> Returns integer. Number of maximum heads supported.
>>
>> None of these should be mandatory as that makes the mdev
>> useless for non-GPU devices.
>>
>> I'd expect to see a 'class' or 'type' attribute in the
>> directory whcih tells you what kind of mdev it is. A
>> valid 'class' value would be 'gpu'. The fb_length,
>> resolution, and heads parameters would only be mandatory
>> when class==gpu.
>>
>
> Hi Daniel,
>
> Here you are proposing to add a class named "gpu", which will make all 
> those gpu
> related attributes mandatory, which libvirt can allow user to better
> parse/present a particular mdev configuration?
>
> I am just wondering if there is another option that we just make all those
> attributes that a mdev device can have as optional but still meaningful to
> libvirt, so libvirt can still parse / recognize them as an class "mdev".

 'mdev' isn't a class - mdev is the name of the kernel module. The class
 refers to the broad capability of the device. class would be things
 like "gpu", "nic", "fpga" or other such things. The point of the class
 is to identify which other attributes will be considered mandatory.


>>>
>>> Thanks Daniel. This class definition makes sense to me.
>>>
>>> However I'm not sure whether we should define such common mandatory 
>>> attributes
>>> of a 'gpu' class now. Intel will go with a 2's power 

Re: [libvirt] summary of current vfio mdev upstreaming status

[Jike Song]

On 09/29/2016 06:58 PM, Kirti Wankhede wrote:
> 
> 
> On 9/29/2016 2:47 PM, Neo Jia wrote:
>> On Thu, Sep 29, 2016 at 04:55:39PM +0800, Jike Song wrote:
>>> Hi all,
>>>
>>> In order to have a clear understanding about the VFIO mdev upstreaming
>>> status, I'd like to summarize it. Please share your opinions on this,
>>> and correct my misunderstandings.
>>>
>>> The whole vfio mdev series can be logically divided into several parts,
>>> they work together to provide the mdev support.
>>
> 
> Thanks Jike for summarizing. We already have separate patch for each of
> these logical parts. I had maintained patch sequence in incremental
> depending order.
> 
>> Hi Jike,
>>
>> Thanks for summarizing this, but I will defer to Kirti to comment on the 
>> actual
>> upstream status of her patches, couples things to note though:
>>
>> 1) iommu type1 patches have been extensively reviewed by Alex already and we
>> have one action item left to implement which is already queued up in v8 
>> patchset.
>>
> 
> That's right Neo.
> 

I'm talking about v7. Sure before that Alex gave full reviews..

>> 2) regarding the sysfs interface and libvirt discussion, I would like to hear
>> what kind of attributes Intel folks are having so far as Daniel is
>> asking about adding a class "gpu" which will pull several attributes as 
>> mandatory.
>>

As Kevin said, no. 

>> Thanks,
>> Neo
>>
>>>
>>>
>>>
>>> PART 1: mdev core driver
>>>
>>> [task]
>>> -   the mdev bus/device support
>>> -   the utilities of mdev lifecycle management
>>> -   the physical device register/unregister interfaces
>>>
>>> [status]
>>> -   basically agreed by community
>>>
>>>
>>> PART 2: vfio bus driver for mdev
>>>
>>> [task]
>>> -   interfaces with vendor drivers
>>> -   the vfio bus implementation
>>>
>>> [status]
>>>
>>> -   basically agreed by community
>>>
> 
> I'm working on v8 version for above patches based on previous discussions.
> 
>>>
>>> PART 3: iommu support for mdev
>>>
>>> [task]
>>> -   iommu support for mdev
>>>
>>> [status]
>>> -   Kirti's v7 implementation, not yet fully reviewed
>>>
>>>
>>> PART 4: sysfs interfaces for mdev
>>>
>>> [task]
>>> -   define the hierarchy of minimal sysfs directories/files
>>> -   check the validity from vendor drivers, init/de-init 
>>> them
>>> [status]
>>> -   interfaces are in discussion
>>>
>>>
> 
> From coding perspective, this is part of mdev core module. I think we
> can't completely separate this part from mdev core module while coding
> it. Yes, this interface is still in discussion and we need to settle
> down on that soon.
> 

I Still think it's possible to separate them, but hey, looking forward to
your implementation :)

>>> PART 6: Documentation
>>>
>>> [task]
>>> -   clearly document the architecture and interfaces
>>> -   coding example for vendor drivers
>>>
>>> [status]
>>> -   N/A
>>>
> 
> I had tried to maintain the document as per changes going on in above
> patches from v6 onward and will continue to update it for each version
> accordingly.
> 
> I had sent out patch with sample driver few hours back wrt v7 patchset.
> And henceforth I'll keep on updating sample driver as per changes in
> mdev modules and add it in my patch series.

Good to know that.

> 
> Thanks,
> Kirti
> 
--
Thanks,
Jike

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

Re: [libvirt] [PATCH 3/3] introduce pull backup

[John Ferlan]

[...]

>>
>> Because it's also dependent upon an x-blockdev-del, it cannot be pushed
>> upstream to libvirt. I know qemu work continues w/r/t blockdev-add and
>> backups, but I don't follow it in detail (not enough time in the day!).
> 
> Ok, at least the patch can be some kind of candidate to be pushed upstream
> as soon as blockdev-del drops x- prefix.
> 

Not as a single patch though - I have a feeling you're going to have a
git branch committed to this for a while...  Consider my recent response
to your 1/3 patch w/r/t qemu's statement about the blockdev-add command...

My *guess* is when 'blockdev-del' is officially supported - that's when
libvirt can "assume" support for blockdev-add (since the x- was taken
off of it - something that we can document when that happens).

[...]


>>
>> BTW: Using QUIESCE would then rely on the guest agent being available
>> for the domain...  Just making a mental note, but yet something you have
>> a dependency upon.
> 
> Well it is just a flag, just as in case of snapshots. As to coding conventions
> etc a lot of stuff is rooted in similar snapshot code and xml.
> 

Something that just needs to be remembered - oh and documented in
virsh.pod...

[...]

>>> diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
>>> index 2ec560e..c1f8c6c 100644
>>> --- a/include/libvirt/virterror.h
>>> +++ b/include/libvirt/virterror.h
>>> @@ -131,6 +131,7 @@ typedef enum {
>>>  VIR_FROM_XENXL = 64,/* Error from Xen xl config code */
>>>  
>>>  VIR_FROM_PERF = 65, /* Error from perf */
>>> +VIR_FROM_DOMAIN_BACKUP = 66,/* Error from domain backup */
>>
>> Use a shorter name, suggest "DOMBKUP"
>>
> 
> but there is VIR_FROM_DOMAIN_SNAPSHOT... and it is more pleasant
> for eyes.
> 

OK - now that I know SNAPSHOT is your basis...  BTW: IIRC the snapshot
code didn't make it all in one patch...

>>>  
>>>  # ifdef VIR_ENUM_SENTINELS
>>>  VIR_ERR_DOMAIN_LAST
>>> diff --git a/po/POTFILES.in b/po/POTFILES.in
>>> index 25dbc84..4cdeb2f 100644
>>> --- a/po/POTFILES.in
>>> +++ b/po/POTFILES.in
>>> @@ -18,6 +18,7 @@ src/bhyve/bhyve_driver.c
>>>  src/bhyve/bhyve_monitor.c
>>>  src/bhyve/bhyve_parse_command.c
>>>  src/bhyve/bhyve_process.c
>>> +src/conf/backup_conf.c
>>
>> why not domain_backup_conf.{c,h}
> 
> similar to snapshot_conf.{c,h}
> 

It's just a file name in the long run; however, what if in the future
the storage driver added some sort of backup, then we'd have
'storage_backup_conf.c' to clearly describe that it was meant for
storage.  I think the domain__conf.c should be used...  And
yes, the same argument can be made for snapshot_conf.c - perhaps it
should be renamed.

[...]


>>> +VIR_ENUM_IMPL(virDomainBackupAddress, VIR_DOMAIN_BACKUP_ADDRESS_LAST,
>>> +  "ip",
>>
>> IPv4, IPv6, IPnotinventedyet
>>
>> why not "tcp", "tls", etc.? That would seemingly allow a bit of code
>> reuse for client transport. There's also virStorageNetHostTransport
> 
> Name 'tcp' is more appropriate indeed )) I would not use 
> virStorageNetHostTransport
> and associated structures and xml becasue they have different semantics.
> In case of backups we have address client can connect to, in case of 
> network disks we have address qemu as client can connect to.
> 

Creating a specific name is fine - I was just pointing out the
StorageNet for names to consider.

>>
>> FYI: also see qemuMonitorGraphicsAddressFamily
>>
> 
> And this one has right semantics. I remember I evaluated it as reuse 
> candidate.
> I thought it was awkward to have port in upper level element:
> 
> 
>   
> 
> 
> if we could declare it outdated and recommend to use new style...
> 
> 
> 
> (autoport, tlsPort etc goes here too...)
> 
> 
> Actually we already do this for address attribute:
> "The address attribute is duplicated as listen attribute in graphics element 
> for backward compatibility. If both are provided they must be equal."
> 
> So if you are ok with this, i would definetly reuse this element and
> associated structures.
> 

Again - just pointed out different things to consider. My knowledge of
blockdev-backup is miniscule (I think I spelled it right).

[...]

>>
>> Why differ from existing transport definitions? Seems like there would
>> be code reuse opportunities.
>>
>> BTW: If you're going to have a network protocol, then shouldn't you also
>> have an authentication mechanism?
> 
> AFAIK there is not any yet for pull backups. The situation is in case of
> migration, it is not secure when moving state and disks. This is what
> Daniel is working on AFAIK.
>

Just something that may have to be considered - although who knows. I've
recently added LUKS support and now am being told about all those corner
cases that weren't considered and where things didn't work quite right -
so I'm just trying to consider various things as they come to mind. With
any luck the end result will be better.

[...]


>>> +
>>> +if ((n = 

Re: [libvirt] [Qemu-devel] [PATCH v4 01/11] tests: Add test case for x86 feature parsing compatibility

[Jonathan Neuschäfer]

On Thu, Sep 29, 2016 at 06:14:49PM -0300, Eduardo Habkost wrote:
> Add a new test case to ensure the existing behavior of the
> feature parsing code wlil be kept.

s/wlil/will/

> 
> Signed-off-by: Eduardo Habkost 


Jonathan Neuschäfer


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

[libvirt] [PATCH v4 11/11] target-i386: Return runnability information on query-cpu-definitions

[Eduardo Habkost]

Fill the "unavailable-features" field on the x86 implementation
of query-cpu-definitions.

Cc: Jiri Denemark 
Cc: libvir-list@redhat.com
Signed-off-by: Eduardo Habkost 
---
Changes v3 -> v4:
* Handle missing XSAVE components cleanly, but looking up
  the original feature that required it
* Use x86_cpu_load_features() function

Changes v2 -> v3:
* Create a x86_cpu_feature_name() function, to
  isolate the code that returns the property name

Changes v1 -> v2:
* Updated to the new schema: no @runnable field, and
  always report @unavailable-features as present
---
 target-i386/cpu.c | 73 +++
 1 file changed, 73 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e099892..4dd3aee 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1945,6 +1945,27 @@ static inline void feat2prop(char *s)
 }
 }
 
+/* Return the feature property name for a feature flag bit */
+static const char *x86_cpu_feature_name(FeatureWord w, int bitnr)
+{
+/* XSAVE components are automatically enabled by other features,
+ * so return the original feature name instead
+ */
+if (w == FEAT_XSAVE_COMP_LO || w == FEAT_XSAVE_COMP_HI) {
+int comp = (w == FEAT_XSAVE_COMP_HI) ? bitnr + 32 : bitnr;
+
+if (comp < ARRAY_SIZE(x86_ext_save_areas) &&
+x86_ext_save_areas[comp].bits) {
+w = x86_ext_save_areas[comp].feature;
+bitnr = ctz32(x86_ext_save_areas[comp].bits);
+}
+}
+
+assert(bitnr < 32);
+assert(w < FEATURE_WORDS);
+return feature_word_info[w].feat_names[bitnr];
+}
+
 /* Compatibily hack to maintain legacy +-feat semantic,
  * where +-feat overwrites any feature set by
  * feat=on|feat even if the later is parsed after +-feat
@@ -2032,6 +2053,56 @@ static void x86_cpu_parse_featurestr(const char 
*typename, char *features,
 }
 }
 
+static void x86_cpu_load_features(X86CPU *cpu, Error **errp);
+
+/* Check for missing features that may prevent the CPU class from
+ * running using the current machine and accelerator.
+ */
+static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,
+ strList **missing_feats)
+{
+X86CPU *xc;
+FeatureWord w;
+Error *err = NULL;
+strList **next = missing_feats;
+
+if (xcc->kvm_required && !kvm_enabled()) {
+strList *new = g_new0(strList, 1);
+new->value = g_strdup("kvm");;
+*missing_feats = new;
+return;
+}
+
+xc = X86_CPU(object_new(object_class_get_name(OBJECT_CLASS(xcc;
+
+x86_cpu_load_features(xc, );
+if (err) {
+/* Errors at x86_cpu_load_features should never happen,
+ * but in case it does, just report the model as not
+ * runnable at all using the "type" property.
+ */
+strList *new = g_new0(strList, 1);
+new->value = g_strdup("type");
+*next = new;
+next = >next;
+}
+
+for (w = 0; w < FEATURE_WORDS; w++) {
+uint32_t filtered = xc->filtered_features[w];
+int i;
+for (i = 0; i < 32; i++) {
+if (filtered & (1UL << i)) {
+strList *new = g_new0(strList, 1);
+new->value = g_strdup(x86_cpu_feature_name(w, i));
+*next = new;
+next = >next;
+}
+}
+}
+
+object_unref(OBJECT(xc));
+}
+
 /* Print all cpuid feature names in featureset
  */
 static void listflags(FILE *f, fprintf_function print, const char **featureset)
@@ -2124,6 +2195,8 @@ static void x86_cpu_definition_entry(gpointer data, 
gpointer user_data)
 
 info = g_malloc0(sizeof(*info));
 info->name = x86_cpu_class_get_model_name(cc);
+x86_cpu_class_check_missing_features(cc, >unavailable_features);
+info->has_unavailable_features = true;
 
 entry = g_malloc0(sizeof(*entry));
 entry->value = info;
-- 
2.7.4

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

[libvirt] [PATCH v4 02/11] target-i386: List CPU models using subclass list

[Eduardo Habkost]

Instead of using the builtin_x86_defs array, use the QOM subclass
list to list CPU models on "-cpu ?" and "query-cpu-definitions".

Signed-off-by: Andreas Färber 
[ehabkost: copied code from a patch by Andreas:
 "target-i386: QOM'ify CPU", from March 2012]
Signed-off-by: Eduardo Habkost 
---
 target-i386/cpu-qom.h |   4 ++
 target-i386/cpu.c | 103 --
 2 files changed, 78 insertions(+), 29 deletions(-)

diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index 5dde658..e724004 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -63,6 +63,10 @@ typedef struct X86CPUClass {
 
 bool kvm_required;
 
+/* Optional description of CPU model.
+ * If unavailable, cpu_def->model_id is used */
+const char *model_description;
+
 DeviceRealize parent_realize;
 void (*parent_reset)(CPUState *cpu);
 } X86CPUClass;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0807e92..ac3646e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1628,6 +1628,9 @@ static void host_x86_cpu_class_init(ObjectClass *oc, void 
*data)
 cpu_x86_fill_model_id(host_cpudef.model_id);
 
 xcc->cpu_def = _cpudef;
+xcc->model_description =
+"KVM processor with all supported host features "
+"(only available in KVM mode)";
 
 /* level, xlevel, xlevel2, and the feature words are initialized on
  * instance_init, because they require KVM to be initialized.
@@ -2098,23 +2101,62 @@ static void listflags(FILE *f, fprintf_function print, 
const char **featureset)
 }
 }
 
-/* generate CPU information. */
+/* Sort alphabetically by type name, listing kvm_required models last. */
+static gint x86_cpu_list_compare(gconstpointer a, gconstpointer b)
+{
+ObjectClass *class_a = (ObjectClass *)a;
+ObjectClass *class_b = (ObjectClass *)b;
+X86CPUClass *cc_a = X86_CPU_CLASS(class_a);
+X86CPUClass *cc_b = X86_CPU_CLASS(class_b);
+const char *name_a, *name_b;
+
+if (cc_a->kvm_required != cc_b->kvm_required) {
+/* kvm_required items go last */
+return cc_a->kvm_required ? 1 : -1;
+} else {
+name_a = object_class_get_name(class_a);
+name_b = object_class_get_name(class_b);
+return strcmp(name_a, name_b);
+}
+}
+
+static GSList *get_sorted_cpu_model_list(void)
+{
+GSList *list = object_class_get_list(TYPE_X86_CPU, false);
+list = g_slist_sort(list, x86_cpu_list_compare);
+return list;
+}
+
+static void x86_cpu_list_entry(gpointer data, gpointer user_data)
+{
+ObjectClass *oc = data;
+X86CPUClass *cc = X86_CPU_CLASS(oc);
+CPUListState *s = user_data;
+char *name = x86_cpu_class_get_model_name(cc);
+const char *desc = cc->model_description;
+if (!desc) {
+desc = cc->cpu_def->model_id;
+}
+
+(*s->cpu_fprintf)(s->file, "x86 %16s  %-48s\n",
+  name, desc);
+g_free(name);
+}
+
+/* list available CPU models and flags */
 void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 {
-X86CPUDefinition *def;
-char buf[256];
 int i;
+CPUListState s = {
+.file = f,
+.cpu_fprintf = cpu_fprintf,
+};
+GSList *list;
 
-for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
-def = _x86_defs[i];
-snprintf(buf, sizeof(buf), "%s", def->name);
-(*cpu_fprintf)(f, "x86 %16s  %-48s\n", buf, def->model_id);
-}
-#ifdef CONFIG_KVM
-(*cpu_fprintf)(f, "x86 %16s  %-48s\n", "host",
-   "KVM processor with all supported host features "
-   "(only available in KVM mode)");
-#endif
+(*cpu_fprintf)(f, "Available CPUs:\n");
+list = get_sorted_cpu_model_list();
+g_slist_foreach(list, x86_cpu_list_entry, );
+g_slist_free(list);
 
 (*cpu_fprintf)(f, "\nRecognized CPUID flags:\n");
 for (i = 0; i < ARRAY_SIZE(feature_word_info); i++) {
@@ -2126,26 +2168,29 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 }
 }
 
-CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
+static void x86_cpu_definition_entry(gpointer data, gpointer user_data)
 {
-CpuDefinitionInfoList *cpu_list = NULL;
-X86CPUDefinition *def;
-int i;
+ObjectClass *oc = data;
+X86CPUClass *cc = X86_CPU_CLASS(oc);
+CpuDefinitionInfoList **cpu_list = user_data;
+CpuDefinitionInfoList *entry;
+CpuDefinitionInfo *info;
 
-for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
-CpuDefinitionInfoList *entry;
-CpuDefinitionInfo *info;
-
-def = _x86_defs[i];
-info = g_malloc0(sizeof(*info));
-info->name = g_strdup(def->name);
+info = g_malloc0(sizeof(*info));
+info->name = x86_cpu_class_get_model_name(cc);
 
-entry = g_malloc0(sizeof(*entry));
-entry->value = info;
-entry->next = cpu_list;
-cpu_list = entry;
-}
+entry = g_malloc0(sizeof(*entry));
+

[libvirt] [PATCH v4 09/11] target-i386: x86_cpu_load_features() function

[Eduardo Habkost]

When probing for CPU model information, we need to reuse the code
that initializes CPUID fields, but not the remaining side-effects
of x86_cpu_realizefn(). Move that code to a separate function
that can be reused later.

Signed-off-by: Eduardo Habkost 
---
Changes series v3 -> v4:
* New patch added to series
---
 target-i386/cpu.c | 67 +++
 1 file changed, 43 insertions(+), 24 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b25657b..e099892 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2993,34 +2993,14 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
 env->features[FEAT_XSAVE_COMP_HI] = mask >> 32;
 }
 
-#define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && \
-   (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 && \
-   (env)->cpuid_vendor3 == CPUID_VENDOR_INTEL_3)
-#define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \
- (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
- (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
-static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
+/* Load CPUID data based on configureured features
+ */
+static void x86_cpu_load_features(X86CPU *cpu, Error **errp)
 {
-CPUState *cs = CPU(dev);
-X86CPU *cpu = X86_CPU(dev);
-X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
 CPUX86State *env = >env;
-Error *local_err = NULL;
-static bool ht_warned;
 FeatureWord w;
 GList *l;
-
-if (xcc->kvm_required && !kvm_enabled()) {
-char *name = x86_cpu_class_get_model_name(xcc);
-error_setg(_err, "CPU model '%s' requires KVM", name);
-g_free(name);
-goto out;
-}
-
-if (cpu->apic_id == UNASSIGNED_APIC_ID) {
-error_setg(errp, "apic-id property was not initialized properly");
-return;
-}
+Error *local_err = NULL;
 
 /*TODO: cpu->host_features incorrectly overwrites features
  * set using "feat=on|off". Once we fix this, we can convert
@@ -3087,6 +3067,45 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
**errp)
 }
 
 x86_cpu_filter_features(cpu);
+
+out:
+if (local_err != NULL) {
+error_propagate(errp, local_err);
+}
+}
+
+#define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && \
+   (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 && \
+   (env)->cpuid_vendor3 == CPUID_VENDOR_INTEL_3)
+#define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \
+ (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
+ (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
+static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
+{
+CPUState *cs = CPU(dev);
+X86CPU *cpu = X86_CPU(dev);
+X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
+CPUX86State *env = >env;
+Error *local_err = NULL;
+static bool ht_warned;
+
+if (xcc->kvm_required && !kvm_enabled()) {
+char *name = x86_cpu_class_get_model_name(xcc);
+error_setg(_err, "CPU model '%s' requires KVM", name);
+g_free(name);
+goto out;
+}
+
+if (cpu->apic_id == UNASSIGNED_APIC_ID) {
+error_setg(errp, "apic-id property was not initialized properly");
+return;
+}
+
+x86_cpu_load_features(cpu, _err);
+if (local_err) {
+goto out;
+}
+
 if (cpu->check_cpuid || cpu->enforce_cpuid) {
 if (x86_cpu_report_filtered_features(cpu) && cpu->enforce_cpuid) {
 error_setg(_err,
-- 
2.7.4

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

[libvirt] [PATCH v4 07/11] target-i386: xsave: Add FP and SSE bits to x86_ext_save_areas

[Eduardo Habkost]

Instead of treating the FP and SSE bits as special cases, add
them to the x86_ext_save_areas array. This will simplify the code
that calculates the supported xsave components and the size of
the xsave area.

Signed-off-by: Eduardo Habkost 
---
Changes series v3 -> v4:
* New patch added to series
---
 target-i386/cpu.c | 22 ++
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c013ed0..b36388e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -535,6 +535,20 @@ typedef struct ExtSaveArea {
 } ExtSaveArea;
 
 static const ExtSaveArea x86_ext_save_areas[] = {
+[XSTATE_FP_BIT] = {
+/* x87 FP state component is always enabled if XSAVE is supported */
+.feature = FEAT_1_ECX, .bits = CPUID_EXT_XSAVE,
+/* x87 state is in the legacy region of the XSAVE area */
+.offset = 0,
+.size = sizeof(X86LegacyXSaveArea) + sizeof(X86XSaveHeader),
+},
+[XSTATE_SSE_BIT] = {
+/* SSE state component is always enabled if XSAVE is supported */
+.feature = FEAT_1_ECX, .bits = CPUID_EXT_XSAVE,
+/* SSE state is in the legacy region of the XSAVE area */
+.offset = 0,
+.size = sizeof(X86LegacyXSaveArea) + sizeof(X86XSaveHeader),
+},
 [XSTATE_YMM_BIT] =
   { .feature = FEAT_1_ECX, .bits = CPUID_EXT_AVX,
 .offset = offsetof(X86XSaveArea, avx_state),
@@ -568,9 +582,9 @@ static const ExtSaveArea x86_ext_save_areas[] = {
 static uint32_t xsave_area_size(uint64_t mask)
 {
 int i;
-uint64_t ret = sizeof(X86LegacyXSaveArea) + sizeof(X86XSaveHeader);
+uint64_t ret = 0;
 
-for (i = 2; i < ARRAY_SIZE(x86_ext_save_areas); i++) {
+for (i = 0; i < ARRAY_SIZE(x86_ext_save_areas); i++) {
 const ExtSaveArea *esa = _ext_save_areas[i];
 if ((mask >> i) & 1) {
 ret = MAX(ret, esa->offset + esa->size);
@@ -2963,8 +2977,8 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
 return;
 }
 
-mask = (XSTATE_FP_MASK | XSTATE_SSE_MASK);
-for (i = 2; i < ARRAY_SIZE(x86_ext_save_areas); i++) {
+mask = 0;
+for (i = 0; i < ARRAY_SIZE(x86_ext_save_areas); i++) {
 const ExtSaveArea *esa = _ext_save_areas[i];
 if (env->features[esa->feature] & esa->bits) {
 mask |= (1ULL << i);
-- 
2.7.4

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

[libvirt] [PATCH v4 03/11] target-i386: Disable VME by default with TCG

[Eduardo Habkost]

VME is already disabled automatically when using TCG. So, instead
of pretending it is there when reporting CPU model data on
query-cpu-* QMP commands (making every CPU model to be reported
as not runnable), we can disable it by default on all CPU models
when using TCG.

Do that by adding a tcg_default_props array that will work like
kvm_default_props.

Signed-off-by: Eduardo Habkost 
---
Changes series v3 -> v4:
* New patch added to series
---
 target-i386/cpu.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ac3646e..3d3f64e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1550,6 +1550,14 @@ static PropValue kvm_default_props[] = {
 { NULL, NULL },
 };
 
+/* TCG-specific defaults that override all CPU models when using TCG
+ */
+static PropValue tcg_default_props[] = {
+{ "vme", "off" },
+{ NULL, NULL },
+};
+
+
 void x86_cpu_change_kvm_default(const char *prop, const char *value)
 {
 PropValue *pv;
@@ -2283,6 +2291,8 @@ static void x86_cpu_load_def(X86CPU *cpu, 
X86CPUDefinition *def, Error **errp)
 }
 
 x86_cpu_apply_props(cpu, kvm_default_props);
+} else if (tcg_enabled()) {
+x86_cpu_apply_props(cpu, tcg_default_props);
 }
 
 env->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR;
-- 
2.7.4

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

[libvirt] [PATCH v4 01/11] tests: Add test case for x86 feature parsing compatibility

[Eduardo Habkost]

Add a new test case to ensure the existing behavior of the
feature parsing code wlil be kept.

Signed-off-by: Eduardo Habkost 
---
Changes series v3 -> v4:
* New patch added to series
---
 tests/test-x86-cpuid-compat.c | 39 +++
 1 file changed, 39 insertions(+)

diff --git a/tests/test-x86-cpuid-compat.c b/tests/test-x86-cpuid-compat.c
index 83162a4..7cff2b5 100644
--- a/tests/test-x86-cpuid-compat.c
+++ b/tests/test-x86-cpuid-compat.c
@@ -3,6 +3,7 @@
 #include "qapi/qmp/qlist.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qint.h"
+#include "qapi/qmp/qbool.h"
 #include "libqtest.h"
 
 static char *get_cpu0_qom_path(void)
@@ -34,6 +35,15 @@ static QObject *qom_get(const char *path, const char *prop)
 return ret;
 }
 
+static bool qom_get_bool(const char *path, const char *prop)
+{
+QBool *value = qobject_to_qbool(qom_get(path, prop));
+bool b = qbool_get_bool(value);
+
+QDECREF(value);
+return b;
+}
+
 typedef struct CpuidTestArgs {
 const char *cmdline;
 const char *property;
@@ -66,10 +76,39 @@ static void add_cpuid_test(const char *name, const char 
*cmdline,
 qtest_add_data_func(name, args, test_cpuid_prop);
 }
 
+static void test_plus_minus(void)
+{
+char *path;
+
+/* Rules:
+ * "-foo" overrides "+foo"
+ * "[+-]foo" overrides "foo=..."
+ * "foo_bar" should be translated to "foo-bar"
+ */
+qtest_start("-cpu 
pentium,-fpu,+fpu,-mce,mce=on,+cx8,cx8=off,+sse4_1,sse4_2=on");
+path = get_cpu0_qom_path();
+
+g_assert_false(qom_get_bool(path, "fpu"));
+g_assert_false(qom_get_bool(path, "mce"));
+g_assert_true(qom_get_bool(path, "cx8"));
+
+/* Test both the original and the alias feature names: */
+g_assert_true(qom_get_bool(path, "sse4-1"));
+g_assert_true(qom_get_bool(path, "sse4.1"));
+
+g_assert_true(qom_get_bool(path, "sse4-2"));
+g_assert_true(qom_get_bool(path, "sse4.2"));
+
+qtest_end();
+g_free(path);
+}
+
 int main(int argc, char **argv)
 {
 g_test_init(, , NULL);
 
+qtest_add_func("x86/cpuid/parsing-plus-minus", test_plus_minus);
+
 /* Original level values for CPU models: */
 add_cpuid_test("x86/cpuid/phenom/level",
"-cpu phenom", "level", 5);
-- 
2.7.4

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

[libvirt] [PATCH v4 00/11] Add runnability info to query-cpu-definitions

[Eduardo Habkost]

This series extends query-cpu-definitions to include an extra
field: "unavailable-features". The new field can be used to find
out reasons that prevent the CPU model from running in the
current host.

This will return information based on the current machine and
accelerator only. In the future we may extend these mechanisms to
allow querying other machines and other accelerators without
restarting QEMU, but it will require some reorganization of
QEMU's main code.

To be able to implement this more cleanly, the series rework some
of the feature/property name code.

This series can be seen in the git branch at:
  https://github.com/ehabkost/qemu-hacks.git 
work/query-cpu-definitions-runnable-info

The series is based on my x86-next branch:
   https://github.com/ehabkost/qemu.git x86-next

Changes v3 -> v4:
* Removed patch "Define CPUID filtering functions before x86_cpu_list"
* New patch: "tests: Add test case for x86 feature parsing compatibility"
* New patch: "target-i386: Disable VME by default with TCG"
  * Disable VME by default on TCG to avoid returning bogus
results for all CPU models in TCG mode
* New patch: "target-i386: Make plus_features/minus_features QOM-based"
* New patch: "target-i386: Remove underscores from property names"
* New patch: "target-i386: Register properties for feature aliases manually"
* New patch: "target-i386: xsave: Add FP and SSE bits to x86_ext_save_areas"
* New patch: "target-i386: x86_cpu_load_features() function"
* On patch: "target-i386: Return runnability information on 
query-cpu-definitions"
  * Added code to handle unsupported XSAVE components cleanly
  * Use x86_cpu_load_features() function

Changes v2 -> v3:
* Small documentation reword
  * Suggested-by: Markus Armbruster 
* Create a x86_cpu_feature_name() function, to
  isolate the code that returns the property name

Changes v1 -> v2:
* Fixed documentation to say "(since 2.7)"
* Removed @runnable field, improved documentation

Example command output:

{ "return": [
{
"unavailable-features": [],
"static": false,
"name": "host"
},
{
"unavailable-features": [],
"static": false,
"name": "qemu64"
},
{
"unavailable-features": [],
"static": false,
"name": "qemu32"
},
{
"unavailable-features": ["npt", "sse4a", "3dnow", "3dnowext", 
"fxsr-opt", "mmxext"],
"static": false,
"name": "phenom"
},
{
"unavailable-features": [],
"static": false,
"name": "pentium3"
},
{
"unavailable-features": [],
"static": false,
"name": "pentium2"
},
{
"unavailable-features": [],
"static": false,
"name": "pentium"
},
{
"unavailable-features": [],
"static": false,
"name": "n270"
},
{
"unavailable-features": [],
"static": false,
"name": "kvm64"
},
{
"unavailable-features": [],
"static": false,
"name": "kvm32"
},
{
"unavailable-features": [],
"static": false,
"name": "coreduo"
},
{
"unavailable-features": [],
"static": false,
"name": "core2duo"
},
{
"unavailable-features": ["3dnow", "3dnowext", "mmxext"],
"static": false,
"name": "athlon"
},
{
"unavailable-features": [],
"static": false,
"name": "Westmere"
},
{
"unavailable-features": ["xgetbv1", "xsavec", "3dnowprefetch", 
"smap", "adx", "rdseed", "mpx", "rtm", "hle"],
"static": false,
"name": "Skylake-Client"
},
{
"unavailable-features": [],
"static": false,
"name": "SandyBridge"
},
{
"unavailable-features": [],
"static": false,
"name": "Penryn"
},
{
"unavailable-features": ["tbm", "fma4", "xop", "3dnowprefetch", 
"misalignsse", "sse4a"],
"static": false,
"name": "Opteron_G5"
},
{
"unavailable-features": ["fma4", "xop", "3dnowprefetch", 
"misalignsse", "sse4a"],
"static": false,
"name": "Opteron_G4"
},
{
"unavailable-features": ["misalignsse", "sse4a"],
"static": false,
"name": "Opteron_G3"
},
{
"unavailable-features": [],
"static": false,
"name": "Opteron_G2"
},
{
"unavailable-features": [],
"static": false,
"name": "Opteron_G1"
},
{

[libvirt] [PATCH v4 10/11] qmp: Add runnability information to query-cpu-definitions

[Eduardo Habkost]

Add a new optional field to query-cpu-definitions schema:
"unavailable-features". It will contain a list of QOM properties
that prevent the CPU model from running in the current host.

Cc: David Hildenbrand 
Cc: Michael Mueller 
Cc: Christian Borntraeger 
Cc: Cornelia Huck 
Cc: Jiri Denemark 
Cc: libvir-list@redhat.com
Reviewed-by: Eric Blake 
Signed-off-by: Eduardo Habkost 
---
Changes v3 -> v4:
* Changed doc to "since 2.8" as we missed 2.7
  * Reported-by: Eric Blake 

Changes v2 -> v3:
* Small documentation reword
  * Suggested-by: Markus Armbruster 

Changes v1 -> v2:
* Remove @runnable field, non-empty @unavailable-features is
  enough to report CPU model as not runnable.
* Documentation updates:
  * Changed to "(since 2.7)";
  * Add more details about the exact meaning of
unavailable-features, and what it would mean to see
read-only QOM properties in the list, and that
implementations can return "type" if there's
no extra information available;
---
 qapi-schema.json | 23 ++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index c3dcf11..abb163b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3114,10 +3114,31 @@
 #  QEMU version, machine type, machine options and accelerator options.
 #  A static model is always migration-safe. (since 2.8)
 #
+# @unavailable-features: #optional List of properties that prevent
+#the CPU model from running in the current
+#host. (since 2.8)
+#
+# @unavailable-features is a list of QOM property names that
+# represent CPU model attributes that prevent the CPU from running.
+# If the QOM property is read-only, that means there's no known
+# way to make the CPU model run in the current host. Implementations
+# that choose not to provide specific information return the
+# property name "type".
+# If the property is read-write, it means that it MAY be possible
+# to run the CPU model in the current host if that property is
+# changed. Management software can use it as hints to suggest or
+# choose an alternative for the user, or just to generate meaningful
+# error messages explaining why the CPU model can't be used.
+# If @unavailable-features is an empty list, the CPU model is
+# runnable using the current host and machine-type.
+# If @unavailable-features is not present, runnability
+# information for the CPU is not available.
+#
 # Since: 1.2.0
 ##
 { 'struct': 'CpuDefinitionInfo',
-  'data': { 'name': 'str', '*migration-safe': 'bool', 'static': 'bool' } }
+  'data': { 'name': 'str', '*migration-safe': 'bool', 'static': 'bool',
+'*unavailable-features': [ 'str' ] } }
 
 ##
 # @query-cpu-definitions:
-- 
2.7.4

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

[libvirt] [PATCH v4 06/11] target-i386: Register properties for feature aliases manually

[Eduardo Habkost]

Instead of keeping the aliases inside the feature name arrays and
require parsing the strings, just register alias properties
manually. This simplifies the property registration code and will
simplify code that needs to look up property names for CPUID
bits.

Signed-off-by: Eduardo Habkost 
---
Changes series v3 -> v4:
* New patch added to series
---
 target-i386/cpu.c | 44 ++--
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7795a7c..c013ed0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -278,12 +278,12 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = 
{
 },
 [FEAT_1_ECX] = {
 .feat_names = {
-"pni|sse3" /* Intel,AMD sse3 */, "pclmulqdq|pclmuldq", "dtes64", 
"monitor",
+"pni" /* Intel,AMD sse3 */, "pclmulqdq", "dtes64", "monitor",
 "ds-cpl", "vmx", "smx", "est",
 "tm2", "ssse3", "cid", NULL,
 "fma", "cx16", "xtpr", "pdcm",
-NULL, "pcid", "dca", "sse4.1|sse4-1",
-"sse4.2|sse4-2", "x2apic", "movbe", "popcnt",
+NULL, "pcid", "dca", "sse4.1",
+"sse4.2", "x2apic", "movbe", "popcnt",
 "tsc-deadline", "aes", "xsave", "osxsave",
 "avx", "f16c", "rdrand", "hypervisor",
 },
@@ -302,9 +302,9 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
 NULL /* cx8 */, NULL /* apic */, NULL, "syscall",
 NULL /* mtrr */, NULL /* pge */, NULL /* mca */, NULL /* cmov */,
 NULL /* pat */, NULL /* pse36 */, NULL, NULL /* Linux mp */,
-"nx|xd", NULL, "mmxext", NULL /* mmx */,
-NULL /* fxsr */, "fxsr-opt|ffxsr", "pdpe1gb", "rdtscp",
-NULL, "lm|i64", "3dnowext", "3dnow",
+"nx", NULL, "mmxext", NULL /* mmx */,
+NULL /* fxsr */, "fxsr-opt", "pdpe1gb", "rdtscp",
+NULL, "lm", "3dnowext", "3dnow",
 },
 .cpuid_eax = 0x8001, .cpuid_reg = R_EDX,
 .tcg_features = TCG_EXT2_FEATURES,
@@ -3323,28 +3323,19 @@ static void x86_cpu_register_feature_bit_props(X86CPU 
*cpu,
FeatureWord w,
int bitnr)
 {
-Object *obj = OBJECT(cpu);
-int i;
-char **names;
 FeatureWordInfo *fi = _word_info[w];
+const char *name = fi->feat_names[bitnr];
 
-if (!fi->feat_names[bitnr]) {
+if (!name) {
 return;
 }
 
-names = g_strsplit(fi->feat_names[bitnr], "|", 0);
-
 /* Property names should use "-" instead of "_" */
-assert(!strchr(names[0], '_'));
-x86_cpu_register_bit_prop(cpu, names[0], >env.features[w], bitnr);
-
-for (i = 1; names[i]; i++) {
-feat2prop(names[i]);
-object_property_add_alias(obj, names[i], obj, names[0],
-  _abort);
-}
-
-g_strfreev(names);
+assert(!strchr(name, '_'));
+/* aliases don't use "|" delimiters anymore, they are registered
+ * manually using object_property_add_alias() */
+assert(!strchr(name, '|'));
+x86_cpu_register_bit_prop(cpu, name, >env.features[w], bitnr);
 }
 
 static void x86_cpu_initfn(Object *obj)
@@ -3392,6 +3383,15 @@ static void x86_cpu_initfn(Object *obj)
 }
 }
 
+/* Alias for feature properties: */
+object_property_add_alias(obj, "sse3", obj, "pni", _abort);
+object_property_add_alias(obj, "pclmuldq", obj, "pclmulqdq", _abort);
+object_property_add_alias(obj, "sse4-1", obj, "sse4.1", _abort);
+object_property_add_alias(obj, "sse4-2", obj, "sse4.2", _abort);
+object_property_add_alias(obj, "xd", obj, "nx", _abort);
+object_property_add_alias(obj, "ffxsr", obj, "fxsr-opt", _abort);
+object_property_add_alias(obj, "i64", obj, "lm", _abort);
+
 x86_cpu_load_def(cpu, xcc->cpu_def, _abort);
 }
 
-- 
2.7.4

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

[libvirt] [PATCH v4 08/11] target-i386: Move warning code outside x86_cpu_filter_features()

[Eduardo Habkost]

x86_cpu_filter_features() will be reused by code that shouldn't
print any warning. Move the warning code to a new
x86_cpu_report_filtered_features() function, and call it from
x86_cpu_realizefn().

Signed-off-by: Eduardo Habkost 
---
Changes v3 -> v4:
* Made x86_cpu_filter_features() void, make
  x86_cpu_report_filtered_features() return true if
  some features were filtered
---
 target-i386/cpu.c | 41 -
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b36388e..b25657b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2163,14 +2163,11 @@ static uint32_t 
x86_cpu_get_supported_feature_word(FeatureWord w,
 
 /*
  * Filters CPU feature words based on host availability of each feature.
- *
- * Returns: 0 if all flags are supported by the host, non-zero otherwise.
  */
-static int x86_cpu_filter_features(X86CPU *cpu)
+static void x86_cpu_filter_features(X86CPU *cpu)
 {
 CPUX86State *env = >env;
 FeatureWord w;
-int rv = 0;
 
 for (w = 0; w < FEATURE_WORDS; w++) {
 uint32_t host_feat =
@@ -2178,15 +2175,22 @@ static int x86_cpu_filter_features(X86CPU *cpu)
 uint32_t requested_features = env->features[w];
 env->features[w] &= host_feat;
 cpu->filtered_features[w] = requested_features & ~env->features[w];
-if (cpu->filtered_features[w]) {
-if (cpu->check_cpuid || cpu->enforce_cpuid) {
-report_unavailable_features(w, cpu->filtered_features[w]);
-}
-rv = 1;
-}
 }
+}
 
-return rv;
+/* Report list of filtered features to stderr.
+ * Returns true if some features were found to be filtered, false otherwise
+ */
+static bool x86_cpu_report_filtered_features(X86CPU *cpu)
+{
+FeatureWord w;
+uint32_t filtered = 0;
+
+for (w = 0; w < FEATURE_WORDS; w++) {
+filtered |= cpu->filtered_features[w];
+report_unavailable_features(w, cpu->filtered_features[w]);
+}
+return filtered;
 }
 
 static void x86_cpu_apply_props(X86CPU *cpu, PropValue *props)
@@ -3082,12 +3086,15 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
**errp)
 env->cpuid_xlevel2 = env->cpuid_min_xlevel2;
 }
 
-if (x86_cpu_filter_features(cpu) && cpu->enforce_cpuid) {
-error_setg(_err,
-   kvm_enabled() ?
-   "Host doesn't support requested features" :
-   "TCG doesn't support requested features");
-goto out;
+x86_cpu_filter_features(cpu);
+if (cpu->check_cpuid || cpu->enforce_cpuid) {
+if (x86_cpu_report_filtered_features(cpu) && cpu->enforce_cpuid) {
+error_setg(_err,
+   kvm_enabled() ?
+   "Host doesn't support requested features" :
+   "TCG doesn't support requested features");
+goto out;
+}
 }
 
 /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
-- 
2.7.4

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

[libvirt] [PATCH v4 04/11] target-i386: Make plus_features/minus_features QOM-based

[Eduardo Habkost]

Instead of using custom feature name lookup code for
plus_features/minus_features, save the property names used in
"[+-]feature" and use object_property_set_bool() to set them.

Signed-off-by: Eduardo Habkost 
---
Changes series v3 -> v4:
* New patch added to series
---
 target-i386/cpu.c | 108 +++---
 1 file changed, 22 insertions(+), 86 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3d3f64e..4eaec0e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -650,85 +650,6 @@ void host_cpuid(uint32_t function, uint32_t count,
 *edx = vec[3];
 }
 
-#define iswhite(c) ((c) && ((c) <= ' ' || '~' < (c)))
-
-/* general substring compare of *[s1..e1) and *[s2..e2).  sx is start of
- * a substring.  ex if !NULL points to the first char after a substring,
- * otherwise the string is assumed to sized by a terminating nul.
- * Return lexical ordering of *s1:*s2.
- */
-static int sstrcmp(const char *s1, const char *e1,
-   const char *s2, const char *e2)
-{
-for (;;) {
-if (!*s1 || !*s2 || *s1 != *s2)
-return (*s1 - *s2);
-++s1, ++s2;
-if (s1 == e1 && s2 == e2)
-return (0);
-else if (s1 == e1)
-return (*s2);
-else if (s2 == e2)
-return (*s1);
-}
-}
-
-/* compare *[s..e) to *altstr.  *altstr may be a simple string or multiple
- * '|' delimited (possibly empty) strings in which case search for a match
- * within the alternatives proceeds left to right.  Return 0 for success,
- * non-zero otherwise.
- */
-static int altcmp(const char *s, const char *e, const char *altstr)
-{
-const char *p, *q;
-
-for (q = p = altstr; ; ) {
-while (*p && *p != '|')
-++p;
-if ((q == p && !*s) || (q != p && !sstrcmp(s, e, q, p)))
-return (0);
-if (!*p)
-return (1);
-else
-q = ++p;
-}
-}
-
-/* search featureset for flag *[s..e), if found set corresponding bit in
- * *pval and return true, otherwise return false
- */
-static bool lookup_feature(uint32_t *pval, const char *s, const char *e,
-   const char **featureset)
-{
-uint32_t mask;
-const char **ppc;
-bool found = false;
-
-for (mask = 1, ppc = featureset; mask; mask <<= 1, ++ppc) {
-if (*ppc && !altcmp(s, e, *ppc)) {
-*pval |= mask;
-found = true;
-}
-}
-return found;
-}
-
-static void add_flagname_to_bitmaps(const char *flagname,
-FeatureWordArray words,
-Error **errp)
-{
-FeatureWord w;
-for (w = 0; w < FEATURE_WORDS; w++) {
-FeatureWordInfo *wi = _word_info[w];
-if (lookup_feature([w], flagname, NULL, wi->feat_names)) {
-break;
-}
-}
-if (w == FEATURE_WORDS) {
-error_setg(errp, "CPU feature %s not found", flagname);
-}
-}
-
 /* CPU class name definitions: */
 
 #define X86_CPU_TYPE_SUFFIX "-" TYPE_X86_CPU
@@ -2015,8 +1936,7 @@ static inline void feat2prop(char *s)
  * feat=on|feat even if the later is parsed after +-feat
  * (i.e. "-x2apic,x2apic=on" will result in x2apic disabled)
  */
-static FeatureWordArray plus_features = { 0 };
-static FeatureWordArray minus_features = { 0 };
+static GList *plus_features, *minus_features;
 
 /* Parse "+feature,-feature,feature=foo" CPU feature string
  */
@@ -2047,10 +1967,14 @@ static void x86_cpu_parse_featurestr(const char 
*typename, char *features,
 
 /* Compatibility syntax: */
 if (featurestr[0] == '+') {
-add_flagname_to_bitmaps(featurestr + 1, plus_features, _err);
+feat2prop(featurestr + 1);
+plus_features = g_list_append(plus_features,
+  g_strdup(featurestr + 1));
 continue;
 } else if (featurestr[0] == '-') {
-add_flagname_to_bitmaps(featurestr + 1, minus_features, 
_err);
+feat2prop(featurestr + 1);
+minus_features = g_list_append(minus_features,
+   g_strdup(featurestr + 1));
 continue;
 }
 
@@ -3066,6 +2990,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
**errp)
 Error *local_err = NULL;
 static bool ht_warned;
 FeatureWord w;
+GList *l;
 
 if (xcc->kvm_required && !kvm_enabled()) {
 char *name = x86_cpu_class_get_model_name(xcc);
@@ -3091,9 +3016,20 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
**errp)
 }
 }
 
-for (w = 0; w < FEATURE_WORDS; w++) {
-cpu->env.features[w] |= plus_features[w];
-cpu->env.features[w] &= ~minus_features[w];
+for (l = plus_features; l; l = l->next) {
+const char *prop = l->data;
+object_property_set_bool(OBJECT(cpu), true, prop, _err);
+if (local_err) {
+

[libvirt] [PATCH v4 05/11] target-i386: Remove underscores from property names

[Eduardo Habkost]

Instead of translating the feature name entries when adding
property names, store the actual property names in the feature
name array.

Signed-off-by: Eduardo Habkost 
---
Changes series v3 -> v4:
* New patch added to series
---
 target-i386/cpu.c | 31 ---
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 4eaec0e..7795a7c 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -279,11 +279,11 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = 
{
 [FEAT_1_ECX] = {
 .feat_names = {
 "pni|sse3" /* Intel,AMD sse3 */, "pclmulqdq|pclmuldq", "dtes64", 
"monitor",
-"ds_cpl", "vmx", "smx", "est",
+"ds-cpl", "vmx", "smx", "est",
 "tm2", "ssse3", "cid", NULL,
 "fma", "cx16", "xtpr", "pdcm",
-NULL, "pcid", "dca", "sse4.1|sse4_1",
-"sse4.2|sse4_2", "x2apic", "movbe", "popcnt",
+NULL, "pcid", "dca", "sse4.1|sse4-1",
+"sse4.2|sse4-2", "x2apic", "movbe", "popcnt",
 "tsc-deadline", "aes", "xsave", "osxsave",
 "avx", "f16c", "rdrand", "hypervisor",
 },
@@ -303,7 +303,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
 NULL /* mtrr */, NULL /* pge */, NULL /* mca */, NULL /* cmov */,
 NULL /* pat */, NULL /* pse36 */, NULL, NULL /* Linux mp */,
 "nx|xd", NULL, "mmxext", NULL /* mmx */,
-NULL /* fxsr */, "fxsr_opt|ffxsr", "pdpe1gb", "rdtscp",
+NULL /* fxsr */, "fxsr-opt|ffxsr", "pdpe1gb", "rdtscp",
 NULL, "lm|i64", "3dnowext", "3dnow",
 },
 .cpuid_eax = 0x8001, .cpuid_reg = R_EDX,
@@ -311,13 +311,13 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = 
{
 },
 [FEAT_8000_0001_ECX] = {
 .feat_names = {
-"lahf_lm", "cmp_legacy", "svm", "extapic",
+"lahf-lm", "cmp-legacy", "svm", "extapic",
 "cr8legacy", "abm", "sse4a", "misalignsse",
 "3dnowprefetch", "osvw", "ibs", "xop",
 "skinit", "wdt", NULL, "lwp",
-"fma4", "tce", NULL, "nodeid_msr",
-NULL, "tbm", "topoext", "perfctr_core",
-"perfctr_nb", NULL, NULL, NULL,
+"fma4", "tce", NULL, "nodeid-msr",
+NULL, "tbm", "topoext", "perfctr-core",
+"perfctr-nb", NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
 },
 .cpuid_eax = 0x8001, .cpuid_reg = R_ECX,
@@ -339,8 +339,8 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
 },
 [FEAT_KVM] = {
 .feat_names = {
-"kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock",
-"kvm_asyncpf", "kvm_steal_time", "kvm_pv_eoi", "kvm_pv_unhalt",
+"kvmclock", "kvm-nopiodelay", "kvm-mmu", "kvmclock",
+"kvm-asyncpf", "kvm-steal-time", "kvm-pv-eoi", "kvm-pv-unhalt",
 NULL, NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
@@ -400,9 +400,9 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
 },
 [FEAT_SVM] = {
 .feat_names = {
-"npt", "lbrv", "svm_lock", "nrip_save",
-"tsc_scale", "vmcb_clean",  "flushbyasid", "decodeassists",
-NULL, NULL, "pause_filter", NULL,
+"npt", "lbrv", "svm-lock", "nrip-save",
+"tsc-scale", "vmcb-clean",  "flushbyasid", "decodeassists",
+NULL, NULL, "pause-filter", NULL,
 "pfthreshold", NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
@@ -414,7 +414,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
 },
 [FEAT_7_0_EBX] = {
 .feat_names = {
-"fsgsbase", "tsc_adjust", NULL, "bmi1",
+"fsgsbase", "tsc-adjust", NULL, "bmi1",
 "hle", "avx2", NULL, "smep",
 "bmi2", "erms", "invpcid", "rtm",
 NULL, NULL, "mpx", NULL,
@@ -3334,7 +3334,8 @@ static void x86_cpu_register_feature_bit_props(X86CPU 
*cpu,
 
 names = g_strsplit(fi->feat_names[bitnr], "|", 0);
 
-feat2prop(names[0]);
+/* Property names should use "-" instead of "_" */
+assert(!strchr(names[0], '_'));
 x86_cpu_register_bit_prop(cpu, names[0], >env.features[w], bitnr);
 
 for (i = 1; names[i]; i++) {
-- 
2.7.4

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

Re: [libvirt] [PATCH 2/3] qemu: special error code in case of no job on cancel block job

[John Ferlan]

On 09/27/2016 05:06 AM, Nikolay Shirokovskiy wrote:
> 
> 
> On 26.09.2016 23:07, John Ferlan wrote:
>>
>>
>> On 09/07/2016 05:24 AM, Nikolay Shirokovskiy wrote:
>>> Special error code helps gracefully handle race conditions on
>>> blockjob cancelling. Consider for example pull backup. We want
>>> to stop it and in the process we cancel blockjob for some of the
>>> exported disks. But at the time this command reaches qemu blockjob
>>> fail for some reason and cancel result and result of stopping
>>> will be an error with quite unexpecte message - no such job (sort of).
>>> Instead we can detect this race and treat as correct cancelling
>>> and wait until fail event will arrive. Then we can report corect
>>> error message with some details from qemu probably.
>>>
>>> Users of domainBlockJobAbort can be affected as -2 is new possible
>>> return code. Not sure if I should stick to the original behaviour in
>>> this case.
>>> ---
>>>  src/qemu/qemu_monitor.c  |  5 +
>>>  src/qemu/qemu_monitor_json.c | 11 +++
>>>  2 files changed, 12 insertions(+), 4 deletions(-)
>>>
>>
>> I haven't looked forward to patch 3/3 yet, but perhaps it'd be better in
>> the "caller" that cares to :
>>
>> if (qemu*() < 0) {
>> virErrorPtr err = virGetLastError();
>> if (err && err->code == VIR_ERR_OPERATION_INVALID) {
>> /* Do something specific */
>> }
>> }
>>
>> Returning -2 alters virDomainBlockJobAbort expected return values and
>> well that's probably not a good idea since we don't know if some caller
>> has said (if virDomainBlockJobAbort() == -1) instead of <= -1
>>
> 
> It is common place in libvirt to have special error codes instead of 0/-1
> only (qemuMonitorJSONCheckCPUx86) and it feels bulky to have checks that
> involve virGetLastError. It looks like other places that use similar 
> construction do not have other choice because of rpc call or smth.

True - my concern was more that virDomainBlockJobAbort now has a new
possible error value of -2 that's not described.  Still just adding to
the documentation that it can now return -1 or -2 probably isn't the
best idea especially if what you're trying to do is resolve/handle an
issue for a specific case within qemu code.

Other API's within the bowels of qemu (monitor, monitor_json, etc)
returning -2 usually is somehow handled by the caller, but before
returning out into what would be API land would return what the API has
documented ({0, -1}, {true, false}, {NULL, address}, {count, 0, -1}, etc.)

Changing an external API return value is a risky proposition - works for
some instances fails for others... I've seen code that does this:

  if (public_api(arg1, arg2) == -1) {
  print some error message
  return failure
  }

even though it's been strongly suggested to do something like

   if (public_api(arg1, arg2) < 0) {
   get specific error (usually via errno) and perhaps
  make a decision based on that... but fall through to
   print some error message
   return failure
   }

> 
> I think when function spawns error when it comes to error code
> value is does not take much care as they are not really semantic
> (at least such common error as invalid operation) 
> so one day the caller can catch 'operation invalid' for different
> reason from different place on the stack.
> 
> As to virDomainBlockJobAbort it is easy to patch)
> 
> I think I should also remove reporting error from 
> qemuMonitorJSONBlockJobError 
> in case of DeviceNotActive however it is used from several places even
> like setting speed. It is legacy of commit b976165c when
> a lot of qemu commands share common error checking code. I think I'd
> better copy this code to qemuMonitorJSONBlockJobCancel and change it there
> instead of common place. What do you think?
> 

I think you need to think about all the callers of
qemuMonitorJSONBlockJobError...  I almost hate to use the word fragile
to describe things, but there is a lot of complexity involved with
respect to all the consumers. I just recall hearing about someone doing
a lot of muttering while implementing the code. The snapshot code in
particular (which you seem to have copied to a degree) has been the
source of some fairly "exotic" bug reports.

I personally don't have a lot of insight into the totality of the
blockdev* code. I'd need to spend some time to really understand things
better and provide a more clear/concise answer...


John

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

Re: [libvirt] [PATCH 1/3] qemu: store guest visible disk size from qemu monitor block info

[John Ferlan]

> 
>> a lot of exposure to that code. I do know there's still upstream qemu
>> work taking place. We really should be very careful about adding
>> anything to libvirt before the upstream work is done. In particular any
>> x-* command support.
>>
> 
> AFAIK full pull backups as well as full/incremennal push backups are
> ready to use in qemu. It is a pity that other commands that helps
> organize pull backup process still has experimental status. One day
> they drop this x- and I doubt that things will changed very radically
> in comparison to what we have today. So if with the help of community
> this patch will be ironed out libvirt will get pull backups instantly
> after qemu be full ready.
> 

FWIW: Happened to be scanning through qemu-devel today and came across
the following topic (which interested me because it's related to other
work I'm involved in):

http://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg07721.html

Anyway a few responses in there's this:

http://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg07748.html

and finally this:

http://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg07773.html

Since you do rely on blockdev-add - I'd just caution you to not rely on
something specific...


John

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

[libvirt] [PATCH] Fix Typo.

[Nitesh Konkar]

Signed-off-by: Nitesh Konkar 
---
 src/libvirt-nodedev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c
index 6ea14b3..bba9768 100644
--- a/src/libvirt-nodedev.c
+++ b/src/libvirt-nodedev.c
@@ -488,7 +488,7 @@ virNodeDeviceRef(virNodeDevicePtr dev)
  * virNodeDeviceDettach:
  * @dev: pointer to the node device
  *
- * Dettach the node device from the node itself so that it may be
+ * Detach the node device from the node itself so that it may be
  * assigned to a guest domain.
  *
  * Depending on the hypervisor, this may involve operations such
-- 
2.1.0

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

[libvirt] Malicious guests and entropy pool access risks

[bancfc]

Hello. While I've been enabling virtio-rng since it became available I 
recently understood that without restrictions a malicious guest can 
potentially starve other VMs' entropy by overusing /dev/random so I set 
the rate limit.


Another question comes to mind. Does the way virtio-rng works pose a 
security risk? - does it allow the guest to spy on the host's entropy 
pool? (If so I'll have to disable it for untrusted VMs immediately)


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

Re: [libvirt] [PATCH V2 0/4]] qemu: report actual vcpu state in virVcpuInfo

[John Ferlan]

On 09/29/2016 11:04 AM, Peter Krempa wrote:
> On Thu, Sep 29, 2016 at 10:30:07 -0400, John Ferlan wrote:
>>
>>
>> On 09/20/2016 04:10 AM, Viktor Mihajlovski wrote:
>>> Currently, the virVcpuInfo returned by virDomainGetVcpus() will always
>>> report a state of VIR_VCPU_RUNNING for each defined domain vcpu even if
>>> the vcpu is currently in the halted state.
>>>
>>> As the monitor interface is in fact reporting the accurate state, it is
>>> rather easy to transport this information with the existing API.
>>>
>>> This is done by
>>> - adding a new state of VIR_VCPU_HALTED
>>> - extending the monitor to pass back the halted state for the vcpus
>>> - adding a new field to the private domain vcpu object reflecting the
>>>   halted state for the vcpu
>>> - modifying the driver code to report the vcpu state based on the halted
>>>   indicator
>>> - extending virsh vcpuinfo to also display the halted state
>>>
>>> The vcpu state is however not recorded in the internal XML format, since
>>> the state can change asynchronously (without notification).
>>>
>>> V2 is a rebase on top of Peter Krempa's CPU hotplug modernization.
>>
>> So, I have a question based on a little bit of testing I did with one of
>> my guests and reading up on the qemu qapi-schema.json which states:
>>
>> # Notes: @halted is a transient state that changes frequently.  By the
>> time the
>> #data is sent to the client, the guest may no longer be halted.
>>
>>
>> It seems "halted" is returned whenever a vCPU is not actively processing
>> anything (or not scheduled), which I suppose is "expected" for idle
>> guests; however, if I used the vcpuinfo command and saw:
> 
> As it was pointed out in the previous posting the "halted" state has
> different meaning on certain arches. On intel it states that the cpu is
> idle and waiting. On s390 it is supposed to mean that the guest did not
> start using it yet.
> 

Hence the sly comment to document the various differences!

Seems it may be more of an "idle" or "active" detector for x86 and if it
were to be added, then a new "stats" value would be created rather than
sharing/using the existing State which is really a detector of vcpu
running or offline for most hypervisors (xen added BLOCKED).

>>
>> # virsh vcpuinfo $dom
>> VCPU:   0
>> CPU:7
>> State:  halted
>> CPU time:   84.8s
>> CPU Affinity:   
> 
> I was worried that this exact change would happen at least for x86. I
> don't think we should do this. This would become misleading to many
> users.
> 
>> ...
>>
>> I might be concerned that it's not "running" or "running (active)" vs.
>> "running (inactive)" (or paused or waiting or something to indicate not
>> actively processing/scheduled). The halted state could be the "norm".
>>
>> So is this more of a "stats" type value vs. purely an "info" type value?
>>  Also, considering hotplug differences w/ CPU's for x86, ppc, s390, and
>> arm - could this be some sort of architecture difference too (I'm using
>> x86)?
> 
> See above
> 
>>
>> Primarily I'm concerned the transient nature of the field based on
>> whether something is scheduled for the thread could lead to some
>> "erroneous" bug reports especially since "running" may not be the
>> dominant state any more.
> 
> I fully agree. This does not seem to be a good thing to do mainly
> because of the x86 implications. I don't think that the benefit for s390
> hotplug would outweigh the semantics change for the running state in
> any way.
>

Again - if something were to be added, then it's a stats only value.
That value would need to be well described for the various cpu
architectures (and it'd be qemu biased). Cannot extend _virVcpuInfo
since it's allocated as a contiguous array and who's to say which side
has which version.


John

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

[libvirt] [PATCH v3 19/18] [RFC] qemu: assure there are always at least 4 open pcie-root-ports 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 domains
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 coldplug (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 this (legacy PCI) case, 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. Since
that condition can only be reached by manual intervention, this is
acceptable.

For machinetypes with pcie-root, 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 those 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 fake slots before the useful slots, it
can lead to errors.

Instead this patch just reserves slots for "fake" devices after doing
the assignment for actual devices - if there were already enough
unoccupied pcie-root-ports, then none will be added; if not, then
enough will be added to support the desired (hardcoded) potential
number of hotplugged devices.

Since there hasn't been any other concrete suggestion for the number
of "available hotpluggable slots" libvirt should assure, this patch
uses "4" as the answer (thanks Drew!) Alternatives could be:

0 - hotplug would only work if the user had thought to add an extra
pcie-root to the config *as an extra step after adding and saving each
new device*. (this would preclude creating a new domain that had a
pcie-root-port available for hotplug - the extra modify/save step
would be needed even during initial domain creation, and would need to
be repeated every time a device was added to the domain)

1 - this is the *minimum* number of hotpluggable slots libvirt
guarantees for legacy PCI domains (with pci-root). Of course on
average these domains are more likely to have *15* slots available
(half of the slots on a pci-bridge).

"anything else" - really any choice made for this is going to be
considered wrong by *somebody*. I hope we can all agree that "0" is a
wrong choice, just because it will require so much ongoing
babysitting.

A couple of us have brought up the idea of having
"availableHotplugSlots" be configurable in the domain. There has also
been sentiment against that. Perhaps it could be configurable in
qemu.conf? (I don't really like that either, though - I think
everything about the domain should be self-contained in the domain
XML, and if something is tunable, it should be tunable separately for
each domain).

In the absense of anything configurable, we will need to pick a number
though. I've done that here, and now we can argue about it (or not :-)
---
 src/qemu/qemu_domain_address.c | 27 ++
 .../qemuxml2argv-aarch64-video-virtio-gpu-pci.args |  4 +++
 ...l2argv-aarch64-virt-2.6-virtio-pci-default.args |  4 +++
 .../qemuxml2argv-aarch64-virtio-pci-default.args   |  4 +++
 ...l2argv-aarch64-virtio-pci-manual-addresses.args |  4 +++
 .../qemuxml2argv-bios-nvram-secure.args|  4 +++
 .../qemuxml2argv-boot-floppy-q35.args  |  4 +++
 .../qemuxml2argv-bootindex-floppy-q35.args |  4 +++
 .../qemuxml2argvdata/qemuxml2argv-intel-iommu.args |  4 +++
 .../qemuxml2argv-machine-smm-opt.args  |  4 +++
 .../qemuxml2argv-pcie-expander-bus.args|  4 +++
 .../qemuxml2argv-pcie-root-port.args   |  2 ++
 tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args |  6 +++-
 .../qemuxml2argv-pcie-switch-upstream-port.args|  4 +++
 .../qemuxml2argv-pcihole64-q35.args|  4 +++
 .../qemuxml2argv-q35-default-devices-only.args |  4 +++
 .../qemuxml2argv-q35-pcie-autoadd.args |  4 +++
 

Re: [libvirt] Cpu Modeling

[Eduardo Habkost]

On Thu, Sep 29, 2016 at 04:21:07PM +0200, Jiri Denemark wrote:
[...]
> > > Slightly related, I don't think we have a way to list CPU features
> > > supported by QEMU over QMP, do we? "-cpu ?" will show them all, but I
> > > couldn't find a QMP command that would give me the same list.
> > 
> > device-list-properties should return all the properties you can
> > set. But I recommend that you don't make libvirt set any of the
> > properties in the list unless: 1) you know what it does; or 2) it
> > is returned by a query-cpu-model-expansion call.
> 
> The use case was for pre-checking whether all CPU features specified in
> domain XML are supported by current QEMU binary. What parameters would
> I need to use for device-list-properties? And would it work with
> -machine none?

You just need the right QOM type name (in x86 it is
"-x86_64-cpu" or "-i386-cpu"). I will send a patch
that includes a "qom-type" field in CpuDefinitionInfo
(query-cpu-definitions) so you know what's the right QOM type
name to query.

I think device-list-properties won't work with an abstract base
type like "x86_64-cpu" or "i386-cpu". But I think it's better
query the right class name for the CPU model you plan to use,
anyway (this way architecture code will be able to have different
CPU models supporting different sets of options, if necessary).

-- 
Eduardo

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

Re: [libvirt] [PATCH] docs: correct version requirements for

[John Ferlan]

On 09/29/2016 01:50 PM, Laine Stump wrote:
> When support was added for the kvm hidden='on' attribute in commit
> d07116, the version requirement was listed as "2.1.0 (QEMU
> only)". However, this was added when libvirt was at version 1.2.8 - it
> is *QEMU* that must be at version 2.1.0 or later.
> 
> This went unnoticed for a very long time (over 2 years). Then a week
> or two ago a new Windows convert in the #virt channel on OFTC was told
> he needed to use this feature (to prevent nvidia drivers in a guest
> from refusing to work due to being run in a virtual machine). There
> was some problem with it being recognized and "someone" (it may have
> been me, or may have been someone else, I don't remember) pointed out
> that the documentation at
> 
>   http://www.libvirt.org/formatdomain.html
> 
> says that it requires libvirt 2.1.0. The next several days were filled
> with agony as a new convert to Linux first tried to upgrade a Linux
> Mint host running their "LTS" version to something newer, then tried
> to install a libvirt build built for Ubuntu onto this, and later back
> to the old LTS Linux Mint. After this he tried building his own
> libvirt from source (with all the expected problems), and finally
> switched to Fedora. In the end it was hours and hours of everybody's
> lives that they will never get back. To now learn that he didn't need
> to do this (his original libvirt version was 1.3.3, so whatever his
> problem was, it was elsewhere) makes the pain all that much worse.
> 
> To prevent this from happening again, this simple patch changes the
> version requirement for the kvm hidden attribute from "2.1.0 (QEMU
> only)" to "1.2.8 (QEMU 2.1.0)".
> ---
>  docs/formatdomain.html.in | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

ACK (safe)


John

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

[libvirt] [PATCH] docs: correct version requirements for

[Laine Stump]

When support was added for the kvm hidden='on' attribute in commit
d07116, the version requirement was listed as "2.1.0 (QEMU
only)". However, this was added when libvirt was at version 1.2.8 - it
is *QEMU* that must be at version 2.1.0 or later.

This went unnoticed for a very long time (over 2 years). Then a week
or two ago a new Windows convert in the #virt channel on OFTC was told
he needed to use this feature (to prevent nvidia drivers in a guest
from refusing to work due to being run in a virtual machine). There
was some problem with it being recognized and "someone" (it may have
been me, or may have been someone else, I don't remember) pointed out
that the documentation at

  http://www.libvirt.org/formatdomain.html

says that it requires libvirt 2.1.0. The next several days were filled
with agony as a new convert to Linux first tried to upgrade a Linux
Mint host running their "LTS" version to something newer, then tried
to install a libvirt build built for Ubuntu onto this, and later back
to the old LTS Linux Mint. After this he tried building his own
libvirt from source (with all the expected problems), and finally
switched to Fedora. In the end it was hours and hours of everybody's
lives that they will never get back. To now learn that he didn't need
to do this (his original libvirt version was 1.3.3, so whatever his
problem was, it was elsewhere) makes the pain all that much worse.

To prevent this from happening again, this simple patch changes the
version requirement for the kvm hidden attribute from "2.1.0 (QEMU
only)" to "1.2.8 (QEMU 2.1.0)".
---
 docs/formatdomain.html.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 7008005..513bc0e 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1703,7 +1703,7 @@
   hidden
   Hide the KVM hypervisor from standard MSR based discovery
   on, off
-  2.1.0 (QEMU only)
+  1.2.8 (QEMU 2.1.0)
 
   
   
-- 
2.7.4

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

Re: [libvirt] [PATCH 0/4] vbox: address thread-safety issues.

[Dawid Zamirski]

On Wed, 2016-09-28 at 13:41 -0400, Dawid Zamirski wrote:
> This patch series solves (at least in my testing) vbox driver
> thread-safety issues that were also outlined on libvirt-users ML [1]
> and I was affected with. 

Just to give a more practical context on the issue I'm trying to solve
with this series, given this simple example code:

===

#define _GNU_SOURCE
#include 
#include 
#include 
#include 
#include 

const char *xml_tmpl = ""
""
"%s"
"3072"
"2"
""
"hvm"
""
""
""
""
""
""
""
""
""
""
""
""
""
""
""
"";

int main(int argc, char *argv[]) {
virConnectPtr conn;
char *vm_xml;
int ret = 0;

if (argc != 2) {
fprintf(stderr, "Please specify VM name as first argument\n");

return 1;
}

conn = virConnectOpen("vbox:///session");

if (conn == NULL) {
fprintf(stderr, "Failed to open connection to
vbox:///session\n");

return 1;
} else {
printf("Connected to vbox successfully...\n");
}

printf("Sleeping for 5s...\n");

sleep(5);
 
if (asprintf(_xml, xml_tmpl, argv[1]) == -1) {
fprintf(stderr, "Failed to allocate memory for VM XML
definition string\n");
ret = 1;

goto cleanup;
}

printf("Defining domain...\n");

virDomainPtr dom = virDomainDefineXML(conn, vm_xml);

if (dom == NULL) {
const char *error = virGetLastErrorMessage();
fprintf(stderr, "Failed to define domain: %s\n", error);
ret = 1;

goto cleanup;
}

printf("Domain defined successfully.\n");
sleep(1);
printf("Undefining domain...\n");

if (virDomainUndefine(dom) == -1) {
const char *error = virGetLastErrorMessage();
fprintf(stderr, "Failed to undefine domain %s\n", error);
fprintf(stderr, "This may require cleanup of the VBOX home dir
from stale template files\n");
ret = 1;

goto cleanup;
}

printf("Domain undefined successfully.\n");

 cleanup:
printf("Closing vbox connection...\n");
virConnectClose(conn);

printf("Done\n");

free(vm_xml);
vm_xml = NULL;

return ret;
}



After compiling (to say, vbox-test):

1. ./vbox-test vm1 # creates/destroys vbox VM named "vm1"
2. while #1 is sleeping, ./vbox-test vm2 # will attempt to do the same
for "vm2" by spawning concurrent vbox connection.
3. result: the 2nd process will segfault libvirtd

with those patches applied both succeed as expected.

Regards,
Dawid

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

[libvirt] [PATCH v2]Pre-assign memory modules slot number and assign alias.

[Nitesh Konkar]

Find the smallest missing slot number and
pre-assign slot numbers and assign aliases
of memory modules. This keeps the slot number
consistent with the alias.

Signed-off-by: Nitesh Konkar 
---
 src/qemu/qemu_alias.c   | 54 +++--
 src/qemu/qemu_command.c |  5 -
 2 files changed, 47 insertions(+), 12 deletions(-)

diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index cc83fec..cdca69a 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -331,23 +331,49 @@ qemuAssignDeviceRNGAlias(virDomainDefPtr def,
 return 0;
 }
 
+static int
+qemuGetSmallestSlotIdx(virDomainDefPtr def)
+{
+size_t i;
+int idx = 0;
+int minidx = 0;
+bool check[100] = {false};
+
+/* Find the missing slot */
+for (i = 0; i < def->nmems; i++) {
+idx = qemuDomainDeviceAliasIndex(>mems[i]->info, "dimm");
+check[idx] = true;
+}
+
+for (i = 0; i < def->nmems; i++) {
+if (!check[i]) {
+minidx = i;
+break;
+}
+}
+
+if (i >= def->nmems)
+   minidx = i;
+
+return minidx;
+}
 
 int
 qemuAssignDeviceMemoryAlias(virDomainDefPtr def,
 virDomainMemoryDefPtr mem)
 {
-size_t i;
-int maxidx = 0;
-int idx;
+int minidx;
 
-for (i = 0; i < def->nmems; i++) {
-if ((idx = qemuDomainDeviceAliasIndex(>mems[i]->info, "dimm")) >= 
maxidx)
-maxidx = idx + 1;
-}
+if (mem->info.addr.dimm.base)
+minidx = mem->info.addr.dimm.slot;
+else
+minidx = qemuGetSmallestSlotIdx(def);
 
-if (virAsprintf(>info.alias, "dimm%d", maxidx) < 0)
+if (virAsprintf(>info.alias, "dimm%d", minidx) < 0)
 return -1;
 
+mem->info.addr.dimm.slot = minidx;
+
 return 0;
 }
 
@@ -381,7 +407,6 @@ qemuAssignDeviceShmemAlias(virDomainDefPtr def,
 return 0;
 }
 
-
 int
 qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
 {
@@ -475,8 +500,15 @@ qemuAssignDeviceAliases(virDomainDefPtr def, 
virQEMUCapsPtr qemuCaps)
 return -1;
 }
 for (i = 0; i < def->nmems; i++) {
-if (virAsprintf(>mems[i]->info.alias, "dimm%zu", i) < 0)
-return -1;
+def->mems[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM;
+if (def->mems[i]->info.addr.dimm.base) {
+if (virAsprintf(>mems[i]->info.alias, "dimm%d", 
def->mems[i]->info.addr.dimm.slot) < 0)
+return -1;
+} else {
+def->mems[i]->info.addr.dimm.slot = i;
+if (virAsprintf(>mems[i]->info.alias, "dimm%zu", i) < 0)
+return -1;
+}
 }
 
 return 0;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index f24a98b..e236918 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3469,8 +3469,11 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem)
   mem->info.alias, mem->info.alias);
 
 if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) {
+if (mem->info.addr.dimm.base)
+virBufferAsprintf(, ",addr=%llu", 
mem->info.addr.dimm.base);
+
 virBufferAsprintf(, ",slot=%d", mem->info.addr.dimm.slot);
-virBufferAsprintf(, ",addr=%llu", mem->info.addr.dimm.base);
+
 }
 
 break;
-- 
1.9.3

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

Re: [libvirt] Cpu Modeling

[Jiri Denemark]

On Fri, Sep 23, 2016 at 15:51:12 -0400, Jason J. Herne wrote:
> On 09/23/2016 08:05 AM, Jiri Denemark wrote:
> > The host-model part of the XML will show the result of
> > query-cpu-model-expansion on "host" model, or the result of querying the
> > hardware directly if we can't ask QEMU for that (which is the current
> > state).
> 
> So the expectation here is that virsh capabilities" reports actual host
> cpu data. So for S390, we can leave our implementation of this "as-is"
> and not report any features here, I'm guessing.

Yes.

> And the  section from virsh domcapabilities will contain the Qemu
> specific supported cpu modeling info. As you stated  name='host-model'> will contain the name (and possibly feature
> details?) of the model returned by qmp query-model-expansion on
> 'host'.

Right, the host-model part should basically contain the CPU
configuration which libvirt will use if a user asks for host-model. For
example (on x86_64, since I have no experience with s390), the following
XML


  Skylake-Client
  Intel
  
  


would result in "-cpu Skylake-Client,+vmx,+tsc_adjust" QEMU command line.

> Furthermore, the  section will list all supported
> model names, as indicated by qmp query-cpu-definitions. Do I have it
> right?

Mostly, the list of supported CPUs is filtered by libvirt so on some
architectures, the  section will list fewer models
than what you can get from QEMU.

> >> 2. virsh cpu-models {arch} will also use a Qemu invocation to gather
> >> cpu model data.
> >
> > No, virsh cpu-models will just list CPU models supported by libvirt
> 
> So, in other words we just spit out whatever models Libvirt managed to
> grab, and cache, from a call to qmp query-cpu-definitions?

No. CPU models supported by libvirt are listed in cpu_map.xml. So we
have two basic sets of CPU models:

- A: models returned query-cpu-definitions
- B: models found in cpu_map.xml

Various interfaces report different sets:
- virsh cpu-models reports B
-  section of domain capabilities XML reports A ∩ B

> > (or an empty list if libvirt supports all models supported by QEMU).
> 
> Can you clarify?

This is a special case of B being empty, which means libvirt does not
filter CPU models and any model supported by QEMU can be used with
libvirt. In this case,  section of domain
capabilities XML will contain A. But virsh cpu-models will report an
empty list.

...
> >> 4. virsh cpu-baseline and cpu-compare will now invoke qemu directly as
> >> well.
> >
> > Perhaps, but before we can do that, we'll probably need to come up with
> > new APIs. It don't think it's critical, though.
> 
> Do you mind elaborating on this a bit? Which APIs do we want to look at
> here? Are you talking about new commands/calls to replace cpu-baseline
> and cpu-compare?

The virConnectCompareCPU, virConnectBaselineCPU don't have enough
parameters to be really useful. We'd need them to take more data (such
as emulator binary, domain type, and machine type) into account when
computing the results. But since we now provide CPU related data in
domain capabilities, I don't think it's critical to create new public
APIs for comparing CPUs. I think we may focus on the internals first.

Jirka

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

Re: [libvirt] [PATCH 3/7] qemu: process: Enforce 'vcpu' order range to <1, maxvcpus>

[Pavel Hrdina]

On Wed, Sep 21, 2016 at 01:49:55PM +0200, Peter Krempa wrote:
> The current code that validates duplicate vcpu order would not work
> properly if the order would exceed def->maxvcpus. Limit the order to the
> interval described.
> ---
>  src/qemu/qemu_process.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

ACK and safe for freeze.

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 2/7] qemu: process: Don't use shifted indexes for vcpu order verification

[Pavel Hrdina]

On Wed, Sep 21, 2016 at 01:49:54PM +0200, Peter Krempa wrote:
> Allocate a one larger bitmap rather than shifting the indexes back to
> zero.
> ---
>  src/qemu/qemu_process.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

ACK and safe for freeze.

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/7] qemu: process: Fix off-by-one in vcpu order duplicate error message

[Pavel Hrdina]

On Wed, Sep 21, 2016 at 01:49:53PM +0200, Peter Krempa wrote:
> The bitmap indexes for the order duplicate check are shifted to 0 since
> vcpu order 0 is not allowed. The error message doesn't need such
> treating though.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1370360
> ---
>  src/qemu/qemu_process.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

ACK and safe for freeze.

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 0/3] Fix io_timeout for sanlock

[Michal Privoznik]

On 29.09.2016 16:50, John Ferlan wrote:
> 
> 
> On 09/29/2016 10:06 AM, Michal Privoznik wrote:
>> On 15.09.2016 16:35, Michal Privoznik wrote:
>>> Just read the 3/3. I didn't know whether I should laugh or cry. I did both.
>>>
>>> Michal Privoznik (3):
>>>   lock_driver_sanlock: Avoid global driver variable whenever possible
>>>   m4: Check for sanlock_write_lockspace
>>>   sanlock: Properly init io_timeout
>>>
>>>  m4/virt-sanlock.m4| 14 +---
>>>  src/locking/lock_driver_sanlock.c | 69 
>>> +--
>>>  2 files changed, 60 insertions(+), 23 deletions(-)
>>>
>>
>> Thanks John for ACKing the series. I'm not quite sure whether I can push
>> it now that we are in the freeze. I mean it can be viewed as a bug fix,
>> so I'm inclined to push it. What are your thoughts?
>>
> 
> From the aspect of you have a legitimate bug that's causing a feature to
> not work properly, sure I agree. From the aspect of I've pushed
> something during a freeze before and was told I shouldn't have, maybe
> I'm not the best person to ask ;-).
> 
> BTW: I understand your point about not wanting to document a specific
> version since it's possible (I suppose) that the API could be backported
> to some earlier release (not that we'd ever do that).
> 
> Still, I'm reacting to a feature someone may have thought was working
> that suddenly will cause a failure to start a domain if they don't have
> the new API, but did have the old API. At least a running guest won't
> "go missing". IOW: how can we inform the user that the minimum version
> we expected now changed because of some change in an underlying package
> we depend on.

Well, I guess they will find out as soon they try to start a domain. If
they are running old sanlock (which they shouldn't at all - we advocate
for using our virlockd), they'll see a sensible error message:

+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("unable to use io_timeout with this version of
sanlock"));

> 
> All that said - push and say sorry later ;-)

Done :-)

Thank you.

Michal

> 
> John
> 

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

Re: [libvirt] [PATCH V2 0/4]] qemu: report actual vcpu state in virVcpuInfo

[Peter Krempa]

On Thu, Sep 29, 2016 at 10:30:07 -0400, John Ferlan wrote:
> 
> 
> On 09/20/2016 04:10 AM, Viktor Mihajlovski wrote:
> > Currently, the virVcpuInfo returned by virDomainGetVcpus() will always
> > report a state of VIR_VCPU_RUNNING for each defined domain vcpu even if
> > the vcpu is currently in the halted state.
> > 
> > As the monitor interface is in fact reporting the accurate state, it is
> > rather easy to transport this information with the existing API.
> > 
> > This is done by
> > - adding a new state of VIR_VCPU_HALTED
> > - extending the monitor to pass back the halted state for the vcpus
> > - adding a new field to the private domain vcpu object reflecting the
> >   halted state for the vcpu
> > - modifying the driver code to report the vcpu state based on the halted
> >   indicator
> > - extending virsh vcpuinfo to also display the halted state
> > 
> > The vcpu state is however not recorded in the internal XML format, since
> > the state can change asynchronously (without notification).
> > 
> > V2 is a rebase on top of Peter Krempa's CPU hotplug modernization.
> 
> So, I have a question based on a little bit of testing I did with one of
> my guests and reading up on the qemu qapi-schema.json which states:
> 
> # Notes: @halted is a transient state that changes frequently.  By the
> time the
> #data is sent to the client, the guest may no longer be halted.
> 
> 
> It seems "halted" is returned whenever a vCPU is not actively processing
> anything (or not scheduled), which I suppose is "expected" for idle
> guests; however, if I used the vcpuinfo command and saw:

As it was pointed out in the previous posting the "halted" state has
different meaning on certain arches. On intel it states that the cpu is
idle and waiting. On s390 it is supposed to mean that the guest did not
start using it yet.

> 
> # virsh vcpuinfo $dom
> VCPU:   0
> CPU:7
> State:  halted
> CPU time:   84.8s
> CPU Affinity:   

I was worried that this exact change would happen at least for x86. I
don't think we should do this. This would become misleading to many
users.

> ...
> 
> I might be concerned that it's not "running" or "running (active)" vs.
> "running (inactive)" (or paused or waiting or something to indicate not
> actively processing/scheduled). The halted state could be the "norm".
> 
> So is this more of a "stats" type value vs. purely an "info" type value?
>  Also, considering hotplug differences w/ CPU's for x86, ppc, s390, and
> arm - could this be some sort of architecture difference too (I'm using
> x86)?

See above

> 
> Primarily I'm concerned the transient nature of the field based on
> whether something is scheduled for the thread could lead to some
> "erroneous" bug reports especially since "running" may not be the
> dominant state any more.

I fully agree. This does not seem to be a good thing to do mainly
because of the x86 implications. I don't think that the benefit for s390
hotplug would outweigh the semantics change for the running state in
any way.

Peter


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

Re: [libvirt] [PATCH 0/3] Fix io_timeout for sanlock

[John Ferlan]

On 09/29/2016 10:06 AM, Michal Privoznik wrote:
> On 15.09.2016 16:35, Michal Privoznik wrote:
>> Just read the 3/3. I didn't know whether I should laugh or cry. I did both.
>>
>> Michal Privoznik (3):
>>   lock_driver_sanlock: Avoid global driver variable whenever possible
>>   m4: Check for sanlock_write_lockspace
>>   sanlock: Properly init io_timeout
>>
>>  m4/virt-sanlock.m4| 14 +---
>>  src/locking/lock_driver_sanlock.c | 69 
>> +--
>>  2 files changed, 60 insertions(+), 23 deletions(-)
>>
> 
> Thanks John for ACKing the series. I'm not quite sure whether I can push
> it now that we are in the freeze. I mean it can be viewed as a bug fix,
> so I'm inclined to push it. What are your thoughts?
> 

>From the aspect of you have a legitimate bug that's causing a feature to
not work properly, sure I agree. From the aspect of I've pushed
something during a freeze before and was told I shouldn't have, maybe
I'm not the best person to ask ;-).

BTW: I understand your point about not wanting to document a specific
version since it's possible (I suppose) that the API could be backported
to some earlier release (not that we'd ever do that).

Still, I'm reacting to a feature someone may have thought was working
that suddenly will cause a failure to start a domain if they don't have
the new API, but did have the old API. At least a running guest won't
"go missing". IOW: how can we inform the user that the minimum version
we expected now changed because of some change in an underlying package
we depend on.

All that said - push and say sorry later ;-)

John

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

Re: [libvirt] summary of current vfio mdev upstreaming status

[Tian, Kevin]

> From: Neo Jia [mailto:c...@nvidia.com]
> Sent: Thursday, September 29, 2016 5:17 PM
> 
> On Thu, Sep 29, 2016 at 04:55:39PM +0800, Jike Song wrote:
> > Hi all,
> >
> > In order to have a clear understanding about the VFIO mdev upstreaming
> > status, I'd like to summarize it. Please share your opinions on this,
> > and correct my misunderstandings.
> >
> > The whole vfio mdev series can be logically divided into several parts,
> > they work together to provide the mdev support.
> 
> Hi Jike,
> 
> Thanks for summarizing this, but I will defer to Kirti to comment on the 
> actual
> upstream status of her patches, couples things to note though:
> 
> 1) iommu type1 patches have been extensively reviewed by Alex already and we
> have one action item left to implement which is already queued up in v8 
> patchset.
> 
> 2) regarding the sysfs interface and libvirt discussion, I would like to hear
> what kind of attributes Intel folks are having so far as Daniel is
> asking about adding a class "gpu" which will pull several attributes as 
> mandatory.
> 

I have replied to Daniel that Intel doesn't plan to support those attributes. 
We think default mdev attributes are enough for our requirements. You
may put them into 'description' attribute for informative purpose...

If both sides agree this assumption, along with Kirti's reply that you don't
require grouping mdev any more, it'd be more promising of pushing whole
mdev bits to upstream then. :-)

Thanks
Kevin

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

Re: [libvirt] [Qemu-devel] [RFC v2] libvirt vGPU QEMU integration

[Tian, Kevin]

> From: Daniel P. Berrange [mailto:berra...@redhat.com]
> Sent: Thursday, September 29, 2016 10:39 PM
> 
> On Thu, Sep 29, 2016 at 02:35:48PM +, Tian, Kevin wrote:
> > > From: Daniel P. Berrange [mailto:berra...@redhat.com]
> > > Sent: Thursday, September 29, 2016 4:06 PM
> > >
> > > On Wed, Sep 28, 2016 at 12:48:33PM -0700, Neo Jia wrote:
> > > > On Tue, Sep 20, 2016 at 10:47:53AM +0100, Daniel P. Berrange wrote:
> > > > > On Tue, Sep 20, 2016 at 02:05:52AM +0530, Kirti Wankhede wrote:
> > > > > >
> > > > > > Hi libvirt experts,
> > > > > >
> > > > > > Thanks for valuable input on v1 version of RFC.
> > > > > >
> > > > > > Quick brief, VFIO based mediated device framework provides a way to
> > > > > > virtualize their devices without SR-IOV, like NVIDIA vGPU, Intel 
> > > > > > KVMGT
> > > > > > and IBM's channel IO. This framework reuses VFIO APIs for all the
> > > > > > functionalities for mediated devices which are currently being used 
> > > > > > for
> > > > > > pass through devices. This framework introduces a set of new sysfs 
> > > > > > files
> > > > > > for device creation and its life cycle management.
> > > > > >
> > > > > > Here is the summary of discussion on v1:
> > > > > > 1. Discover mediated device:
> > > > > > As part of physical device initialization process, vendor driver 
> > > > > > will
> > > > > > register their physical devices, which will be used to create 
> > > > > > virtual
> > > > > > device (mediated device, aka mdev) to the mediated framework.
> > > > > >
> > > > > > Vendor driver should specify mdev_supported_types in directory 
> > > > > > format.
> > > > > > This format is class based, for example, display class directory 
> > > > > > format
> > > > > > should be as below. We need to define such set for each class of 
> > > > > > devices
> > > > > > which would be supported by mediated device framework.
> > > > > >
> > > > > >  --- mdev_destroy
> > > > > >  --- mdev_supported_types
> > > > > >  |-- 11
> > > > > >  |   |-- create
> > > > > >  |   |-- name
> > > > > >  |   |-- fb_length
> > > > > >  |   |-- resolution
> > > > > >  |   |-- heads
> > > > > >  |   |-- max_instances
> > > > > >  |   |-- params
> > > > > >  |   |-- requires_group
> > > > > >  |-- 12
> > > > > >  |   |-- create
> > > > > >  |   |-- name
> > > > > >  |   |-- fb_length
> > > > > >  |   |-- resolution
> > > > > >  |   |-- heads
> > > > > >  |   |-- max_instances
> > > > > >  |   |-- params
> > > > > >  |   |-- requires_group
> > > > > >  |-- 13
> > > > > >  |-- create
> > > > > >  |-- name
> > > > > >  |-- fb_length
> > > > > >  |-- resolution
> > > > > >  |-- heads
> > > > > >  |-- max_instances
> > > > > >  |-- params
> > > > > >  |-- requires_group
> > > > > >
> > > > > >
> > > > > > In the above example directory '11' represents a type id of mdev 
> > > > > > device.
> > > > > > 'name', 'fb_length', 'resolution', 'heads', 'max_instance' and
> > > > > > 'requires_group' would be Read-Only files that vendor would provide 
> > > > > > to
> > > > > > describe about that type.
> > > > > >
> > > > > > 'create':
> > > > > > Write-only file. Mandatory.
> > > > > > Accepts string to create mediated device.
> > > > > >
> > > > > > 'name':
> > > > > > Read-Only file. Mandatory.
> > > > > > Returns string, the name of that type id.
> > > > >
> > > > > Presumably this is a human-targetted title/description of
> > > > > the device.
> > > > >
> > > > > >
> > > > > > 'fb_length':
> > > > > > Read-only file. Mandatory.
> > > > > > Returns {K,M,G}, size of framebuffer.
> > > > > >
> > > > > > 'resolution':
> > > > > > Read-Only file. Mandatory.
> > > > > > Returns 'hres x vres' format. Maximum supported resolution.
> > > > > >
> > > > > > 'heads':
> > > > > > Read-Only file. Mandatory.
> > > > > > Returns integer. Number of maximum heads supported.
> > > > >
> > > > > None of these should be mandatory as that makes the mdev
> > > > > useless for non-GPU devices.
> > > > >
> > > > > I'd expect to see a 'class' or 'type' attribute in the
> > > > > directory whcih tells you what kind of mdev it is. A
> > > > > valid 'class' value would be 'gpu'. The fb_length,
> > > > > resolution, and heads parameters would only be mandatory
> > > > > when class==gpu.
> > > > >
> > > >
> > > > Hi Daniel,
> > > >
> > > > Here you are proposing to add a class named "gpu", which will make all 
> > > > those gpu
> > > > related attributes mandatory, which libvirt can allow user to better
> > > > parse/present a particular mdev configuration?
> > > >
> > > > I am just wondering if there is another option that we just make all 
> > > > those
> > > > attributes that a mdev device can have as optional but still meaningful 
> > > > to
> > > > libvirt, so libvirt can still parse / recognize them as an class "mdev".
> > >
> > > 'mdev' isn't a class - mdev is 

Re: [libvirt] [PATCH V2 4/4] qemu: Ensure reported VCPU state is current in driver API

[John Ferlan]

On 09/20/2016 04:11 AM, Viktor Mihajlovski wrote:
> Refresh the VCPU halted states in API functions returning domain
> VCPU state information to make sure it's current. This affects
> qemuDomainGetVcpus and  qemuDomainGetStatsVcpu
> 
> Signed-off-by: Viktor Mihajlovski 
> Reviewed-by: Bjoern Walk 
> Reviewed-by: Hao QingFeng 
> Signed-off-by: Boris Fiuczynski 
> ---
>  src/qemu/qemu_driver.c | 23 +--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index e29180d..7105d26 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1478,13 +1478,17 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm,
>  virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(vm->def, i);
>  pid_t vcpupid = qemuDomainGetVcpuPid(vm, i);
>  virVcpuInfoPtr vcpuinfo = info + ncpuinfo;
> +bool vcpuhalted = qemuDomainGetVcpuHalted(vm, i);
>  
>  if (!vcpu->online)
>  continue;
>  
>  if (info) {
>  vcpuinfo->number = i;
> -vcpuinfo->state = VIR_VCPU_RUNNING;
> +if (vcpuhalted)
> +vcpuinfo->state = VIR_VCPU_HALTED;

And this causes the client to see "halted" even though the vcpu may be
running, but just not busy.

Also if (vm->def->virtType == VIR_DOMAIN_VIRT_QEMU), then we'll always
be halted since qemuDomainRefreshVcpuHalted will avoid the refetch of data.


> +else
> +vcpuinfo->state = VIR_VCPU_RUNNING;
>  
>  if (qemuGetProcessInfo(>cpuTime,
> >cpu, NULL,
> @@ -5370,6 +5374,7 @@ qemuDomainGetVcpus(virDomainPtr dom,
> unsigned char *cpumaps,
> int maplen)
>  {

The opposite end of virDomainGetVcpus a/k/a 'virsh vcpuinfo'

> +virQEMUDriverPtr driver = dom->conn->privateData;
>  virDomainObjPtr vm;
>  int ret = -1;
>  
> @@ -5385,6 +5390,13 @@ qemuDomainGetVcpus(virDomainPtr dom,
>  goto cleanup;
>  }
>  
> +if (qemuDomainRefreshVcpuHalted(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   "%s",
> +   _("could not refresh CPU states"));

This overwrites what message qemuDomainRefreshVcpuHalted should have
generated.  Besides the "%s", could be on the previous line...

> +goto cleanup;
> +}
> +
>  ret = qemuDomainHelperGetVcpus(vm, info, NULL, maxinfo, cpumaps, maplen);
>  
>   cleanup:
> @@ -18863,7 +18875,7 @@ qemuDomainGetStatsBalloon(virQEMUDriverPtr driver,
>  
>  
>  static int
> -qemuDomainGetStatsVcpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
> +qemuDomainGetStatsVcpu(virQEMUDriverPtr driver,
> virDomainObjPtr dom,
> virDomainStatsRecordPtr record,
> int *maxparams,
> @@ -18893,6 +18905,13 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver 
> ATTRIBUTE_UNUSED,
>  VIR_ALLOC_N(cpuwait, virDomainDefGetVcpus(dom->def)) < 0)
>  goto cleanup;
>  
> +if (qemuDomainRefreshVcpuHalted(driver, dom, QEMU_ASYNC_JOB_NONE) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   "%s",
> +   _("could not refresh CPU states"));

Same comment

John

> +goto cleanup;
> +}
> +
>  if (qemuDomainHelperGetVcpus(dom, cpuinfo, cpuwait,
>   virDomainDefGetVcpus(dom->def),
>   NULL, 0) < 0) {
> 

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

Re: [libvirt] [Qemu-devel] [RFC v2] libvirt vGPU QEMU integration

[Daniel P. Berrange]

On Thu, Sep 29, 2016 at 02:35:48PM +, Tian, Kevin wrote:
> > From: Daniel P. Berrange [mailto:berra...@redhat.com]
> > Sent: Thursday, September 29, 2016 4:06 PM
> > 
> > On Wed, Sep 28, 2016 at 12:48:33PM -0700, Neo Jia wrote:
> > > On Tue, Sep 20, 2016 at 10:47:53AM +0100, Daniel P. Berrange wrote:
> > > > On Tue, Sep 20, 2016 at 02:05:52AM +0530, Kirti Wankhede wrote:
> > > > >
> > > > > Hi libvirt experts,
> > > > >
> > > > > Thanks for valuable input on v1 version of RFC.
> > > > >
> > > > > Quick brief, VFIO based mediated device framework provides a way to
> > > > > virtualize their devices without SR-IOV, like NVIDIA vGPU, Intel KVMGT
> > > > > and IBM's channel IO. This framework reuses VFIO APIs for all the
> > > > > functionalities for mediated devices which are currently being used 
> > > > > for
> > > > > pass through devices. This framework introduces a set of new sysfs 
> > > > > files
> > > > > for device creation and its life cycle management.
> > > > >
> > > > > Here is the summary of discussion on v1:
> > > > > 1. Discover mediated device:
> > > > > As part of physical device initialization process, vendor driver will
> > > > > register their physical devices, which will be used to create virtual
> > > > > device (mediated device, aka mdev) to the mediated framework.
> > > > >
> > > > > Vendor driver should specify mdev_supported_types in directory format.
> > > > > This format is class based, for example, display class directory 
> > > > > format
> > > > > should be as below. We need to define such set for each class of 
> > > > > devices
> > > > > which would be supported by mediated device framework.
> > > > >
> > > > >  --- mdev_destroy
> > > > >  --- mdev_supported_types
> > > > >  |-- 11
> > > > >  |   |-- create
> > > > >  |   |-- name
> > > > >  |   |-- fb_length
> > > > >  |   |-- resolution
> > > > >  |   |-- heads
> > > > >  |   |-- max_instances
> > > > >  |   |-- params
> > > > >  |   |-- requires_group
> > > > >  |-- 12
> > > > >  |   |-- create
> > > > >  |   |-- name
> > > > >  |   |-- fb_length
> > > > >  |   |-- resolution
> > > > >  |   |-- heads
> > > > >  |   |-- max_instances
> > > > >  |   |-- params
> > > > >  |   |-- requires_group
> > > > >  |-- 13
> > > > >  |-- create
> > > > >  |-- name
> > > > >  |-- fb_length
> > > > >  |-- resolution
> > > > >  |-- heads
> > > > >  |-- max_instances
> > > > >  |-- params
> > > > >  |-- requires_group
> > > > >
> > > > >
> > > > > In the above example directory '11' represents a type id of mdev 
> > > > > device.
> > > > > 'name', 'fb_length', 'resolution', 'heads', 'max_instance' and
> > > > > 'requires_group' would be Read-Only files that vendor would provide to
> > > > > describe about that type.
> > > > >
> > > > > 'create':
> > > > > Write-only file. Mandatory.
> > > > > Accepts string to create mediated device.
> > > > >
> > > > > 'name':
> > > > > Read-Only file. Mandatory.
> > > > > Returns string, the name of that type id.
> > > >
> > > > Presumably this is a human-targetted title/description of
> > > > the device.
> > > >
> > > > >
> > > > > 'fb_length':
> > > > > Read-only file. Mandatory.
> > > > > Returns {K,M,G}, size of framebuffer.
> > > > >
> > > > > 'resolution':
> > > > > Read-Only file. Mandatory.
> > > > > Returns 'hres x vres' format. Maximum supported resolution.
> > > > >
> > > > > 'heads':
> > > > > Read-Only file. Mandatory.
> > > > > Returns integer. Number of maximum heads supported.
> > > >
> > > > None of these should be mandatory as that makes the mdev
> > > > useless for non-GPU devices.
> > > >
> > > > I'd expect to see a 'class' or 'type' attribute in the
> > > > directory whcih tells you what kind of mdev it is. A
> > > > valid 'class' value would be 'gpu'. The fb_length,
> > > > resolution, and heads parameters would only be mandatory
> > > > when class==gpu.
> > > >
> > >
> > > Hi Daniel,
> > >
> > > Here you are proposing to add a class named "gpu", which will make all 
> > > those gpu
> > > related attributes mandatory, which libvirt can allow user to better
> > > parse/present a particular mdev configuration?
> > >
> > > I am just wondering if there is another option that we just make all those
> > > attributes that a mdev device can have as optional but still meaningful to
> > > libvirt, so libvirt can still parse / recognize them as an class "mdev".
> > 
> > 'mdev' isn't a class - mdev is the name of the kernel module. The class
> > refers to the broad capability of the device. class would be things
> > like "gpu", "nic", "fpga" or other such things. The point of the class
> > is to identify which other attributes will be considered mandatory.
> > 
> > 
> 
> Thanks Daniel. This class definition makes sense to me.
> 
> However I'm not sure whether we should define such common mandatory attributes
> of 

Re: [libvirt] [Qemu-devel] [RFC v2] libvirt vGPU QEMU integration

[Tian, Kevin]

> From: Daniel P. Berrange [mailto:berra...@redhat.com]
> Sent: Thursday, September 29, 2016 4:06 PM
> 
> On Wed, Sep 28, 2016 at 12:48:33PM -0700, Neo Jia wrote:
> > On Tue, Sep 20, 2016 at 10:47:53AM +0100, Daniel P. Berrange wrote:
> > > On Tue, Sep 20, 2016 at 02:05:52AM +0530, Kirti Wankhede wrote:
> > > >
> > > > Hi libvirt experts,
> > > >
> > > > Thanks for valuable input on v1 version of RFC.
> > > >
> > > > Quick brief, VFIO based mediated device framework provides a way to
> > > > virtualize their devices without SR-IOV, like NVIDIA vGPU, Intel KVMGT
> > > > and IBM's channel IO. This framework reuses VFIO APIs for all the
> > > > functionalities for mediated devices which are currently being used for
> > > > pass through devices. This framework introduces a set of new sysfs files
> > > > for device creation and its life cycle management.
> > > >
> > > > Here is the summary of discussion on v1:
> > > > 1. Discover mediated device:
> > > > As part of physical device initialization process, vendor driver will
> > > > register their physical devices, which will be used to create virtual
> > > > device (mediated device, aka mdev) to the mediated framework.
> > > >
> > > > Vendor driver should specify mdev_supported_types in directory format.
> > > > This format is class based, for example, display class directory format
> > > > should be as below. We need to define such set for each class of devices
> > > > which would be supported by mediated device framework.
> > > >
> > > >  --- mdev_destroy
> > > >  --- mdev_supported_types
> > > >  |-- 11
> > > >  |   |-- create
> > > >  |   |-- name
> > > >  |   |-- fb_length
> > > >  |   |-- resolution
> > > >  |   |-- heads
> > > >  |   |-- max_instances
> > > >  |   |-- params
> > > >  |   |-- requires_group
> > > >  |-- 12
> > > >  |   |-- create
> > > >  |   |-- name
> > > >  |   |-- fb_length
> > > >  |   |-- resolution
> > > >  |   |-- heads
> > > >  |   |-- max_instances
> > > >  |   |-- params
> > > >  |   |-- requires_group
> > > >  |-- 13
> > > >  |-- create
> > > >  |-- name
> > > >  |-- fb_length
> > > >  |-- resolution
> > > >  |-- heads
> > > >  |-- max_instances
> > > >  |-- params
> > > >  |-- requires_group
> > > >
> > > >
> > > > In the above example directory '11' represents a type id of mdev device.
> > > > 'name', 'fb_length', 'resolution', 'heads', 'max_instance' and
> > > > 'requires_group' would be Read-Only files that vendor would provide to
> > > > describe about that type.
> > > >
> > > > 'create':
> > > > Write-only file. Mandatory.
> > > > Accepts string to create mediated device.
> > > >
> > > > 'name':
> > > > Read-Only file. Mandatory.
> > > > Returns string, the name of that type id.
> > >
> > > Presumably this is a human-targetted title/description of
> > > the device.
> > >
> > > >
> > > > 'fb_length':
> > > > Read-only file. Mandatory.
> > > > Returns {K,M,G}, size of framebuffer.
> > > >
> > > > 'resolution':
> > > > Read-Only file. Mandatory.
> > > > Returns 'hres x vres' format. Maximum supported resolution.
> > > >
> > > > 'heads':
> > > > Read-Only file. Mandatory.
> > > > Returns integer. Number of maximum heads supported.
> > >
> > > None of these should be mandatory as that makes the mdev
> > > useless for non-GPU devices.
> > >
> > > I'd expect to see a 'class' or 'type' attribute in the
> > > directory whcih tells you what kind of mdev it is. A
> > > valid 'class' value would be 'gpu'. The fb_length,
> > > resolution, and heads parameters would only be mandatory
> > > when class==gpu.
> > >
> >
> > Hi Daniel,
> >
> > Here you are proposing to add a class named "gpu", which will make all 
> > those gpu
> > related attributes mandatory, which libvirt can allow user to better
> > parse/present a particular mdev configuration?
> >
> > I am just wondering if there is another option that we just make all those
> > attributes that a mdev device can have as optional but still meaningful to
> > libvirt, so libvirt can still parse / recognize them as an class "mdev".
> 
> 'mdev' isn't a class - mdev is the name of the kernel module. The class
> refers to the broad capability of the device. class would be things
> like "gpu", "nic", "fpga" or other such things. The point of the class
> is to identify which other attributes will be considered mandatory.
> 
> 

Thanks Daniel. This class definition makes sense to me.

However I'm not sure whether we should define such common mandatory attributes
of a 'gpu' class now. Intel will go with a 2's power sharing of type 
definition... actual 
type name to be finalized, but an example looks like below:

[GVTG-SKL-x2]: available instances (2)
[GVTG-SKL-x4]: available instances (4)
[GVTG-SKL-x8]: available instances (8)
...

User can create different types of vGPUs simultaneously. A GVTG-SKL-x2 type
vGPU will 

Re: [libvirt] [PATCH V2 3/4] qemu: Add domain support for VCPU halted state

[John Ferlan]

On 09/20/2016 04:11 AM, Viktor Mihajlovski wrote:
> Adding a field to the domain's private vcpu object to hold the halted
> state information.
> Adding two functions in support of the halted state:
> - qemuDomainGetVcpuHalted: retrieve the halted state from a
>   private vcpu object
> - qemuDomainRefreshVcpuHalted: obtain the per-vcpu halted states
>   via qemu monitor and store the results in the private vcpu objects
> 
> Signed-off-by: Viktor Mihajlovski 
> Reviewed-by: Bjoern Walk 
> Reviewed-by: Hao QingFeng 
> Signed-off-by: Boris Fiuczynski 
> ---
>  src/qemu/qemu_domain.c | 69 
> ++
>  src/qemu/qemu_domain.h |  5 
>  2 files changed, 74 insertions(+)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 3f16dbe..3fb9b4f 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5956,6 +5956,75 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
>  return ret;
>  }
>  
> +/**
> + * qemuDomainGetVcpuHalted:
> + * @vm: domain object
> + * @vcpu: cpu id
> + *
> + * Returns the vCPU halted state.
> +  */
> +bool
> +qemuDomainGetVcpuHalted(virDomainObjPtr vm,
> +unsigned int vcpuid)
> +{
> +virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(vm->def, vcpuid);
> +return QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->halted;
> +}
> +
> +/**
> + * qemuDomainRefreshVcpuHalted:
> + * @driver: qemu driver data
> + * @vm: domain object
> + * @asyncJob: current asynchronous job type
> + *
> + * Updates vCPU halted state in the private data of @vm.
> + *
> + * Returns number of detected vCPUs on success, -1 on error and reports
> + * an appropriate error, -2 if the domain doesn't exist any more.

Neither of the callers checks -1 or -2, just < 0, so is this really
necessary?

> + */
> +int
> +qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver,
> +virDomainObjPtr vm,
> +int asyncJob)
> +{
> +virDomainVcpuDefPtr vcpu;
> +qemuMonitorCPUInfoPtr info = NULL;
> +size_t maxvcpus = virDomainDefGetVcpusMax(vm->def);
> +size_t i;
> +bool hotplug;
> +int rc;
> +int ret = -1;
> +
> +/* Not supported currently for TCG, see qemuDomainRefreshVcpuInfo */
> +if (vm->def->virtType == VIR_DOMAIN_VIRT_QEMU)
> +return 0;

Since the "default" is halted = true could we run into a situation where
"halted" (or "running (inactive)") is always set for TCG...

> +
> +hotplug = qemuDomainSupportsNewVcpuHotplug(vm);
> +
> +if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
> +return -1;
> +
> +rc = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), , maxvcpus, 
> hotplug);
> +
> +if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> +ret = -2;

I see no need to ret = -2;

> +goto cleanup;
> +}
> +
> +if (rc < 0)
> +goto cleanup;
> +
> +for (i = 0; i < maxvcpus; i++) {
> +vcpu = virDomainDefGetVcpu(vm->def, i);
> +QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->halted = info[i].halted;
> +}
> +
> +ret = 0;
> +
> + cleanup:
> +qemuMonitorCPUInfoFree(info, maxvcpus);
> +return ret;
> +}
>  
>  bool
>  qemuDomainSupportsNicdev(virDomainDefPtr def,
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index a1404d0..03e58c5 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -317,6 +317,7 @@ struct _qemuDomainVcpuPrivate {
>  pid_t tid; /* vcpu thread id */
>  int enable_id; /* order in which the vcpus were enabled in qemu */
>  char *alias;
> +bool halted; /* vcpu halted state */

Another less than necessary comment ;-)

John

>  
>  /* information for hotpluggable cpus */
>  char *type;
> @@ -662,6 +663,10 @@ int qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
>virDomainObjPtr vm,
>int asyncJob,
>bool state);
> +bool qemuDomainGetVcpuHalted(virDomainObjPtr vm, unsigned int vcpu);
> +int qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver,
> +virDomainObjPtr vm,
> +int asyncJob);
>  
>  bool qemuDomainSupportsNicdev(virDomainDefPtr def,
>virDomainNetDefPtr net);
> 

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

Re: [libvirt] [PATCH V2 2/4] qemu: Add monitor support for CPU halted state

[John Ferlan]

On 09/20/2016 04:10 AM, Viktor Mihajlovski wrote:
> Extended the qemuMonitorCPUInfo with a halted flag. Extract the halted
> flag for both text and JSON monitor.
> 
> Signed-off-by: Viktor Mihajlovski 
> Signed-off-by: Boris Fiuczynski 
> ---
>  src/qemu/qemu_monitor.c  | 6 +-
>  src/qemu/qemu_monitor.h  | 5 +
>  src/qemu/qemu_monitor_json.c | 3 +++
>  src/qemu/qemu_monitor_text.c | 8 +++-
>  tests/qemumonitorjsontest.c  | 8 
>  5 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index b9e2910..c61fac7 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -1677,6 +1677,7 @@ qemuMonitorCPUInfoClear(qemuMonitorCPUInfoPtr cpus,
>  cpus[i].thread_id = -1;
>  cpus[i].vcpus = 0;
>  cpus[i].tid = 0;
> +cpus[i].halted = true;
>  
>  VIR_FREE(cpus[i].qom_path);
>  VIR_FREE(cpus[i].alias);
> @@ -1725,8 +1726,10 @@ qemuMonitorGetCPUInfoLegacy(struct 
> qemuMonitorQueryCpusEntry *cpuentries,
>  size_t i;
>  
>  for (i = 0; i < maxvcpus; i++) {
> -if (i < ncpuentries)
> +if (i < ncpuentries) {
>  vcpus[i].tid = cpuentries[i].tid;
> +vcpus[i].halted = cpuentries[i].halted;
> +}
>  
>  /* for legacy hotplug to work we need to fake the vcpu count added by
>   * enabling a given vcpu */
> @@ -1864,6 +1867,7 @@ qemuMonitorGetCPUInfoHotplug(struct 
> qemuMonitorQueryHotpluggableCpusEntry *hotpl
>  }
>  
>  vcpus[anyvcpu].tid = cpuentries[j].tid;
> +vcpus[anyvcpu].halted = cpuentries[j].halted;
>  }
>  
>  return 0;
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 615ab3e..8d0aa01 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -394,6 +394,8 @@ int qemuMonitorSystemPowerdown(qemuMonitorPtr mon);
>  struct qemuMonitorQueryCpusEntry {
>  pid_t tid;
>  char *qom_path;
> +/* halted indicator */

I can't believe I'm typing this, I'll blame pkrempa's influence ;-) -
but the comment is unnecessary since the name of the variable makes it
really obvious.

> +bool halted;
>  };
>  void qemuMonitorQueryCpusFree(struct qemuMonitorQueryCpusEntry *entries,
>size_t nentries);
> @@ -441,6 +443,9 @@ struct _qemuMonitorCPUInfo {
>  
>  /* internal for use in the matching code */
>  char *qom_path;
> +
> +/* halted indicator */

Ditto

John

> +bool halted;
>  };
>  typedef struct _qemuMonitorCPUInfo qemuMonitorCPUInfo;
>  typedef qemuMonitorCPUInfo *qemuMonitorCPUInfoPtr;
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index de746f1..64f9d01 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -1349,6 +1349,7 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data,
>  for (i = 0; i < ncpus; i++) {
>  virJSONValuePtr entry = virJSONValueArrayGet(data, i);
>  int thread = 0;
> +bool halted = true;
>  const char *qom_path;
>  if (!entry) {
>  ret = -2;
> @@ -1358,9 +1359,11 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data,
>  /* Some older qemu versions don't report the thread_id so treat this 
> as
>   * non-fatal, simply returning no data */
>  ignore_value(virJSONValueObjectGetNumberInt(entry, "thread_id", 
> ));
> +ignore_value(virJSONValueObjectGetBoolean(entry, "halted", ));
>  qom_path = virJSONValueObjectGetString(entry, "qom_path");
>  
>  cpus[i].tid = thread;
> +cpus[i].halted = halted;
>  if (VIR_STRDUP(cpus[i].qom_path, qom_path) < 0)
>  goto cleanup;
>  }
> diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
> index ff7cc79..f975347 100644
> --- a/src/qemu/qemu_monitor_text.c
> +++ b/src/qemu/qemu_monitor_text.c
> @@ -521,7 +521,7 @@ qemuMonitorTextQueryCPUs(qemuMonitorPtr mon,
>   * (qemu) info cpus
>   * * CPU #0: pc=0x000f0c4a thread_id=30019
>   *   CPU #1: pc=0xfff0 thread_id=30020
> - *   CPU #2: pc=0xfff0 thread_id=30021
> + *   CPU #2: pc=0xfff0 (halted) thread_id=30021
>   *
>   */
>  line = qemucpus;
> @@ -541,6 +541,12 @@ qemuMonitorTextQueryCPUs(qemuMonitorPtr mon,
>  
>  cpu.tid = tid;
>  
> +/* Extract halted indicator */
> +if ((offset = strstr(line, "(halted)")) != NULL)
> +cpu.halted = true;
> +else
> +cpu.halted = false;
> +
>  if (VIR_APPEND_ELEMENT_COPY(cpus, ncpus, cpu) < 0) {
>  ret = -1;
>  goto cleanup;
> diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
> index 9e195d7..8749ea1 100644
> --- a/tests/qemumonitorjsontest.c
> +++ b/tests/qemumonitorjsontest.c
> @@ -1221,10 

Re: [libvirt] [PATCH V2 1/4] domain: Add new VCPU state "halted"

[John Ferlan]

On 09/20/2016 04:10 AM, Viktor Mihajlovski wrote:
> QEMU virtual CPUs can assume a halted state, which - depending on
> the architecture - can indicate whether a CPU is in use by the
> guest operating system.
> 
> A new VCPU state halted is introduced in preparation of the
> support in the QEMU driver (and optionally others where deemed
> appropriate).
> 
> Signed-off-by: Viktor Mihajlovski 
> Reviewed-by: Bjoern Walk 
> Signed-off-by: Boris Fiuczynski 
> ---
>  include/libvirt/libvirt-domain.h | 1 +
>  tools/virsh-domain.c | 3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 

FWIW: Could be last patch, but that's just an ordering thing.

Considering points raised in review of other patches in this series,
extra credit to modify virsh.pod for 'vcpuinfo' (and 'domstats') to
describe the states ;-) (especially if they differ per architecture!).

Maybe there just needs to be "common" place to describe the VCPU states
much like there is a section describing the Domain States

> diff --git a/include/libvirt/libvirt-domain.h 
> b/include/libvirt/libvirt-domain.h
> index f1c35d9..0da2003 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -1682,6 +1682,7 @@ typedef enum {
>  VIR_VCPU_OFFLINE= 0,/* the virtual CPU is offline */
>  VIR_VCPU_RUNNING= 1,/* the virtual CPU is running */
>  VIR_VCPU_BLOCKED= 2,/* the virtual CPU is blocked on resource */
> +VIR_VCPU_HALTED = 3,/* the virtual CPU is halted */
>  
>  # ifdef VIR_ENUM_SENTINELS
>  VIR_VCPU_LAST
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 3829b17..a6b239b 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -181,7 +181,8 @@ VIR_ENUM_IMPL(virshDomainVcpuState,
>VIR_VCPU_LAST,
>N_("offline"),
>N_("running"),
> -  N_("blocked"))
> +  N_("blocked"),
> +  N_("halted"))

I've got some concerns over using "halted" here considering on my
findings...

>  
>  static const char *
>  virshDomainVcpuStateToString(int state)
> 

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

Re: [libvirt] [PATCH V2 0/4]] qemu: report actual vcpu state in virVcpuInfo

[John Ferlan]

On 09/20/2016 04:10 AM, Viktor Mihajlovski wrote:
> Currently, the virVcpuInfo returned by virDomainGetVcpus() will always
> report a state of VIR_VCPU_RUNNING for each defined domain vcpu even if
> the vcpu is currently in the halted state.
> 
> As the monitor interface is in fact reporting the accurate state, it is
> rather easy to transport this information with the existing API.
> 
> This is done by
> - adding a new state of VIR_VCPU_HALTED
> - extending the monitor to pass back the halted state for the vcpus
> - adding a new field to the private domain vcpu object reflecting the
>   halted state for the vcpu
> - modifying the driver code to report the vcpu state based on the halted
>   indicator
> - extending virsh vcpuinfo to also display the halted state
> 
> The vcpu state is however not recorded in the internal XML format, since
> the state can change asynchronously (without notification).
> 
> V2 is a rebase on top of Peter Krempa's CPU hotplug modernization.

So, I have a question based on a little bit of testing I did with one of
my guests and reading up on the qemu qapi-schema.json which states:

# Notes: @halted is a transient state that changes frequently.  By the
time the
#data is sent to the client, the guest may no longer be halted.


It seems "halted" is returned whenever a vCPU is not actively processing
anything (or not scheduled), which I suppose is "expected" for idle
guests; however, if I used the vcpuinfo command and saw:

# virsh vcpuinfo $dom
VCPU:   0
CPU:7
State:  halted
CPU time:   84.8s
CPU Affinity:   
...

I might be concerned that it's not "running" or "running (active)" vs.
"running (inactive)" (or paused or waiting or something to indicate not
actively processing/scheduled). The halted state could be the "norm".

So is this more of a "stats" type value vs. purely an "info" type value?
 Also, considering hotplug differences w/ CPU's for x86, ppc, s390, and
arm - could this be some sort of architecture difference too (I'm using
x86)?

Primarily I'm concerned the transient nature of the field based on
whether something is scheduled for the thread could lead to some
"erroneous" bug reports especially since "running" may not be the
dominant state any more.

John


> 
> Viktor Mihajlovski (4):
>   domain: Add new VCPU state "halted"
>   qemu: Add monitor support for CPU halted state
>   qemu: Add domain support for VCPU halted state
>   qemu: Ensure reported VCPU state is current in driver API
> 
>  include/libvirt/libvirt-domain.h |  1 +
>  src/qemu/qemu_domain.c   | 69 
> 
>  src/qemu/qemu_domain.h   |  5 +++
>  src/qemu/qemu_driver.c   | 23 --
>  src/qemu/qemu_monitor.c  |  6 +++-
>  src/qemu/qemu_monitor.h  |  5 +++
>  src/qemu/qemu_monitor_json.c |  3 ++
>  src/qemu/qemu_monitor_text.c |  8 -
>  tests/qemumonitorjsontest.c  |  8 ++---
>  tools/virsh-domain.c |  3 +-
>  10 files changed, 122 insertions(+), 9 deletions(-)
> 

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

Re: [libvirt] [RFC v2] libvirt vGPU QEMU integration

[Kirti Wankhede]

 7. Hot-plug

 It is same syntax to create a virtual device for hot-plug.  
>>>
>>> How do groups work with hotplug?  Can a device be creating into an
>>> existing, running group?  Can a device be removed from an existing,
>>> running group?  Thanks,
>>>   
>>
>> Group is for vendor driver to decide when to commit resources for a
>> group of devices to support multiple devices in a domain. It will be
>> upto vendor driver how it manage it.
>
> Then it's not sufficiently specified for libvirt to use.  Thanks,
> 

 Can you clarify what if required for libvirt to use?  
>>>
>>> Everything we're discussing above.  We can't simply say that group
>>> management is defined by the vendor driver, that means that libvirt
>>> needs to know how to uniquely behave for each vendor driver.  We want
>>> to create a consistent framework where libvirt doesn't care what the
>>> vendor driver is.  Therefore we need to make group manipulation fully
>>> specified, regardless of the vendor driver.  Thanks,
>>>  
>>
>> There is no communication between different vendor drivers. So if
>> libvirt creates a group with one device from one vendor and one device
>> from another vendor, both vendor driver would think they have a single
>> device and accordingly commit resources for that single device from its
>> own driver. Ideally nothing would fail. But that is not the main aim of
>> grouping, right?
>>
>> To make this independent of vendor driver, we had initial proposal of
>> having same UUID and different instance numbers. But that was also not
>> acceptable.
> 
> We're defining the infrastructure, we get to define the rules, but we
> need to define all the rules so that libvirt can actually implement to
> them.  We can't say "groups are this nebulous thing and each vendor
> will manage them however they want", nobody wants to care that NVIDIA
> did it one way and Intel did it another and we need to support both.
> That's a design failure.  Therefore we \must\ make the group definition
> we need well specified, any hopefully generic enough that others can
> use it.  If they cannot then we'd need to add a new type of group.
> Therefore we need to \specify\ things like for what set of mdev should
> a group be created?  Do I need a group for a single device?  How do we
> introspect a group to know which mdevs it contains?  Can we dynamically
> add or remove mdev devices from a group?  Can we attach multiple
> groups to the same user?  Can we change the association of an mdev
> device without removing it and recreating it? etc, etc, etc.  We don't
> need vendor coordination, we need a specification of exactly how this
> type of group is meant to work. Thanks,
> 

We decided to handle "multiple vGPUs per VM" through internal design
changes. With that we don't need grouping in mdev framework. Hope this
helps to accelerate and settle on sysfs interface soon.

Thanks,
Kirti



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

Re: [libvirt] [PATCH v5 0/2] qemu/gluster: add option for tuning debug logging level

[Peter Krempa]

On Thu, Sep 22, 2016 at 01:04:17 +0530, Prasanna Kumar Kalever wrote:
> This series run basic sanity and other tests:
> 1. make syntax-check
> 2. make check
> 3. VIR_TEST_VERBOSE=1 ./tests/qemuargv2xmltest
> 4. VIR_TEST_VERBOSE=1 ./tests/qemuxml2argvtest
> 5. VIR_TEST_VERBOSE=1 ./tests/qemucapabilitiestest
> 
> v5:
> Avoild copying variable glusterDebugLevel to disk->src (virStorageSourcePtri),
> instead get debug_level from cfg as per Jiri Denemark review on v4
> Thanks Jirka

The patches still should be in reverse order and I still don't think
that's impossible to do a proper feature check for the qemu capability.

I'll pick them up and try to figure it out once I have time.

Peter

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

Re: [libvirt] Cpu Modeling

[Jiri Denemark]

On Fri, Sep 23, 2016 at 15:23:45 -0300, Eduardo Habkost wrote:
> On Fri, Sep 23, 2016 at 02:05:59PM +0200, Jiri Denemark wrote:
> > It's not critical for query-cpu-model-expansion, but do we have an
> > interface we can use to check whether the new commands are supported by
> > a QEMU binary? Trying and checking for
> > 
> > {"error": {"class": "GenericError", "desc": "this feature or command
> > is not currently supported"}}
> > 
> > is not really possible. At least we'd need a specific error class.
> 
> I believe query-qmp-schema and/or query-commands can be used for
> that.
> 
> But you may still have the command returning the error above if
> it is simply not implemented in that architecture. I will start a
> qemu-devel thread to look for better solutions.

Yes, that's my concern.

...
> > No, virsh cpu-models will just list CPU models supported by libvirt (or
> > an empty list if libvirt supports all models supported by QEMU). The CPU
> > model data from QEMU can be found in domain capabilities XML.
> 
> That's interesting to know. I was often confused about the
> interfaces that seem to be generic (not depend on a specific QEMU
> binary) but seemed to depend on information from the QEMU binary
> (virsh capabilities and virsh cpu-models). It looks like I was
> looking at the wrong commands.

Those are just old interfaces. We may add newer versions for some of
them that would take more input parameters, but I don't feel a strong
need for it right now.

And the domain capabilities XML gained support for showing CPU related
stuff only recently (it will be included in the upcoming 2.3.0 release),
so it's not like you had a choice where to look at :-)

...
> > Slightly related, I don't think we have a way to list CPU features
> > supported by QEMU over QMP, do we? "-cpu ?" will show them all, but I
> > couldn't find a QMP command that would give me the same list.
> 
> device-list-properties should return all the properties you can
> set. But I recommend that you don't make libvirt set any of the
> properties in the list unless: 1) you know what it does; or 2) it
> is returned by a query-cpu-model-expansion call.

The use case was for pre-checking whether all CPU features specified in
domain XML are supported by current QEMU binary. What parameters would
I need to use for device-list-properties? And would it work with
-machine none?

...
> > Well, IIUC the new commands are not supported on any other architecture
> > right now, are they? Anyway, I don't think we need to switch everything
> > at the same time. If we have a way to detect what commands are supported
> > for a specific QEMU binary, we can implement the code in libvirt and use
> > when we can or falling back to the current way.
> 
> Right now the commands are available only on s390x. I plan to
> have them implemented in x86 before QEMU 2.8 is out.

That would be nice.

Jirka

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

[libvirt] [PATCH v3.5 14/18] 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)
---

change from v3: reorganize to short circuit the entire check when the
root bus isn't PCI, rather than just preventing the
slot reservation. Also improve the comment.

 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 89cab1d..6df373d 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -1694,8 +1694,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;
@@ -1704,23 +1702,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

Re: [libvirt] [PATCH 0/3] Fix io_timeout for sanlock

[Michal Privoznik]

On 15.09.2016 16:35, Michal Privoznik wrote:
> Just read the 3/3. I didn't know whether I should laugh or cry. I did both.
> 
> Michal Privoznik (3):
>   lock_driver_sanlock: Avoid global driver variable whenever possible
>   m4: Check for sanlock_write_lockspace
>   sanlock: Properly init io_timeout
> 
>  m4/virt-sanlock.m4| 14 +---
>  src/locking/lock_driver_sanlock.c | 69 
> +--
>  2 files changed, 60 insertions(+), 23 deletions(-)
> 

Thanks John for ACKing the series. I'm not quite sure whether I can push
it now that we are in the freeze. I mean it can be viewed as a bug fix,
so I'm inclined to push it. What are your thoughts?

Michal

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

Re: [libvirt] [PATCH 3/3] sanlock: Properly init io_timeout

[Michal Privoznik]

On 28.09.2016 23:59, John Ferlan wrote:
> 
> 
> On 09/15/2016 10:35 AM, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1292984
>>
>> Hold on to your hats, because this is gonna be wild.
>>
>> In bd3e16a3 I've tried to expose sanlock io_timeout. What I had
>> not realized (because there is like no documentation for sanlock
>> at all) was very unusual way their APIs work. Basically, what we
>> do currently is:
>>
>> sanlock_add_lockspace_timeout(, io_timeout);
>>
>> which adds a lockspace to sanlock daemon. One would expect that
>> io_timeout sets the io_timeout for it. Nah! That's where you are
>> completely off the tracks. It sets timeout for next lockspace you
>> will probably add later. Therefore:
>>
>>sanlock_add_lockspace_timeout(, io_timeout = 10);
>>/* adds new lockspace with default io_timeout */
>>
>>sanlock_add_lockspace_timeout(, io_timeout = 20);
>>/* adds new lockspace with io_timeout = 10 */
>>
>>sanlock_add_lockspace_timeout(, io_timeout = 40);
>>/* adds new lockspace with io_timeout = 20 */
>>
>> And so on. You get the picture.
>> Fortunately, we don't allow setting io_timeout per domain or per
>> domain disk. So we just need to set the default used in the very
>> first step and hope for the best (as all the io_timeout-s used
>> later will have the same value).
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/locking/lock_driver_sanlock.c | 25 -
>>  1 file changed, 24 insertions(+), 1 deletion(-)
>>
> 
> Any thoughts about modifying src/locking/sanlock.conf in order add some
> text there that indicates support for 'io_timeout' requires at least
> sanlock 2.7 (or something similar).

Huh, well, versions are tricky a bit. Somebody might just backport stuff
and all of a sudden this might work with sanlock-2.6 too.

I think we just avoid giving these kind of advice.

Michal

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

Re: [libvirt] [PATCH v3 03/18] qemu: new functions qemuDomainMachineHasPCI[e]Root()

[Andrea Bolognani]

On Tue, 2016-09-20 at 15:14 -0400, Laine Stump wrote:
> 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 | 28 
>  src/qemu/qemu_domain.h |  2 ++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 3f16dbe..227134e 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5263,6 +5263,34 @@ 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 true;

Please leave an empty line here...

> +return false;
> +}
> +
> +
> +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 true;

... and here.

You might want to consider reversing the polarity of the
neutron flow^W^W^W^W^Wmodel check, so that the function
looks like

  if (...)
  return false;

  if (...)
  return false;

  return true;

ACK, with the empty lines added, whether or not you decide
to go for this change.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 7/7] qemu: Allow making vcpus hotpluggable with virDomainSetVcpusFlags

[Shivaprasad G Bhat]

On 09/29/2016 05:50 PM, Peter Krempa wrote:

On Thu, Sep 29, 2016 at 17:37:53 +0530, Shivaprasad G Bhat wrote:


On 09/29/2016 05:32 PM, Peter Krempa wrote:

On Thu, Sep 29, 2016 at 16:29:13 +0530, Shivaprasad G Bhat wrote:

On 09/21/2016 05:19 PM, Peter Krempa wrote:

Implement support for VIR_DOMAIN_VCPU_HOTPLUGGABLE so that users can
choose to make vcpus added by the API removable.
---
src/qemu/qemu_driver.c | 22 --
1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 357be4e..8453628 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4912,7 +4912,8 @@ qemuDomainSetVcpusLive(virQEMUDriverPtr driver,
 */
static void
qemuDomainSetVcpusConfig(virDomainDefPtr def,
- unsigned int nvcpus)
+ unsigned int nvcpus,
+ bool hotpluggable)
{
virDomainVcpuDefPtr vcpu;

Here  if (curvcpus == nvcpus)

   return
  we still need to allow if someone wants to switch from hotpluggable
= yes to no/ vice versa.

No. As no new vcpus were added there's nothing to turn to hotpluggable.
The flag turns only the newly added vcpus as hotpluggable.

What if someone had the hotplug = yes. And He wants to make it "no".  ?
With no changes to the vcpus.

You can make two calls to the API, decreasing and re-increasing the
count. Or edit the XML directly since its a non-simple change. If the
API call would change existing vcpus to either hotpluggable or not it
would break configurations. Eg. as PPC requires first 8 vcpus to be non
hotpluggable vs. just one on intel.


Agreed. Thanks!

ACK the series.

-Shivaprasad

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

Re: [libvirt] support for qemu werror and rerror

[Vasiliy Tolstov]

2016-09-29 16:38 GMT+03:00 Laine Stump :
> Support is already there (since libvirt-0.9.7), handled by the error_policy,
> and rerror_policy attributes of the  subelement of a disk. search
> for "rerror_policy" in http://www.libvirt.org/formatdomain.html for details.


Sorry i miss it. Thanks!

-- 
Vasiliy Tolstov,
e-mail: v.tols...@selfip.ru

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

Re: [libvirt] [PATCH v3 02/18] qemu: replace a lot of "def->controllers[i]" with equivalent "cont"

[Andrea Bolognani]

On Tue, 2016-09-20 at 15:14 -0400, Laine Stump wrote:
> There's no functional change here. This pointer was just used so many
> times that the extra long lines became annoying.
> ---
>  src/qemu/qemu_domain_address.c | 100 
>+
>  1 file changed, 51 insertions(+), 49 deletions(-)
[...]
> @@ -798,21 +800,21 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def,
>  if (virDomainPCIAddressReserveAddr(addrs, _addr,
> flags, false, true) < 
>0)
>  goto cleanup;
> -def->controllers[i]->info.type
> +cont->info.type
>  = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;

You can now join this line with the one above it :)

> -def->controllers[i]->info.addr.pci.domain = 0;
> -def->controllers[i]->info.addr.pci.bus = 0;
> -def->controllers[i]->info.addr.pci.slot = tmp_addr.slot;
> -def->controllers[i]->info.addr.pci.function = 0;
> -def->controllers[i]->info.addr.pci.multi
> +cont->info.addr.pci.domain = 0;
> +cont->info.addr.pci.bus = 0;
> +cont->info.addr.pci.slot = tmp_addr.slot;
> +cont->info.addr.pci.function = 0;
> +cont->info.addr.pci.multi
> = VIR_TRISTATE_SWITCH_ON;

Same here.

[...]
> @@ -1014,9 +1018,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
>   * controller/bus to connect it to on the upstream side.
>   */
>  flags = virDomainPCIControllerModelToConnectType(model);
> -if (virDomainPCIAddressReserveNextSlot(addrs,
> -   
> >controllers[i]->info,
> -   flags) < 0)
> +if (virDomainPCIAddressReserveNextSlot(addrs, >info, 
> flags) < 0)

This is >80 columns, so "flags) < 0)" will have to remain
on its own line.

[...]
> @@ -1147,12 +1151,10 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
> false, foundAddr) < 0)
>  goto error;
>  
> -def->controllers[i]->info.type = 
> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
> -def->controllers[i]->info.addr.pci = addr;
> +cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
> +cont->info.addr.pci = addr;
>  } else {
> -if (virDomainPCIAddressReserveNextSlot(addrs,
> -   
> >controllers[i]->info,
> -   flags) < 0)
> +if (virDomainPCIAddressReserveNextSlot(addrs, >info, 
> flags) < 0)

Same here.

ACK once you address the comments above.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] support for qemu werror and rerror

[Laine Stump]

On 09/29/2016 08:35 AM, Vasiliy Tolstov wrote:

Sometimes ago qemu anounced support for action for disk:

werror=action,rerror=action

Specify which action to take on write and read errors. Valid actions
are: "ignore" (ignore the error and try to continue), "stop" (pause
QEMU), "report" (report the error to the guest), "enospc" (pause QEMU
only if the host disk is full; report the error to the guest
otherwise). The default setting iswerror=enospc and rerror=report.

Does somebody can write this support to libvirt ? Or how can i enable
werror and rerror ?



Support is already there (since libvirt-0.9.7), handled by the 
error_policy, and rerror_policy attributes of the  subelement of 
a disk. search for "rerror_policy" in 
http://www.libvirt.org/formatdomain.html for details.



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

[libvirt] [PATCH] allow snapshots of network sheepdog disks

[Vasiliy Tolstov]

Sometimes ago in f7c1410b0ee5b878e81f2eddf86c609947a9b27c ability to
snapshot sheepdog disk removed. But sheepdog have ability to store vm
state inside special object type.

Vasiliy Tolstov (1):
  sheepdog: allow snapshot

 src/qemu/qemu_driver.c | 6 ++
 1 file changed, 6 insertions(+)

-- 
2.7.4

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

[libvirt] [PATCH] sheepdog: allow snapshot

[Vasiliy Tolstov]

partially revert f7c1410b0ee5b878e81f2eddf86c609947a9b27c because
sheepdog allow to store vm state inside vdi

Signed-off-by: Vasiliy Tolstov 
---
 src/qemu/qemu_driver.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index db99c414d458..816514d2d909 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -13887,6 +13887,12 @@ qemuDomainSnapshotPrepare(virConnectPtr conn,
   active) < 0)
 goto cleanup;
 
+/* sheepdog allow to store memory inside the vdi */
+if (vm->def->disks[i]->src->type == VIR_STORAGE_TYPE_NETWORK &&
+(vm->def->disks[i]->src->protocol == 
VIR_STORAGE_NET_PROTOCOL_SHEEPDOG)) {
+break;
+}
+
 if (vm->def->disks[i]->src->format > 0 &&
 vm->def->disks[i]->src->format != VIR_STORAGE_FILE_QCOW2) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-- 
2.7.4

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

Re: [libvirt] [PATCH v3 01/18] conf: restrict what type of buses will accept a pci-bridge

[Andrea Bolognani]

On Tue, 2016-09-20 at 15:14 -0400, Laine Stump wrote:
> A pci-bridge has *almost* the same rules as a legacy PCI endpoint
> device for where it can be automatically connected, and until now both
> had been considered identical. There is one pairing that is okay when
> specifically requested by the user (i.e. manual assignment), but we
> want to avoid it when auto-assigning addresses - plugging a pci-bridge
> directly into pcie-root (it is cleaner to plug in a dmi-to-pci-bridge,
> then plug the pci-bridge into that).
> 
> In order to allow that difference, this patch makes a separate
> CONNECT_TYPE for pci-bridge, and uses it to restrict auto-assigned
> addresses for pci-bridges to be only on pci-root, pci-expander-bus,
> dmi-to-pci-bridge, or on another pci-bridge.
> 
> NB: As with other discouraged-but-seem-to-work configurations
> (e.g. plugging a legacy PCI device into a pcie-root-port) if someone
> *really* wants to, they can still force a pci-bridge to be plugged
> into pcie-root (by manually specifying its PCI address.)
> ---
>  src/conf/domain_addr.c | 24 
>  src/conf/domain_addr.h |  4 +++-
>  2 files changed, 19 insertions(+), 9 deletions(-)

ACK

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] Cpu Modeling

[Jason J. Herne]

Polite Ping? :)

On 09/23/2016 03:51 PM, Jason J. Herne wrote:

On 09/23/2016 08:05 AM, Jiri Denemark wrote:

On Thu, Sep 22, 2016 at 14:47:36 -0400, Jason J. Herne wrote:

...

1. We will invoke qemu to gather the host cpu data used for virsh
capabilities. Today this data seems to be collected directly from the
host hardware for most (all?) architectures.


Not really, virsh capabilities will still contain data gathered directly
from the host hardware. But, we have virsh domcapabilities for
displaying data tight to a specific QEMU binary. Since yesterday
afternoon we have support for displaying CPU related data in the domain
capabilities XML; see
http://libvirt.org/formatdomaincaps.html#elementsCPU



The host-model part of the XML will show the result of
query-cpu-model-expansion on "host" model, or the result of querying the
hardware directly if we can't ask QEMU for that (which is the current
state).


So the expectation here is that virsh capabilities" reports actual host
cpu data. So for S390, we can leave our implementation of this "as-is"
and not report any features here, I'm guessing. And the 
section from virsh domcapabilities will contain the Qemu specific
supported cpu modeling info. As you stated 
will contain the name (and possibly feature details?) of the model
returned by qmp query-model-expansion on 'host'. Furthermore, the
 section will list all supported model names, as
indicated by qmp query-cpu-definitions. Do I have it right?


2. virsh cpu-models {arch} will also use a Qemu invocation to gather
cpu model data.


No, virsh cpu-models will just list CPU models supported by libvirt


So, in other words we just spit out whatever models Libvirt managed to
grab, and cache, from a call to qmp query-cpu-definitions? Or am I
missing something?


(or an empty list if libvirt supports all models supported by QEMU).


Can you clarify? It seems odd that an empty list would be returned here.
I thought the point was to list all supported cpu models. For x86 today
I imagine it is the list found in cpu_map.xml. For s390, this will be
the list we get back from qmp query-cpu-definitions, which as you mention
below, is found in the domain capabilities XML.


The CPU model data from QEMU can be found in domain capabilities XML.


3. Most architectures seem to use a "map" (xml file containing cpu
model data that ships with Libvirt) to satisfy #1 and #2 above. Our
new method does not use this map as it gets all of the data directly
from Qemu.


Even if we switch some CPU operations to work through QEMU, we may still
need to use the cpu_map.xml file for some other operations, or other
hypervisor driver. It all depends on what we need to do for a given
architecture. For example, ARM does not use cpu_map.xml at all even now.

Slightly related, I don't think we have a way to list CPU features
supported by QEMU over QMP, do we? "-cpu ?" will show them all, but I
couldn't find a QMP command that would give me the same list.



query-model-expansion will return all features of a given model name. Model
names can be enumerated via query-cpu-definitions.


4. virsh cpu-baseline and cpu-compare will now invoke qemu directly as
well.


Perhaps, but before we can do that, we'll probably need to come up with
new APIs. It don't think it's critical, though.


Do you mind elaborating on this a bit? Which APIs do we want to look at
here? Are you talking about new commands/calls to replace cpu-baseline
and cpu-compare?

Thanks again for your time.



--
-- Jason J. Herne (jjhe...@linux.vnet.ibm.com)

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

[libvirt] support for qemu werror and rerror

[Vasiliy Tolstov]

Sometimes ago qemu anounced support for action for disk:

werror=action,rerror=action

Specify which action to take on write and read errors. Valid actions
are: "ignore" (ignore the error and try to continue), "stop" (pause
QEMU), "report" (report the error to the guest), "enospc" (pause QEMU
only if the host disk is full; report the error to the guest
otherwise). The default setting iswerror=enospc and rerror=report.

Does somebody can write this support to libvirt ? Or how can i enable
werror and rerror ?

-- 
Vasiliy Tolstov,
e-mail: v.tols...@selfip.ru

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

Re: [libvirt] [PATCH] qemu: Only use memory-backend-file with NUMA if needed

[Martin Kletzander]

On Tue, Sep 27, 2016 at 04:29:07PM +0200, Michal Privoznik wrote:

On 23.09.2016 15:20, Martin Kletzander wrote:

If this reminds you of a commit message from around a year ago, it's
41c2aa729f0af084ede95ee9a06219a2dd5fb5df and yes, we're dealing with
"the same thing" again.  Or f309db1f4d51009bad0d32e12efc75530b66836b and
it's similar.

There is a logic in place that if there is no real need for
memory-backend-file, qemuBuildMemoryBackendStr() returns 0.  However
that wasn't the case with hugepage backing.  The reason for that was
that we abused the 'pagesize' variable for storing that information, but
we should rather have a separate one that specifies whether we really
need the new object for hugepage backing.  And that variable should be
set only if this particular NUMA cell needs special treatment WRT
hugepages.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1372153

Signed-off-by: Martin Kletzander 
---

Notes:
This fixes migration from older libvirts.  By "older", I mean
pre-(circa-)1.2.7, also in some cases pre-1.2.11, in some other cases
pre-v1.2.20.  It's pretty messy.  It could be back-ported as far as it's
easy to do.

 src/qemu/qemu_command.c   |  8 +---
 tests/qemuxml2argvdata/qemuxml2argv-hugepages-pages2.args | 10 --
 2 files changed, 9 insertions(+), 9 deletions(-)


ACK



Since this was ACKed before the freeze, I'll pushed it in a while,
mainly because I need to back-port it to -maint branches anyway.


Michal

--
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 7/7] qemu: Allow making vcpus hotpluggable with virDomainSetVcpusFlags

[Peter Krempa]

On Thu, Sep 29, 2016 at 17:37:53 +0530, Shivaprasad G Bhat wrote:
> 
> 
> On 09/29/2016 05:32 PM, Peter Krempa wrote:
> > On Thu, Sep 29, 2016 at 16:29:13 +0530, Shivaprasad G Bhat wrote:
> >> On 09/21/2016 05:19 PM, Peter Krempa wrote:
> >>> Implement support for VIR_DOMAIN_VCPU_HOTPLUGGABLE so that users can
> >>> choose to make vcpus added by the API removable.
> >>> ---
> >>>src/qemu/qemu_driver.c | 22 --
> >>>1 file changed, 16 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> >>> index 357be4e..8453628 100644
> >>> --- a/src/qemu/qemu_driver.c
> >>> +++ b/src/qemu/qemu_driver.c
> >>> @@ -4912,7 +4912,8 @@ qemuDomainSetVcpusLive(virQEMUDriverPtr driver,
> >>> */
> >>>static void
> >>>qemuDomainSetVcpusConfig(virDomainDefPtr def,
> >>> - unsigned int nvcpus)
> >>> + unsigned int nvcpus,
> >>> + bool hotpluggable)
> >>>{
> >>>virDomainVcpuDefPtr vcpu;
> >> Here  if (curvcpus == nvcpus)
> >>
> >>   return
> >>  we still need to allow if someone wants to switch from hotpluggable
> >> = yes to no/ vice versa.
> > No. As no new vcpus were added there's nothing to turn to hotpluggable.
> > The flag turns only the newly added vcpus as hotpluggable.
> 
> What if someone had the hotplug = yes. And He wants to make it "no".  ? 
> With no changes to the vcpus.

You can make two calls to the API, decreasing and re-increasing the
count. Or edit the XML directly since its a non-simple change. If the
API call would change existing vcpus to either hotpluggable or not it
would break configurations. Eg. as PPC requires first 8 vcpus to be non
hotpluggable vs. just one on intel.

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

Re: [libvirt] [PATCH 7/7] qemu: Allow making vcpus hotpluggable with virDomainSetVcpusFlags

[Shivaprasad G Bhat]

On 09/29/2016 05:32 PM, Peter Krempa wrote:

On Thu, Sep 29, 2016 at 16:29:13 +0530, Shivaprasad G Bhat wrote:

On 09/21/2016 05:19 PM, Peter Krempa wrote:

Implement support for VIR_DOMAIN_VCPU_HOTPLUGGABLE so that users can
choose to make vcpus added by the API removable.
---
   src/qemu/qemu_driver.c | 22 --
   1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 357be4e..8453628 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4912,7 +4912,8 @@ qemuDomainSetVcpusLive(virQEMUDriverPtr driver,
*/
   static void
   qemuDomainSetVcpusConfig(virDomainDefPtr def,
- unsigned int nvcpus)
+ unsigned int nvcpus,
+ bool hotpluggable)
   {
   virDomainVcpuDefPtr vcpu;

Here  if (curvcpus == nvcpus)

  return
 we still need to allow if someone wants to switch from hotpluggable
= yes to no/ vice versa.

No. As no new vcpus were added there's nothing to turn to hotpluggable.
The flag turns only the newly added vcpus as hotpluggable.


What if someone had the hotplug = yes. And He wants to make it "no".  ? 
With no changes to the vcpus.





   size_t curvcpus = virDomainDefGetVcpus(def);
@@ -4933,7 +4934,12 @@ qemuDomainSetVcpusConfig(virDomainDefPtr def,
   continue;

   vcpu->online = true;
-vcpu->hotpluggable = VIR_TRISTATE_BOOL_NO;
+if (hotpluggable) {
+vcpu->hotpluggable = VIR_TRISTATE_BOOL_YES;
+def->individualvcpus = true;
+} else {
+vcpu->hotpluggable = VIR_TRISTATE_BOOL_NO;
+}

   if (++curvcpus == nvcpus)
   break;

Can we add checks here to see on PPC, the config is valid with a check
when topology is given in xml to see (curvcpus%threads_per_core == 0)

No. For PPC and all the weird archs that don't have thread level hotplug
we can't really know what to use and what is a legitimate configuration
until we start the VM and query qemu.


Otherwise with virsh setvcpus rhel71 13 --config --hotpluggable
for a guest with topology 
we would see,
2016-09-29 10:12:05.929+: 1137: error :
qemuProcessValidateHotpluggableVcpus:4829 : unsupported configuration:
vcpus '12' and '13' are in the same hotplug group but differ in
configuration

Yes, you can configure the same thing manually in the XML.


OR

Even when setvcpus live to a number not leading to a complete core, we
have checks leading to sensible error (error: unsupported configuration:
target vm vcpu granularity does not allow the desired vcpu count ) . So,
in case of --config also may be we can add the check to bring the
consistency.

As said above. Libvirt can't surely detect that the "weird" approach is
needed.

Peter


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

Re: [libvirt] [PATCH 7/7] qemu: Allow making vcpus hotpluggable with virDomainSetVcpusFlags

[Peter Krempa]

On Thu, Sep 29, 2016 at 16:29:13 +0530, Shivaprasad G Bhat wrote:
> 
> On 09/21/2016 05:19 PM, Peter Krempa wrote:
> > Implement support for VIR_DOMAIN_VCPU_HOTPLUGGABLE so that users can
> > choose to make vcpus added by the API removable.
> > ---
> >   src/qemu/qemu_driver.c | 22 --
> >   1 file changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 357be4e..8453628 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -4912,7 +4912,8 @@ qemuDomainSetVcpusLive(virQEMUDriverPtr driver,
> >*/
> >   static void
> >   qemuDomainSetVcpusConfig(virDomainDefPtr def,
> > - unsigned int nvcpus)
> > + unsigned int nvcpus,
> > + bool hotpluggable)
> >   {
> >   virDomainVcpuDefPtr vcpu;
> Here  if (curvcpus == nvcpus)
> 
>  return
> we still need to allow if someone wants to switch from hotpluggable 
> = yes to no/ vice versa.

No. As no new vcpus were added there's nothing to turn to hotpluggable.
The flag turns only the newly added vcpus as hotpluggable.

> 
> 
> >   size_t curvcpus = virDomainDefGetVcpus(def);
> > @@ -4933,7 +4934,12 @@ qemuDomainSetVcpusConfig(virDomainDefPtr def,
> >   continue;
> >
> >   vcpu->online = true;
> > -vcpu->hotpluggable = VIR_TRISTATE_BOOL_NO;
> > +if (hotpluggable) {
> > +vcpu->hotpluggable = VIR_TRISTATE_BOOL_YES;
> > +def->individualvcpus = true;
> > +} else {
> > +vcpu->hotpluggable = VIR_TRISTATE_BOOL_NO;
> > +}
> >
> >   if (++curvcpus == nvcpus)
> >   break;
> 
> Can we add checks here to see on PPC, the config is valid with a check 
> when topology is given in xml to see (curvcpus%threads_per_core == 0)

No. For PPC and all the weird archs that don't have thread level hotplug
we can't really know what to use and what is a legitimate configuration
until we start the VM and query qemu.

> Otherwise with virsh setvcpus rhel71 13 --config --hotpluggable
> for a guest with topology 
> we would see,
> 2016-09-29 10:12:05.929+: 1137: error : 
> qemuProcessValidateHotpluggableVcpus:4829 : unsupported configuration: 
> vcpus '12' and '13' are in the same hotplug group but differ in 
> configuration

Yes, you can configure the same thing manually in the XML.

> 
> OR
> 
> Even when setvcpus live to a number not leading to a complete core, we 
> have checks leading to sensible error (error: unsupported configuration: 
> target vm vcpu granularity does not allow the desired vcpu count ) . So, 
> in case of --config also may be we can add the check to bring the 
> consistency.

As said above. Libvirt can't surely detect that the "weird" approach is
needed.

Peter


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

Re: [libvirt] summary of current vfio mdev upstreaming status

[Daniel P. Berrange]

On Thu, Sep 29, 2016 at 04:55:39PM +0800, Jike Song wrote:
> Hi all,
> 
> In order to have a clear understanding about the VFIO mdev upstreaming
> status, I'd like to summarize it. Please share your opinions on this,
> and correct my misunderstandings.
> 
> The whole vfio mdev series can be logically divided into several parts,
> they work together to provide the mdev support.
> 
> 
> 
> PART 1: mdev core driver
> 
>   [task]
>   -   the mdev bus/device support
>   -   the utilities of mdev lifecycle management
>   -   the physical device register/unregister interfaces
> 
>   [status]
>   -   basically agreed by community
> 
> 
> PART 2: vfio bus driver for mdev
> 
>   [task]
>   -   interfaces with vendor drivers
>   -   the vfio bus implementation
> 
>   [status]
> 
>   -   basically agreed by community
> 
> 
> PART 3: iommu support for mdev
> 
>   [task]
>   -   iommu support for mdev
> 
>   [status]
>   -   Kirti's v7 implementation, not yet fully reviewed
> 
> 
> PART 4: sysfs interfaces for mdev
> 
>   [task]
>   -   define the hierarchy of minimal sysfs directories/files
>   -   check the validity from vendor drivers, init/de-init 
> them
>   [status]
>   -   interfaces are in discussion
> 
> 
> PART 6: Documentation
> 
>   [task]
>   -   clearly document the architecture and interfaces
>   -   coding example for vendor drivers
> 
>   [status]
>   -   N/A
>

IMHO documentation should really be near the front as that's spelling out
the design. The implementation patches are then reviewed in reference to
the design documentation.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

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

Re: [libvirt] [Qemu-devel] summary of current vfio mdev upstreaming status

[Kirti Wankhede]

On 9/29/2016 3:16 PM, Xiao Guangrong wrote:
> 
> 
> On 09/29/2016 05:36 PM, Neo Jia wrote:
>> On Thu, Sep 29, 2016 at 05:05:47PM +0800, Xiao Guangrong wrote:
>>>
>>>
>>> On 09/29/2016 04:55 PM, Jike Song wrote:
 Hi all,

 In order to have a clear understanding about the VFIO mdev upstreaming
 status, I'd like to summarize it. Please share your opinions on this,
 and correct my misunderstandings.

 The whole vfio mdev series can be logically divided into several parts,
 they work together to provide the mdev support.
>>>
>>> I think what Jike want to suggest is how about partially push/develop
>>> the
>>> mdev. As jike listed, there are some parts can be independent and
>>> they have
>>> mostly been agreed.
>>>
>>> Such development plan can make the discussion be much efficient in the
>>> community. Also it make the possibility that Intel, Nvdia, IBM can focus
>>> on different parts and co-develop it.
>>
>> Hi Guangrong,
>>
>> JFYI. we are preparing v8 patches to accommodate most comments we have
>> discussed
>> so far and we will also include several things that we have decided on
>> sysfs.
>>
>> I definitely would like to see more interactive discussions especially
>> on the
>> sysfs class front from intel folks.
>>
>> Regarding the patch development and given the current status,
>> especially where
>> we are and what we have been through, I am very confident that we
>> should be able
>> to fully handle this ourselves, but thanks for offering help anyway!
>>
>> We should be able to react as fast as possible based on the public
>> mailing list
>> discussions, so again I don't think that part is an issue.
> 
> We understand the progress goes okay. However, splitting such big patchset
> into small parts is a better way to push large code to upstream - it is
> better for review and validation and we can quickly iterate the code if
> there are new issues exposed during the review of new version.
> 
> Particularly, based on the current situation that the sysfs interfaces are
> far from the way to be decided, it is definitely a good idea to push the
> basic infrastructure first, later let's focal on the ABI part - sysfs
> interface design.
> 

Yes, we will have v8 series soon out for review even if sysfs interface
discussion is not settled down to get other pieces reviewed and tested.
In this patch set, we will have basic set of sysfs interface which we
all have agreed on until now like 'create' and 'remove'.

Thanks,
Kirti

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

Re: [libvirt] summary of current vfio mdev upstreaming status

[Kirti Wankhede]

On 9/29/2016 2:47 PM, Neo Jia wrote:
> On Thu, Sep 29, 2016 at 04:55:39PM +0800, Jike Song wrote:
>> Hi all,
>>
>> In order to have a clear understanding about the VFIO mdev upstreaming
>> status, I'd like to summarize it. Please share your opinions on this,
>> and correct my misunderstandings.
>>
>> The whole vfio mdev series can be logically divided into several parts,
>> they work together to provide the mdev support.
> 

Thanks Jike for summarizing. We already have separate patch for each of
these logical parts. I had maintained patch sequence in incremental
depending order.

> Hi Jike,
> 
> Thanks for summarizing this, but I will defer to Kirti to comment on the 
> actual
> upstream status of her patches, couples things to note though:
> 
> 1) iommu type1 patches have been extensively reviewed by Alex already and we
> have one action item left to implement which is already queued up in v8 
> patchset.
> 

That's right Neo.

> 2) regarding the sysfs interface and libvirt discussion, I would like to hear
> what kind of attributes Intel folks are having so far as Daniel is
> asking about adding a class "gpu" which will pull several attributes as 
> mandatory.
> 
> Thanks,
> Neo
> 
>>
>>
>>
>> PART 1: mdev core driver
>>
>>  [task]
>>  -   the mdev bus/device support
>>  -   the utilities of mdev lifecycle management
>>  -   the physical device register/unregister interfaces
>>
>>  [status]
>>  -   basically agreed by community
>>
>>
>> PART 2: vfio bus driver for mdev
>>
>>  [task]
>>  -   interfaces with vendor drivers
>>  -   the vfio bus implementation
>>
>>  [status]
>>
>>  -   basically agreed by community
>>

I'm working on v8 version for above patches based on previous discussions.

>>
>> PART 3: iommu support for mdev
>>
>>  [task]
>>  -   iommu support for mdev
>>
>>  [status]
>>  -   Kirti's v7 implementation, not yet fully reviewed
>>
>>
>> PART 4: sysfs interfaces for mdev
>>
>>  [task]
>>  -   define the hierarchy of minimal sysfs directories/files
>>  -   check the validity from vendor drivers, init/de-init 
>> them
>>  [status]
>>  -   interfaces are in discussion
>>
>>

>From coding perspective, this is part of mdev core module. I think we
can't completely separate this part from mdev core module while coding
it. Yes, this interface is still in discussion and we need to settle
down on that soon.

>> PART 6: Documentation
>>
>>  [task]
>>  -   clearly document the architecture and interfaces
>>  -   coding example for vendor drivers
>>
>>  [status]
>>  -   N/A
>>

I had tried to maintain the document as per changes going on in above
patches from v6 onward and will continue to update it for each version
accordingly.

I had sent out patch with sample driver few hours back wrt v7 patchset.
And henceforth I'll keep on updating sample driver as per changes in
mdev modules and add it in my patch series.

Thanks,
Kirti

>>
>> What I'm curious here is 'PART 4', which is needed by other parts to
>> perform further steps, is it possible to accelerate the process somehow? :-)
>>
>>
>> --
>> Thanks,
>> Jike
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [libvirt] [PATCH 7/7] qemu: Allow making vcpus hotpluggable with virDomainSetVcpusFlags

[Shivaprasad G Bhat]

On 09/21/2016 05:19 PM, Peter Krempa wrote:

Implement support for VIR_DOMAIN_VCPU_HOTPLUGGABLE so that users can
choose to make vcpus added by the API removable.
---
  src/qemu/qemu_driver.c | 22 --
  1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 357be4e..8453628 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4912,7 +4912,8 @@ qemuDomainSetVcpusLive(virQEMUDriverPtr driver,
   */
  static void
  qemuDomainSetVcpusConfig(virDomainDefPtr def,
- unsigned int nvcpus)
+ unsigned int nvcpus,
+ bool hotpluggable)
  {
  virDomainVcpuDefPtr vcpu;

Here  if (curvcpus == nvcpus)

return
   we still need to allow if someone wants to switch from hotpluggable 
= yes to no/ vice versa.




  size_t curvcpus = virDomainDefGetVcpus(def);
@@ -4933,7 +4934,12 @@ qemuDomainSetVcpusConfig(virDomainDefPtr def,
  continue;

  vcpu->online = true;
-vcpu->hotpluggable = VIR_TRISTATE_BOOL_NO;
+if (hotpluggable) {
+vcpu->hotpluggable = VIR_TRISTATE_BOOL_YES;
+def->individualvcpus = true;
+} else {
+vcpu->hotpluggable = VIR_TRISTATE_BOOL_NO;
+}

  if (++curvcpus == nvcpus)
  break;


Can we add checks here to see on PPC, the config is valid with a check 
when topology is given in xml to see (curvcpus%threads_per_core == 0)


Otherwise with virsh setvcpus rhel71 13 --config --hotpluggable
for a guest with topology 
we would see,
2016-09-29 10:12:05.929+: 1137: error : 
qemuProcessValidateHotpluggableVcpus:4829 : unsupported configuration: 
vcpus '12' and '13' are in the same hotplug group but differ in 
configuration


OR

Even when setvcpus live to a number not leading to a complete core, we 
have checks leading to sensible error (error: unsupported configuration: 
target vm vcpu granularity does not allow the desired vcpu count ) . So, 
in case of --config also may be we can add the check to bring the 
consistency.


Otherwise the series looks good with the above two cases addressed . ACK

Thanks,
Shivaprasad



@@ -4960,7 +4966,8 @@ qemuDomainSetVcpusInternal(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
 virDomainDefPtr def,
 virDomainDefPtr persistentDef,
-   unsigned int nvcpus)
+   unsigned int nvcpus,
+   bool hotpluggable)
  {
  virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
  int ret = -1;
@@ -4985,7 +4992,7 @@ qemuDomainSetVcpusInternal(virQEMUDriverPtr driver,
  goto cleanup;

  if (persistentDef) {
-qemuDomainSetVcpusConfig(persistentDef, nvcpus);
+qemuDomainSetVcpusConfig(persistentDef, nvcpus, hotpluggable);

  if (virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef) 
< 0)
  goto cleanup;
@@ -5008,12 +5015,14 @@ qemuDomainSetVcpusFlags(virDomainPtr dom,
  virDomainObjPtr vm = NULL;
  virDomainDefPtr def;
  virDomainDefPtr persistentDef;
+bool hotpluggable = !!(flags & VIR_DOMAIN_VCPU_HOTPLUGGABLE);
  int ret = -1;

  virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
VIR_DOMAIN_AFFECT_CONFIG |
VIR_DOMAIN_VCPU_MAXIMUM |
-  VIR_DOMAIN_VCPU_GUEST, -1);
+  VIR_DOMAIN_VCPU_GUEST |
+  VIR_DOMAIN_VCPU_HOTPLUGGABLE, -1);

  if (!(vm = qemuDomObjFromDomain(dom)))
  goto cleanup;
@@ -5032,7 +5041,8 @@ qemuDomainSetVcpusFlags(virDomainPtr dom,
  else if (flags & VIR_DOMAIN_VCPU_MAXIMUM)
  ret = qemuDomainSetVcpusMax(driver, def, persistentDef, nvcpus);
  else
-ret = qemuDomainSetVcpusInternal(driver, vm, def, persistentDef, 
nvcpus);
+ret = qemuDomainSetVcpusInternal(driver, vm, def, persistentDef,
+ nvcpus, hotpluggable);

   endjob:
  qemuDomainObjEndJob(driver, vm);


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

Re: [libvirt] [PATCH] vsh: Write out history on "quit" or "exit" in interactive mode

[Erik Skultety]

On 29/09/16 12:49, John Ferlan wrote:
> 
> 
> On 09/29/2016 03:06 AM, Erik Skultety wrote:
>> On 28/09/16 21:27, John Ferlan wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1379895
>>>
>>> Introduced by commit id '834c5720'.
>>>
>>> During the code motion and creation of vsh.c, the function 'vshDeinit()'
>>> in the (new) vsh.c was altered from whence it came in virsh.c such that
>>> calling 'vshReadlineDeinit(ctl)' was conditional on "ctl->imode".
>>>
>>> This causes a problem for the interactive running if the "quit" and "exit"
>>> commands are used because 'cmdQuit' will clear ctl->imode, thus when the
>>> interactive loop in main() of virsh.c exits because ctl->mode is clear and
>>> virshDeinit is called which calls vshDeinit, the history file is now not
>>> written. Conversely, if one had exited the interactive loop via pressing
>>> D the file would be created because loop control is broken on EOF
>>> and ctl->imode is not set to false.
>>>
>>> This patch will remove the conditional call to vshReadlineDeinit and
>>> restore the former behaviour.
>>>
>>> Signed-off-by: John Ferlan 
>>> ---
>>>  I realize some people don't like the comment I added; however, I'd
>>>  rather be "safe" when purely reading the code than expect someone
>>>  to do a git log -p looking at commit messages for "removed" lines of code.
>>>
>>>  An alternative approach would be to create/set a new boolean value
>>>  such as "iquit" (or "iexit") during cmdQuit and then in vshDeinit make
>>>  calling vshReadlineDeinit() conditional on "ctl->imode || ctl->iquit"
>>>
>>>  Calling the vshReadlineDeinit during non interactive mode would have no
>>>  affect since vshReadlineInit is only called if ctl->imode is set. The
>>>  Deinit function will essentially do nothing other than a couple of
>>>  VIR_FREE(NULL); and one extra "if"
>>>
>>>  tools/vsh.c | 8 ++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/vsh.c b/tools/vsh.c
>>> index 041113f..eba3f0f 100644
>>> --- a/tools/vsh.c
>>> +++ b/tools/vsh.c
>>> @@ -3093,8 +3093,12 @@ vshInitReload(vshControl *ctl)
>>>  void
>>>  vshDeinit(vshControl *ctl)
>>>  {
>>> -if (ctl->imode)
>>> -vshReadlineDeinit(ctl);
>>> +/* Don't make calling vshReadlineDeinit conditional on imode. During
>>> + * interactive mode when "quit" or "exit" is typed, 'imode' is set
>>> + * to false so if this were conditional on imode, then history wouldn't
>>> + * be written when the "quit" or "exit" commands were used instead of
>>> + * when D is used */
>>
>> Well, the commit message is already pretty verbose. I do understand your
>> concern about having to wade through log -p output, especially when you
>> encounter something like 834c5720 which truly is evil (I started over
>> like 3 times with 3 different ugly results), but once you make friends
>> with gitk, tig or whatever interactive tool, everything becomes a little
>> more convenient. In conclusion, I think something like "Don't make
>> calling of vshReadlineDeinit conditional on active interactive mode."
>> would suffice.
>>
>> ACK
>> Erik
>>
> 
> I'll adjust the message before pushing. And yes, I use gitk and git log
> -p frequently, but not everyone does and the concern is someone alters
> the code without looking at the history. Considering it took 13+ months

Hmm, in my opinion that's just telling us that either noone is truly
depending on that feature or they're just used to hit D
automatically...

> to have someone realize it - I wanted some way to make clear that
> someone needs to look at the history before just changing this. I can
> completely understand why the conditional was added though since the
> Init calls gate on ctl->imode
> 
> Shall I assume it's "safe" for the freeze too?
> 

Sure, I'm sorry I didn't write it explicitly, I automatically assumed
that to be safe, since a) it's a bugfix, b) it's a regression that
should really be fixed as soon as possible, my bad :)...

Erik

> John
>>> +vshReadlineDeinit(ctl);
>>>  vshCloseLogFile(ctl);
>>>  }
>>>  
>>>
>>
>>




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

Re: [libvirt] [PATCH] vsh: Write out history on "quit" or "exit" in interactive mode

[John Ferlan]

On 09/29/2016 03:06 AM, Erik Skultety wrote:
> On 28/09/16 21:27, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1379895
>>
>> Introduced by commit id '834c5720'.
>>
>> During the code motion and creation of vsh.c, the function 'vshDeinit()'
>> in the (new) vsh.c was altered from whence it came in virsh.c such that
>> calling 'vshReadlineDeinit(ctl)' was conditional on "ctl->imode".
>>
>> This causes a problem for the interactive running if the "quit" and "exit"
>> commands are used because 'cmdQuit' will clear ctl->imode, thus when the
>> interactive loop in main() of virsh.c exits because ctl->mode is clear and
>> virshDeinit is called which calls vshDeinit, the history file is now not
>> written. Conversely, if one had exited the interactive loop via pressing
>> D the file would be created because loop control is broken on EOF
>> and ctl->imode is not set to false.
>>
>> This patch will remove the conditional call to vshReadlineDeinit and
>> restore the former behaviour.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  I realize some people don't like the comment I added; however, I'd
>>  rather be "safe" when purely reading the code than expect someone
>>  to do a git log -p looking at commit messages for "removed" lines of code.
>>
>>  An alternative approach would be to create/set a new boolean value
>>  such as "iquit" (or "iexit") during cmdQuit and then in vshDeinit make
>>  calling vshReadlineDeinit() conditional on "ctl->imode || ctl->iquit"
>>
>>  Calling the vshReadlineDeinit during non interactive mode would have no
>>  affect since vshReadlineInit is only called if ctl->imode is set. The
>>  Deinit function will essentially do nothing other than a couple of
>>  VIR_FREE(NULL); and one extra "if"
>>
>>  tools/vsh.c | 8 ++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/vsh.c b/tools/vsh.c
>> index 041113f..eba3f0f 100644
>> --- a/tools/vsh.c
>> +++ b/tools/vsh.c
>> @@ -3093,8 +3093,12 @@ vshInitReload(vshControl *ctl)
>>  void
>>  vshDeinit(vshControl *ctl)
>>  {
>> -if (ctl->imode)
>> -vshReadlineDeinit(ctl);
>> +/* Don't make calling vshReadlineDeinit conditional on imode. During
>> + * interactive mode when "quit" or "exit" is typed, 'imode' is set
>> + * to false so if this were conditional on imode, then history wouldn't
>> + * be written when the "quit" or "exit" commands were used instead of
>> + * when D is used */
> 
> Well, the commit message is already pretty verbose. I do understand your
> concern about having to wade through log -p output, especially when you
> encounter something like 834c5720 which truly is evil (I started over
> like 3 times with 3 different ugly results), but once you make friends
> with gitk, tig or whatever interactive tool, everything becomes a little
> more convenient. In conclusion, I think something like "Don't make
> calling of vshReadlineDeinit conditional on active interactive mode."
> would suffice.
> 
> ACK
> Erik
> 

I'll adjust the message before pushing. And yes, I use gitk and git log
-p frequently, but not everyone does and the concern is someone alters
the code without looking at the history. Considering it took 13+ months
to have someone realize it - I wanted some way to make clear that
someone needs to look at the history before just changing this. I can
completely understand why the conditional was added though since the
Init calls gate on ctl->imode

Shall I assume it's "safe" for the freeze too?

John
>> +vshReadlineDeinit(ctl);
>>  vshCloseLogFile(ctl);
>>  }
>>  
>>
> 
> 

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

Re: [libvirt] [RFC] daemon: remove hardcode dep on libvirt-guests

[Nikolay Shirokovskiy]

On 29.09.2016 13:06, Daniel P. Berrange wrote:
> On Thu, Sep 29, 2016 at 01:00:15PM +0300, Nikolay Shirokovskiy wrote:
>>
>>
>> On 29.09.2016 12:21, Jiri Denemark wrote:
>>> On Thu, Sep 29, 2016 at 11:33:04 +0300, Nikolay Shirokovskiy wrote:
 Hi, all.

 In virtuozzo mgmt we do not use libvirt-guests service. First because
 we need do extra steps on domain start and second we want to decice
 whether to suspend or to shutdown a domain on per domain basis. Starting
 is not the problem but system shutdown is. As domain in systemd based
 systems is just another unit we need to set ordering dependency so
 that domain will not be killed before mgmt service as ba79e387 do for
 libvirt-guest service. So let's remove this hardcode. I see 2 options.
>>>
>>> I don't see hard dependency anywhere in libvirtd.service, it just says
>>>
>>> Before=libvirt-guests.service
>>
>> Nope, I meant this code from mentioned commit:
>>
>> @@ -243,8 +243,10 @@ int virSystemdCreateMachine(const char *name,
>>iscontainer ? "container" : "vm",
>>(unsigned int)pidleader,
>>rootdir ? rootdir : "",
>> -  1, "Slice", "s",
>> -  slicename) < 0)
>> +  3,
>> +  "Slice", "s", slicename,
>> +  "After", "as", 1, "libvirtd.service",
>> +  "Before", "as", 1, "libvirt-guests.service") < 0)
>>
>> This makes domain a special kind of unit (scope) and sets its ordering 
>> dependency.
>> You can't do this with libvirtd.service itself because its different unit.
> 
> IIUC, the problem is that you want to replace 'libvirt-guests.service'
> with a different impl ?
> 
> The way systemd tends to deal with things that must be a configurable
> choice in this way is to define a target, and sysadmins can then make
> arbitrary services dependancies of that target.
> 
> eg, we'd set 'Before: virt-guest-shutdown.target', and then make
> libvirt-guests.service be a part of that target by default. You can
> then have ability to turn off libvirt-guest.service and put your
> own custom thing inside virt-guest-shutdown.taget
> 

Thanx!

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

Re: [libvirt] [RFC] daemon: remove hardcode dep on libvirt-guests

[Daniel P. Berrange]

On Thu, Sep 29, 2016 at 01:00:15PM +0300, Nikolay Shirokovskiy wrote:
> 
> 
> On 29.09.2016 12:21, Jiri Denemark wrote:
> > On Thu, Sep 29, 2016 at 11:33:04 +0300, Nikolay Shirokovskiy wrote:
> >> Hi, all.
> >>
> >> In virtuozzo mgmt we do not use libvirt-guests service. First because
> >> we need do extra steps on domain start and second we want to decice
> >> whether to suspend or to shutdown a domain on per domain basis. Starting
> >> is not the problem but system shutdown is. As domain in systemd based
> >> systems is just another unit we need to set ordering dependency so
> >> that domain will not be killed before mgmt service as ba79e387 do for
> >> libvirt-guest service. So let's remove this hardcode. I see 2 options.
> > 
> > I don't see hard dependency anywhere in libvirtd.service, it just says
> > 
> > Before=libvirt-guests.service
> 
> Nope, I meant this code from mentioned commit:
> 
> @@ -243,8 +243,10 @@ int virSystemdCreateMachine(const char *name,
>iscontainer ? "container" : "vm",
>(unsigned int)pidleader,
>rootdir ? rootdir : "",
> -  1, "Slice", "s",
> -  slicename) < 0)
> +  3,
> +  "Slice", "s", slicename,
> +  "After", "as", 1, "libvirtd.service",
> +  "Before", "as", 1, "libvirt-guests.service") < 0)
> 
> This makes domain a special kind of unit (scope) and sets its ordering 
> dependency.
> You can't do this with libvirtd.service itself because its different unit.

IIUC, the problem is that you want to replace 'libvirt-guests.service'
with a different impl ?

The way systemd tends to deal with things that must be a configurable
choice in this way is to define a target, and sysadmins can then make
arbitrary services dependancies of that target.

eg, we'd set 'Before: virt-guest-shutdown.target', and then make
libvirt-guests.service be a part of that target by default. You can
then have ability to turn off libvirt-guest.service and put your
own custom thing inside virt-guest-shutdown.taget

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

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

Re: [libvirt] [RFC] daemon: remove hardcode dep on libvirt-guests

[Nikolay Shirokovskiy]

On 29.09.2016 12:21, Jiri Denemark wrote:
> On Thu, Sep 29, 2016 at 11:33:04 +0300, Nikolay Shirokovskiy wrote:
>> Hi, all.
>>
>> In virtuozzo mgmt we do not use libvirt-guests service. First because
>> we need do extra steps on domain start and second we want to decice
>> whether to suspend or to shutdown a domain on per domain basis. Starting
>> is not the problem but system shutdown is. As domain in systemd based
>> systems is just another unit we need to set ordering dependency so
>> that domain will not be killed before mgmt service as ba79e387 do for
>> libvirt-guest service. So let's remove this hardcode. I see 2 options.
> 
> I don't see hard dependency anywhere in libvirtd.service, it just says
> 
> Before=libvirt-guests.service

Nope, I meant this code from mentioned commit:

@@ -243,8 +243,10 @@ int virSystemdCreateMachine(const char *name,
   iscontainer ? "container" : "vm",
   (unsigned int)pidleader,
   rootdir ? rootdir : "",
-  1, "Slice", "s",
-  slicename) < 0)
+  3,
+  "Slice", "s", slicename,
+  "After", "as", 1, "libvirtd.service",
+  "Before", "as", 1, "libvirt-guests.service") < 0)

This makes domain a special kind of unit (scope) and sets its ordering 
dependency.
You can't do this with libvirtd.service itself because its different unit.

Nikolay

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

Re: [libvirt] [Qemu-devel] summary of current vfio mdev upstreaming status

[Xiao Guangrong]

On 09/29/2016 05:36 PM, Neo Jia wrote:

On Thu, Sep 29, 2016 at 05:05:47PM +0800, Xiao Guangrong wrote:



On 09/29/2016 04:55 PM, Jike Song wrote:

Hi all,

In order to have a clear understanding about the VFIO mdev upstreaming
status, I'd like to summarize it. Please share your opinions on this,
and correct my misunderstandings.

The whole vfio mdev series can be logically divided into several parts,
they work together to provide the mdev support.


I think what Jike want to suggest is how about partially push/develop the
mdev. As jike listed, there are some parts can be independent and they have
mostly been agreed.

Such development plan can make the discussion be much efficient in the
community. Also it make the possibility that Intel, Nvdia, IBM can focus
on different parts and co-develop it.


Hi Guangrong,

JFYI. we are preparing v8 patches to accommodate most comments we have discussed
so far and we will also include several things that we have decided on sysfs.

I definitely would like to see more interactive discussions especially on the
sysfs class front from intel folks.

Regarding the patch development and given the current status, especially where
we are and what we have been through, I am very confident that we should be able
to fully handle this ourselves, but thanks for offering help anyway!

We should be able to react as fast as possible based on the public mailing list
discussions, so again I don't think that part is an issue.


We understand the progress goes okay. However, splitting such big patchset
into small parts is a better way to push large code to upstream - it is
better for review and validation and we can quickly iterate the code if
there are new issues exposed during the review of new version.

Particularly, based on the current situation that the sysfs interfaces are
far from the way to be decided, it is definitely a good idea to push the
basic infrastructure first, later let's focal on the ABI part - sysfs
interface design.

Thanks!





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

Re: [libvirt] [Qemu-devel] summary of current vfio mdev upstreaming status

[Neo Jia]

On Thu, Sep 29, 2016 at 05:05:47PM +0800, Xiao Guangrong wrote:
> 
> 
> On 09/29/2016 04:55 PM, Jike Song wrote:
> > Hi all,
> > 
> > In order to have a clear understanding about the VFIO mdev upstreaming
> > status, I'd like to summarize it. Please share your opinions on this,
> > and correct my misunderstandings.
> > 
> > The whole vfio mdev series can be logically divided into several parts,
> > they work together to provide the mdev support.
> 
> I think what Jike want to suggest is how about partially push/develop the
> mdev. As jike listed, there are some parts can be independent and they have
> mostly been agreed.
> 
> Such development plan can make the discussion be much efficient in the
> community. Also it make the possibility that Intel, Nvdia, IBM can focus
> on different parts and co-develop it.

Hi Guangrong,

JFYI. we are preparing v8 patches to accommodate most comments we have discussed
so far and we will also include several things that we have decided on sysfs.

I definitely would like to see more interactive discussions especially on the
sysfs class front from intel folks.

Regarding the patch development and given the current status, especially where
we are and what we have been through, I am very confident that we should be able
to fully handle this ourselves, but thanks for offering help anyway!

We should be able to react as fast as possible based on the public mailing list
discussions, so again I don't think that part is an issue.

Thanks,
Neo

> 
> The maintainer can hold these development patches in local branch before
> pushing the full-functionality version to upstream.
> 
> Thanks!
> 
> 

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

Re: [libvirt] [RFC] daemon: remove hardcode dep on libvirt-guests

[Jiri Denemark]

On Thu, Sep 29, 2016 at 11:33:04 +0300, Nikolay Shirokovskiy wrote:
> Hi, all.
> 
> In virtuozzo mgmt we do not use libvirt-guests service. First because
> we need do extra steps on domain start and second we want to decice
> whether to suspend or to shutdown a domain on per domain basis. Starting
> is not the problem but system shutdown is. As domain in systemd based
> systems is just another unit we need to set ordering dependency so
> that domain will not be killed before mgmt service as ba79e387 do for
> libvirt-guest service. So let's remove this hardcode. I see 2 options.

I don't see hard dependency anywhere in libvirtd.service, it just says

Before=libvirt-guests.service

> 1. Take dependant service name from config.
> 2. Introduce intermediate service that will specify real dependency.
> Looks like this one is more flexible.

Do you just need to stick another service between libvirtd and
libvirt-guests? If so, it should be fairly easy to do with just two
additional lines in the new services:

foo.service
[Unit]
Before=libvirt-guests.service
After=libvirtd.service

In case you want to get rid of libvirt-guests completely, just disable
it and you can even avoid the Before=libvirt-guests.service line in your
new service, although keeping it there won't hurt.

Jirka

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

Re: [libvirt] summary of current vfio mdev upstreaming status

[Neo Jia]

On Thu, Sep 29, 2016 at 04:55:39PM +0800, Jike Song wrote:
> Hi all,
> 
> In order to have a clear understanding about the VFIO mdev upstreaming
> status, I'd like to summarize it. Please share your opinions on this,
> and correct my misunderstandings.
> 
> The whole vfio mdev series can be logically divided into several parts,
> they work together to provide the mdev support.

Hi Jike,

Thanks for summarizing this, but I will defer to Kirti to comment on the actual
upstream status of her patches, couples things to note though:

1) iommu type1 patches have been extensively reviewed by Alex already and we
have one action item left to implement which is already queued up in v8 
patchset.

2) regarding the sysfs interface and libvirt discussion, I would like to hear
what kind of attributes Intel folks are having so far as Daniel is
asking about adding a class "gpu" which will pull several attributes as 
mandatory.

Thanks,
Neo

> 
> 
> 
> PART 1: mdev core driver
> 
>   [task]
>   -   the mdev bus/device support
>   -   the utilities of mdev lifecycle management
>   -   the physical device register/unregister interfaces
> 
>   [status]
>   -   basically agreed by community
> 
> 
> PART 2: vfio bus driver for mdev
> 
>   [task]
>   -   interfaces with vendor drivers
>   -   the vfio bus implementation
> 
>   [status]
> 
>   -   basically agreed by community
> 
> 
> PART 3: iommu support for mdev
> 
>   [task]
>   -   iommu support for mdev
> 
>   [status]
>   -   Kirti's v7 implementation, not yet fully reviewed
> 
> 
> PART 4: sysfs interfaces for mdev
> 
>   [task]
>   -   define the hierarchy of minimal sysfs directories/files
>   -   check the validity from vendor drivers, init/de-init 
> them
>   [status]
>   -   interfaces are in discussion
> 
> 
> PART 6: Documentation
> 
>   [task]
>   -   clearly document the architecture and interfaces
>   -   coding example for vendor drivers
> 
>   [status]
>   -   N/A
> 
> 
> What I'm curious here is 'PART 4', which is needed by other parts to
> perform further steps, is it possible to accelerate the process somehow? :-)
> 
> 
> --
> Thanks,
> Jike
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [libvirt] [Qemu-devel] summary of current vfio mdev upstreaming status

[Xiao Guangrong]

On 09/29/2016 04:55 PM, Jike Song wrote:

Hi all,

In order to have a clear understanding about the VFIO mdev upstreaming
status, I'd like to summarize it. Please share your opinions on this,
and correct my misunderstandings.

The whole vfio mdev series can be logically divided into several parts,
they work together to provide the mdev support.


I think what Jike want to suggest is how about partially push/develop the
mdev. As jike listed, there are some parts can be independent and they have
mostly been agreed.

Such development plan can make the discussion be much efficient in the
community. Also it make the possibility that Intel, Nvdia, IBM can focus
on different parts and co-develop it.

The maintainer can hold these development patches in local branch before
pushing the full-functionality version to upstream.

Thanks!


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

Re: [libvirt] [libvirt-php][PATCH 00/11] Resolve reverse include order

[Michal Privoznik]

On 28.09.2016 14:00, Vasiliy Tolstov wrote:
> 2016-09-27 16:11 GMT+03:00 Michal Privoznik :
>> These are not pushed yet as they might be somewhat controversial.
>> I'll wait if somebody wants to review them.
>>
>> The ultimate goal is to, well drop libvirt-php.h completely. It
>> is needless. But before going there we must distribute the
>> interesting parts from it around. Therefore I'm introducing some
>> modules (like it should have been done from the start).
> 
> 
> I like this idea to move extension helper to external files.
> 

Yeah, thank you. I've pushed the patches now.

I know this wasn't an explicit ACK, but things work slightly different
here compared to libvirt :-)

Michal

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

Re: [libvirt] [PATCH v2] virsh domdisplay: introduce '--all' for showing all possible graphical display

[Chen Hanxiao]

At 2016-09-29 14:27:56, "Michal Privoznik"  wrote:
>On 28.09.2016 15:31, Chen Hanxiao wrote:
>> From: Chen Hanxiao 
>> 
>> For one VM, it could had more than one graphical display.
>> Such as we coud add both vnc and spice display to a VM.
>> 
>> This patch introduces '--all' for showing all
>> possible graphical display of a active VM.
>> 
>> Signed-off-by: Chen Hanxiao 
>> ---
>> v2: VIR_FREE befor use in loops
>> add descriptions in virsh.pod
>> 
>>  tools/virsh-domain.c | 15 ++-
>>  tools/virsh.pod  |  3 ++-
>>  2 files changed, 16 insertions(+), 2 deletions(-)
>
>This one looks better. But I've got some points.
>
>> 
>> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
>> index 3829b17..a6124b6 100644
>> --- a/tools/virsh-domain.c
>> +++ b/tools/virsh-domain.c
>> @@ -10648,6 +10648,10 @@ static const vshCmdOptDef opts_domdisplay[] = {
>>   .help = N_("select particular graphical display "
>>  "(e.g. \"vnc\", \"spice\", \"rdp\")")
>>  },
>> +{.name = "all",
>> + .type = VSH_OT_BOOL,
>> + .help = N_("show all possible graphical displays")
>> +},
>>  {.name = NULL}
>>  };
>>  
>
>What should happen if users pass both --type and --all? In that case the 
>semantics for --all is changed I guess and according to this code we would 
>print all URIs for given type. However, there can be just one graphical type 
>per domain and thus one URI.

We had code:
  if (type && STRNEQ(type, scheme[iter])) 
  continue;
in the front of the loop.
Maybe we should ignore --type if --all specified.

[...]

>
>Almost. You forgot to update the list of arguments:
>
>-=item B I [I<--include-password>] [[I<--type>] B]
>+=item B I [I<--include-password>] [[I<--type>] B] 
>[I<--all>]
>
>Normally, I'd fix this before pushing and just point it out in review, but now 
>we are in the freeze and this is a feature (during freeze only bugfixes can be 
>pushed). Moreover, there's the unclear behaviour I described above.
>

I'll send a v3 patch to address these issues for the next version of libvirt.

Regards,
- Chen

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

[libvirt] [PATCH 2/2] qemu: Add support for preallocated fd memory

[Jaroslav Safka]

This second change introduces support for preallocated
shared file descriptor based memory backing.
It allows vhost-user to be used without hugepages.

Used xml elements:

  
  
  

---
 src/qemu/qemu_command.c| 156 -
 src/qemu/qemu_command.h|   4 +
 .../qemuxml2argv-fd-memory-no-numa-topology.args   |  34 +
 .../qemuxml2argv-fd-memory-no-numa-topology.xml|  96 +
 .../qemuxml2argv-fd-memory-numa-topology.args  |  34 +
 .../qemuxml2argv-fd-memory-numa-topology.xml   |  99 +
 tests/qemuxml2argvtest.c   |  24 
 7 files changed, 414 insertions(+), 33 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-fd-memory-no-numa-topology.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-fd-memory-no-numa-topology.xml
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology.xml

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index f24a98b..ff661ef 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3275,15 +3275,11 @@ qemuBuildMemoryBackendStr(unsigned long long size,
 if (!(props = virJSONValueNewObject()))
 return -1;
 
-if (pagesize) {
-if (qemuGetHupageMemPath(cfg, pagesize, _path) < 0)
-goto cleanup;
-
+if (def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) {
 *backendType = "memory-backend-file";
 
 if (virJSONValueObjectAdd(props,
-  "b:prealloc", true,
-  "s:mem-path", mem_path,
+  "s:mem-path", def->mem.mem_path,
   NULL) < 0)
 goto cleanup;
 
@@ -3299,18 +3295,61 @@ qemuBuildMemoryBackendStr(unsigned long long size,
 break;
 
 case VIR_NUMA_MEM_ACCESS_DEFAULT:
+if (def->mem.access == VIR_DOMAIN_MEMORY_ACCESS_SHARED) {
+if (virJSONValueObjectAdd(props, "b:share", true, NULL) < 0)
+goto cleanup;
+}
+break;
+
 case VIR_NUMA_MEM_ACCESS_LAST:
 break;
 }
+
+force = true;
 } else {
-if (memAccess) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("Shared memory mapping is supported "
- "only with hugepages"));
-goto cleanup;
-}
+if (pagesize) {
+if (qemuGetHupageMemPath(cfg, pagesize, _path) < 0)
+goto cleanup;
 
-*backendType = "memory-backend-ram";
+*backendType = "memory-backend-file";
+
+if (virJSONValueObjectAdd(props,
+  "b:prealloc", true,
+  "s:mem-path", mem_path,
+  NULL) < 0)
+goto cleanup;
+
+switch (memAccess) {
+case VIR_NUMA_MEM_ACCESS_SHARED:
+if (virJSONValueObjectAdd(props, "b:share", true, NULL) < 0)
+goto cleanup;
+break;
+
+case VIR_NUMA_MEM_ACCESS_PRIVATE:
+if (virJSONValueObjectAdd(props, "b:share", false, NULL) < 0)
+goto cleanup;
+break;
+
+case VIR_NUMA_MEM_ACCESS_DEFAULT:
+if (def->mem.access == VIR_DOMAIN_MEMORY_ACCESS_SHARED) {
+if (virJSONValueObjectAdd(props, "b:share", true, NULL) < 
0)
+goto cleanup;
+}
+break;
+
+case VIR_NUMA_MEM_ACCESS_LAST:
+break;
+}
+} else {
+if (memAccess) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("Shared memory mapping is supported "
+ "only with hugepages"));
+goto cleanup;
+}
+
+*backendType = "memory-backend-ram";
+}
 }
 
 if (virJSONValueObjectAdd(props, "U:size", size * 1024, NULL) < 0)
@@ -7153,28 +7192,35 @@ qemuBuildMemPathStr(virQEMUDriverConfigPtr cfg,
 const long system_page_size = virGetSystemPageSizeKB();
 char *mem_path = NULL;
 
-/*
- *  No-op if hugepages were not requested.
- */
-if (!def->mem.nhugepages)
+if (def->mem.allocation == VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE) {
+virCommandAddArgList(cmd, "-mem-prealloc", NULL);
 return 0;
+}
 
-/* There is one special case: if user specified "huge"
- * pages of regular system pages size.
- * And there is nothing to do in this case.
- */
-if (def->mem.hugepages[0].size == system_page_size)
-return 0;
+if (def->mem.nhugepages) {
 
-if 

[libvirt] summary of current vfio mdev upstreaming status

[Jike Song]

Hi all,

In order to have a clear understanding about the VFIO mdev upstreaming
status, I'd like to summarize it. Please share your opinions on this,
and correct my misunderstandings.

The whole vfio mdev series can be logically divided into several parts,
they work together to provide the mdev support.



PART 1: mdev core driver

[task]
-   the mdev bus/device support
-   the utilities of mdev lifecycle management
-   the physical device register/unregister interfaces

[status]
-   basically agreed by community


PART 2: vfio bus driver for mdev

[task]
-   interfaces with vendor drivers
-   the vfio bus implementation

[status]

-   basically agreed by community


PART 3: iommu support for mdev

[task]
-   iommu support for mdev

[status]
-   Kirti's v7 implementation, not yet fully reviewed


PART 4: sysfs interfaces for mdev

[task]
-   define the hierarchy of minimal sysfs directories/files
-   check the validity from vendor drivers, init/de-init 
them
[status]
-   interfaces are in discussion


PART 6: Documentation

[task]
-   clearly document the architecture and interfaces
-   coding example for vendor drivers

[status]
-   N/A


What I'm curious here is 'PART 4', which is needed by other parts to
perform further steps, is it possible to accelerate the process somehow? :-)


--
Thanks,
Jike

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

[libvirt] [PATCH 1/2] conf: Add support for preallocated fd memory

[Jaroslav Safka]

This first change introduces xml parsing support for preallocated
shared file descriptor based memory backing.
It allows vhost-user to be used without hugepages.

New xml elements:

  
  
  

---
 docs/schemas/domaincommon.rng  |  37 +
 src/conf/domain_conf.c | 149 -
 src/conf/domain_conf.h |  34 +
 .../qemuxml2argv-memorybacking-set.xml |  32 +
 .../qemuxml2argv-memorybacking-unset.xml   |  32 +
 .../qemuxml2xmlout-memorybacking-set.xml   |  40 ++
 .../qemuxml2xmlout-memorybacking-unset.xml |  40 ++
 tests/qemuxml2xmltest.c|   3 +
 8 files changed, 334 insertions(+), 33 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memorybacking-set.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memorybacking-unset.xml
 create mode 100644 
tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-set.xml
 create mode 100644 
tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-unset.xml

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 95c7882..99f93b7 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -559,6 +559,43 @@
 
   
 
+
+  
+
+
+  
+file
+anonymous
+  
+
+
+  
+
+  
+
+
+  
+
+
+  
+
+  
+shared
+private
+  
+
+  
+
+
+  
+
+  
+immediate
+ondemand
+  
+
+  
+
   
 
   
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a06da46..7f73569 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -829,6 +829,21 @@ VIR_ENUM_IMPL(virDomainDiskMirrorState, 
VIR_DOMAIN_DISK_MIRROR_STATE_LAST,
   "abort",
   "pivot")
 
+VIR_ENUM_IMPL(virDomainMemorySource, VIR_DOMAIN_MEMORY_SOURCE_LAST,
+  "none",
+  "file",
+  "anonymous")
+
+VIR_ENUM_IMPL(virDomainMemoryAccess, VIR_DOMAIN_MEMORY_ACCESS_LAST,
+  "none",
+  "shared",
+  "private")
+
+VIR_ENUM_IMPL(virDomainMemoryAllocation, VIR_DOMAIN_MEMORY_ALLOCATION_LAST,
+  "none",
+  "immediate",
+  "ondemand")
+
 VIR_ENUM_IMPL(virDomainLoader,
   VIR_DOMAIN_LOADER_TYPE_LAST,
   "rom",
@@ -16179,48 +16194,101 @@ virDomainDefParseXML(xmlDocPtr xml,
 }
 VIR_FREE(tmp);
 
-if ((n = virXPathNodeSet("./memoryBacking/hugepages/page", ctxt, )) 
< 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("cannot extract hugepages nodes"));
-goto error;
+tmp = virXPathString("string(./memoryBacking/source/@type)", ctxt);
+if (tmp) {
+if ((def->mem.source = virDomainMemorySourceTypeFromString(tmp)) < 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("unknown memoryBacking/source/type '%s'"), tmp);
+goto error;
+}
+VIR_FREE(tmp);
+if (def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) {
+tmp = virXPathString("string(./memoryBacking/source/@path)", ctxt);
+if (!tmp && VIR_STRDUP(tmp, VIR_DOMAIN_MEMORY_DEFAULT_PATH) < 0)
+goto error;
+
+def->mem.mem_path = tmp;
+tmp = NULL;
+}
 }
 
-if (n) {
-if (VIR_ALLOC_N(def->mem.hugepages, n) < 0)
+tmp = virXPathString("string(./memoryBacking/access/@mode)", ctxt);
+if (tmp) {
+if ((def->mem.access = virDomainMemoryAccessTypeFromString(tmp)) < 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("unknown memoryBacking/access/mode '%s'"), tmp);
 goto error;
+}
+VIR_FREE(tmp);
+}
 
-for (i = 0; i < n; i++) {
-if (virDomainHugepagesParseXML(nodes[i], ctxt,
-   >mem.hugepages[i]) < 0)
+tmp = virXPathString("string(./memoryBacking/allocation/@mode)", ctxt);
+if (tmp) {
+if ((def->mem.allocation = 
virDomainMemoryAllocationTypeFromString(tmp)) < 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("unknown memoryBacking/allocation/mode '%s'"), 
tmp);
+goto error;
+}
+VIR_FREE(tmp);
+}
+
+if (virXPathNode("./memoryBacking/hugepages", 

[libvirt] [PATCH 0/2] Add support for preallocated fd memory

[Jaroslav Safka]

Hi the xml element to be extended with additional children is the memoryBacking 
element.

We would like to introduce 3 new elements source,access and allocation


 
 
  


If allocation is immediate then -mem-prealloc should be added to the qemu 
commanline.
If source is file then
-object memory-backend-file,id=mem,size=1024M,mem-path=/var/lib/libvirt/qemu 
-numa node,memdev=mem 
Should be added to the qemu commandline

If access is shared then the share=on parameter should be added to the 
memory-backend-file e.g.
-object 
memory-backend-file,id=mem,size=1024M,mem-path=/var/lib/libvirt/qemu,share=on

Jaroslav Safka (2):
  Add support for preallocated fd memory
  Add support for preallocated fd memory

 docs/schemas/domaincommon.rng  |  37 +
 src/conf/domain_conf.c | 149 +++-
 src/conf/domain_conf.h |  34 +
 src/qemu/qemu_command.c| 156 -
 src/qemu/qemu_command.h|   4 +
 .../qemuxml2argv-fd-memory-no-numa-topology.args   |  34 +
 .../qemuxml2argv-fd-memory-no-numa-topology.xml|  96 +
 .../qemuxml2argv-fd-memory-numa-topology.args  |  34 +
 .../qemuxml2argv-fd-memory-numa-topology.xml   |  99 +
 .../qemuxml2argv-memorybacking-set.xml |  32 +
 .../qemuxml2argv-memorybacking-unset.xml   |  32 +
 tests/qemuxml2argvtest.c   |  24 
 .../qemuxml2xmlout-memorybacking-set.xml   |  40 ++
 .../qemuxml2xmlout-memorybacking-unset.xml |  40 ++
 tests/qemuxml2xmltest.c|   3 +
 15 files changed, 748 insertions(+), 66 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-fd-memory-no-numa-topology.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-fd-memory-no-numa-topology.xml
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-fd-memory-numa-topology.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memorybacking-set.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memorybacking-unset.xml
 create mode 100644 
tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-set.xml
 create mode 100644 
tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-unset.xml

-- 
2.5.5

--
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.

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

[libvirt] [RFC] daemon: remove hardcode dep on libvirt-guests

[Nikolay Shirokovskiy]

Hi, all.

In virtuozzo mgmt we do not use libvirt-guests service. First because
we need do extra steps on domain start and second we want to decice
whether to suspend or to shutdown a domain on per domain basis. Starting
is not the problem but system shutdown is. As domain in systemd based
systems is just another unit we need to set ordering dependency so
that domain will not be killed before mgmt service as ba79e387 do for
libvirt-guest service. So let's remove this hardcode. I see 2 options.

1. Take dependant service name from config.
2. Introduce intermediate service that will specify real dependency.
Looks like this one is more flexible.

By the way AFAIK this is the only place that harcode libvirt-guests in libvirtd.

Nikolay

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

Re: [libvirt] [RFC v2] libvirt vGPU QEMU integration

[Neo Jia]

On Thu, Sep 29, 2016 at 09:03:40AM +0100, Daniel P. Berrange wrote:
> On Wed, Sep 28, 2016 at 12:22:35PM -0700, Neo Jia wrote:
> > On Thu, Sep 22, 2016 at 03:26:38PM +0100, Daniel P. Berrange wrote:
> > > On Thu, Sep 22, 2016 at 08:19:21AM -0600, Alex Williamson wrote:
> > > > On Thu, 22 Sep 2016 09:41:20 +0530
> > > > Kirti Wankhede  wrote:
> > > > 
> > > > > > My concern is that a type id seems arbitrary but we're 
> > > > > > specifying that
> > > > > > it be unique.  We already have something unique, the name.  So 
> > > > > > why try
> > > > > > to make the type id unique as well?  A vendor can accidentally 
> > > > > > create
> > > > > > their vendor driver so that a given name means something very
> > > > > > specific.  On the other hand they need to be extremely 
> > > > > > deliberate to
> > > > > > coordinate that a type id means a unique thing across all their 
> > > > > > product
> > > > > > lines.
> > > > > >   
> > > > > 
> > > > >  Let me clarify, type id should be unique in the list of
> > > > >  mdev_supported_types. You can't have 2 directories in with same 
> > > > >  name.
> > > > > >>>
> > > > > >>> Of course, but does that mean it's only unique to the machine I'm
> > > > > >>> currently running on?  Let's say I have a Tesla P100 on my system 
> > > > > >>> and
> > > > > >>> type-id 11 is named "GRID-M60-0B".  At some point in the future I
> > > > > >>> replace the Tesla P100 with a Q1000 (made up).  Is type-id 11 on 
> > > > > >>> that
> > > > > >>> new card still going to be a "GRID-M60-0B"?  If not then we've 
> > > > > >>> based
> > > > > >>> our XML on the wrong attribute.  If the new device does not 
> > > > > >>> support
> > > > > >>> "GRID-M60-0B" then we should generate an error, not simply 
> > > > > >>> initialize
> > > > > >>> whatever type-id 11 happens to be on this new card.
> > > > > >>> 
> > > > > >>
> > > > > >> If there are 2 M60 in the system then you would find '11' type 
> > > > > >> directory
> > > > > >> in mdev_supported_types of both M60. If you have P100, '11' type 
> > > > > >> would
> > > > > >> not be there in its mdev_supported_types, it will have different 
> > > > > >> types.
> > > > > >>
> > > > > >> For example, if you replace M60 with P100, but XML is not updated. 
> > > > > >> XML
> > > > > >> have type '11'. When libvirt would try to create mdev device, 
> > > > > >> libvirt
> > > > > >> would have to find 'create' file in sysfs in following directory 
> > > > > >> format:
> > > > > >>
> > > > > >>  --- mdev_supported_types
> > > > > >>  |-- 11
> > > > > >>  |   |-- create
> > > > > >>
> > > > > >> but now for P100, '11' directory is not there, so libvirt should 
> > > > > >> throw
> > > > > >> error on not able to find '11' directory.  
> > > > > > 
> > > > > > This really seems like an accident waiting to happen.  What happens
> > > > > > when the user replaces their M60 with an Intel XYZ device that 
> > > > > > happens
> > > > > > to expose a type 11 mdev class gpu device?  How is libvirt supposed 
> > > > > > to
> > > > > > know that the XML used to refer to a GRID-M60-0B and now it's an
> > > > > > INTEL-IGD-XYZ?  Doesn't basing the XML entry on the name and 
> > > > > > removing
> > > > > > yet another arbitrary requirement that we have some sort of globally
> > > > > > unique type-id database make a lot of sense?  The same issue applies
> > > > > > for simple debug-ability, if I'm reviewing the XML for a domain and 
> > > > > > the
> > > > > > name is the primary index for the mdev device, I know what it is.
> > > > > > Seeing type-id='11' is meaningless.
> > > > > >  
> > > > > 
> > > > > Let me clarify again, type '11' is a string that vendor driver would
> > > > > define (see my previous reply below) it could be "11" or 
> > > > > "GRID-M60-0B".
> > > > > If 2 vendors used same string we can't control that. right?
> > > > > 
> > > > > 
> > > > >  Lets remove 'id' from type id in XML if that is the concern. 
> > > > >  Supported
> > > > >  types is going to be defined by vendor driver, so let vendor 
> > > > >  driver
> > > > >  decide what to use for directory name and same should be used in 
> > > > >  device
> > > > >  xml file, it could be '11' or "GRID M60-0B":
> > > > > 
> > > > >  
> > > > >    my-vgpu
> > > > >    pci__86_00_0
> > > > >    
> > > > >  

Re: [libvirt] [Qemu-devel] [RFC v2] libvirt vGPU QEMU integration

[Daniel P. Berrange]

On Wed, Sep 28, 2016 at 12:48:33PM -0700, Neo Jia wrote:
> On Tue, Sep 20, 2016 at 10:47:53AM +0100, Daniel P. Berrange wrote:
> > On Tue, Sep 20, 2016 at 02:05:52AM +0530, Kirti Wankhede wrote:
> > > 
> > > Hi libvirt experts,
> > > 
> > > Thanks for valuable input on v1 version of RFC.
> > > 
> > > Quick brief, VFIO based mediated device framework provides a way to
> > > virtualize their devices without SR-IOV, like NVIDIA vGPU, Intel KVMGT
> > > and IBM's channel IO. This framework reuses VFIO APIs for all the
> > > functionalities for mediated devices which are currently being used for
> > > pass through devices. This framework introduces a set of new sysfs files
> > > for device creation and its life cycle management.
> > > 
> > > Here is the summary of discussion on v1:
> > > 1. Discover mediated device:
> > > As part of physical device initialization process, vendor driver will
> > > register their physical devices, which will be used to create virtual
> > > device (mediated device, aka mdev) to the mediated framework.
> > > 
> > > Vendor driver should specify mdev_supported_types in directory format.
> > > This format is class based, for example, display class directory format
> > > should be as below. We need to define such set for each class of devices
> > > which would be supported by mediated device framework.
> > > 
> > >  --- mdev_destroy
> > >  --- mdev_supported_types
> > >  |-- 11
> > >  |   |-- create
> > >  |   |-- name
> > >  |   |-- fb_length
> > >  |   |-- resolution
> > >  |   |-- heads
> > >  |   |-- max_instances
> > >  |   |-- params
> > >  |   |-- requires_group
> > >  |-- 12
> > >  |   |-- create
> > >  |   |-- name
> > >  |   |-- fb_length
> > >  |   |-- resolution
> > >  |   |-- heads
> > >  |   |-- max_instances
> > >  |   |-- params
> > >  |   |-- requires_group
> > >  |-- 13
> > >  |-- create
> > >  |-- name
> > >  |-- fb_length
> > >  |-- resolution
> > >  |-- heads
> > >  |-- max_instances
> > >  |-- params
> > >  |-- requires_group
> > > 
> > > 
> > > In the above example directory '11' represents a type id of mdev device.
> > > 'name', 'fb_length', 'resolution', 'heads', 'max_instance' and
> > > 'requires_group' would be Read-Only files that vendor would provide to
> > > describe about that type.
> > > 
> > > 'create':
> > > Write-only file. Mandatory.
> > > Accepts string to create mediated device.
> > > 
> > > 'name':
> > > Read-Only file. Mandatory.
> > > Returns string, the name of that type id.
> > 
> > Presumably this is a human-targetted title/description of
> > the device.
> > 
> > > 
> > > 'fb_length':
> > > Read-only file. Mandatory.
> > > Returns {K,M,G}, size of framebuffer.
> > > 
> > > 'resolution':
> > > Read-Only file. Mandatory.
> > > Returns 'hres x vres' format. Maximum supported resolution.
> > > 
> > > 'heads':
> > > Read-Only file. Mandatory.
> > > Returns integer. Number of maximum heads supported.
> > 
> > None of these should be mandatory as that makes the mdev
> > useless for non-GPU devices.
> > 
> > I'd expect to see a 'class' or 'type' attribute in the
> > directory whcih tells you what kind of mdev it is. A
> > valid 'class' value would be 'gpu'. The fb_length,
> > resolution, and heads parameters would only be mandatory
> > when class==gpu.
> > 
> 
> Hi Daniel,
> 
> Here you are proposing to add a class named "gpu", which will make all those 
> gpu
> related attributes mandatory, which libvirt can allow user to better
> parse/present a particular mdev configuration?
> 
> I am just wondering if there is another option that we just make all those
> attributes that a mdev device can have as optional but still meaningful to
> libvirt, so libvirt can still parse / recognize them as an class "mdev".

'mdev' isn't a class - mdev is the name of the kernel module. The class
refers to the broad capability of the device. class would be things
like "gpu", "nic", "fpga" or other such things. The point of the class
is to identify which other attributes will be considered mandatory.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

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

Re: [libvirt] [RFC v2] libvirt vGPU QEMU integration

[Daniel P. Berrange]

On Wed, Sep 28, 2016 at 12:22:35PM -0700, Neo Jia wrote:
> On Thu, Sep 22, 2016 at 03:26:38PM +0100, Daniel P. Berrange wrote:
> > On Thu, Sep 22, 2016 at 08:19:21AM -0600, Alex Williamson wrote:
> > > On Thu, 22 Sep 2016 09:41:20 +0530
> > > Kirti Wankhede  wrote:
> > > 
> > > > > My concern is that a type id seems arbitrary but we're specifying 
> > > > > that
> > > > > it be unique.  We already have something unique, the name.  So 
> > > > > why try
> > > > > to make the type id unique as well?  A vendor can accidentally 
> > > > > create
> > > > > their vendor driver so that a given name means something very
> > > > > specific.  On the other hand they need to be extremely deliberate 
> > > > > to
> > > > > coordinate that a type id means a unique thing across all their 
> > > > > product
> > > > > lines.
> > > > >   
> > > > 
> > > >  Let me clarify, type id should be unique in the list of
> > > >  mdev_supported_types. You can't have 2 directories in with same 
> > > >  name.
> > > > >>>
> > > > >>> Of course, but does that mean it's only unique to the machine I'm
> > > > >>> currently running on?  Let's say I have a Tesla P100 on my system 
> > > > >>> and
> > > > >>> type-id 11 is named "GRID-M60-0B".  At some point in the future I
> > > > >>> replace the Tesla P100 with a Q1000 (made up).  Is type-id 11 on 
> > > > >>> that
> > > > >>> new card still going to be a "GRID-M60-0B"?  If not then we've based
> > > > >>> our XML on the wrong attribute.  If the new device does not support
> > > > >>> "GRID-M60-0B" then we should generate an error, not simply 
> > > > >>> initialize
> > > > >>> whatever type-id 11 happens to be on this new card.
> > > > >>> 
> > > > >>
> > > > >> If there are 2 M60 in the system then you would find '11' type 
> > > > >> directory
> > > > >> in mdev_supported_types of both M60. If you have P100, '11' type 
> > > > >> would
> > > > >> not be there in its mdev_supported_types, it will have different 
> > > > >> types.
> > > > >>
> > > > >> For example, if you replace M60 with P100, but XML is not updated. 
> > > > >> XML
> > > > >> have type '11'. When libvirt would try to create mdev device, libvirt
> > > > >> would have to find 'create' file in sysfs in following directory 
> > > > >> format:
> > > > >>
> > > > >>  --- mdev_supported_types
> > > > >>  |-- 11
> > > > >>  |   |-- create
> > > > >>
> > > > >> but now for P100, '11' directory is not there, so libvirt should 
> > > > >> throw
> > > > >> error on not able to find '11' directory.  
> > > > > 
> > > > > This really seems like an accident waiting to happen.  What happens
> > > > > when the user replaces their M60 with an Intel XYZ device that happens
> > > > > to expose a type 11 mdev class gpu device?  How is libvirt supposed to
> > > > > know that the XML used to refer to a GRID-M60-0B and now it's an
> > > > > INTEL-IGD-XYZ?  Doesn't basing the XML entry on the name and removing
> > > > > yet another arbitrary requirement that we have some sort of globally
> > > > > unique type-id database make a lot of sense?  The same issue applies
> > > > > for simple debug-ability, if I'm reviewing the XML for a domain and 
> > > > > the
> > > > > name is the primary index for the mdev device, I know what it is.
> > > > > Seeing type-id='11' is meaningless.
> > > > >  
> > > > 
> > > > Let me clarify again, type '11' is a string that vendor driver would
> > > > define (see my previous reply below) it could be "11" or "GRID-M60-0B".
> > > > If 2 vendors used same string we can't control that. right?
> > > > 
> > > > 
> > > >  Lets remove 'id' from type id in XML if that is the concern. 
> > > >  Supported
> > > >  types is going to be defined by vendor driver, so let vendor driver
> > > >  decide what to use for directory name and same should be used in 
> > > >  device
> > > >  xml file, it could be '11' or "GRID M60-0B":
> > > > 
> > > >  
> > > >    my-vgpu
> > > >    pci__86_00_0
> > > >    
> > > >  

[libvirt] [PATCH 1/1] Add simple sample driver for mediated device framework

[Kirti Wankhede]

Sample driver creates mdev device that simulates serial port over PCI card.

Signed-off-by: Kirti Wankhede 
Signed-off-by: Neo Jia 
Change-Id: I857f8f12f8b275f2498dfe8c628a5cdc7193b1b2
---
 Documentation/mdev/Makefile   |   14 +
 Documentation/mdev/mtty.c | 1202 +
 Documentation/{ => mdev}/vfio-mediated-device.txt |   61 ++
 3 files changed, 1277 insertions(+)
 create mode 100644 Documentation/mdev/Makefile
 create mode 100644 Documentation/mdev/mtty.c
 rename Documentation/{ => mdev}/vfio-mediated-device.txt (78%)

diff --git a/Documentation/mdev/Makefile b/Documentation/mdev/Makefile
new file mode 100644
index ..ff6f8a324c85
--- /dev/null
+++ b/Documentation/mdev/Makefile
@@ -0,0 +1,14 @@
+#
+# Makefile for mtty.c file
+#
+KDIR:=/lib/modules/$(shell uname -r)/build
+
+obj-m:=mtty.o
+
+default:
+   $(MAKE) -C $(KDIR) SUBDIRS=$(PWD) modules
+
+clean:
+   @rm -rf .*.cmd *.mod.c *.o *.ko .tmp*
+   @rm -rf Module.* Modules.* modules.* .tmp_versions
+
diff --git a/Documentation/mdev/mtty.c b/Documentation/mdev/mtty.c
new file mode 100644
index ..ce29d54b4275
--- /dev/null
+++ b/Documentation/mdev/mtty.c
@@ -0,0 +1,1202 @@
+/*
+ * Mediated virtual PCI serial host device driver
+ *
+ * Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved.
+ * Author: Neo Jia 
+ * Kirti Wankhede 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Sample driver that creates mdev device that simulates serial port over PCI
+ * card.
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+/*
+ * #defines
+ */
+
+#define VERSION_STRING  "0.1"
+#define DRIVER_AUTHOR   "NVIDIA Corporation"
+
+#define MTTY_CLASS_NAME "mtty"
+
+#define MTTY_NAME   "mtty"
+
+#define MTTY_CONFIG_SPACE_SIZE  0xff
+#define MTTY_IO_BAR_SIZE0x8
+#define MTTY_MMIO_BAR_SIZE  0x10
+
+#define STORE_LE16(addr, val)   (*(u16 *)addr = val)
+#define STORE_LE32(addr, val)   (*(u32 *)addr = val)
+
+#define MAX_FIFO_SIZE   16
+
+#define CIRCULAR_BUF_INC_IDX(idx)(idx = (idx + 1) & (MAX_FIFO_SIZE - 1))
+
+#define MTTY_VFIO_PCI_OFFSET_SHIFT   40
+
+#define MTTY_VFIO_PCI_OFFSET_TO_INDEX(off)   (off >> 
MTTY_VFIO_PCI_OFFSET_SHIFT)
+#define MTTY_VFIO_PCI_INDEX_TO_OFFSET(index) ((u64)(index) << 
MTTY_VFIO_PCI_OFFSET_SHIFT)
+#define MTTY_VFIO_PCI_OFFSET_MASK(((u64)(1) << MTTY_VFIO_PCI_OFFSET_SHIFT) 
- 1)
+
+
+/*
+ * Global Structures
+ */
+
+struct mtty_dev {
+dev_t   vd_devt;
+struct class*vd_class;
+struct cdev vd_cdev;
+struct idr  vd_idr;
+struct device   dev;
+} mtty_dev;
+
+struct mdev_region_info {
+u64 start;
+u64 phys_start;
+u32 size;
+u64 vfio_offset;
+};
+
+#if defined(DEBUG_REGS)
+const char *wr_reg[] = {
+"TX",
+"IER",
+"FCR",
+"LCR",
+"MCR",
+"LSR",
+"MSR",
+"SCR"
+};
+
+const char *rd_reg[] = {
+"RX",
+"IER",
+"IIR",
+"LCR",
+"MCR",
+"LSR",
+"MSR",
+"SCR"
+};
+#endif
+
+// loop back buffer
+struct rxtx {
+u8 fifo[MAX_FIFO_SIZE];
+u8 head, tail;
+u8 count;
+};
+
+struct serial_port {
+u8 uart_reg[8]; /* 8 registers */
+struct rxtx rxtx;   /* loop back buffer */
+bool dlab;
+bool overrun;
+u16 divisor;
+u8 fcr; /* FIFO control register */
+u8 max_fifo_size;
+u8 intr_trigger_level;  /* interrupt trigger level */
+};
+
+/* State of each mdev device */
+struct mdev_state {
+int irq_fd;
+struct file *intx_file;
+struct file *msi_file;
+int irq_index;
+u8 *vconfig;
+struct mutex ops_lock;
+struct mdev_device *mdev;
+struct mdev_region_info region_info[VFIO_PCI_NUM_REGIONS];
+u32 bar_mask[VFIO_PCI_NUM_REGIONS];
+struct list_head next;
+struct serial_port s[2];
+struct mutex rxtx_lock;
+};
+
+struct mutex mdev_list_lock;
+struct list_head mdev_devices_list;
+
+static struct file_operations vd_fops = {
+.owner  = THIS_MODULE,
+};
+
+/* function prototypes */
+
+static int mtty_dev_mdev_trigger_interrupt(uuid_le uuid);
+
+/* Helper functions */
+static struct mdev_state *find_mdev_state_by_uuid(uuid_le uuid)
+{
+struct mdev_state *mds;
+
+list_for_each_entry(mds, _devices_list, next) {
+if (uuid_le_cmp(mds->mdev->uuid, uuid) == 0)
+return mds;
+}
+
+return NULL;
+}
+
+void dump_buffer(char *buf, uint32_t count)
+{
+#if defined(DEBUG)
+int i;
+
+pr_info("Buffer: \n");
+for (i = 0; i < count; i++) {
+

[libvirt] [PATCH 0/1] Sample driver for mediated device framework

[Kirti Wankhede]

This sample driver is with reference to v7 patch of Mediated device support:
https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg03798.html
https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg03799.html
https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg03800.html
https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg03811.html
https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg03802.html

We are working on v8 version of this patchset. This sample is to get the feel of
how mediated device framework works.

This is a simple sample driver that creates device that simulates serial
port over PCI card. Updated vfio-mediated-device.txt with details of how it
works with mdev framework.

We will have v8 version of mediated device framework soon and this sample driver
will get updated accordingly. But this will enable people/public to get more
insights of this framework and how it is supposed to work.

Kirti Wankhede (1):
  Add simple sample driver for mediated device framework

 Documentation/mdev/Makefile   |   14 +
 Documentation/mdev/mtty.c | 1202 +
 Documentation/{ => mdev}/vfio-mediated-device.txt |   61 ++
 3 files changed, 1277 insertions(+)
 create mode 100644 Documentation/mdev/Makefile
 create mode 100644 Documentation/mdev/mtty.c
 rename Documentation/{ => mdev}/vfio-mediated-device.txt (78%)

-- 
2.7.0

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

Re: [libvirt] [PATCH] network: Add support for configuring dhcp lease time

[Martin Wilck]

On Wed, 2016-09-28 at 17:10 +0100, Daniel P. Berrange wrote:

> > I believe that setting "dhcp-authoritative" will be a major
> > improvement
> > for many setups. Without it, VMs are *never* able to reacquire
> > their
> > expired lease. With it, reacquiring the lease would work most of
> > the
> > time (as far as I'm concerned, practically always). I reckon the
> > dnsmasq man page recommends it for a reason "when dnsmasq is
> > definitely
> > the only DHCP server on a network".
> 
> 
> That certainly matches our deployment model for dnsmasq, so unless
> there's a obvious downside to it, it seems reasonable to add that.

I'm glad to read that. Is anything needed besides 
http://www.spinics.net/linux/fedora/libvir/msg136813.html
and follow-ups?

Martin

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

Re: [libvirt] [PATCH] vsh: Write out history on "quit" or "exit" in interactive mode

[Erik Skultety]

On 28/09/16 21:27, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1379895
> 
> Introduced by commit id '834c5720'.
> 
> During the code motion and creation of vsh.c, the function 'vshDeinit()'
> in the (new) vsh.c was altered from whence it came in virsh.c such that
> calling 'vshReadlineDeinit(ctl)' was conditional on "ctl->imode".
> 
> This causes a problem for the interactive running if the "quit" and "exit"
> commands are used because 'cmdQuit' will clear ctl->imode, thus when the
> interactive loop in main() of virsh.c exits because ctl->mode is clear and
> virshDeinit is called which calls vshDeinit, the history file is now not
> written. Conversely, if one had exited the interactive loop via pressing
> D the file would be created because loop control is broken on EOF
> and ctl->imode is not set to false.
> 
> This patch will remove the conditional call to vshReadlineDeinit and
> restore the former behaviour.
> 
> Signed-off-by: John Ferlan 
> ---
>  I realize some people don't like the comment I added; however, I'd
>  rather be "safe" when purely reading the code than expect someone
>  to do a git log -p looking at commit messages for "removed" lines of code.
> 
>  An alternative approach would be to create/set a new boolean value
>  such as "iquit" (or "iexit") during cmdQuit and then in vshDeinit make
>  calling vshReadlineDeinit() conditional on "ctl->imode || ctl->iquit"
> 
>  Calling the vshReadlineDeinit during non interactive mode would have no
>  affect since vshReadlineInit is only called if ctl->imode is set. The
>  Deinit function will essentially do nothing other than a couple of
>  VIR_FREE(NULL); and one extra "if"
> 
>  tools/vsh.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/vsh.c b/tools/vsh.c
> index 041113f..eba3f0f 100644
> --- a/tools/vsh.c
> +++ b/tools/vsh.c
> @@ -3093,8 +3093,12 @@ vshInitReload(vshControl *ctl)
>  void
>  vshDeinit(vshControl *ctl)
>  {
> -if (ctl->imode)
> -vshReadlineDeinit(ctl);
> +/* Don't make calling vshReadlineDeinit conditional on imode. During
> + * interactive mode when "quit" or "exit" is typed, 'imode' is set
> + * to false so if this were conditional on imode, then history wouldn't
> + * be written when the "quit" or "exit" commands were used instead of
> + * when D is used */

Well, the commit message is already pretty verbose. I do understand your
concern about having to wade through log -p output, especially when you
encounter something like 834c5720 which truly is evil (I started over
like 3 times with 3 different ugly results), but once you make friends
with gitk, tig or whatever interactive tool, everything becomes a little
more convenient. In conclusion, I think something like "Don't make
calling of vshReadlineDeinit conditional on active interactive mode."
would suffice.

ACK
Erik

> +vshReadlineDeinit(ctl);
>  vshCloseLogFile(ctl);
>  }
>  
> 




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

Re: [libvirt] [PATCH] qemu: Only use memory-backend-file with NUMA if needed

[Martin Kletzander]

On Wed, Sep 28, 2016 at 12:00:22PM +1000, Sam Bobroff wrote:

On Fri, Sep 23, 2016 at 03:20:56PM +0200, Martin Kletzander wrote:

If this reminds you of a commit message from around a year ago, it's
41c2aa729f0af084ede95ee9a06219a2dd5fb5df and yes, we're dealing with
"the same thing" again.  Or f309db1f4d51009bad0d32e12efc75530b66836b and
it's similar.

There is a logic in place that if there is no real need for
memory-backend-file, qemuBuildMemoryBackendStr() returns 0.  However
that wasn't the case with hugepage backing.  The reason for that was
that we abused the 'pagesize' variable for storing that information, but
we should rather have a separate one that specifies whether we really
need the new object for hugepage backing.  And that variable should be
set only if this particular NUMA cell needs special treatment WRT
hugepages.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1372153

Signed-off-by: Martin Kletzander 


Hi Martin,

I tested your patch and it works but it seems to make the snapshot/migration
information incompatible with the current master code. e.g. after applying the
patch I can't load a snapshot that was created before applying the patch
(obviously only for a guest configuration that is affected by the patch). The
error I get is this:

$ virsh snapshot-revert tmp --snapshotname 1475026515
error: operation failed: Unknown ramblock "/objects/ram-node0", cannot accept 
migration
error while loading state for instance 0x0 of device 'ram'
Error -22 while loading VM state

Presumably this is caused by the same change that fixes some other migrations,
but is this case a problem?



Yes, that's the problem.  But since we cannot know which was the
previous notation used, we need to pick one.  And previously we picked
the one that works with most QEMUs, even the old ones, unless we need
the new notation for some setting.  That is mostly for the migration (or
save/restore, which is basically the same thing) to work, even though it
looks like we're breaking it due to that.  That's why I will back-port
this to older maintenance branches so that we have it working across all
versions.  Some saves will still not be working, but there is no way how
to make everything work (due to the reason described in 2nd sentence),
so I'm making it work for everything from now on (no matter which
version), plus everything before the patch that "broke" it.


Cheers,
Sam.

---

Notes:
This fixes migration from older libvirts.  By "older", I mean
pre-(circa-)1.2.7, also in some cases pre-1.2.11, in some other cases
pre-v1.2.20.  It's pretty messy.  It could be back-ported as far as it's
easy to do.



Using just a few words I tried to describe that here ^^.  Any of those
explanations might be hard to follow due to -ENOCOFFEE, sorry if that's
the case =)


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

Re: [libvirt] [PATCH 0/7] qemu: vcpu coldplug and ordering fixes

[Peter Krempa]

On Wed, Sep 21, 2016 at 13:49:52 +0200, Peter Krempa wrote:
> Few fixes for various isues connected to coldplug and vcpu order.
> 
> Peter Krempa (7):
>   qemu: process: Fix off-by-one in vcpu order duplicate error message
>   qemu: process: Don't use shifted indexes for vcpu order verification
>   qemu: process: Enforce 'vcpu' order range to <1,maxvcpus>
>   qemu: Fix coldplug of vcpus
>   qemu: vcpu: Clear vcpu order information rather than making it invalid
>   lib: Introduce VIR_DOMAIN_VCPU_HOTPLUGGABLE for virDomainSetVcpusFlags
>   qemu: Allow making vcpus hotpluggable with virDomainSetVcpusFlags

Ping

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

Re: [libvirt] [PATCH v2] virsh domdisplay: introduce '--all' for showing all possible graphical display

[Michal Privoznik]

On 28.09.2016 15:31, Chen Hanxiao wrote:
> From: Chen Hanxiao 
> 
> For one VM, it could had more than one graphical display.
> Such as we coud add both vnc and spice display to a VM.
> 
> This patch introduces '--all' for showing all
> possible graphical display of a active VM.
> 
> Signed-off-by: Chen Hanxiao 
> ---
> v2: VIR_FREE befor use in loops
> add descriptions in virsh.pod
> 
>  tools/virsh-domain.c | 15 ++-
>  tools/virsh.pod  |  3 ++-
>  2 files changed, 16 insertions(+), 2 deletions(-)

This one looks better. But I've got some points.

> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 3829b17..a6124b6 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -10648,6 +10648,10 @@ static const vshCmdOptDef opts_domdisplay[] = {
>   .help = N_("select particular graphical display "
>  "(e.g. \"vnc\", \"spice\", \"rdp\")")
>  },
> +{.name = "all",
> + .type = VSH_OT_BOOL,
> + .help = N_("show all possible graphical displays")
> +},
>  {.name = NULL}
>  };
>  

What should happen if users pass both --type and --all? In that case the 
semantics for --all is changed I guess and according to this code we would 
print all URIs for given type. However, there can be just one graphical type 
per domain and thus one URI.

> @@ -10671,6 +10675,7 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
>  int tmp;
>  int flags = 0;
>  bool params = false;
> +bool all = vshCommandOptBool(cmd, "all");
>  const char *xpath_fmt = 
> "string(/domain/devices/graphics[@type='%s']/%s)";
>  virSocketAddr addr;
>  
> @@ -10701,6 +10706,7 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
>  continue;
>  
>  /* Create our XPATH lookup for the current display's port */
> +VIR_FREE(xpath);
>  if (virAsprintf(, xpath_fmt, scheme[iter], "@port") < 0)
>  goto cleanup;
>  
> @@ -10733,6 +10739,7 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
>  
>  /* Attempt to get the listening addr if set for the current
>   * graphics scheme */
> +VIR_FREE(listen_addr);
>  listen_addr = virXPathString(xpath, ctxt);
>  VIR_FREE(xpath);
>  
> @@ -10788,6 +10795,7 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
>  goto cleanup;
>  
>  /* Attempt to get the password */
> +VIR_FREE(passwd);
>  passwd = virXPathString(xpath, ctxt);
>  VIR_FREE(xpath);
>  
> @@ -10840,12 +10848,17 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
>  }
>  
>  /* Print out our full URI */
> +VIR_FREE(output);
>  output = virBufferContentAndReset();
>  vshPrint(ctl, "%s", output);
>  
>  /* We got what we came for so return successfully */
>  ret = true;
> -break;
> +if (!all) {
> +break;
> +} else {
> +vshPrint(ctl, "\n");
> +}
>  }
>  
>  if (!ret) {
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 49abda9..6255b36 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -1229,7 +1229,8 @@ Output a URI which can be used to connect to the 
> graphical display of the
>  domain via VNC, SPICE or RDP.  The particular graphical display type can
>  be selected using the B parameter (e.g. "vnc", "spice", "rdp").  If
>  I<--include-password> is specified, the SPICE channel password will be
> -included in the URI.
> +included in the URI. If I<--all> is specified, then all show all possible
> +graphical displays, for a VM could have more than one graphical displays.
>  

Almost. You forgot to update the list of arguments:

-=item B I [I<--include-password>] [[I<--type>] B]
+=item B I [I<--include-password>] [[I<--type>] B] 
[I<--all>]

Normally, I'd fix this before pushing and just point it out in review, but now 
we are in the freeze and this is a feature (during freeze only bugfixes can be 
pushed). Moreover, there's the unclear behaviour I described above.


Michal

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