date:20180103

Re: [libvirt] [PATCH v3.1 3/6] libxl: add support for CPUID features policy

2018-01-03 Thread Joao Martins

On 01/04/2018 12:43 AM, Marek Marczykowski-Górecki wrote:
> On Wed, Jan 03, 2018 at 05:00:10PM -0700, Jim Fehlig wrote:
>> On 12/19/2017 06:19 AM, Joao Martins wrote:
>>> On 12/13/2017 07:09 PM, Marek Marczykowski-Górecki wrote:
 +/*
 + * Translate CPU feature name from libvirt to libxl (from_libxl=false) or 
 from
 + * libxl to libvirt (from_libxl=true).
 + */
 +const char *
 +xenTranslateCPUFeature(const char *feature_name, bool from_libxl)
 +{
 +static const char *translation_table[][2] = {
 +/* libvirt name, libxl name */
 +{ "cx16", "cmpxchg16" },
 +{ "cid", "cntxid" },
 +{ "ds_cpl", "dscpl" },
 +{ "pclmuldq", "pclmulqdq" },
 +{ "pni", "sse3" },
 +{ "ht", "htt" },
 +{ "pn", "psn" },
 +{ "clflush", "clfsh" },
 +{ "sep", "sysenter" },
 +{ "cx8", "cmpxchg8" },
 +{ "nodeid_msr", "nodeid" },
 +{ "cr8legacy", "altmovcr8" },
 +{ "lahf_lm", "lahfsahf" },
 +{ "cmp_legacy", "cmplegacy" },
 +{ "fxsr_opt", "ffxsr" },
 +{ "pdpe1gb", "page1gb" },
 +};
 +size_t i;
 +
 +for (i = 0; i < ARRAY_CARDINALITY(translation_table); i++)
 +if (STREQ(translation_table[i][from_libxl], feature_name))
 +return translation_table[i][!from_libxl];
 +return feature_name;
 +}
 +
>>>
>>> Cc'ing Jim as he may have some thoughts on the direction of this.
>>>
>>> What happens if the admin decides to change /usr/share/libvirt/cpu_map.xml?
>>
>> Is changing existing content likely/practical?
>>
>>> Also you can also add new leafs/features to cpu_map.xml
>>
>> Right. And I suppose the table in libxl_cpuid.c can grow too. And in cases
>> where they differ we'd need to update the static table, which we'll probably
>> only remember to do when receiving bug reports. So I like the idea of making
>> this more dynamic, but I apparently don't have enough brain power today to
>> understand your suggestion :-).
>>
>>> Maybe an idea instead is to have a table with all leafs that libxl has 
>>> hardcoded
>>> (i.e. cpuid_flags array on
>>> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/libxl/libxl_cpuid.c;hb=HEAD#l92).
>>
>> Where would this table reside? In src/cpu/?
>>
>>> Then adding a new cpu driver routine to look for cpuid data based on feature
>>> name (and the reverse too). Finally you would populate this translation 
>>> table at
>>> driver load time, or you could even get away without a translation table 
>>> (but
>>> would be a little more inefficient?).
>>
>> I'm having difficulty understanding how this provides a dynamic solution.
>> Wouldn't the table you mention above need extended anytime the hardcoded one
>> in libxl was extended? Which would probably only happen after receiving a
>> bug report? :-)
> 
> Probably... And worse thing about such table is it needs to contain all
> entries from said libxl_cpuid.c. My solution require only listing
> entries with mismatching names.
> 
/nods and it would be a gigantic table most likely.

> Alternative would be to not use "new libxl syntax", but "old xend
> syntax" (which is still supported by libxl). The later allow to list
> specific bits instead of feature names. That was implemented in v1 of
> this patch series[1]... The problem with that approach is currently libvirt
> does not expose API for lookup of individual features, but only to
> construct full CPU and then enumerate its CPUID.

I kinda liked your xend version, provided we could lookup the feature bits as
you are hinting here.

> That means it isn't
> feasible for the current approach (mode='host-passthrough', then
> enable/disable individual features). See discussion on v1.
> Adding such API to libvirt cpu driver is beyond my knowledge of libvirt
> code and available time :/
> 
The main reason I suggesting this out was because this patch was hardcoding
libvirt feature names. Maybe it's not an issue :)

Joao

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

Re: [libvirt] [PATCH v3.1 3/6] libxl: add support for CPUID features policy

2018-01-03 Thread Joao Martins

On 01/04/2018 12:00 AM, Jim Fehlig wrote:
> On 12/19/2017 06:19 AM, Joao Martins wrote:
>> On 12/13/2017 07:09 PM, Marek Marczykowski-Górecki wrote:
>>> Convert CPU features policy into libxl cpuid policy settings. Use new
>>> ("libxl") syntax, which allow to enable/disable specific bits, using
>>> host CPU as a base. For this reason, only "host-passthrough" mode is
>>> accepted.
>>> Libxl do not have distinction between "force" and "required" policy
>>> (there is only "force") and also between "forbid" and "disable" (there
>>> is only "disable"). So, merge them appropriately. If anything, "require"
>>> and "forbid" should be enforced outside of specific driver.
>>>
>>> Signed-off-by: Marek Marczykowski-Górecki 
>>
>> This is quite neat Marek :)
>>
>> I have one comment towards the end, suggesting an alternative to a static
>> translation table.
>>
>>> ---
>>> Changes since v3:
>>>   - compile fix (one more 
>>> s/libxlTranslateCPUFeature/xenTranslateCPUFeature/)
>>> Changes since v2:
>>>   - drop spurious changes
>>>   - move libxlTranslateCPUFeature function to xen_xl.c, to be reused by
>>>   xenconfig driver
>>> Changes since v1:
>>>   - use new ("libxl") syntax to set only bits explicitly mentioned in
>>>   domain XML
>>> ---
>>>   src/libxl/libxl_conf.c | 35 +--
>>>   src/xenconfig/xen_xl.c | 34 ++
>>>   src/xenconfig/xen_xl.h |  2 ++
>>>   3 files changed, 69 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>>> index 184610966..5eae6d10f 100644
>>> --- a/src/libxl/libxl_conf.c
>>> +++ b/src/libxl/libxl_conf.c
>>> @@ -50,6 +50,7 @@
>>>   #include "secret_util.h"
>>>   #include "cpu/cpu.h"
>>>   #include "xen_common.h"
>>> +#include "xen_xl.h"
>>>   
>>>   
>>>   #define VIR_FROM_THIS VIR_FROM_LIBXL
>>> @@ -357,6 +358,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>>>   bool hasHwVirt = false;
>>>   int nested_hvm = -1;
>>>   bool svm = false, vmx = false;
>>> +char xlCPU[32];
>>>   
>>>   if (def->cpu->mode != (VIR_CPU_MODE_HOST_PASSTHROUGH)) {
>>>   virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>> @@ -379,15 +381,44 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>>>   case VIR_CPU_FEATURE_DISABLE:
>>>   case VIR_CPU_FEATURE_FORBID:
>>>   if ((vmx && STREQ(def->cpu->features[i].name, 
>>> "vmx")) ||
>>> -(svm && STREQ(def->cpu->features[i].name, 
>>> "svm")))
>>> +(svm && STREQ(def->cpu->features[i].name, 
>>> "svm"))) {
>>>   nested_hvm = 0;
>>> +continue;
>>> +}
>>> +
>>> +snprintf(xlCPU,
>>> +sizeof(xlCPU),
>>> +"%s=0",
>>> +xenTranslateCPUFeature(
>>> +def->cpu->features[i].name,
>>> +false));
>>> +if (libxl_cpuid_parse_config(&b_info->cpuid, 
>>> xlCPU)) {
>>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>> +_("unsupported cpu feature '%s'"),
>>> +def->cpu->features[i].name);
>>> +return -1;
>>> +}
>>>   break;
>>>   
>>>   case VIR_CPU_FEATURE_FORCE:
>>>   case VIR_CPU_FEATURE_REQUIRE:
>>>   if ((vmx && STREQ(def->cpu->features[i].name, 
>>> "vmx")) ||
>>> -(svm && STREQ(def->cpu->features[i].name, 
>>> "svm")))
>>> +(svm && STREQ(def->cpu->features[i].name, 
>>> "svm"))) {
>>>   nested_hvm = 1;
>>> +continue;
>>> +}
>>> +
>>> +snprintf(xlCPU,
>>> +sizeof(xlCPU),
>>> +"%s=1",
>>> +xenTranslateCPUFeature(
>>> +def->cpu->features[i].name, 
>>> false));
>>> +if (libxl_cpuid_parse_config(&b_info->cpuid, 
>>> xlCPU)) {
>>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>> +_("unsupported cpu feature '%s'"),
>>> +def->cpu->features[i].name);
>>> +return -1;
>>> +}
>>>   break;
>>>   case VIR_CPU_FEATURE_OPTIO

Re: [libvirt] [PATCH v3 4/6] tests: check CPU features handling in libxl driver

2018-01-03 Thread Jim Fehlig


On 12/09/2017 07:10 PM, Marek Marczykowski-Górecki wrote:

Test enabling/disabling individual CPU features and also setting
nested HVM support, which is also controlled by CPU features node.

Signed-off-by: Marek Marczykowski-Górecki 
---
Changes since v1:
  - rewritten to Jim's test suite for libxl_domain_config generator

Cc: Jim Fehlig 
---
  tests/libxlxml2domconfigdata/fullvirt-cpuid.json | 64 +-
  tests/libxlxml2domconfigdata/fullvirt-cpuid.xml  | 37 ++-
  tests/libxlxml2domconfigtest.c   |  1 +-
  3 files changed, 102 insertions(+)
  create mode 100644 tests/libxlxml2domconfigdata/fullvirt-cpuid.json
  create mode 100644 tests/libxlxml2domconfigdata/fullvirt-cpuid.xml


Nice! I don't see anything being discussed in 3/6 affecting this patch.

Reviewed-by: Jim Fehlig 

Regards,
Jim



