Re: [libvirt] [PATCH 1/1] qemu: Add support for multiple versions of 'pseries' machine type
On 09/15/2014 07:46 PM, Ján Tomko wrote: On 09/13/2014 05:28 PM, Pradipta Kr. Banerjee wrote: qemu: Add support for multiple versions of 'pseries' machine type qemu for IBM Power processor architecture is adding functionality for supporting multiple 'pseries' machine type versions, each with different capabilities. This patch is for supporting the same Signed-off-by: Pradipta Kr. Banerjee bpra...@in.ibm.com --- src/qemu/qemu_command.c | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) ACK. Would you like to add a qemuxml2argvtest for 'pseries-2.1' machine with these devices? As of now there are no changes to qemu command line invocation based on different machine type. If required for this patch I'll go ahead and add the test cases for pseries-2.1 and pseries-2.2. pseries-2.2 is the latest which aliases to pseries. Please do let me know your thoughts. Jan -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list -- Regards, Pradipta Kumar B(bpra...@in.ibm.com) IBM Systems Technology Labs, India. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] ***UNCHECKED*** Re: [PATCH V2 1/2] cpu: Handle only high order 16 bits of PVR for IBM Power processors
On 09/15/2014 02:56 PM, Daniel P. Berrange wrote: On Sat, Sep 13, 2014 at 12:00:53PM +0530, Pradipta Kr. Banerjee wrote: cpu: Handle only high order 16 bits of PVR for IBM Power processors IBM Power processors encode PVR as CPU family in higher 16 bits and a CPU version in lower 16 bits. Since there is no significant change in behavior between versions, there is no point to add every single CPU version in cpu_map.xml Signed-off-by: Pradipta Kr. Banerjee bpra...@in.ibm.com --- src/cpu/cpu_map.xml | 22 -- src/cpu/cpu_powerpc.c | 7 ++- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml index 18c7b0d..f2f13fb 100644 --- a/src/cpu/cpu_map.xml +++ b/src/cpu/cpu_map.xml @@ -602,30 +602,24 @@ vendor name='IBM'/ !-- IBM-based CPU models -- -model name='POWER7' +model name='power6' vendor name='IBM'/ - pvr value='0x003f0200'/ + pvr value='0x003e'/ /model -model name='POWER7_v2.1' +model name='power7' vendor name='IBM'/ - pvr value='0x003f0201'/ + pvr value='0x003f'/ /model -model name='POWER7_v2.3' +model name='power7+' vendor name='IBM'/ - pvr value='0x003f0203'/ + pvr value='0x004a'/ /model -model name='POWER7+_v2.1' +model name='power8' vendor name='IBM'/ - pvr value='0x004a0201'/ + pvr value='0x004b'/ /model - -model name='POWER8_v1.0' - vendor name='IBM'/ - pvr value='0x004b0100'/ -/model Changing names of all the CPU models in this way is going to break every existing XML config on PPC. So NACK to that. Hi Dan, One thing to note is that official KVM support on Power processor is from Power8 only. So possibility of breakage is very limited imo. However I see your point. What is your advise? Should I retain all the existing processor models in the cpu_map.xml ? Regards, Daniel -- Regards, Pradipta Kumar B(bpra...@in.ibm.com) IBM Systems Technology Labs, India. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] libvirtd crash when defining scsi storage pool
On 09/03/2014 03:28 PM, John Ferlan wrote: [found this in my probably should look at this one some day pile..] On 06/21/2014 12:57 PM, Pradipta Kr. Banerjee wrote: libvirtd crashes when there is an existing SCSI pool with adapter type as 'scsi_host' and defining a new SCSI pool with adapter type as 'fc_host' and parent attribute missing. For eg when defining a storage-pool with the following XML will crash libvirtd if there already exists a SCSI pool with adapter type 'scsi_host' pool type='scsi' nameTEST_SCSI_FC_POOL/name source adapter type='fc_host' wwnn='1234567890abcdef' wwpn='abcdef1234567890'/ /source target path/dev/disk/by-path/path /target /pool This happens because for fc_host, adapter 'name' is not relevant whereas for scsi_host its mandatory attribute. However the check in libvirt for finding duplicate storage pools doesn't take that into account while comparing, resulting into crash This patch fixes the issue Going to need to clean the above up, but if you also provided the existing scsi/scsi_host pool def that would have been good! Signed-off-by: Pradipta Kr. Banerjee bpra...@in.ibm.com --- src/conf/storage_conf.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 8b6fd79..54a4589 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2126,8 +2126,10 @@ virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr pools, STREQ(pool-def-source.adapter.data.fchost.wwpn, def-source.adapter.data.fchost.wwpn)) matchpool = pool; -} else if (pool-def-source.adapter.type == - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST){ +} else if ((pool-def-source.adapter.type == +VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST)\ + (def-source.adapter.type == + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST)) { My assumption is this issue is a result of commit id '9f781da' Beyond the - it needs a bit a refresh due to other changes since that time - I'm wondering if you've only solved half the potential problem You are right. What if the fc_host is defined and you're coming in with a 'scsi_host' def-source.adapter.type? (see a few lines earlier) This will result in crash too.. I'll send a V2 based on your suggestion if (STREQ(pool-def-source.adapter.data.name, def-source.adapter.data.name)) matchpool = pool; So instead of what's here, how about the following diff: break; case VIR_STORAGE_POOL_SCSI: if (pool-def-source.adapter.type == +VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST +def-source.adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { if (STREQ(pool-def-source.adapter.data.fchost.wwnn, def-source.adapter.data.fchost.wwnn) @@ -2124,6 +2126,8 @@ virStoragePoolSourceFindDuplicate(virStoragePoolObjListPtr def-source.adapter.data.fchost.wwpn)) matchpool = pool; } else if (pool-def-source.adapter.type == + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST + def-source.adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { if (pool-def-source.adapter.data.scsi_host.name) { if (STREQ(pool-def-source.adapter.data.scsi_host.name, John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list -- Regards, Pradipta Kumar B(bpra...@in.ibm.com) IBM Systems Technology Labs, India. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V1 1/2] cpu: Handle only high order 16 bits of PVR for IBM Power processors
On 08/20/2014 09:02 AM, Martin Kletzander wrote: On Tue, Aug 19, 2014 at 11:00:12PM +0530, Pradipta Kr. Banerjee wrote: IBM Power processors encode PVR as CPU family in higher 16 bits and a CPU version in lower 16 bits. Since there is no significant change in behavior between versions, there is no point to add every single CPU version in cpu_map.xml and check for the same in cpu_powerpc.c Just an idea, but what if we select the model based on the whole pvr and only in case none is found, we will fallback to the first one that has the same high 16 bits? When we have already power8_v1.0 and you're adding power8, we will never select the second one in the way you have implemented it. Actually none of the model_vX.X are required. Only base processor versions are required. Should I send a V2 with those changes ? Additionally PowerKVM doesn't support specifying cpu model for guests (qemu cmdline: -cpu MODEL). The only way possible is to use qemu compat mode (qemu cmdline: -cpu host,compat=model) support of which is yet to be added in libvirt. The only supported models in compat modes are power6,power7,power8. There was an RFC posted on the same - https://www.redhat.com/archives/libvir-list/2014-June/msg01338.html Please let me know your thoughts. Signed-off-by: Pradipta Kr. Banerjee bpra...@in.ibm.com --- src/cpu/cpu_powerpc.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c index 67cb9ff..18dbc99 100644 --- a/src/cpu/cpu_powerpc.c +++ b/src/cpu/cpu_powerpc.c @@ -93,7 +93,12 @@ ppcModelFindPVR(const struct ppc_map *map, model = map-models; while (model != NULL) { -if (model-data.pvr == pvr) +/*IBM PowerPC processors encode PVR as CPU family in higher 16 bits and + *a CPU version in lower 16 bits. Since there is no significant change + *in behavior between versions, there is no point to add every single + *CPU version in cpu_map.xml + */ +if ((model-data.pvr 0x) == (pvr 0x)) return model; model = model-next; -- 1.9.3 -- 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 -- Regards, Pradipta Kumar B(bpra...@in.ibm.com) IBM Systems Technology Labs, India. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RESEND V2 1/1] Add hw random number generator (/dev/hwrng) to cgroup ACL
On 01/27/2014 10:21 PM, Eric Blake wrote: On 01/16/2014 06:41 AM, Pradipta Kr. Banerjee wrote: From: Pradipta Kr. Banerjee bpra...@in.ibm.com Creating a qemu VM with /dev/hwrng as backend RNG device throws the following error - Could not open '/dev/hwrng': Permission denied This patch fixes the issue Signed-off-by: Pradipta Kr. Banerjee bpra...@in.ibm.com --- v2: Added acl code as part of per-VM cgroup setup src/qemu/qemu_cgroup.c | 12 1 file changed, 12 insertions(+) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index f0cacd0..8e2076e 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -533,6 +533,18 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver, goto cleanup; } +if (vm-def-rng +(vm-def-rng-backend == VIR_DOMAIN_RNG_BACKEND_RANDOM)) { +VIR_DEBUG(Setting Cgroup ACL for RNG device); +rv = virCgroupAllowDevicePath(priv-cgroup, vm-def-rng-source.file, + VIR_CGROUP_DEVICE_RW); Indentation is off. ACK with that fixed, so I pushed the patch. Congrats on your first libvirt patch, and apologies for the delayed review. Thanks Eric and no problems at all.. -- Regards, Pradipta -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] virsh nodecpustats erroneously returns stats for offline cpus on Linux
ping. On 12/19/2013 10:10 AM, Pradipta Kr. Banerjee wrote: From: Pradipta Kr. Banerjee bpra...@in.ibm.com virsh nodecpustats erroneously returns stats for offline cpus on Linux To retrieve node cpu statistics on Linux system, the linuxNodeGetCPUstats function performs a minimal match of the input cpuid with the cpuid read from /proc/cpustat. On systems with larger cpus this can result in erroneous data. For eg if /proc/stat has similar data cpu 3645648 485 361475 3691194868 176595 40645 69850 0 1526172 0 cpu0 494045 62 42763 138516619 16107 2339 2431 0 248345 0 cpu4 402646 59 34745 138617562 17738 1413 1196 0 193948 0 cpu8 318662 32 32119 138715131 7903 927 982 0 115429 0 cpu12 247004 33 30465 138791438 5251 759 913 0 51938 0 and cpu 1,2 are offline, then 'virsh nodecpustats 1' displays data for cpu12 whereas virsh nodecpustats 2 correctly throws the following error error: Unable to get node cpu stats error: Invalid cpuNum in linuxNodeGetCPUStats This patch fixes the above mentioned problem Signed-off-by: Pradipta Kr. Banerjee bpra...@in.ibm.com --- src/nodeinfo.c | 13 + 1 file changed, 13 insertions(+) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 1838547..c9e5ff1 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -614,6 +614,8 @@ linuxNodeGetCPUStats(FILE *procstat, unsigned long long usr, ni, sys, idle, iowait; unsigned long long irq, softirq, steal, guest, guest_nice; char cpu_header[3 + INT_BUFSIZE_BOUND(cpuNum)]; +char cpu_header_read[3 + INT_BUFSIZE_BOUND(cpuNum)]; +char cpu_header_fmt[8]; if ((*nparams) == 0) { /* Current number of cpu stats supported by linux */ @@ -631,8 +633,11 @@ linuxNodeGetCPUStats(FILE *procstat, if (cpuNum == VIR_NODE_CPU_STATS_ALL_CPUS) { strcpy(cpu_header, cpu); +snprintf(cpu_header_fmt, sizeof(cpu_header_fmt), %%%ds, 3); } else { snprintf(cpu_header, sizeof(cpu_header), cpu%d, cpuNum); +snprintf(cpu_header_fmt, sizeof(cpu_header_fmt), %%%ds, + (int)(3 + INT_BUFSIZE_BOUND(cpuNum))); } while (fgets(line, sizeof(line), procstat) != NULL) { @@ -649,6 +654,14 @@ linuxNodeGetCPUStats(FILE *procstat, continue; } +/* + * Process with stats gathering only if the cpuid from /proc/stat + * matches exactly with the input cpuid +*/ +sscanf(buf, cpu_header_fmt, cpu_header_read); +if (STRNEQ(cpu_header, cpu_header_read)) +continue; + for (i = 0; i *nparams; i++) { virNodeCPUStatsPtr param = params[i]; -- 1.8.3.1 -- Regards, Pradipta -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V2 1/1] Add hw random number generator (/dev/hwrng) to cgroup ACL
Any comments.. Should I rebase and resend the patch against the latest release. Please advise. On 12/05/2013 02:00 PM, Pradipta Kr. Banerjee wrote: From: Pradipta Kr. Banerjee bpra...@in.ibm.com Creating a qemu VM with /dev/hwrng as backend RNG device throws the following error - Could not open '/dev/hwrng': Permission denied This patch fixes the issue Signed-off-by: Pradipta Kr. Banerjee bpra...@in.ibm.com --- src/qemu/qemu_cgroup.c | 12 1 file changed, 12 insertions(+) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index f0cacd0..8e2076e 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -533,6 +533,18 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver, goto cleanup; } +if (vm-def-rng +(vm-def-rng-backend == VIR_DOMAIN_RNG_BACKEND_RANDOM)) { +VIR_DEBUG(Setting Cgroup ACL for RNG device); +rv = virCgroupAllowDevicePath(priv-cgroup, vm-def-rng-source.file, + VIR_CGROUP_DEVICE_RW); +virDomainAuditCgroupPath(vm, priv-cgroup, allow, + vm-def-rng-source.file, rw, rv == 0); +if (rv 0 + !virLastErrorIsSystemErrno(ENOENT)) +goto cleanup; +} + ret = 0; cleanup: virObjectUnref(cfg); -- Regards, Pradipta -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] virsh nodecpustats erroneously returns stats for offline cpus on Linux
[snip] for (i = 0; i *nparams; i++) { virNodeCPUStatsPtr param = params[i]; What about this? diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 1838547..aa1ad81 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -637,8 +637,9 @@ linuxNodeGetCPUStats(FILE *procstat, while (fgets(line, sizeof(line), procstat) != NULL) { char *buf = line; +char **buf_header = virStringSplit(buf, , 2); -if (STRPREFIX(buf, cpu_header)) { /* aka logical CPU time */ +if (STREQ(buf_header[0], cpu_header)) { /* aka logical CPU time */ size_t i; if (sscanf(buf, @@ -697,6 +698,7 @@ linuxNodeGetCPUStats(FILE *procstat, ret = 0; goto cleanup; } +virStringFreeList(buf_header); } virReportInvalidArg(cpuNum, This is definitely better and lesser amount of code.. I think the version with virStringSplit would need some fine tuning since in its current form it will not free the memory for the failure cases..Comments ? Also can some expert here provide some tips on whether in this particular circumstance it should be fine to allocate/realloc/free memory inside the while loop. Would be very helpful.. Thanks, Pradipta Thanks -- 1.8.3.1 -- 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 1/1] virsh nodecpustats erroneously returns stats for offline cpus on Linux
On 12/19/2013 02:02 PM, Bing Bu Cao wrote: On 12/19/2013 12:40 PM, Pradipta Kr. Banerjee wrote: From: Pradipta Kr. Banerjee bpra...@in.ibm.com virsh nodecpustats erroneously returns stats for offline cpus on Linux To retrieve node cpu statistics on Linux system, the linuxNodeGetCPUstats function performs a minimal match of the input cpuid with the cpuid read from /proc/cpustat. On systems with larger cpus this can result in erroneous data. For eg if /proc/stat has similar data cpu 3645648 485 361475 3691194868 176595 40645 69850 0 1526172 0 cpu0 494045 62 42763 138516619 16107 2339 2431 0 248345 0 cpu4 402646 59 34745 138617562 17738 1413 1196 0 193948 0 cpu8 318662 32 32119 138715131 7903 927 982 0 115429 0 cpu12 247004 33 30465 138791438 5251 759 913 0 51938 0 and cpu 1,2 are offline, then 'virsh nodecpustats 1' displays data for cpu12 whereas virsh nodecpustats 2 correctly throws the following error error: Unable to get node cpu stats error: Invalid cpuNum in linuxNodeGetCPUStats This patch fixes the above mentioned problem Signed-off-by: Pradipta Kr. Banerjee bpra...@in.ibm.com --- src/nodeinfo.c | 13 + 1 file changed, 13 insertions(+) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 1838547..c9e5ff1 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -614,6 +614,8 @@ linuxNodeGetCPUStats(FILE *procstat, unsigned long long usr, ni, sys, idle, iowait; unsigned long long irq, softirq, steal, guest, guest_nice; char cpu_header[3 + INT_BUFSIZE_BOUND(cpuNum)]; +char cpu_header_read[3 + INT_BUFSIZE_BOUND(cpuNum)]; +char cpu_header_fmt[8]; if ((*nparams) == 0) { /* Current number of cpu stats supported by linux */ @@ -631,8 +633,11 @@ linuxNodeGetCPUStats(FILE *procstat, if (cpuNum == VIR_NODE_CPU_STATS_ALL_CPUS) { strcpy(cpu_header, cpu); +snprintf(cpu_header_fmt, sizeof(cpu_header_fmt), %%%ds, 3); } else { snprintf(cpu_header, sizeof(cpu_header), cpu%d, cpuNum); +snprintf(cpu_header_fmt, sizeof(cpu_header_fmt), %%%ds, + (int)(3 + INT_BUFSIZE_BOUND(cpuNum))); } while (fgets(line, sizeof(line), procstat) != NULL) { @@ -649,6 +654,14 @@ linuxNodeGetCPUStats(FILE *procstat, continue; } +/* + * Process with stats gathering only if the cpuid from /proc/stat + * matches exactly with the input cpuid +*/ +sscanf(buf, cpu_header_fmt, cpu_header_read); +if (STRNEQ(cpu_header, cpu_header_read)) +continue; + for (i = 0; i *nparams; i++) { virNodeCPUStatsPtr param = params[i]; What about this? diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 1838547..aa1ad81 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -637,8 +637,9 @@ linuxNodeGetCPUStats(FILE *procstat, while (fgets(line, sizeof(line), procstat) != NULL) { char *buf = line; +char **buf_header = virStringSplit(buf, , 2); -if (STRPREFIX(buf, cpu_header)) { /* aka logical CPU time */ +if (STREQ(buf_header[0], cpu_header)) { /* aka logical CPU time */ size_t i; if (sscanf(buf, @@ -697,6 +698,7 @@ linuxNodeGetCPUStats(FILE *procstat, ret = 0; goto cleanup; } +virStringFreeList(buf_header); } virReportInvalidArg(cpuNum, This is definitely better and lesser amount of code.. Thanks -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list -- Regards, Pradipta -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] Make virtio as the default net device for PPC64
On 12/12/2013 05:27 PM, Daniel P. Berrange wrote: On Thu, Dec 12, 2013 at 05:18:13PM +0530, Pradipta Kr. Banerjee wrote: From: Pradipta Kr. Banerjee bpra...@in.ibm.com Make virtio as the default net device for PPC64 Why ? Hi Daniel, rtl8139 is not supported and maintained. Either of e1000, spapr-vlan or virtio are supported and improved upon. So its better to default to one of the above and virtio is the saner default. Daniel -- Regards, Pradipta -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] Make virtio as the default net device for PPC64
On 12/12/2013 05:48 PM, Daniel P. Berrange wrote: On Thu, Dec 12, 2013 at 05:43:27PM +0530, Pradipta Kumar Banerjee wrote: On 12/12/2013 05:27 PM, Daniel P. Berrange wrote: On Thu, Dec 12, 2013 at 05:18:13PM +0530, Pradipta Kr. Banerjee wrote: From: Pradipta Kr. Banerjee bpra...@in.ibm.com Make virtio as the default net device for PPC64 Why ? Hi Daniel, rtl8139 is not supported and maintained. Either of e1000, spapr-vlan or virtio are supported and improved upon. So its better to default to one of the above and virtio is the saner default. IOW that has nothing todo with PPC64 architecture, which means that is not an acceptable reason to special case this for PPC64. My response was specific to ppc64. Sorry I should have put it clearly. rtl8139 is not supported and maintained for ppc64. It also lacks functionality like ppc64 guest network boot. Daniel -- Regards, Pradipta -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V2 1/1] Add hw random number generator (/dev/hwrng) to cgroup ACL
Hi, Any comments !! On 12/05/2013 02:00 PM, Pradipta Kr. Banerjee wrote: From: Pradipta Kr. Banerjee bpra...@in.ibm.com Creating a qemu VM with /dev/hwrng as backend RNG device throws the following error - Could not open '/dev/hwrng': Permission denied This patch fixes the issue Signed-off-by: Pradipta Kr. Banerjee bpra...@in.ibm.com --- src/qemu/qemu_cgroup.c | 12 1 file changed, 12 insertions(+) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index f0cacd0..8e2076e 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -533,6 +533,18 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver, goto cleanup; } +if (vm-def-rng +(vm-def-rng-backend == VIR_DOMAIN_RNG_BACKEND_RANDOM)) { +VIR_DEBUG(Setting Cgroup ACL for RNG device); +rv = virCgroupAllowDevicePath(priv-cgroup, vm-def-rng-source.file, + VIR_CGROUP_DEVICE_RW); +virDomainAuditCgroupPath(vm, priv-cgroup, allow, + vm-def-rng-source.file, rw, rv == 0); +if (rv 0 + !virLastErrorIsSystemErrno(ENOENT)) +goto cleanup; +} + ret = 0; cleanup: virObjectUnref(cfg); -- Regards, Pradipta -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Add hw random number generator (/dev/hwrng) to default qemu cgroup ACL
On 11/18/2013 09:35 PM, Eric Blake wrote: On 11/18/2013 08:20 AM, Daniel P. Berrange wrote: static const char *const defaultDeviceACL[] = { /dev/null, /dev/full, /dev/zero, -/dev/random, /dev/urandom, +/dev/random, /dev/urandom, /dev/hwrng, /dev/ptmx, /dev/kvm, /dev/kqemu, /dev/rtc, /dev/hpet, /dev/vfio/vfio, NULL, NACK, for any device listed in the XML, we should add it in the per-VM cgroups setup code. The existing /dev/random /dev/urandom devices are there because they are used for basic crypto libraries unrelated to the XML config. /dev/urandom, probably. /dev/random - really? Isn't that a DoS potential for one guest to starve entropy from other guests, in spite of sVirt? I think you are right .. The /dev/hwrng device does not fall into this scenario. Agreed. -- Regards, Pradipta -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Add hw random number generator (/dev/hwrng) to default qemu cgroup ACL
On 11/18/2013 08:45 PM, Eric Blake wrote: On 11/18/2013 06:28 AM, Pradipta Kr. Banerjee wrote: Creating a qemu VM with /dev/hwrng as backed RNG device throws the following error - Could not open '/dev/hwrng': Operation not permitted This patch fixes the issue Signed-off-by: Pradipta Kr. Banerjee bpra...@in.ibm.com --- src/qemu/qemu_cgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) This is probably not the right fix. This says that we are making the random device to all possible guests, and that a compromised guest can then open() the device and starve it of entropy to the detriment of other guests. I think the _real_ fix is to quit exposing /dev/random by default (/dev/urandom may be okay), and to instead teach the code for RNG devices to add an ACL change for either random device (/dev/random or /dev/hwrng) only for the guests that actually use RNG backed by a hardware device. That way, guests not using an RNG device cannot starve entropy from guests that depend on it. Right..Thanks for the suggestion Eric. diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index ace7e35..d9ebb30 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -39,7 +39,7 @@ static const char *const defaultDeviceACL[] = { /dev/null, /dev/full, /dev/zero, -/dev/random, /dev/urandom, +/dev/random, /dev/urandom, /dev/hwrng, /dev/ptmx, /dev/kvm, /dev/kqemu, /dev/rtc, /dev/hpet, /dev/vfio/vfio, NULL, -- Regards, Pradipta -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list