diff --git a/tests/libxlxml2domconfigdata/fullvirt-cpuid.json 
b/tests/libxlxml2domconfigdata/fullvirt-cpuid.json
new file mode 100644
index 000..234e592
--- /dev/null
+++ b/tests/libxlxml2domconfigdata/fullvirt-cpuid.json
@@ -0,0 +1,64 @@
+{
+"c_info": {
+"type": "hvm",
+"name": "XenGuest2",
+"uuid": "c7a5fdb2-cdaf-9455-926a-d65c16db1809"
+},
+"b_info": {
+"max_vcpus": 1,
+"avail_vcpus": [
+0
+],
+"max_memkb": 592896,
+"target_memkb": 403456,
+"video_memkb": 8192,
+"shadow_memkb": 5656,
+"cpuid": [
+{
+"leaf": 1,
+"ecx": "xx00",
+"edx": "xxx1"
+}
+],
+"sched_params": {
+"weight": 1000
+},
+"type.hvm": {
+"pae": "True",
+"apic": "True",
+"acpi": "True",
+"vga": {
+
+},
+"nested_hvm": "False",
+"vnc": {
+"enable": "False"
+},
+"sdl": {
+"enable": "False"
+},
+"spice": {
+
+},
+"boot": "c",
+"rdm": {
+
+}
+},
+"arch_arm": {
+
+}
+},
+"disks": [
+{
+"pdev_path": "/dev/HostVG/XenGuest2",
+"vdev": "hda",
+"backend": "phy",
+"format": "raw",
+"removable": 1,
+"readwrite": 1
+}
+],
+"on_reboot": "restart",
+"on_crash": "restart"
+}
diff --git a/tests/libxlxml2domconfigdata/fullvirt-cpuid.xml 
b/tests/libxlxml2domconfigdata/fullvirt-cpuid.xml
new file mode 100644
index 000..e9eba2e
--- /dev/null
+++ b/tests/libxlxml2domconfigdata/fullvirt-cpuid.xml
@@ -0,0 +1,37 @@
+
+  XenGuest2
+  c7a5fdb2-cdaf-9455-926a-d65c16db1809
+  592896
+  403456
+  1
+  
+hvm
+  
+  
+
+
+
+  
+  
+
+
+
+  
+  
+  destroy
+  restart
+  restart
+  
+
+  
+  
+  
+  
+
+
+
+
+  
+
+  
+
diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c
index bd4c3af..db0af23 100644
--- a/tests/libxlxml2domconfigtest.c
+++ b/tests/libxlxml2domconfigtest.c
@@ -191,6 +191,7 @@ mymain(void)
  DO_TEST("moredevs-hvm");
  DO_TEST("vnuma-hvm");
  DO_TEST("multiple-ip");
+DO_TEST("fullvirt-cpuid");
  
  return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;

  }



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

Re: [libvirt] [PATCH v3 1/6] libxl: error out on not supported CPU mode, instead of silently ignoring

2018-01-03 Thread Jim Fehlig


On 12/09/2017 07:10 PM, Marek Marczykowski-Górecki wrote:

This change make libvirt XML with plain  element invalid for libxl,
which affect not only upcoming CPUID support, but also NUMA. In fact,
default mode 'custom' does not match what the driver actually does, so
it was a bug. Adjust xenconfig driver accordingly.


Should we change the default mode in the post-parse callback if NUMA or CPU 
features are defined within ? That would allow existing configs to continue 
working, although I doubt there are many since all of this is new to 3.10.0.



But nevertheless this commit break some configurations that were working
before.


I guess that is fine if we explicitly require mode='host-passthrough'.


---
Changes since v2:
  - change separated from 'libxl: add support for CPUID features policy'
---
  src/libxl/libxl_conf.c  | 10 --
  src/xenconfig/xen_xl.c  |  1 +-
  tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.xml |  2 +-
  tests/xlconfigdata/test-fullvirt-vnuma-nodistances.xml  |  2 +-
  tests/xlconfigdata/test-fullvirt-vnuma-partialdist.xml  |  2 +-
  tests/xlconfigdata/test-fullvirt-vnuma.xml  |  2 +-
  6 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 970cff2..f39e783 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -353,11 +353,17 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
def->features[VIR_DOMAIN_FEATURE_ACPI] ==
VIR_TRISTATE_SWITCH_ON);
  
-if (caps &&

-def->cpu && def->cpu->mode == (VIR_CPU_MODE_HOST_PASSTHROUGH)) {
+if (caps && def->cpu) {
  bool hasHwVirt = false;
  bool svm = false, vmx = false;
  
+if (def->cpu->mode != (VIR_CPU_MODE_HOST_PASSTHROUGH)) {


While changing this the unneeded parens around VIR_CPU_MODE_HOST_PASSTHROUGH can 
be dropped.


Regards,
Jim


+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("unsupported cpu mode '%s'"),
+   virCPUModeTypeToString(def->cpu->mode));
+return -1;
+}
+
  if (ARCH_IS_X86(def->os.arch)) {
  vmx = virCPUCheckFeature(caps->host.arch, caps->host.cpu, 
"vmx");
  svm = virCPUCheckFeature(caps->host.arch, caps->host.cpu, 
"svm");
diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
index 532d667..9e239a7 100644
--- a/src/xenconfig/xen_xl.c
+++ b/src/xenconfig/xen_xl.c
@@ -492,6 +492,7 @@ xenParseXLVnuma(virConfPtr conf,
  goto cleanup;
  }
  
+cpu->mode = VIR_CPU_MODE_HOST_PASSTHROUGH;

  cpu->type = VIR_CPU_TYPE_GUEST;
  def->cpu = cpu;
  
diff --git a/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.xml b/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.xml

index e3639eb..3c486ad 100644
--- a/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.xml
+++ b/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.xml
@@ -14,7 +14,7 @@
  
  

-  
+  
  

  
diff --git a/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.xml 
b/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.xml
index 9cab3ca..17c9ca5 100644
--- a/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.xml
+++ b/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.xml
@@ -14,7 +14,7 @@
  
  

-  
+  
  


diff --git a/tests/xlconfigdata/test-fullvirt-vnuma-partialdist.xml 
b/tests/xlconfigdata/test-fullvirt-vnuma-partialdist.xml
index 084b889..291fc37 100644
--- a/tests/xlconfigdata/test-fullvirt-vnuma-partialdist.xml
+++ b/tests/xlconfigdata/test-fullvirt-vnuma-partialdist.xml
@@ -14,7 +14,7 @@
  
  

-  
+  
  

  
diff --git a/tests/xlconfigdata/test-fullvirt-vnuma.xml 
b/tests/xlconfigdata/test-fullvirt-vnuma.xml
index 5368b0d..9a9f495 100644
--- a/tests/xlconfigdata/test-fullvirt-vnuma.xml
+++ b/tests/xlconfigdata/test-fullvirt-vnuma.xml
@@ -14,7 +14,7 @@
  
  

-  
+  
  

  



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

Re: [libvirt] [PATCH v3.1 3/6] libxl: add support for CPUID features policy

2018-01-03 Thread Marek Marczykowski-Górecki

On Wed, Jan 03, 2018 at 05:00:10PM -0700, Jim Fehlig wrote:
> On 12/19/2017 06:19 AM, Joao Martins wrote:
> > On 12/13/2017 07:09 PM, Marek Marczykowski-Górecki wrote:
> > > +/*
> > > + * Translate CPU feature name from libvirt to libxl (from_libxl=false) 
> > > or from
> > > + * libxl to libvirt (from_libxl=true).
> > > + */
> > > +const char *
> > > +xenTranslateCPUFeature(const char *feature_name, bool from_libxl)
> > > +{
> > > +static const char *translation_table[][2] = {
> > > +/* libvirt name, libxl name */
> > > +{ "cx16", "cmpxchg16" },
> > > +{ "cid", "cntxid" },
> > > +{ "ds_cpl", "dscpl" },
> > > +{ "pclmuldq", "pclmulqdq" },
> > > +{ "pni", "sse3" },
> > > +{ "ht", "htt" },
> > > +{ "pn", "psn" },
> > > +{ "clflush", "clfsh" },
> > > +{ "sep", "sysenter" },
> > > +{ "cx8", "cmpxchg8" },
> > > +{ "nodeid_msr", "nodeid" },
> > > +{ "cr8legacy", "altmovcr8" },
> > > +{ "lahf_lm", "lahfsahf" },
> > > +{ "cmp_legacy", "cmplegacy" },
> > > +{ "fxsr_opt", "ffxsr" },
> > > +{ "pdpe1gb", "page1gb" },
> > > +};
> > > +size_t i;
> > > +
> > > +for (i = 0; i < ARRAY_CARDINALITY(translation_table); i++)
> > > +if (STREQ(translation_table[i][from_libxl], feature_name))
> > > +return translation_table[i][!from_libxl];
> > > +return feature_name;
> > > +}
> > > +
> > 
> > Cc'ing Jim as he may have some thoughts on the direction of this.
> > 
> > What happens if the admin decides to change /usr/share/libvirt/cpu_map.xml?
> 
> Is changing existing content likely/practical?
> 
> > Also you can also add new leafs/features to cpu_map.xml
> 
> Right. And I suppose the table in libxl_cpuid.c can grow too. And in cases
> where they differ we'd need to update the static table, which we'll probably
> only remember to do when receiving bug reports. So I like the idea of making
> this more dynamic, but I apparently don't have enough brain power today to
> understand your suggestion :-).
>
> > Maybe an idea instead is to have a table with all leafs that libxl has 
> > hardcoded
> > (i.e. cpuid_flags array on
> > http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/libxl/libxl_cpuid.c;hb=HEAD#l92).
> 
> Where would this table reside? In src/cpu/?
> 
> > Then adding a new cpu driver routine to look for cpuid data based on feature
> > name (and the reverse too). Finally you would populate this translation 
> > table at
> > driver load time, or you could even get away without a translation table 
> > (but
> > would be a little more inefficient?).
> 
> I'm having difficulty understanding how this provides a dynamic solution.
> Wouldn't the table you mention above need extended anytime the hardcoded one
> in libxl was extended? Which would probably only happen after receiving a
> bug report? :-)

Probably... And worse thing about such table is it needs to contain all
entries from said libxl_cpuid.c. My solution require only listing
entries with mismatching names.

Alternative would be to not use "new libxl syntax", but "old xend
syntax" (which is still supported by libxl). The later allow to list
specific bits instead of feature names. That was implemented in v1 of
this patch series[1]... The problem with that approach is currently libvirt
does not expose API for lookup of individual features, but only to
construct full CPU and then enumerate its CPUID. That means it isn't
feasible for the current approach (mode='host-passthrough', then
enable/disable individual features). See discussion on v1.
Adding such API to libvirt cpu driver is beyond my knowledge of libvirt
code and available time :/

> Hmm, the more I think about it, the more I convince myself that knowing
> libvirt and libxl use different names for a CPU feature is static
> information. I'm afraid I don't have any better ideas beyond Marek's neat
> trick.
> 
> Regards,
> Jim

[1] https://www.redhat.com/archives/libvir-list/2017-June/msg01292.html

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?


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

Re: [libvirt] [PATCH 8/9] qemu: Add support for resctrl

2018-01-03 Thread John Ferlan


Need to beef up this commit message.

On 12/13/2017 10:39 AM, Martin Kletzander wrote:
> Signed-off-by: Martin Kletzander 
> ---
>  src/qemu/qemu_process.c | 61 
> +
>  1 file changed, 56 insertions(+), 5 deletions(-)
> 

Is there anything special that needs to be done at libvirtd restart time?

If hot{plug|unplug} isn't supported if a vcpu has a vcputune defined,
then we probably need to inhibit that.  Otherwise, what needs to be done
to support it?

John

> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index a0f430f89f06..8abd9a5a8da3 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2507,6 +2507,33 @@ qemuProcessSetupEmulator(virDomainObjPtr vm)
>  }
>  
>  
> +static int
> +qemuProcessResctrlCreate(virQEMUDriverPtr driver,
> + virDomainObjPtr vm)
> +{
> +int ret = -1;
> +size_t i = 0;
> +virCapsPtr caps = virQEMUDriverGetCapabilities(driver, false);
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +
> +if (!caps)
> +return -1;
> +
> +for (i = 0; i < vm->def->ncachetunes; i++) {
> +if (virResctrlAllocCreate(caps->host.resctrl,
> +  vm->def->cachetunes[i]->alloc,
> +  "qemu",
> +  priv->machineName) < 0)
> +goto cleanup;
> +}
> +
> +ret = 0;
> + cleanup:
> +virObjectUnref(caps);
> +return ret;
> +}
> +
> +
>  static int
>  qemuProcessInitPasswords(virConnectPtr conn,
>   virQEMUDriverPtr driver,
> @@ -5013,12 +5040,26 @@ qemuProcessSetupVcpu(virDomainObjPtr vm,
>  {
>  pid_t vcpupid = qemuDomainGetVcpuPid(vm, vcpuid);
>  virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(vm->def, vcpuid);
> +size_t i = 0;
>  
> -return qemuProcessSetupPid(vm, vcpupid, VIR_CGROUP_THREAD_VCPU,
> -   vcpuid, vcpu->cpumask,
> -   vm->def->cputune.period,
> -   vm->def->cputune.quota,
> -   &vcpu->sched);
> +if (qemuProcessSetupPid(vm, vcpupid, VIR_CGROUP_THREAD_VCPU,
> +vcpuid, vcpu->cpumask,
> +vm->def->cputune.period,
> +vm->def->cputune.quota,
> +&vcpu->sched) < 0)
> +return -1;
> +
> +for (i = 0; i < vm->def->ncachetunes; i++) {
> +virDomainCachetuneDefPtr ct = vm->def->cachetunes[i];
> +
> +if (virBitmapIsBitSet(ct->vcpus, vcpuid)) {
> +if (virResctrlAllocAddPID(ct->alloc, vcpupid) < 0)
> +return -1;
> +break;
> +}
> +}
> +
> +return 0;
>  }
>  
>  
> @@ -5891,6 +5932,10 @@ qemuProcessLaunch(virConnectPtr conn,
>  if (qemuProcessSetupEmulator(vm) < 0)
>  goto cleanup;
>  
> +VIR_DEBUG("Setting up resctrlfs");
> +if (qemuProcessResctrlCreate(driver, vm) < 0)
> +goto cleanup;
> +
>  VIR_DEBUG("Setting domain security labels");
>  if (qemuSecuritySetAllLabel(driver,
>  vm,
> @@ -6539,6 +6584,12 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>   vm->def->name);
>  }
>  
> +/* Remove resctrl allocation after cgroups are cleaned up which makes it
> + * kind of safer (although removing the allocation should work even with
> + * pids in tasks file */
> +for (i = 0; i < vm->def->ncachetunes; i++)
> +virResctrlAllocRemove(vm->def->cachetunes[i]->alloc);
> +
>  qemuProcessRemoveDomainStatus(driver, vm);
>  
>  /* Remove VNC and Spice ports from port reservation bitmap, but only if
> 

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

Re: [libvirt] [PATCH 9/9] docs: Add CAT (resctrl) support into news.xml

2018-01-03 Thread John Ferlan



On 12/13/2017 10:39 AM, Martin Kletzander wrote:
> Signed-off-by: Martin Kletzander 
> ---
>  docs/news.xml | 9 +
>  1 file changed, 9 insertions(+)
> 

Just for completeness... Obviously this will need final tweaks... So far
so good though...

John

> diff --git a/docs/news.xml b/docs/news.xml
> index 0ec9857e2cab..ac3298de01a8 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -35,6 +35,15 @@
>  
>
>  
> +  
> +
> +  Added support for CAT (Cache allocation Technology)
> +
> +
> +  Domain vCPU threads can now have allocated some parts of host cache
> +  using the cachetune element in cputune.
> +
> +  
>  
>  
>  
> 

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

Re: [libvirt] [PATCH 7/9] tests: Add virresctrltest

2018-01-03 Thread John Ferlan



On 12/13/2017 10:39 AM, Martin Kletzander wrote:
> This test initializes capabilities from vircaps2xmldata (since it exists there
> already) and then requests list of free bitmaps (all unallocated space) from
> virresctrl.c
> 
> Desirable outputs are saved in virresctrldata.
> 
> Signed-off-by: Martin Kletzander 
> ---
>  tests/Makefile.am  |   8 +-
>  tests/virresctrldata/resctrl-cdp.schemata  |   2 +
>  .../virresctrldata/resctrl-skx-twocaches.schemata  |   1 +
>  tests/virresctrldata/resctrl-skx.schemata  |   1 +
>  tests/virresctrldata/resctrl.schemata  |   1 +
>  tests/virresctrltest.c | 102 
> +
>  6 files changed, 114 insertions(+), 1 deletion(-)
>  create mode 100644 tests/virresctrldata/resctrl-cdp.schemata
>  create mode 100644 tests/virresctrldata/resctrl-skx-twocaches.schemata
>  create mode 100644 tests/virresctrldata/resctrl-skx.schemata
>  create mode 100644 tests/virresctrldata/resctrl.schemata
>  create mode 100644 tests/virresctrltest.c
> 

Looks reasonable...

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v3.1 3/6] libxl: add support for CPUID features policy

2018-01-03 Thread Jim Fehlig


On 12/19/2017 06:19 AM, Joao Martins wrote:

On 12/13/2017 07:09 PM, Marek Marczykowski-Górecki wrote:

Convert CPU features policy into libxl cpuid policy settings. Use new
("libxl") syntax, which allow to enable/disable specific bits, using
host CPU as a base. For this reason, only "host-passthrough" mode is
accepted.
Libxl do not have distinction between "force" and "required" policy
(there is only "force") and also between "forbid" and "disable" (there
is only "disable"). So, merge them appropriately. If anything, "require"
and "forbid" should be enforced outside of specific driver.

Signed-off-by: Marek Marczykowski-Górecki 


This is quite neat Marek :)

I have one comment towards the end, suggesting an alternative to a static
translation table.


---
Changes since v3:
  - compile fix (one more s/libxlTranslateCPUFeature/xenTranslateCPUFeature/)
Changes since v2:
  - drop spurious changes
  - move libxlTranslateCPUFeature function to xen_xl.c, to be reused by
  xenconfig driver
Changes since v1:
  - use new ("libxl") syntax to set only bits explicitly mentioned in
  domain XML
---
  src/libxl/libxl_conf.c | 35 +--
  src/xenconfig/xen_xl.c | 34 ++
  src/xenconfig/xen_xl.h |  2 ++
  3 files changed, 69 insertions(+), 2 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 184610966..5eae6d10f 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -50,6 +50,7 @@
  #include "secret_util.h"
  #include "cpu/cpu.h"
  #include "xen_common.h"
+#include "xen_xl.h"
  
  
  #define VIR_FROM_THIS VIR_FROM_LIBXL

@@ -357,6 +358,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
  bool hasHwVirt = false;
  int nested_hvm = -1;
  bool svm = false, vmx = false;
+char xlCPU[32];
  
  if (def->cpu->mode != (VIR_CPU_MODE_HOST_PASSTHROUGH)) {

  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -379,15 +381,44 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
  case VIR_CPU_FEATURE_DISABLE:
  case VIR_CPU_FEATURE_FORBID:
  if ((vmx && STREQ(def->cpu->features[i].name, 
"vmx")) ||
-(svm && STREQ(def->cpu->features[i].name, 
"svm")))
+(svm && STREQ(def->cpu->features[i].name, 
"svm"))) {
  nested_hvm = 0;
+continue;
+}
+
+snprintf(xlCPU,
+sizeof(xlCPU),
+"%s=0",
+xenTranslateCPUFeature(
+def->cpu->features[i].name,
+false));
+if (libxl_cpuid_parse_config(&b_info->cpuid, 
xlCPU)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+_("unsupported cpu feature '%s'"),
+def->cpu->features[i].name);
+return -1;
+}
  break;
  
  case VIR_CPU_FEATURE_FORCE:

  case VIR_CPU_FEATURE_REQUIRE:
  if ((vmx && STREQ(def->cpu->features[i].name, 
"vmx")) ||
-(svm && STREQ(def->cpu->features[i].name, 
"svm")))
+(svm && STREQ(def->cpu->features[i].name, 
"svm"))) {
  nested_hvm = 1;
+continue;
+}
+
+snprintf(xlCPU,
+sizeof(xlCPU),
+"%s=1",
+xenTranslateCPUFeature(
+def->cpu->features[i].name, false));
+if (libxl_cpuid_parse_config(&b_info->cpuid, 
xlCPU)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+_("unsupported cpu feature '%s'"),
+def->cpu->features[i].name);
+return -1;
+}
  break;
  case VIR_CPU_FEATURE_OPTIONAL:
  case VIR_CPU_FEATURE_LAST:
diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
index 317c7a08d..356ca3a4b 100644
--- a/src/xenconfig/xen_xl.c
+++ b/src/xenconfig/xen_xl.c
@@ -225,6 +225,40 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, 
virCapsPtr caps)
  return 0;
  }
  
+/*

+ * Translate CPU feature name from libvirt to libxl (from_libxl=false) or from
+ * libxl to libvirt (from_libxl=true).
+ */
+const c

Re: [libvirt] [PATCH 6/9] conf: Add support for cputune/cachetune

2018-01-03 Thread John Ferlan



On 12/13/2017 10:39 AM, Martin Kletzander wrote:
> More info in the documentation, this is basically the XML parsing/formatting
> support, schemas, tests and documentation for the new cputune/cachetune 
> element
> that will get used by following patches.
> 
> Signed-off-by: Martin Kletzander 
> ---
>  docs/formatdomain.html.in  |  54 +
>  docs/schemas/domaincommon.rng  |  32 +++
>  src/conf/domain_conf.c | 251 
> +
>  src/conf/domain_conf.h |  13 ++
>  tests/genericxml2xmlindata/cachetune-cdp.xml   |  36 +++
>  .../cachetune-colliding-allocs.xml |  30 +++
>  .../cachetune-colliding-tunes.xml  |  32 +++
>  .../cachetune-colliding-types.xml  |  30 +++
>  tests/genericxml2xmlindata/cachetune-small.xml |  29 +++
>  tests/genericxml2xmlindata/cachetune.xml   |  33 +++
>  tests/genericxml2xmltest.c |  10 +
>  11 files changed, 550 insertions(+)
>  create mode 100644 tests/genericxml2xmlindata/cachetune-cdp.xml
>  create mode 100644 tests/genericxml2xmlindata/cachetune-colliding-allocs.xml
>  create mode 100644 tests/genericxml2xmlindata/cachetune-colliding-tunes.xml
>  create mode 100644 tests/genericxml2xmlindata/cachetune-colliding-types.xml
>  create mode 100644 tests/genericxml2xmlindata/cachetune-small.xml
>  create mode 100644 tests/genericxml2xmlindata/cachetune.xml
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 01db83e60820..623860cc0e95 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -689,6 +689,10 @@
>  -1
>  
>  
> +
> +  
> +  
> +
>
>...
>  
> @@ -834,6 +838,56 @@
>  Since 1.2.13
>
>  
> +  cachetuneSince 3.10.0

At least 4.0.0 if not 4.1.0

> +  
> +Optional cachetune element can control allocations for 
> CPU
> +caches using the resctrlfs on the host. Whether or not is this 
> supported

s/resctrlfs/Resource Control File System/

Although this defaults to /sys/fs/resctrl, I'm not sure if we want to
state the default or not.  It *is* the path we will use though and I
don't believe we have a way to alter that default (yet).


> +can be gathered from capabilities where some limitations like minimum
> +size and required granularity are reported as well.  The required
> +attribute vcpus specifies to which vCPUs this allocation
> +applies. A vCPU can only be member of one cachetune 
> element
> +allocations. Supported subelements are:
> +
> +  cache
> +  
> +This element controls the allocation of CPU cache and has the
> +following attributes:
> +
> +  level
> +  
> +Host cache level from which to allocate.
> +  
> +  id
> +  
> +Host cache id from which to allocate.

Oh - should have read this first ;-)... Maybe rename @id internally to
hostCacheID... Shall I assume someone setting this up would know how to
determine how to get these values from the host?

> +  
> +  type
> +  
> +Type of allocation.  Can be code for code
> +(instructions), data for data or 
> both
> +for both code and data (unified).  Currently the allocation 
> can
> +be done only with the same type as the host supports, meaning
> +you cannot request both for host with CDP
> +(code/data prioritization) enabled.
> +  
> +  size
> +  
> +The size of the region to allocate. The value by default is 
> in
> +bytes, but the unit attribute can be used to 
> scale
> +the value.
> +  
> +  unit (optional)
> +  
> +If specified it is the unit such as KiB, MiB, GiB, or TiB
> +(described in the memory element
> +for Memory 
> Allocation)
> +in which size is specified, defaults to bytes.
> +  
> +
> +  
> +
> +
> +  
>  
>  
>  

[...]

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 66e21c4bdbee..ec8d760c7971 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2883,6 +2883,17 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
>  VIR_FREE(loader);
>  }
>  
> +static void
> +virDomainCachetuneDefFre

Re: [libvirt] [PATCH] maint: Update to latest gnulib

2018-01-03 Thread Eric Blake

On 01/03/2018 08:46 AM, Peter Krempa wrote:

>>
>> No bright ideas on this other than perhaps only including changes just
>> prior to the particular one that breaks things or somehow revert just
>> that one in our local copy.
> 
> How about just killing that stupid syntax check in our local copy?

As in, tweaking local-checks-to-skip to exclude the copyright date
check? I'd be okay with that idea, so that we don't hit this problem in
future years.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



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 v2] maint: Update to latest gnulib

2018-01-03 Thread Eric Blake

On 01/02/2018 11:20 PM, Michal Privoznik wrote:
> On 01/02/2018 11:23 PM, Eric Blake wrote:
>> From: Michal Privoznik 
>>
>> Unfortunately, since gnulib's commit of 2c5d558745 there's an
>> unused parameter to stat_time_normalize() function which gnulib
>> developers don't want to fix yet [1]. Therefore, we have to work
>> around it by temporarily providing a downstream patch.

>> ---
>>  .gnulib   |  2 +-
>>  bootstrap |  4 ++--
>>  gnulib/local/lib/stat-time.h.diff | 13 +
>>  3 files changed, 16 insertions(+), 3 deletions(-)
>>  create mode 100644 gnulib/local/lib/stat-time.h.diff
> 
> Looks like gnulib can be fixed after all. I mean, both Jim an Paul
> agreed with your patch for _GL_UNUSED. Therefore we can just update the
> submodule after you push the gnulib fix.

Indeed. I've now pushed just the .gnulib and bootstrap changes (no tweak
to gnulib/local/ after all), pointing to gnulib commit 7e7c5c795, and
pushed under the gnulib maintenance rule now that the patch is no longer
ugly.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



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 5/9] resctrl: Add functions to work with resctrl allocations

2018-01-03 Thread John Ferlan

On 12/13/2017 10:39 AM, Martin Kletzander wrote:
> With this commit we finally have a way to read and manipulate basic resctrl
> settings.  Locking is done only on exposed functions that read/write from/to
> resctrlfs.  Not in functions that are exposed in virresctrlpriv.h as those are
> only supposed to be used from tests.
> 
> Signed-off-by: Martin Kletzander 
> ---
>  src/Makefile.am   |2 +-
>  src/libvirt_private.syms  |   11 +
>  src/util/virresctrl.c | 1041 
> -
>  src/util/virresctrl.h |   62 +++
>  src/util/virresctrlpriv.h |   27 ++
>  5 files changed, 1141 insertions(+), 2 deletions(-)
>  create mode 100644 src/util/virresctrlpriv.h
> 

Geez, as referenced by the cover letter, I'm glad this is the non way
too complicated parts of the code (tongue firmly planted in my cheek
with the obligatory eye roll emoji).

So, IIUC virResctrlInfoPtr is the "host" world and virResctrlAllocPtr is
the "guest" world, right?  If so might be something to add to the commit
messages to make things just slightly more clear.

Lots of code here - hopefully another set of eyes can look at this too -
I'm sure I'll miss something as it's been very difficult to review this
in one (long) session...

Note to self, no sense in grep-ing for "alloc" or "free" any more after
this code is pushed as they're liberally used.  Kind of funny to see
"alloc_free" in the same line ;-)

The other usage that was really confusing is @cache w/ @[n]masks and
@[n]sizes.  It seems to be used as the array index for both and it's
never really crystal clear over the relationship between masks and sizes.

> diff --git a/src/Makefile.am b/src/Makefile.am
> index 4c022d1e44b9..9b4fd0c1d597 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -167,7 +167,7 @@ UTIL_SOURCES = \
>   util/virprocess.c util/virprocess.h \
>   util/virqemu.c util/virqemu.h \
>   util/virrandom.h util/virrandom.c \
> - util/virresctrl.h util/virresctrl.c \
> + util/virresctrl.h util/virresctrl.c util/virresctrlpriv.h \
>   util/virrotatingfile.h util/virrotatingfile.c \
>   util/virscsi.c util/virscsi.h \
>   util/virscsihost.c util/virscsihost.h \
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 1a4f304f5e1f..a8009d913669 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2548,6 +2548,17 @@ virRandomInt;
>  # util/virresctrl.h
>  virCacheTypeFromString;
>  virCacheTypeToString;
> +virResctrlAllocAddPID;
> +virResctrlAllocCreate;
> +virResctrlAllocForeachSize;
> +virResctrlAllocFormat;
> +virResctrlAllocGetFree;
> +virResctrlAllocNew;
> +virResctrlAllocRemove;
> +virResctrlAllocSetID;
> +virResctrlAllocSetSize;
> +virResctrlAllocUpdateMask;
> +virResctrlAllocUpdateSize;
>  virResctrlGetInfo;
>  virResctrlInfoGetCache;
>  virResctrlInfoNew;
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index a681322905be..32ab84b6f121 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -18,8 +18,12 @@
>  
>  #include 
>  
> -#include "virresctrl.h"
> +#include 
> +#include 
> +#include 
> +#include 
>  
> +#include "virresctrlpriv.h"
>  #include "c-ctype.h"
>  #include "count-one-bits.h"
>  #include "viralloc.h"
> @@ -151,6 +155,156 @@ virResctrlInfoNew(void)
>  }
>  
>  
> +/* Alloc-related definitions and AllocClass-related functions */
> +typedef struct _virResctrlAllocPerType virResctrlAllocPerType;
> +typedef virResctrlAllocPerType *virResctrlAllocPerTypePtr;
> +struct _virResctrlAllocPerType {
> +/* There could be bool saying whether this is set or not, but since 
> everything
> + * in virResctrlAlloc (and most of libvirt) goes with pointer arrays we 
> would
> + * have to have one more level of allocation anyway, so this stays 
> faithful to
> + * the concept */
> +unsigned long long **sizes;
> +size_t nsizes;

Perhaps something I should have noted in patch 2 - these @sizes are what
exactly?  Is it a page size or just a "max size" in ?bytes for something
stored in the cache?

> +
> +/* Mask for each cache */
> +virBitmapPtr *masks;
> +size_t nmasks;

And of course, what does the mask represent?

Just a quick comment would suffice - for the future readers as well.

> +};
> +
> +typedef struct _virResctrlAllocPerLevel virResctrlAllocPerLevel;
> +typedef virResctrlAllocPerLevel *virResctrlAllocPerLevelPtr;
> +struct _virResctrlAllocPerLevel {
> +virResctrlAllocPerTypePtr *types; /* Indexed with enum virCacheType */
> +};
> +
> +struct _virResctrlAlloc {
> +virObject parent;
> +
> +virResctrlAllocPerLevelPtr *levels;
> +size_t nlevels;

These represent the "L#" for the directory, right?  Examples would be
L1both, L2code, l3data, right?

Just trying to provide enough information so that some future reader
will have a chance to understand the design at it's simplist level.

> +
> +c

Re: [libvirt] [PATCH] nodedev: Fix failing to parse PCI address for non-PCI network devices

2018-01-03 Thread Jim Fehlig


On 01/03/2018 01:40 AM, Erik Skultety wrote:

On Fri, Dec 22, 2017 at 01:05:26PM +0800, Fei Li wrote:

Commit 8708ca01c added virNetDevSwitchdevFeature to check whether
the NIC had Switchdev capabilities; however this causes errors for
network devices whose address is not in PCI format, like qeth device
whose address is 0.0.0800, when calling virPCIDeviceAddressParse.

Change virReportError to VIR_DEBUG to avoid these two error messages
occuring when starting the libvirtd service, just leave their callers
to handle the return values respectively.

Signed-off-by: Fei Li 
---
  src/util/virpci.c | 6 ++
  1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/util/virpci.c b/src/util/virpci.c
index fe57bef32..880d7baba 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -2561,7 +2561,7 @@ logStrToLong_ui(char const *s,

  ret = virStrToLong_ui(s, end_ptr, base, result);
  if (ret != 0)
-VIR_ERROR(_("Failed to convert '%s' to unsigned int"), s);
+VIR_DEBUG("Failed to convert '%s' to unsigned int", s);
  return ret;
  }

@@ -2638,9 +2638,7 @@ virPCIGetDeviceAddressFromSysfsLink(const char 
*device_link)
  goto out;

  if (virPCIDeviceAddressParse(config_address, bdf) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Failed to parse PCI config address '%s'"),
-   config_address);
+VIR_DEBUG("Failed to parse PCI config address '%s'", config_address);
  VIR_FREE(bdf);
  goto out;
  }


I am not familiar with switchdev feature, but this is certainly not the correct
fix, errors do have their meaning and by simply mangling the log level of
messages to suit your use case is not the way.


I'll suggested this change when Fei and I discussed this bug off list. I'm far 
from an expert on this code as well, but I made the suggestions since some 
callers of virPCIGetDeviceAddressFromSysfsLink() report their own errors when it 
returns NULL and others treat a NULL return value as non-fatal. E.g. the 
virNetDevSwitchdevFeature()->virNetDevGetPCIDevice()->virPCIGetDeviceAddressFromSysfsLink() 
call chain.


In addition, virPCIGetDeviceAddressFromSysfsLink() already has

if (!virFileExists(device_link)) {
VIR_DEBUG("'%s' does not exist", device_link);
return NULL;
}

If changing error reporting to debug was acceptable, Fei's patch would need 
amended to change one more instance of virReportError().



Again, I'm far from an expert
in this area, but IMHO you should special case handling of devices supporting
this feature by, e.g. using the VIR_NET_DEV_FEAT_SWITCHDEV enum or create
another one if suitable/appropriate in this case and skip parsing the PCI
address completely.


IIUC, we are trying to determine if the interface supports 
VIR_NET_DEV_FEAT_SWITCHDEV in virNetDevSwitchdevFeature(), and part of that is 
checking if the interface is backed by a PCI device. But like you, I'm not 
familiar with this code and may be missing something. Adding the author of the 
feature to cc for suggestions.


Regards,
Jim

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

[libvirt] [PATCH 4/4] apparmor, virt-aa-helper: drop static channel rule

2018-01-03 Thread Christian Ehrhardt

This is now covered by DomainSetPathLabel being implemented in apparmor.

Signed-off-by: Christian Ehrhardt 
---
 src/security/virt-aa-helper.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 07ece73..f7ccae0 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -1353,8 +1353,6 @@ main(int argc, char **argv)
   LOCALSTATEDIR, ctl->def->name);
 virBufferAsprintf(&buf, "  
\"%s/lib/libvirt/qemu/domain-%d-%.*s/*\" rw,\n",
   LOCALSTATEDIR, ctl->def->id, 20, 
ctl->def->name);
-virBufferAsprintf(&buf, "  
\"%s/lib/libvirt/qemu/channel/target/domain-%d-%.*s/*\" rw,\n",
-  LOCALSTATEDIR, ctl->def->id, 20, 
ctl->def->name);
 virBufferAsprintf(&buf, "  \"%s/run/libvirt/**/%s.pid\" 
rwk,\n",
   LOCALSTATEDIR, ctl->def->name);
 virBufferAsprintf(&buf, "  \"/run/libvirt/**/%s.pid\" rwk,\n",
-- 
2.7.4

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

[libvirt] [PATCH 1/4] security, apparmor: implement domainSetPathLabel

2018-01-03 Thread Christian Ehrhardt

This came up in discussions around huge pages, but it will cover
more per guest paths that should be added to the guests apparmor profile:
 - keys via qemuDomainWriteMasterKeyFile
 - per domain dirs via qemuProcessMakeDir
 - memory backing paths via qemuProcessBuildDestroyMemoryPathsImpl

Signed-off-by: Christian Ehrhardt 
---
 src/security/security_apparmor.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 1db94c6..dcd6f52 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -953,6 +953,13 @@ AppArmorSetSavedStateLabel(virSecurityManagerPtr mgr,
 return reload_profile(mgr, def, savefile, true);
 }
 
+static int
+AppArmorSetPathLabel(virSecurityManagerPtr mgr,
+   virDomainDefPtr def,
+   const char *path)
+{
+return reload_profile(mgr, def, path, true);
+}
 
 static int
 AppArmorRestoreSavedStateLabel(virSecurityManagerPtr mgr,
@@ -1045,6 +1052,8 @@ virSecurityDriver virAppArmorSecurityDriver = {
 .domainSetSavedStateLabel   = AppArmorSetSavedStateLabel,
 .domainRestoreSavedStateLabel   = AppArmorRestoreSavedStateLabel,
 
+.domainSetPathLabel = AppArmorSetPathLabel,
+
 .domainSetSecurityImageFDLabel  = AppArmorSetFDLabel,
 .domainSetSecurityTapFDLabel= AppArmorSetFDLabel,
 
-- 
2.7.4

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

[libvirt] [PATCH 2/4] security: full path option for DomainSetPathLabel

2018-01-03 Thread Christian Ehrhardt

virSecurityManagerDomainSetPathLabel is used to make a path known
to the security modules, but today is used interchangably for
 - paths to files/dirs to be accessed directly
 - paths to a dir, but the access will actually be to files therein

Depending on the security module it is important to know which of
these types it will be.

The argument fullpath augments the call to the implementations of
DomainSetPathLabel that can - per security module - decide if extra
actions shall be taken.

For now dac/selinux handle this as before, but apparmor will make
use of it to add a wildcard to the path that was passed.

Signed-off-by: Christian Ehrhardt 
---
 src/qemu/qemu_domain.c   |  2 +-
 src/qemu/qemu_process.c  |  4 ++--
 src/security/security_apparmor.c | 17 +++--
 src/security/security_dac.c  |  3 ++-
 src/security/security_driver.h   |  3 ++-
 src/security/security_manager.c  |  5 +++--
 src/security/security_manager.h  |  3 ++-
 src/security/security_selinux.c  |  3 ++-
 src/security/security_stack.c|  5 +++--
 9 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 70fb406..ac3e182 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -692,7 +692,7 @@ qemuDomainWriteMasterKeyFile(virQEMUDriverPtr driver,
 }
 
 if (qemuSecurityDomainSetPathLabel(driver->securityManager,
-   vm->def, path) < 0)
+   vm->def, path, false) < 0)
 goto cleanup;
 
 ret = 0;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index a0f430f..1a0923a 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3401,7 +3401,7 @@ qemuProcessBuildDestroyMemoryPathsImpl(virQEMUDriverPtr 
driver,
 }
 
 if (qemuSecurityDomainSetPathLabel(driver->securityManager,
-   def, path) < 0) {
+   def, path, true) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
 _("Unable to label %s"), path);
 return -1;
@@ -4514,7 +4514,7 @@ qemuProcessMakeDir(virQEMUDriverPtr driver,
 }
 
 if (qemuSecurityDomainSetPathLabel(driver->securityManager,
-   vm->def, path) < 0)
+   vm->def, path, true) < 0)
 goto cleanup;
 
 ret = 0;
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index dcd6f52..60a8e08 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -956,9 +956,22 @@ AppArmorSetSavedStateLabel(virSecurityManagerPtr mgr,
 static int
 AppArmorSetPathLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
-   const char *path)
+   const char *path,
+   bool fullpath)
 {
-return reload_profile(mgr, def, path, true);
+int rc = -1;
+char *full_path = NULL;
+
+if (fullpath) {
+if (virAsprintf(&full_path, "%s/{,**}", path) < 0)
+return -1;
+rc = reload_profile(mgr, def, full_path, true);
+VIR_FREE(full_path);
+}
+else
+rc = reload_profile(mgr, def, path, true);
+
+return rc;
 }
 
 static int
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 609d259..60c4f09 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -2081,7 +2081,8 @@ virSecurityDACGetBaseLabel(virSecurityManagerPtr mgr,
 static int
 virSecurityDACDomainSetPathLabel(virSecurityManagerPtr mgr,
  virDomainDefPtr def,
- const char *path)
+ const char *path,
+ bool fullpath ATTRIBUTE_UNUSED)
 {
 virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
 virSecurityLabelDefPtr seclabel;
diff --git a/src/security/security_driver.h b/src/security/security_driver.h
index 47dad8b..20168a6 100644
--- a/src/security/security_driver.h
+++ b/src/security/security_driver.h
@@ -139,7 +139,8 @@ typedef int (*virSecurityDomainRestoreInputLabel) 
(virSecurityManagerPtr mgr,
virDomainInputDefPtr input);
 typedef int (*virSecurityDomainSetPathLabel) (virSecurityManagerPtr mgr,
   virDomainDefPtr def,
-  const char *path);
+  const char *path,
+  bool fullpath);
 typedef int (*virSecurityDomainSetChardevLabel) (virSecurityManagerPtr mgr,
  virDomainDefPtr def,
  virDomainChrSourceDefPtr 
dev_source,
diff --git a/src/security/secu

[libvirt] [PATCH 3/4] security, apparmor: add (Set|Restore)ChardevLabel

2018-01-03 Thread Christian Ehrhardt

Since 1b4f66e "security: introduce virSecurityManager
(Set|Restore)ChardevLabel" this is a public API of security manager.

Implementing this in apparmor avoids miss any rules that should be
added for devices labeled via these calls.

Signed-off-by: Christian Ehrhardt 
---
 src/security/security_apparmor.c | 74 
 1 file changed, 74 insertions(+)

diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 60a8e08..e91f157 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -946,6 +946,77 @@ AppArmorRestoreSecurityHostdevLabel(virSecurityManagerPtr 
mgr,
 }
 
 static int
+AppArmorSetChardevLabel(virSecurityManagerPtr mgr,
+virDomainDefPtr def,
+virDomainChrSourceDefPtr dev_source,
+bool chardevStdioLogd ATTRIBUTE_UNUSED)
+{
+char *in = NULL, *out = NULL;
+int ret = -1;
+
+virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def,
+SECURITY_APPARMOR_NAME);
+if (!secdef)
+return 0;
+
+switch ((virDomainChrType) dev_source->type) {
+case VIR_DOMAIN_CHR_TYPE_DEV:
+case VIR_DOMAIN_CHR_TYPE_FILE:
+case VIR_DOMAIN_CHR_TYPE_UNIX:
+case VIR_DOMAIN_CHR_TYPE_PTY:
+ret = reload_profile(mgr, def, dev_source->data.file.path, true);
+break;
+
+case VIR_DOMAIN_CHR_TYPE_PIPE:
+if (virAsprintf(&in, "%s.in", dev_source->data.file.path) < 0 ||
+virAsprintf(&out, "%s.out", dev_source->data.file.path) < 0)
+goto done;
+if (virFileExists(in)) {
+if (reload_profile(mgr, def, in, true) < 0)
+goto done;
+}
+if (virFileExists(out)) {
+if (reload_profile(mgr, def, out, true) < 0)
+goto done;
+}
+ret = reload_profile(mgr, def, dev_source->data.file.path, true);
+break;
+
+case VIR_DOMAIN_CHR_TYPE_SPICEPORT:
+case VIR_DOMAIN_CHR_TYPE_NULL:
+case VIR_DOMAIN_CHR_TYPE_VC:
+case VIR_DOMAIN_CHR_TYPE_STDIO:
+case VIR_DOMAIN_CHR_TYPE_UDP:
+case VIR_DOMAIN_CHR_TYPE_TCP:
+case VIR_DOMAIN_CHR_TYPE_SPICEVMC:
+case VIR_DOMAIN_CHR_TYPE_NMDM:
+case VIR_DOMAIN_CHR_TYPE_LAST:
+ret = 0;
+break;
+}
+
+ done:
+VIR_FREE(in);
+VIR_FREE(out);
+return ret;
+}
+
+static int
+AppArmorRestoreChardevLabel(virSecurityManagerPtr mgr,
+virDomainDefPtr def,
+virDomainChrSourceDefPtr dev_source 
ATTRIBUTE_UNUSED,
+bool chardevStdioLogd ATTRIBUTE_UNUSED)
+{
+virSecurityLabelDefPtr secdef =
+virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME);
+
+if (!secdef)
+return 0;
+
+return reload_profile(mgr, def, NULL, false);
+}
+
+static int
 AppArmorSetSavedStateLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
const char *savefile)
@@ -1067,6 +1138,9 @@ virSecurityDriver virAppArmorSecurityDriver = {
 
 .domainSetPathLabel = AppArmorSetPathLabel,
 
+.domainSetSecurityChardevLabel  = AppArmorSetChardevLabel,
+.domainRestoreSecurityChardevLabel  = AppArmorRestoreChardevLabel,
+
 .domainSetSecurityImageFDLabel  = AppArmorSetFDLabel,
 .domainSetSecurityTapFDLabel= AppArmorSetFDLabel,
 
-- 
2.7.4

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

[libvirt] [PATCH 0/4] apparmor: implement more domain callbacks

2018-01-03 Thread Christian Ehrhardt

Based on a discussion in [1] I found that the AppArmor security
module lacked some callbacks. Implementing those not only fixes
the issue I had before but will also cover a few more cases I
didn't even run into so far.

[1]: https://www.redhat.com/archives/libvir-list/2017-December/msg00726.html

Christian Ehrhardt (4):
  security, apparmor: implement domainSetPathLabel
  security: full path option for DomainSetPathLabel
  security, apparmor: add (Set|Restore)ChardevLabel
  apparmor, virt-aa-helper: drop static channel rule

 src/qemu/qemu_domain.c   |  2 +-
 src/qemu/qemu_process.c  |  4 +-
 src/security/security_apparmor.c | 96 
 src/security/security_dac.c  |  3 +-
 src/security/security_driver.h   |  3 +-
 src/security/security_manager.c  |  5 ++-
 src/security/security_manager.h  |  3 +-
 src/security/security_selinux.c  |  3 +-
 src/security/security_stack.c|  5 ++-
 src/security/virt-aa-helper.c|  2 -
 10 files changed, 113 insertions(+), 13 deletions(-)

-- 
2.7.4

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

Re: [libvirt] [PATCH 06/12] apparmor, libvirt-qemu: Allow access to hugepage mounts

2018-01-03 Thread Christian Ehrhardt

[...]

>> To me, 1 feels most correct cause while the other two fix hugepages,
>> there seem to be lurking bugs since we aren't implementing
>> domainSetPathLabel.
>>
>
> I work on #1 a while and I think we can do a lot good here.
> Yet while I'm convinced at the changes this is currently a debugging 
> nightmare.
> I guess it wants to become a 2018 submission.

Note: I'll not user reply-to onto this thread to keep threading somewhat sane.
Also the new submission, while inspired by this discussion, is a
totally separate thing.

> So for now keep this hugepage patch out of consideration when looking
> at applying all those with many +1's.

So as I expected the hugepage patch of this series will be covered by
the new series that I submit.
But I wanted to ask for all the others changes in the current series
here to be considered - most have one or two acks already.
Let me know if more is needed to commit them.

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

钅艮彳亍卡出售他人账户洗￥钱专用，送礼专用加球球 4 4 5 4 9 6 1 0 8 备用，以防不时之需

2018-01-03 Thread hrtolj




钅艮  彳亍  卡  出  售  他人账户洗￥钱专用，送礼专用  加球球  4 4 5 4 9 6 1 0 8备用，以防不时之需
 
 
 
 
 
应晖当然知道她要和他商量什么，接口说：“正好，我也有事情请你帮忙。”


当她睁开一双倦眼，橡眺地，见到一个人。
"道具吧，我没见过么？张牙舞爪的，小角色！"
我问：
见她迷惑，便问：

Re: [libvirt] [PATCH] apparmor: fix virt-aa-helper profile

2018-01-03 Thread Jamie Strandboge

On Wed, 2018-01-03 at 10:55 +0100, Cédric Bosdonnat wrote:
> Fix rule introduced by commit 0f33025a:
>   * to handle /var/run not being a symlink to /run
>   * to be properly parsed: missing comma at the end.
> ---
>  examples/apparmor/usr.lib.libvirt.virt-aa-helper | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> index 9c822b644..105f09e43 100644
> --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> @@ -51,7 +51,7 @@ profile virt-aa-helper
> /usr/{lib,lib64}/libvirt/virt-aa-helper {
>/var/lib/libvirt/images/** r,
>/{media,mnt,opt,srv}/** r,
># For virt-sandbox
> -  /run/libvirt/**/[sv]d[a-z] r
> +  /{,var/}run/libvirt/**/[sv]d[a-z] r,
>  
LGTM. +1 to commit as is.

-- 
Jamie Strandboge | http://www.canonical.com

signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] storage: Fixing missing 'backingStore' tag from volume XML dumps.

2018-01-03 Thread Peter Krempa

On Tue, Jan 02, 2018 at 16:53:13 -0200, Julio Faracco wrote:
> Hi guys,
> 
> Any possibility to include a test case for this scenario?

You can look into adding it to virstoragetest if you want to pursue
adding the test.

I'll push this patch in the meanwhile.

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

Re: [libvirt] [PATCH] maint: Update to latest gnulib

2018-01-03 Thread Michal Privoznik

On 01/03/2018 03:46 PM, Peter Krempa wrote:
> On Tue, Jan 02, 2018 at 08:09:37 -0500, John Ferlan wrote:
>>
>>
>> On 01/02/2018 04:28 AM, Michal Privoznik wrote:
>>> Unfortunately, since gnulib's commit of 2c5d558745 there's an
>>> unused parameter to stat_time_normalize() function which gnulib
>>> developers don't want to fix [1]. Therefore, we have to work
>>> around it by temporarily suspending -Wunused-parameter.
>>>
>>> 1: http://lists.gnu.org/archive/html/bug-gnulib/2018-01/msg0.html
>>>
>>> Signed-off-by: Michal Privoznik 
>>> ---
>>>
>>> While we have 'gnulib update free' push rule, this one is not trivial at
>>> all and thus I have not pushed it. It's ugly and I don't like it. So any
>>> ideas are welcome.
>>>
>>>  .gnulib| 2 +-
>>>  bootstrap  | 4 ++--
>>>  src/storage/storage_util.c | 3 +++
>>>  3 files changed, 6 insertions(+), 3 deletions(-)
>>>
>>
>>
>> No bright ideas on this other than perhaps only including changes just
>> prior to the particular one that breaks things or somehow revert just
>> that one in our local copy.
> 
> How about just killing that stupid syntax check in our local copy?
> 

That wouldn't help either. We could not upgrade to newer gnulib unless
it's fixed. But fortunately, it looks like we're close to an agreement
how to fix gnulib so we can update it in our repo too.

michal

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

Re: [libvirt] [PATCH v2] qemuBuildMemPathStr: Forbid memoryBacking/access for non-numa case

2018-01-03 Thread Ján Tomko


On Wed, Jan 03, 2018 at 07:06:01AM +0100, Michal Privoznik wrote:

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

If a domain has no numa nodes, that means we don't put any
memory-backend-file onto the qemu command line. That in turn
means we can't set access='shared'. Therefore, we should produce
an error instead of ignoring the setting silently.

Signed-off-by: Michal Privoznik 
---

diff to v2:
- switched from check at cmd line generation to validation callback (as
 requested in review).

src/qemu/qemu_domain.c  | 29 -
tests/qemuxml2argvdata/hugepages-memaccess3.xml | 87 +
tests/qemuxml2argvtest.c|  3 +
3 files changed, 118 insertions(+), 1 deletion(-)
create mode 100644 tests/qemuxml2argvdata/hugepages-memaccess3.xml



ACK

Jan


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

Re: [libvirt] [PATCH v2 0/3] Storage pool common object fixes

2018-01-03 Thread John Ferlan


ping?

Tks -

John

On 12/18/2017 07:56 AM, John Ferlan wrote:
> v1: https://www.redhat.com/archives/libvir-list/2017-December/msg00543.html
> 
> Changes since v1... 
> 
>  * Added a patch to handle a NULL return with pool obj lock
> 
>  * Alter the IsDuplicate API to use a bool parameter
> 
>  * Use the IsDuplicate API from the test driver. This would have generated
>the correct error message about a duplicate UUID instead of the Duplicate
>key that was generated. Ran virt-manager tests prior to Cole's fixes and
>of course after.
> 
> John Ferlan (3):
>   conf: Need to unlock pools on object allocation failure
>   conf: Use bool for @check_active parameter
>   test: Use virStoragePoolObjIsDuplicate for storage define/create
> 
>  src/conf/virstorageobj.c |  4 ++--
>  src/conf/virstorageobj.h |  2 +-
>  src/storage/storage_driver.c |  4 ++--
>  src/test/test_driver.c   | 11 ---
>  4 files changed, 9 insertions(+), 12 deletions(-)
> 

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

Re: [libvirt] [PATCH v4 00/13] Move qemu command line controller checks to qemuDomainDeviceDefValidateController* checks

2018-01-03 Thread John Ferlan



ping?

Tks -

John

On 12/12/2017 10:06 AM, John Ferlan wrote:
> v3: https://www.redhat.com/archives/libvir-list/2017-December/msg00209.html
> 
> Differences since v3:
> 
>  * Pushed first 4 ACK'd patches of v3
> 
>  * Rework/Separate out a few patches for the SCSI handling
> 
>  * Alter the PCI handling as requested by code review (although I still
>had a question to a review comment regarding whether or not the model
>should be handled in PostParse).
> 
>  * Alter the SATA code as indicated by code review
> 
>  * Add Andrea's patch into the series
> 
> Andrea Bolognani (1):
>   qemu: Add missing checks for pcie-root-port options
> 
> John Ferlan (12):
>   qemu: Introduce qemuDomainDeviceDefValidateControllerAttributes
>   qemu: Move model set for qemuBuildControllerDevStr
>   qemu: Adjust SCSI controller switch in qemuBuildControllerDevStr
>   qemu: Add check for iothread attribute in validate controller
>   qemu: Introduce qemuDomainDeviceDefValidateControllerSCSI
>   qemu: Introduce qemuDomainDeviceDefValidateControllerPCI
>   qemu: Use virDomainPCIControllerOpts in qemuBuildControllerDevStr
>   qemu: Move PCI command modelName check to controller def validate
>   qemu: Move PCI command modelName TypeToString to controller def
> validate
>   qemu: Move PCI more command checks to controller def validate
>   qemu: Complete PCI command checks to controller def validate
>   qemu: Introduce qemuDomainDeviceDefValidateControllerSATA
> 
>  src/qemu/qemu_command.c | 403 --
>  src/qemu/qemu_domain.c  | 420 
> +++-
>  tests/qemumemlocktest.c |  14 ++
>  tests/qemuxml2xmltest.c |   5 +-
>  4 files changed, 466 insertions(+), 376 deletions(-)
> 

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

Re: [libvirt] [PATCH] maint: Update to latest gnulib

2018-01-03 Thread Peter Krempa

On Tue, Jan 02, 2018 at 08:09:37 -0500, John Ferlan wrote:
> 
> 
> On 01/02/2018 04:28 AM, Michal Privoznik wrote:
> > Unfortunately, since gnulib's commit of 2c5d558745 there's an
> > unused parameter to stat_time_normalize() function which gnulib
> > developers don't want to fix [1]. Therefore, we have to work
> > around it by temporarily suspending -Wunused-parameter.
> > 
> > 1: http://lists.gnu.org/archive/html/bug-gnulib/2018-01/msg0.html
> > 
> > Signed-off-by: Michal Privoznik 
> > ---
> > 
> > While we have 'gnulib update free' push rule, this one is not trivial at
> > all and thus I have not pushed it. It's ugly and I don't like it. So any
> > ideas are welcome.
> > 
> >  .gnulib| 2 +-
> >  bootstrap  | 4 ++--
> >  src/storage/storage_util.c | 3 +++
> >  3 files changed, 6 insertions(+), 3 deletions(-)
> > 
> 
> 
> No bright ideas on this other than perhaps only including changes just
> prior to the particular one that breaks things or somehow revert just
> that one in our local copy.

How about just killing that stupid syntax check in our local copy?


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

Re: [libvirt] [PATCH 0/2] qemu: Don't log partial buffer reads from qemu monitor

2018-01-03 Thread Ján Tomko


On Thu, Dec 21, 2017 at 12:16:50PM +0100, Peter Krempa wrote:

I was debugging a case where 200 snapshots of a disk would result in a
VERY long reconnect time after libvirtd restart when debug logging was
enabled.

I've figured out that qemu responds with 9MiB of json after calling
"query-named-block-nodes" and this resulted in > 26 GiB of libvirtd
debug log just to process the message.

I'll report the qemu flaw separately.

Peter Krempa (2):
 util: probe: Add quiet versions of the "PROBE" macro
 qemu: monitor: Decrease logging verbosity

src/qemu/qemu_monitor.c  | 4 ++--
src/qemu/qemu_monitor_json.c | 3 +++
src/util/virprobe.h  | 8 
3 files changed, 13 insertions(+), 2 deletions(-)



ACK series with the typos fixed.

Jan


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

Re: [libvirt] [PATCH] apparmor: fix virt-aa-helper profile

2018-01-03 Thread Cedric Bosdonnat

On Wed, 2018-01-03 at 11:54 +0100, intrigeri wrote:
> Cédric Bosdonnat:
> >  * to handle /var/run not being a symlink to /run
> 
> Does this still really exist in any distro that has chances to run
> a recent libvirt?

At least some people tweak their distro for that, since the openSUSE
AppArmor does it ;)

--
Cedric

> If yes, then:
> 
> > -  /run/libvirt/**/[sv]d[a-z] r
> > +  /{,var/}run/libvirt/**/[sv]d[a-z] r,
> 
> +1
> 
> And in any case, +1 the missing comma.
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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

Re: [libvirt] [PATCH] util: fix another wrong description

2018-01-03 Thread Erik Skultety

On Wed, Jan 03, 2018 at 05:56:35PM +0800, Chen Hanxiao wrote:
> From: Chen Hanxiao 
>
> commit 9026d1152c236ac7a7ab25845220a8e14d6bc630
> forgot to change the referenced @result variable.
> This patch completed this.
>
> Signed-off-by: Chen Hanxiao 

Sigh, should have payed more attention. To avoid sending more patches which
would be description/commentary oriented, I went through the module and found a
few more issues which I squashed into this patch.

Erik

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

Re: [libvirt] [PATCH] apparmor: fix virt-aa-helper profile

2018-01-03 Thread intrigeri

Cédric Bosdonnat:
>  * to handle /var/run not being a symlink to /run

Does this still really exist in any distro that has chances to run
a recent libvirt?

If yes, then:

> -  /run/libvirt/**/[sv]d[a-z] r
> +  /{,var/}run/libvirt/**/[sv]d[a-z] r,

+1

And in any case, +1 the missing comma.

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

[libvirt] [PATCH] util: fix another wrong description

2018-01-03 Thread Chen Hanxiao

From: Chen Hanxiao 

commit 9026d1152c236ac7a7ab25845220a8e14d6bc630
forgot to change the referenced @result variable.
This patch completed this.

Signed-off-by: Chen Hanxiao 
---
 src/util/virstring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virstring.c b/src/util/virstring.c
index 0cb06bdc9..b6e7e279c 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -1041,7 +1041,7 @@ int virStringSortRevCompare(const void *a, const void *b)
  * @matches: pointer to an array to be filled with NULL terminated list of 
matches
  *
  * Performs a POSIX extended regex search against a string and return all 
matching substrings.
- * The @result value should be freed with virStringListFree() when no longer
+ * The @matches value should be freed with virStringListFree() when no longer
  * required.
  *
  * @code
-- 
2.14.3

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

Re: [libvirt] [PATCH] util: fix a wrong description

2018-01-03 Thread Chen Hanxiao



At 2018-01-03 17:46:02, "Ján Tomko"  wrote:
>On Sat, Dec 23, 2017 at 05:49:08PM +0800, Chen Hanxiao wrote:
>>From: Chen Hanxiao 
>>
>>We don't have @result. Use the right one: @matches
>>
>>Signed-off-by: Chen Hanxiao 
>>---
>> src/util/virstring.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>diff --git a/src/util/virstring.c b/src/util/virstring.c
>>index b2ebce27f..0cb06bdc9 100644
>>--- a/src/util/virstring.c
>>+++ b/src/util/virstring.c
>>@@ -1038,7 +1038,7 @@ int virStringSortRevCompare(const void *a, const void 
>>*b)
>>  * @str: string to search
>>  * @regexp: POSIX Extended regular expression pattern used for matching
>>  * @max_matches: maximum number of substrings to return
>>- * @result: pointer to an array to be filled with NULL terminated list of 
>>matches
>>+ * @matches: pointer to an array to be filled with NULL terminated list of 
>>matches
>>  *
>>  * Performs a POSIX extended regex search against a string and return all 
>> matching substrings.
>>  * The @result value should be freed with virStringListFree() when no longer
>
>The same old variable name is referenced here in the context.
>

Sorry for that uncompleted fix.
Will be fixed soon.

Regards,
- Chen

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

[libvirt] [PATCH] apparmor: fix virt-aa-helper profile

2018-01-03 Thread Cédric Bosdonnat

Fix rule introduced by commit 0f33025a:
  * to handle /var/run not being a symlink to /run
  * to be properly parsed: missing comma at the end.
---
 examples/apparmor/usr.lib.libvirt.virt-aa-helper | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper 
b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
index 9c822b644..105f09e43 100644
--- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
+++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
@@ -51,7 +51,7 @@ profile virt-aa-helper 
/usr/{lib,lib64}/libvirt/virt-aa-helper {
   /var/lib/libvirt/images/** r,
   /{media,mnt,opt,srv}/** r,
   # For virt-sandbox
-  /run/libvirt/**/[sv]d[a-z] r
+  /{,var/}run/libvirt/**/[sv]d[a-z] r,
 
   /**.img r,
   /**.raw r,
-- 
2.15.1

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

Re: [libvirt] [PATCH] Fixed documentation for destroy storage pool

2018-01-03 Thread Erik Skultety

On Sat, Dec 30, 2017 at 09:15:34AM +0100, fran...@telecos.upc.edu wrote:
> From: Francesc Guasch 
>
> ---
>  lib/Sys/Virt/StoragePool.pm | 7 ++-
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/lib/Sys/Virt/StoragePool.pm b/lib/Sys/Virt/StoragePool.pm
> index 0bc1d50..2ba5101 100644
> --- a/lib/Sys/Virt/StoragePool.pm
> +++ b/lib/Sys/Virt/StoragePool.pm
> @@ -115,14 +115,11 @@ C method in L.
>
>  Remove the configuration associated with a storage pool previously defined
>  with the C method in L. If the storage pool 
> is
> -running, you probably want to use the C or C
> -methods instead.
> +running, you probably want to use the C method instead.

If you want to make the pool unmanaged by libvirt, destroy doesn't help at
all since it would only stop a running pool, but wouldn't undefine it.
Therefore, we should either omit the sentence completely or use something like
this: 'Calling C on a running pool makes it transient, thus leaving
the underlying object intact, so if object discard is desired, C should
be called first.'
However, truth to be told, even my suggested sentence isn't correct, since
undefine on running pools results in an error - we need to fix that since it
should behave the same way as domains and make them transient. Maybe we can
drop the additional sentence now and update it later when things work the
expected way.

Erik

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

Re: [libvirt] [PATCH] util: fix a wrong description

2018-01-03 Thread Ján Tomko


On Sat, Dec 23, 2017 at 05:49:08PM +0800, Chen Hanxiao wrote:

From: Chen Hanxiao 

We don't have @result. Use the right one: @matches

Signed-off-by: Chen Hanxiao 
---
src/util/virstring.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virstring.c b/src/util/virstring.c
index b2ebce27f..0cb06bdc9 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -1038,7 +1038,7 @@ int virStringSortRevCompare(const void *a, const void *b)
 * @str: string to search
 * @regexp: POSIX Extended regular expression pattern used for matching
 * @max_matches: maximum number of substrings to return
- * @result: pointer to an array to be filled with NULL terminated list of 
matches
+ * @matches: pointer to an array to be filled with NULL terminated list of 
matches
 *
 * Performs a POSIX extended regex search against a string and return all 
matching substrings.
 * The @result value should be freed with virStringListFree() when no longer


The same old variable name is referenced here in the context.

Jan


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

Re: [libvirt] [PATCH] util: fix a wrong description

2018-01-03 Thread Erik Skultety

On Sat, Dec 23, 2017 at 05:49:08PM +0800, Chen Hanxiao wrote:
> From: Chen Hanxiao 
>
> We don't have @result. Use the right one: @matches
>
> Signed-off-by: Chen Hanxiao 

I slightly adjusted the commit message, but

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH] apparmor: allow unix stream for p2p migrations

2018-01-03 Thread Michal Privoznik

On 12/19/2017 02:13 PM, Christian Ehrhardt wrote:
> On live migration with --p2p like:
>  $ virsh migrate --live --p2p kvmguest-bionic-normal \
>qemu+ssh://10.6.221.80/system
> 
> We hit an apparmor deny like:
>   apparmor="DENIED" operation="file_inherit"
>   profile="/usr/sbin/libvirtd" pid=23477 comm="ssh" family="unix"
>   sock_type="stream" protocol=0 requested_mask="send receive"
>   denied_mask="send" addr=none peer_addr=none peer="unconfined"
> 
> The rule is not perfect, but can't be restricted further at the moment
> (new upstream kernel features needed). For now the lack of a profile on the
> peer as well as comm not being a conditional on rules do not allow to filter
> further.
> 
> Signed-off-by: Christian Ehrhardt 
> ---
>  examples/apparmor/usr.sbin.libvirtd | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/examples/apparmor/usr.sbin.libvirtd 
> b/examples/apparmor/usr.sbin.libvirtd
> index 8d61d15..febe8a4 100644
> --- a/examples/apparmor/usr.sbin.libvirtd
> +++ b/examples/apparmor/usr.sbin.libvirtd
> @@ -53,6 +53,9 @@
>network packet dgram,
>network packet raw,
>  
> +  # for --p2p migrations
> +  unix (send, receive) type=stream addr=none peer=(label=unconfined 
> addr=none),
> +
>ptrace (trace) peer=unconfined,
>ptrace (trace) peer=/usr/sbin/libvirtd,
>ptrace (trace) peer=/usr/sbin/dnsmasq,
> 

ACKed and pushed.

Michal

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

Re: [libvirt] [PATCH 0/2] Set hostname in lxc containers

2018-01-03 Thread Michal Privoznik

On 12/18/2017 03:56 PM, Cédric Bosdonnat wrote:
> Hey there,
> 
> Here are two commits to set a transient hostname on lxc containers based
> on the guest name.
> 
> Cédric Bosdonnat (2):
>   Add virStringFilterChars() string utility
>   lxc: set a hostname based on the container name
> 
>  src/libvirt_private.syms |  1 +
>  src/lxc/lxc_container.c  | 35 +++
>  src/util/virstring.c | 24 
>  src/util/virstring.h |  1 +
>  tests/virstringtest.c| 46 ++
>  5 files changed, 107 insertions(+)
> 

ACK

Michal

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

Re: [libvirt] [PATCH] nodedev: Fix failing to parse PCI address for non-PCI network devices

2018-01-03 Thread Erik Skultety

On Fri, Dec 22, 2017 at 01:05:26PM +0800, Fei Li wrote:
> Commit 8708ca01c added virNetDevSwitchdevFeature to check whether
> the NIC had Switchdev capabilities; however this causes errors for
> network devices whose address is not in PCI format, like qeth device
> whose address is 0.0.0800, when calling virPCIDeviceAddressParse.
>
> Change virReportError to VIR_DEBUG to avoid these two error messages
> occuring when starting the libvirtd service, just leave their callers
> to handle the return values respectively.
>
> Signed-off-by: Fei Li 
> ---
>  src/util/virpci.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index fe57bef32..880d7baba 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -2561,7 +2561,7 @@ logStrToLong_ui(char const *s,
>
>  ret = virStrToLong_ui(s, end_ptr, base, result);
>  if (ret != 0)
> -VIR_ERROR(_("Failed to convert '%s' to unsigned int"), s);
> +VIR_DEBUG("Failed to convert '%s' to unsigned int", s);
>  return ret;
>  }
>
> @@ -2638,9 +2638,7 @@ virPCIGetDeviceAddressFromSysfsLink(const char 
> *device_link)
>  goto out;
>
>  if (virPCIDeviceAddressParse(config_address, bdf) < 0) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> -   _("Failed to parse PCI config address '%s'"),
> -   config_address);
> +VIR_DEBUG("Failed to parse PCI config address '%s'", config_address);
>  VIR_FREE(bdf);
>  goto out;
>  }

I am not familiar with switchdev feature, but this is certainly not the correct
fix, errors do have their meaning and by simply mangling the log level of
messages to suit your use case is not the way. Again, I'm far from an expert
in this area, but IMHO you should special case handling of devices supporting
this feature by, e.g. using the VIR_NET_DEV_FEAT_SWITCHDEV enum or create
another one if suitable/appropriate in this case and skip parsing the PCI
address completely.

Erik

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

41 matches

Mail list logo