[PATCH] kvm tool: Remove unused variables from the QCOW code
Newer versions of GCC notice the following unused variables: qcow.c: In function ‘qcow_read_l1_table’: qcow.c:56:7: error: variable ‘page_offset’ set but not used [-Werror=unused-but-set-variable] qcow.c:54:6: error: variable ‘map_offset’ set but not used [-Werror=unused-but-set-variable] cc1: all warnings being treated as errors make: *** [qcow.o] Error 1 Remove them. Signed-off-by: Ingo Molnar mi...@elte.hu --- diff --git a/tools/kvm/qcow.c b/tools/kvm/qcow.c index 1764975..a3e0537 100644 --- a/tools/kvm/qcow.c +++ b/tools/kvm/qcow.c @@ -51,9 +51,6 @@ int qcow_read_l1_table(struct qcow *q) struct qcow1_header *h = q-header; struct qcow_table *table; u64 table_offset; - u64 map_offset; - const long page_size = sysconf(_SC_PAGESIZE); - long page_offset; u32 l1_i; q-table = table = calloc(1, sizeof(struct qcow_table)); @@ -63,9 +60,6 @@ int qcow_read_l1_table(struct qcow *q) table-table_size = qcow1_get_table_size(q); table_offset= h-l1_table_offset; - map_offset = table_offset page_size; - page_offset = table_offset (~page_size); - table-l1_table = calloc(table-table_size, sizeof(u64)); if (!table-l1_table) goto error; -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH 2/2 V7] qemu, qmp: add inject-nmi qmp command
Blue Swirl blauwir...@gmail.com writes: On Wed, Apr 13, 2011 at 4:08 PM, Luiz Capitulino lcapitul...@redhat.com wrote: On Tue, 12 Apr 2011 21:31:18 +0300 Blue Swirl blauwir...@gmail.com wrote: On Tue, Apr 12, 2011 at 10:52 AM, Avi Kivity a...@redhat.com wrote: On 04/11/2011 08:15 PM, Blue Swirl wrote: On Mon, Apr 11, 2011 at 10:01 AM, Markus Armbrusterarm...@redhat.com wrote: Avi Kivitya...@redhat.com writes: On 04/08/2011 12:41 AM, Anthony Liguori wrote: And it's a good thing to have, but exposing this as the only API to do something as simple as generating a guest crash dump is not the friendliest thing in the world to do to users. nmi is a fine name for something that corresponds to a real-life nmi button (often labeled NMI). Agree. We could also introduce an alias mechanism for user friendly names, so nmi could be used in addition of full path. Aliases could be useful for device paths as well. Yes. Perhaps limited to the human monitor. I'd limit all debugging commands (including NMI) to the human monitor. Why? Do they have any real use in production environment? Also, we should have the freedom to change the debugging facilities (for example, to improve some internal implementation) as we want without regard to compatibility to previous versions. For what it's worth, Lai (original poster) has been trying for many months to get inject-nmi into QMP, and I suspect he has a really good reason for his super-human persistence. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] KVM: emulator: do not needlesly sync registers from emulator ctxt to vcpu
On Thu, Mar 31, 2011 at 12:06:41PM +0200, Gleb Natapov wrote: Currently we sync registers back and forth before/after exiting to userspace for IO, but during IO device model shouldn't need to read/write the registers, so we can as well skip those sync points. The only exaception is broken vmware backdor interface. The new code sync registers content during IO only if registers are read from/written to by userspace in the middle of the IO operation and this almost never happens in practise. Signed-off-by: Gleb Natapov g...@redhat.com --- v1-v2 - rebased onto see emulation patchset. Applied, thanks. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: Add CPUID support for VIA CPU
On 04/14/2011 06:14 AM, brill...@viatech.com.cn wrote: On 04/13/2011 02:05 PM, brill...@viatech.com.cn wrote: +/* cpuid 0xC001.edx */ + const u32 kvm_supported_word5_x86_features = +F(XSTORE) | F(XSTORE_EN) | F(XCRYPT) | F(XCRYPT_EN) | +F(ACE2) | F(ACE2_EN) | F(PHE) | F(PHE_EN) | +F(PMM) | F(PMM_EN); + Are all of these features save wrt save/restore? (do they all act on state in standard registers?) Do they need any control register bits to be active or MSRs to configure? These features depend on instructions for the PadLock hardware engine of VIA CPU. The PadLock instructions just act on standard registers like general X86 instructions, and the features have been enabled when the CPU leave the factory, so there is no need to activate any control register bits or configure MSRs. I see there is a dependency on EFLAGS[30]. Does a VM entry clear this bit? If not, we have to do it ourselves. Yes, PadLock hardware engine has some association with EFLAGS[30], but it just required that the EFLAGS[30] should be set to 0 before using PadLock ACE instructions. It is recommanded that execute instruction sequence pushf;popf to clear this bit before using ACE instructions. AFAIK, the VM entry never sets the EFLAGS[30] bit, so it seems that we do not have to do it ourselves. I don't think we need to. kvm kernel code doesn't use padlock; other sources which might set EFLAGS[30] are: - the guest; but VMEXIT clears EFLAGS - userspace; but syscall/sysenter/int instructions clear EFLAGS[30] - another kernel thread; there is likely a popf in the context switch path somewhere (we should verify this) -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] kvm tool: add QCOW verions 1 read/write support
Am 13.04.2011 21:26, schrieb Prasad Joshi: The patch only implements the basic read write support for QCOW version 1 images. Many of the QCOW features are not implmented, for example - image creation - snapshot - copy-on-write - encryption Yay, more forks, more code duplication! Have you thought about a way to actually share code with qemu instead of repeating Xen's mistake of copying code, modifying it until merges are no longer possible and then let it bitrot? If you shared code, you also wouldn't have to use an obsolete, but simple-to-implement format, but could use some of the better maintained formats of qemu, like qcow2. Also at least your qcow1.c is lacking the copyright header. Please add this, otherwise you're violating the license. Kevin -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] kvm tool: add QCOW verions 1 read/write support
* Kevin Wolf kw...@redhat.com wrote: Also at least your qcow1.c is lacking the copyright header. Please add this, otherwise you're violating the license. AFAICT it's not a copy, it's a reimplementation - and he credited you in the CREDITS file, for providing a reference implementation. But we can credit you to the header as well - Pekka? Thanks, Ingo -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] kvm tool: add QCOW verions 1 read/write support
Am 14.04.2011 10:07, schrieb Ingo Molnar: * Kevin Wolf kw...@redhat.com wrote: Also at least your qcow1.c is lacking the copyright header. Please add this, otherwise you're violating the license. AFAICT it's not a copy, it's a reimplementation - and he credited you in the CREDITS file, for providing a reference implementation. But we can credit you to the header as well - Pekka? It's actually not my code, but Fabrice's. I compared get_cluster_offset() and it looks similar enough to me to qualify as a modified copy. Kevin -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] kvm tool: add QCOW verions 1 read/write support
* Kevin Wolf kw...@redhat.com wrote: Also at least your qcow1.c is lacking the copyright header. Please add this, otherwise you're violating the license. Am 14.04.2011 10:07, schrieb Ingo Molnar: AFAICT it's not a copy, it's a reimplementation - and he credited you in the CREDITS file, for providing a reference implementation. But we can credit you to the header as well - Pekka? On Thu, Apr 14, 2011 at 11:12 AM, Kevin Wolf kw...@redhat.com wrote: It's actually not my code, but Fabrice's. I compared get_cluster_offset() and it looks similar enough to me to qualify as a modified copy. It's actually me who asked to drop the license banners from the file and move it to CREDITS. Parasd, mind sending a patch that adds it back to the files? Pekka -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] kvm tool: add QCOW verions 1 read/write support
On Thu, Apr 14, 2011 at 11:02 AM, Kevin Wolf kw...@redhat.com wrote: Have you thought about a way to actually share code with qemu instead of repeating Xen's mistake of copying code, modifying it until merges are no longer possible and then let it bitrot? No we haven't and we're not planning to copy QEMU code as-is but re-implement support for formats we're interested in. On Thu, Apr 14, 2011 at 11:02 AM, Kevin Wolf kw...@redhat.com wrote: If you shared code, you also wouldn't have to use an obsolete, but simple-to-implement format, but could use some of the better maintained formats of qemu, like qcow2. We're adding QCOW2 too and hopefully other non-QEMU formats as well. On Thu, Apr 14, 2011 at 11:02 AM, Kevin Wolf kw...@redhat.com wrote: Also at least your qcow1.c is lacking the copyright header. Please add this, otherwise you're violating the license. Thanks, will fix! Pekka -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] kvm tool: add QCOW verions 1 read/write support
Am 14.04.2011 10:15, schrieb Pekka Enberg: * Kevin Wolf kw...@redhat.com wrote: Also at least your qcow1.c is lacking the copyright header. Please add this, otherwise you're violating the license. Am 14.04.2011 10:07, schrieb Ingo Molnar: AFAICT it's not a copy, it's a reimplementation - and he credited you in the CREDITS file, for providing a reference implementation. But we can credit you to the header as well - Pekka? On Thu, Apr 14, 2011 at 11:12 AM, Kevin Wolf kw...@redhat.com wrote: It's actually not my code, but Fabrice's. I compared get_cluster_offset() and it looks similar enough to me to qualify as a modified copy. It's actually me who asked to drop the license banners from the file and move it to CREDITS. Parasd, mind sending a patch that adds it back to the files? Heh, I just saw your mail from yesterday. Missed it because I wasn't CCed on v1. The license is pretty clear about it: * The above copyright notice and this permission notice shall be included in * all copies or substantial portions of the Software. You could discuss if it would be enough to copy the license text into CREDITS, but leaving it in the file is the usual and expected way. Anyway, license issues are not my favourite topic, I would rather discuss ways of sharing code instead of creating more unmergeable forks. Don't you feel that it will hurt both sides if you continue this way? Kevin -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] kvm tool: add QCOW verions 1 read/write support
* Kevin Wolf kw...@redhat.com wrote: Am 14.04.2011 10:07, schrieb Ingo Molnar: * Kevin Wolf kw...@redhat.com wrote: Also at least your qcow1.c is lacking the copyright header. Please add this, otherwise you're violating the license. AFAICT it's not a copy, it's a reimplementation - and he credited you in the CREDITS file, for providing a reference implementation. But we can credit you to the header as well - Pekka? It's actually not my code, but Fabrice's. I compared get_cluster_offset() and it looks similar enough to me to qualify as a modified copy. Well, i asked Prasad whether he copied any code and he said no (so it's not a copy AFAICT) - but i nevertheless asked him to add proper attribution in any case because he used it as a reference. Thanks, Ingo -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] kvm tool: add QCOW verions 1 read/write support
* Kevin Wolf kw...@redhat.com wrote: Am 14.04.2011 10:15, schrieb Pekka Enberg: * Kevin Wolf kw...@redhat.com wrote: Also at least your qcow1.c is lacking the copyright header. Please add this, otherwise you're violating the license. Am 14.04.2011 10:07, schrieb Ingo Molnar: AFAICT it's not a copy, it's a reimplementation - and he credited you in the CREDITS file, for providing a reference implementation. But we can credit you to the header as well - Pekka? On Thu, Apr 14, 2011 at 11:12 AM, Kevin Wolf kw...@redhat.com wrote: It's actually not my code, but Fabrice's. I compared get_cluster_offset() and it looks similar enough to me to qualify as a modified copy. It's actually me who asked to drop the license banners from the file and move it to CREDITS. Parasd, mind sending a patch that adds it back to the files? Heh, I just saw your mail from yesterday. Missed it because I wasn't CCed on v1. The license is pretty clear about it: * The above copyright notice and this permission notice shall be included in * all copies or substantial portions of the Software. Note that Prasad's v0 patch did not include this copyright notice - because he wrote the file from scratch. I asked him to attribute the Qemu reference implementation in any case - which he understood as copying the copyright license verbatim. Thanks, Ingo -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] kvm tool: add QCOW verions 1 read/write support
Am 14.04.2011 10:21, schrieb Pekka Enberg: On Thu, Apr 14, 2011 at 11:02 AM, Kevin Wolf kw...@redhat.com wrote: Have you thought about a way to actually share code with qemu instead of repeating Xen's mistake of copying code, modifying it until merges are no longer possible and then let it bitrot? No we haven't and we're not planning to copy QEMU code as-is but re-implement support for formats we're interested in. Okay. I might not consider it wise, but in the end it's your decision. I'm just curious why you think this is the better way? Kevin -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] kvm tool: add QCOW verions 1 read/write support
Hi Kevin! Am 14.04.2011 10:21, schrieb Pekka Enberg: On Thu, Apr 14, 2011 at 11:02 AM, Kevin Wolf kw...@redhat.com wrote: Have you thought about a way to actually share code with qemu instead of repeating Xen's mistake of copying code, modifying it until merges are no longer possible and then let it bitrot? No we haven't and we're not planning to copy QEMU code as-is but re-implement support for formats we're interested in. On Thu, Apr 14, 2011 at 11:31 AM, Kevin Wolf kw...@redhat.com wrote: Okay. I might not consider it wise, but in the end it's your decision. I'm just curious why you think this is the better way? Well, how would you go about sharing the code without copying in practical terms? We're aiming for standalone tool in tools/kvm of the Linux kernel so I don't see how we could do that. Pekka -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] kvm tool: add QCOW verions 1 read/write support
On Thu, Apr 14, 2011 at 11:32:21AM +0300, Pekka Enberg wrote: Hi Kevin! Am 14.04.2011 10:21, schrieb Pekka Enberg: On Thu, Apr 14, 2011 at 11:02 AM, Kevin Wolf kw...@redhat.com wrote: Have you thought about a way to actually share code with qemu instead of repeating Xen's mistake of copying code, modifying it until merges are no longer possible and then let it bitrot? No we haven't and we're not planning to copy QEMU code as-is but re-implement support for formats we're interested in. On Thu, Apr 14, 2011 at 11:31 AM, Kevin Wolf kw...@redhat.com wrote: Okay. I might not consider it wise, but in the end it's your decision. I'm just curious why you think this is the better way? Well, how would you go about sharing the code without copying in practical terms? We're aiming for standalone tool in tools/kvm of the Linux kernel so I don't see how we could do that. You mean you would not be ok with linking with anything other then glibc? Pekka -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] kvm tool: add QCOW verions 1 read/write support
Am 14.04.2011 10:32, schrieb Pekka Enberg: Hi Kevin! Am 14.04.2011 10:21, schrieb Pekka Enberg: On Thu, Apr 14, 2011 at 11:02 AM, Kevin Wolf kw...@redhat.com wrote: Have you thought about a way to actually share code with qemu instead of repeating Xen's mistake of copying code, modifying it until merges are no longer possible and then let it bitrot? No we haven't and we're not planning to copy QEMU code as-is but re-implement support for formats we're interested in. On Thu, Apr 14, 2011 at 11:31 AM, Kevin Wolf kw...@redhat.com wrote: Okay. I might not consider it wise, but in the end it's your decision. I'm just curious why you think this is the better way? Well, how would you go about sharing the code without copying in practical terms? We're aiming for standalone tool in tools/kvm of the Linux kernel so I don't see how we could do that. Well, copying in itself is not a big problem as long as the copies are kept in sync. It's a bit painful, but manageable. Implementing every image format twice (and implementing image formats in a reliable and performing way isn't trivial) is much more painful. If you take the approach of getting inspired by qemu and then writing your own code, the code will read pretty much the same, but be different enough that a diff between both trees is useless and a patch against one tree is meaningless for the other one. The block drivers are relatively isolated in qemu, so I think they wouldn't pull in too many dependencies. Kevin -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] kvm tool: add QCOW verions 1 read/write support
On Thu, Apr 14, 2011 at 9:27 AM, Ingo Molnar mi...@elte.hu wrote: * Kevin Wolf kw...@redhat.com wrote: Am 14.04.2011 10:15, schrieb Pekka Enberg: * Kevin Wolf kw...@redhat.com wrote: Also at least your qcow1.c is lacking the copyright header. Please add this, otherwise you're violating the license. Am 14.04.2011 10:07, schrieb Ingo Molnar: AFAICT it's not a copy, it's a reimplementation - and he credited you in the CREDITS file, for providing a reference implementation. But we can credit you to the header as well - Pekka? On Thu, Apr 14, 2011 at 11:12 AM, Kevin Wolf kw...@redhat.com wrote: It's actually not my code, but Fabrice's. I compared get_cluster_offset() and it looks similar enough to me to qualify as a modified copy. It's actually me who asked to drop the license banners from the file and move it to CREDITS. Parasd, mind sending a patch that adds it back to the files? Heh, I just saw your mail from yesterday. Missed it because I wasn't CCed on v1. The license is pretty clear about it: * The above copyright notice and this permission notice shall be included in * all copies or substantial portions of the Software. Note that Prasad's v0 patch did not include this copyright notice - because he wrote the file from scratch. I asked him to attribute the Qemu reference implementation in any case - which he understood as copying the copyright license verbatim. Oh God. I was walking to university on seventh cloud, hoping to work on caching the l2 table. Now I find myslef under the controversy. Frankly speaking I am very new to understand the intricacies of licensing, I won't mind copying the copyright notice in the code if it is necessary. I am also okay with attributing the code to developers of QEMU. Speaking about the code of finding the cluster offset is so basic, that I thought it should be implemented the way it is done in the QEMU. But none of the code is copied from the QEMU sources. The complete code is written from scratch with QEMU sources as a reference. I avoid sending the first few versions of the patches on KVM mailing list, the code contains lots of structural mistakes and needs revisions. I don;t want to flood mailing list with those revisions. The kvm tool community is kind enough to guide me in improving the code, when infact any one of them can write the code in lesser time than mine. Forgive me for my lack of knowledge, I hope this problem would be solved soon. Thanks and Regards, Prasad Thanks, Ingo -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] kvm tool: add QCOW verions 1 read/write support
Kevin Wolf kw...@redhat.com writes: Am 14.04.2011 10:32, schrieb Pekka Enberg: Hi Kevin! Am 14.04.2011 10:21, schrieb Pekka Enberg: On Thu, Apr 14, 2011 at 11:02 AM, Kevin Wolf kw...@redhat.com wrote: Have you thought about a way to actually share code with qemu instead of repeating Xen's mistake of copying code, modifying it until merges are no longer possible and then let it bitrot? No we haven't and we're not planning to copy QEMU code as-is but re-implement support for formats we're interested in. On Thu, Apr 14, 2011 at 11:31 AM, Kevin Wolf kw...@redhat.com wrote: Okay. I might not consider it wise, but in the end it's your decision. I'm just curious why you think this is the better way? Well, how would you go about sharing the code without copying in practical terms? We're aiming for standalone tool in tools/kvm of the Linux kernel so I don't see how we could do that. Well, copying in itself is not a big problem as long as the copies are kept in sync. It's a bit painful, but manageable. Implementing every image format twice (and implementing image formats in a reliable and performing way isn't trivial) is much more painful. If you take the approach of getting inspired by qemu and then writing your own code, the code will read pretty much the same, but be different enough that a diff between both trees is useless and a patch against one tree is meaningless for the other one. The block drivers are relatively isolated in qemu, so I think they wouldn't pull in too many dependencies. Are you suggesting to turn QEMU's block drivers into a reasonably general-purpose library? -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] kvm tool: add QCOW verions 1 read/write support
* Prasad Joshi prasadjoshi...@gmail.com wrote: Speaking about the code of finding the cluster offset is so basic, that I thought it should be implemented the way it is done in the QEMU. But none of the code is copied from the QEMU sources. The complete code is written from scratch with QEMU sources as a reference. [...] I think we should put that info into a prominent place in the source file together with a very prominent acknowledgement to the Qemu implementation as well. Thanks, Ingo -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] kvm tool: add QCOW verions 1 read/write support
Am 14.04.2011 11:26, schrieb Markus Armbruster: Kevin Wolf kw...@redhat.com writes: Am 14.04.2011 10:32, schrieb Pekka Enberg: Hi Kevin! Am 14.04.2011 10:21, schrieb Pekka Enberg: On Thu, Apr 14, 2011 at 11:02 AM, Kevin Wolf kw...@redhat.com wrote: Have you thought about a way to actually share code with qemu instead of repeating Xen's mistake of copying code, modifying it until merges are no longer possible and then let it bitrot? No we haven't and we're not planning to copy QEMU code as-is but re-implement support for formats we're interested in. On Thu, Apr 14, 2011 at 11:31 AM, Kevin Wolf kw...@redhat.com wrote: Okay. I might not consider it wise, but in the end it's your decision. I'm just curious why you think this is the better way? Well, how would you go about sharing the code without copying in practical terms? We're aiming for standalone tool in tools/kvm of the Linux kernel so I don't see how we could do that. Well, copying in itself is not a big problem as long as the copies are kept in sync. It's a bit painful, but manageable. Implementing every image format twice (and implementing image formats in a reliable and performing way isn't trivial) is much more painful. If you take the approach of getting inspired by qemu and then writing your own code, the code will read pretty much the same, but be different enough that a diff between both trees is useless and a patch against one tree is meaningless for the other one. The block drivers are relatively isolated in qemu, so I think they wouldn't pull in too many dependencies. Are you suggesting to turn QEMU's block drivers into a reasonably general-purpose library? I would hesitate to turn it into an external library, because I really don't feel like maintaining API compatibility across versions. That's simply not doable with the block layer as of today. For the long term it's something that we may consider, but would certainly require some serious work. If some changes are needed to make it more reusable in the short term (while still copying the code over), I probably wouldn't be opposed to that. Kevin -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] kvm tool: add QCOW verions 1 read/write support
On Thu, Apr 14, 2011 at 11:26:07AM +0200, Markus Armbruster wrote: Kevin Wolf kw...@redhat.com writes: Am 14.04.2011 10:32, schrieb Pekka Enberg: Hi Kevin! Am 14.04.2011 10:21, schrieb Pekka Enberg: On Thu, Apr 14, 2011 at 11:02 AM, Kevin Wolf kw...@redhat.com wrote: Have you thought about a way to actually share code with qemu instead of repeating Xen's mistake of copying code, modifying it until merges are no longer possible and then let it bitrot? No we haven't and we're not planning to copy QEMU code as-is but re-implement support for formats we're interested in. On Thu, Apr 14, 2011 at 11:31 AM, Kevin Wolf kw...@redhat.com wrote: Okay. I might not consider it wise, but in the end it's your decision. I'm just curious why you think this is the better way? Well, how would you go about sharing the code without copying in practical terms? We're aiming for standalone tool in tools/kvm of the Linux kernel so I don't see how we could do that. Well, copying in itself is not a big problem as long as the copies are kept in sync. It's a bit painful, but manageable. Implementing every image format twice (and implementing image formats in a reliable and performing way isn't trivial) is much more painful. If you take the approach of getting inspired by qemu and then writing your own code, the code will read pretty much the same, but be different enough that a diff between both trees is useless and a patch against one tree is meaningless for the other one. The block drivers are relatively isolated in qemu, so I think they wouldn't pull in too many dependencies. Are you suggesting to turn QEMU's block drivers into a reasonably general-purpose library? This is useful not just for native kvm, but also for people writing tools or automating their KVM setups. Stefan -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] kvm tool: add QCOW verions 1 read/write support
On 4/14/11 12:23 PM, Prasad Joshi wrote: Oh God. I was walking to university on seventh cloud, hoping to work on caching the l2 table. Now I find myslef under the controversy. Again, it's my fault, not yours. I asked to remove the license banner on top of qcow1.c because it doesn't contain any copied code. I will fix it up. Thanks! Pekka -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] KVM: Add CPUID support for VIA CPU
On 04/14/2011 06:14 AM, brill...@viatech.com.cn wrote: On 04/13/2011 02:05 PM, brill...@viatech.com.cn wrote: +/* cpuid 0xC001.edx */ + const u32 kvm_supported_word5_x86_features = +F(XSTORE) | F(XSTORE_EN) | F(XCRYPT) | F(XCRYPT_EN) | +F(ACE2) | F(ACE2_EN) | F(PHE) | F(PHE_EN) | +F(PMM) | F(PMM_EN); + Are all of these features save wrt save/restore? (do they all act on state in standard registers?) Do they need any control registerbits to be active or MSRs to configure? These features depend on instructions for the PadLock hardware engine of VIA CPU. The PadLock instructions just act on standard registers like general X86 instructions, and the features have been enabled when the CPU leave the factory, so there is no need to activate any control register bits or configure MSRs. I see there is a dependency on EFLAGS[30]. Does a VM entry clear this bit? If not, we have to do it ourselves. Yes, PadLock hardware engine has some association with EFLAGS[30], but it just required that the EFLAGS[30] should be set to 0 before using PadLock ACE instructions. It is recommanded that execute instruction sequence pushf;popf to clear this bit before using ACE instructions. AFAIK, the VM entry never sets the EFLAGS[30] bit, so it seems that we do not have to do it ourselves. I don't think we need to. kvm kernel code doesn't use padlock; other sources which might set EFLAGS[30] are: - the guest; but VMEXIT clears EFLAGS - userspace; but syscall/sysenter/int instructions clear EFLAGS[30] - another kernel thread; there is likely a popf in the context switch path somewhere (we should verify this) Thank you very much for telling me where the EFLAGS[30] might be set and cleared. In fact, adding the CPUID support into kvm kernel code is just to provide correct result for the calling of the kvm_arch_get_supported_cpuid function in Qemu-kvm application. There is no dependency on EFLAGS when calling VIA CPUID instruction. Before using padlock engine functions, the application first need detect is the features exist through cpuid instruction, then use ACE and other instructions such as PHE/RNG/PMM. And as previously said, only the ACE instructions required that the EFLAGS[30] should be set to 0, It is recommanded that execute instruction sequence pushf;popf to clear this bit before using ACE instructions. I have re-submitted this patch, please check it. Thanks! -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH 2/2 V7] qemu,qmp: add inject-nmi qmp command
On Wed, Apr 13, 2011 at 10:56:21PM +0300, Blue Swirl wrote: On Wed, Apr 13, 2011 at 4:08 PM, Luiz Capitulino lcapitul...@redhat.com wrote: On Tue, 12 Apr 2011 21:31:18 +0300 Blue Swirl blauwir...@gmail.com wrote: On Tue, Apr 12, 2011 at 10:52 AM, Avi Kivity a...@redhat.com wrote: On 04/11/2011 08:15 PM, Blue Swirl wrote: On Mon, Apr 11, 2011 at 10:01 AM, Markus Armbrusterarm...@redhat.com wrote: Avi Kivitya...@redhat.com writes: On 04/08/2011 12:41 AM, Anthony Liguori wrote: And it's a good thing to have, but exposing this as the only API to do something as simple as generating a guest crash dump is not the friendliest thing in the world to do to users. nmi is a fine name for something that corresponds to a real-life nmi button (often labeled NMI). Agree. We could also introduce an alias mechanism for user friendly names, so nmi could be used in addition of full path. Aliases could be useful for device paths as well. Yes. Perhaps limited to the human monitor. I'd limit all debugging commands (including NMI) to the human monitor. Why? Do they have any real use in production environment? Also, we should have the freedom to change the debugging facilities (for example, to improve some internal implementation) as we want without regard to compatibility to previous versions. We have users of libvirt requesting that we add an API for triggering a NMI. They want this for support in a production environment, to be able to initiate Windows crash dumps. We really don't want to have to use HMP passthrough for this, instead of a proper QMP command. More generally I don't want to see stuff in HMP, that isn't in the QMP. We already have far too much that we have to do via HMP passthrough in libvirt due to lack of QMP commands, to the extent that we might as well have just ignored QMP and continued with HMP for everything. If we want the flexibility to change the debugging commands between releases then we should come up with a plan to do this within the scope of QMP, not restrict them to HMP only. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] kvm tool: add QCOW verions 1 read/write support
Kevin Wolf kw...@redhat.com writes: Am 14.04.2011 11:26, schrieb Markus Armbruster: Kevin Wolf kw...@redhat.com writes: Am 14.04.2011 10:32, schrieb Pekka Enberg: Hi Kevin! Am 14.04.2011 10:21, schrieb Pekka Enberg: On Thu, Apr 14, 2011 at 11:02 AM, Kevin Wolf kw...@redhat.com wrote: Have you thought about a way to actually share code with qemu instead of repeating Xen's mistake of copying code, modifying it until merges are no longer possible and then let it bitrot? No we haven't and we're not planning to copy QEMU code as-is but re-implement support for formats we're interested in. On Thu, Apr 14, 2011 at 11:31 AM, Kevin Wolf kw...@redhat.com wrote: Okay. I might not consider it wise, but in the end it's your decision. I'm just curious why you think this is the better way? Well, how would you go about sharing the code without copying in practical terms? We're aiming for standalone tool in tools/kvm of the Linux kernel so I don't see how we could do that. Well, copying in itself is not a big problem as long as the copies are kept in sync. It's a bit painful, but manageable. Implementing every image format twice (and implementing image formats in a reliable and performing way isn't trivial) is much more painful. If you take the approach of getting inspired by qemu and then writing your own code, the code will read pretty much the same, but be different enough that a diff between both trees is useless and a patch against one tree is meaningless for the other one. The block drivers are relatively isolated in qemu, so I think they wouldn't pull in too many dependencies. Are you suggesting to turn QEMU's block drivers into a reasonably general-purpose library? I would hesitate to turn it into an external library, because I really don't feel like maintaining API compatibility across versions. That's simply not doable with the block layer as of today. For the long term it's something that we may consider, but would certainly require some serious work. If some changes are needed to make it more reusable in the short term (while still copying the code over), I probably wouldn't be opposed to that. Unless we make QEMU's block drivers usable outside QEMU (and that means at least a static library without API guarantees), we can hardly chide others for reimplementing them. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: Add CPUID support for VIA CPU
On 04/14/2011 12:54 PM, brill...@viatech.com.cn wrote: On 04/14/2011 06:14 AM, brill...@viatech.com.cn wrote: On 04/13/2011 02:05 PM, brill...@viatech.com.cn wrote: + /* cpuid 0xC001.edx */ + const u32 kvm_supported_word5_x86_features = + F(XSTORE) | F(XSTORE_EN) | F(XCRYPT) | F(XCRYPT_EN) | + F(ACE2) | F(ACE2_EN) | F(PHE) | F(PHE_EN) | + F(PMM) | F(PMM_EN); + Are all of these features save wrt save/restore? (do they all act on state in standard registers?) Do they need any control register bits to be active or MSRs to configure? These features depend on instructions for the PadLock hardware engine of VIA CPU. The PadLock instructions just act on standard registers like general X86 instructions, and the features have been enabled when the CPU leave the factory, so there is no need to activate any control register bits or configure MSRs. I see there is a dependency on EFLAGS[30]. Does a VM entry clear this bit? If not, we have to do it ourselves. Yes, PadLock hardware engine has some association with EFLAGS[30], but it just required that the EFLAGS[30] should be set to 0 before using PadLock ACE instructions. It is recommanded that execute instruction sequence pushf;popf to clear this bit before using ACE instructions. AFAIK, the VM entry never sets the EFLAGS[30] bit, so it seems that we do not have to do it ourselves. I don't think we need to. kvm kernel code doesn't use padlock; other sources which might set EFLAGS[30] are: - the guest; but VMEXIT clears EFLAGS - userspace; but syscall/sysenter/int instructions clear EFLAGS[30] - another kernel thread; there is likely a popf in the context switch path somewhere (we should verify this) Thank you very much for telling me where the EFLAGS[30] might be set and cleared. In fact, adding the CPUID support into kvm kernel code is just to provide correct result for the calling of the kvm_arch_get_supported_cpuid function in Qemu-kvm application. That may not be sufficient for correct operation. Consider: - guest executes a padlock instruction - cpu sets EFLAGS[30] - external interrupt - VMEXIT (saves EFLAGS in GUEST_RFLAGS with EFLAGS[30] set) - external interrupt is processed, causes a task switch - EFLAGS[30] is cleared - some other process uses padlock instructions, which causes a reload of key information - switch back to kvm - VM entry (loads EFLAGS from GUEST_RFLAGS; still has EFLAGS[30] set) - guest executes following padlock instruction, doesn't reload key information so I think the code as is causes data corruption. There is no dependency on EFLAGS when calling VIA CPUID instruction. Before using padlock engine functions, the application first need detect is the features exist through cpuid instruction, then use ACE and other instructions such as PHE/RNG/PMM. And as previously said, only the ACE instructions required that the EFLAGS[30] should be set to 0, It is recommanded that execute instruction sequence pushf;popf to clear this bit before using ACE instructions. The problem is that VM entry reloads eflags from saved state and is not affected by popf. I have re-submitted this patch, please check it. Thanks! wrt cpuid it seems reasonable but that's we need to clear the EFLAGS[30] issue first. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Network performance with small packets
On Tue, 12 Apr 2011 23:01:12 +0300, Michael S. Tsirkin m...@redhat.com wrote: On Thu, Mar 10, 2011 at 12:19:42PM +1030, Rusty Russell wrote: Here's an old patch where I played with implementing this: ... virtio: put last_used and last_avail index into ring itself. Generally, the other end of the virtio ring doesn't need to see where you're up to in consuming the ring. However, to completely understand what's going on from the outside, this information must be exposed. For example, if you want to save and restore a virtio_ring, but you're not the consumer because the kernel is using it directly. Fortunately, we have room to expand: This seems to be true for x86 kvm and lguest but is it true for s390? Yes, as the ring is page aligned so there's always room. Will this last bit work on s390? If I understand correctly the memory is allocated by host there? They have to offer the feature, so if the have some way of allocating non-page-aligned amounts of memory, they'll have to add those extra 2 bytes. So I think it's OK... Rusty. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 V2] kvm tools: Set up tun interface using ioctls
On 04/14/2011 01:24 PM, Sasha Levin wrote:
Use ioctls to assign IP address and bring interface up instead of using
ifconfig.
Not breaking aliasing rules this time.
Signed-off-by: Sasha Levin levinsasha...@gmail.com
---
tools/kvm/virtio-net.c | 29 ++---
1 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/tools/kvm/virtio-net.c b/tools/kvm/virtio-net.c
index ec70d5c..622cfc6 100644
--- a/tools/kvm/virtio-net.c
+++ b/tools/kvm/virtio-net.c
@@ -14,6 +14,9 @@
#include sys/ioctl.h
#include assert.h
#include fcntl.h
+#include arpa/inet.h
+#include sys/types.h
+#include sys/socket.h
#define VIRTIO_NET_IRQ 14
#define VIRTIO_NET_QUEUE_SIZE128
@@ -276,7 +279,9 @@ static struct pci_device_header virtio_net_pci_device = {
static void virtio_net__tap_init(void)
{
struct ifreq ifr;
-
+ int sock = socket(AF_INET, SOCK_STREAM, 0);
+ struct sockaddr_in sin = {0};
+
net_device.tap_fd = open(/dev/net/tun, O_RDWR);
if (net_device.tap_fd 0)
die(Unable to open /dev/net/tun\n);
@@ -291,9 +296,27 @@ static void virtio_net__tap_init(void)
ioctl(net_device.tap_fd, TUNSETNOCSUM, 1);
+
+ memset(ifr, 0, sizeof(ifr));
+
+ strncpy(ifr.ifr_name, net_device.tap_name, sizeof(net_device.tap_name));
+
/*FIXME: Remove this after user can specify ip address and netmask*/
- if (system(ifconfig tap0 192.168.33.2) 0)
- warning(Can not set ip address on tap0);
+ sin.sin_addr.s_addr = inet_addr(192.168.33.2);
+ memcpy((ifr.ifr_addr), sin, sizeof(ifr.ifr_addr));
+ ifr.ifr_addr.sa_family = AF_INET;
+
+ if (ioctl(sock, SIOCSIFADDR, ifr) 0)
+ warning(Can not set ip address on tap device);
+
+ memset(ifr, 0, sizeof(ifr));
+ strncpy(ifr.ifr_name, net_device.tap_name, sizeof(net_device.tap_name));
+ ioctl(sock, SIOCGIFFLAGS, ifr);
+ ifr.ifr_flags |= IFF_UP | IFF_RUNNING;
+ if (ioctl(sock, SIOCSIFFLAGS, ifr) 0)
+ warning(Could not bring tap device up);
+
+ close(sock);
}
static void virtio_net__io_thread_init(struct kvm *self)
Looks good to me.
Reviewed-by: Asias He asias.he...@gmail.com
--
Best Regards,
Asias He
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] kvm tool: add QCOW verions 1 read/write support
On 04/14/2011 03:52 AM, Kevin Wolf wrote: Well, copying in itself is not a big problem as long as the copies are kept in sync. It's a bit painful, but manageable. Implementing every image format twice (and implementing image formats in a reliable and performing way isn't trivial) is much more painful. If you take the approach of getting inspired by qemu and then writing your own code, the code will read pretty much the same, but be different enough that a diff between both trees is useless and a patch against one tree is meaningless for the other one. The block drivers are relatively isolated in qemu, so I think they wouldn't pull in too many dependencies. The block layer has some interesting bits though that I can understand wanting to avoid (like aio emulation). In the very least, we've learned the hard way that getting an image format right is very hard. A good test suite is worth it's weight in gold for something like this. When taking the approach of starting fresh, doing test driven development is definitely worth considering. Regards, Anthony Liguori Kevin -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Network performance with small packets
On Thu, Apr 14, 2011 at 08:58:41PM +0930, Rusty Russell wrote: On Tue, 12 Apr 2011 23:01:12 +0300, Michael S. Tsirkin m...@redhat.com wrote: On Thu, Mar 10, 2011 at 12:19:42PM +1030, Rusty Russell wrote: Here's an old patch where I played with implementing this: ... virtio: put last_used and last_avail index into ring itself. Generally, the other end of the virtio ring doesn't need to see where you're up to in consuming the ring. However, to completely understand what's going on from the outside, this information must be exposed. For example, if you want to save and restore a virtio_ring, but you're not the consumer because the kernel is using it directly. Fortunately, we have room to expand: This seems to be true for x86 kvm and lguest but is it true for s390? Yes, as the ring is page aligned so there's always room. Will this last bit work on s390? If I understand correctly the memory is allocated by host there? They have to offer the feature, so if the have some way of allocating non-page-aligned amounts of memory, they'll have to add those extra 2 bytes. So I think it's OK... Rusty. To clarify, my concern is that we always seem to try to map these extra 2 bytes, which thinkably might fail? -- MST -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] kvm tools: Add QCOW version 1 read-only support
From: Prasad Joshi prasadjoshi...@gmail.com
The patch only implements the basic read-only support for QCOW version 1
images. Many of the QCOW features are not implemented:
- write
- image creation
- snapshot
- copy-on-write
- encryption
The code is based on the following QCOW 1 image format specification:
http://people.gnome.org/~markmc/qcow-image-format-version-1.html
Cc: Asias He asias.he...@gmail.com
Cc: Cyrill Gorcunov gorcu...@gmail.com
Cc: Ingo Molnar mi...@elte.hu
Cc: Kevin Wolf kw...@redhat.com
Cc: Sasha Levin levinsasha...@gmail.com
Cc: Stefan Hajnoczi stefa...@linux.vnet.ibm.com
Signed-off-by: Prasad Joshi prasadjoshi...@gmail.com
Signed-off-by: Pekka Enberg penb...@kernel.org
---
FYI, I dropped commit 910c791 (kvm tools: Add QCOW version 1 read/write
support) from the GIT repository and re-implemented read-only QCOW1 support
from scratch.
tools/kvm/Makefile |1 +
tools/kvm/disk-image.c |5 +
tools/kvm/include/kvm/qcow.h| 41 +++
tools/kvm/include/linux/byteorder.h |7 +
tools/kvm/include/linux/types.h | 19 +++
tools/kvm/qcow.c| 227 +++
6 files changed, 300 insertions(+), 0 deletions(-)
create mode 100644 tools/kvm/include/kvm/qcow.h
create mode 100644 tools/kvm/include/linux/byteorder.h
create mode 100644 tools/kvm/qcow.c
diff --git a/tools/kvm/Makefile b/tools/kvm/Makefile
index 6895113..28dd369 100644
--- a/tools/kvm/Makefile
+++ b/tools/kvm/Makefile
@@ -34,6 +34,7 @@ OBJS+= util/strbuf.o
OBJS+= kvm-help.o
OBJS+= kvm-cmd.o
OBJS+= kvm-run.o
+OBJS+= qcow.o
DEPS := $(patsubst %.o,%.d,$(OBJS))
diff --git a/tools/kvm/disk-image.c b/tools/kvm/disk-image.c
index 5d0f342..fff71b4 100644
--- a/tools/kvm/disk-image.c
+++ b/tools/kvm/disk-image.c
@@ -1,6 +1,7 @@
#include kvm/disk-image.h
#include kvm/read-write.h
+#include kvm/qcow.h
#include kvm/util.h
#include sys/types.h
@@ -131,6 +132,10 @@ struct disk_image *disk_image__open(const char *filename,
bool readonly)
if (fd 0)
return NULL;
+ self = qcow_probe(fd);
+ if (self)
+ return self;
+
self = raw_image__probe(fd, readonly);
if (self)
return self;
diff --git a/tools/kvm/include/kvm/qcow.h b/tools/kvm/include/kvm/qcow.h
new file mode 100644
index 000..4be2597
--- /dev/null
+++ b/tools/kvm/include/kvm/qcow.h
@@ -0,0 +1,41 @@
+#ifndef KVM__QCOW_H
+#define KVM__QCOW_H
+
+#include linux/types.h
+
+#define QCOW_MAGIC (('Q' 24) | ('F' 16) | ('I' 8) | 0xfb)
+#define QCOW1_VERSION 1
+
+#define QCOW_OFLAG_COMPRESSED (1LL 63)
+
+struct qcow_table {
+ u32 table_size;
+ u64 *l1_table;
+};
+
+struct qcow {
+ void*header;
+ struct qcow_table table;
+ int fd;
+};
+
+struct qcow1_header {
+ u32 magic;
+ u32 version;
+
+ u64 backing_file_offset;
+ u32 backing_file_size;
+ u32 mtime;
+
+ u64 size; /* in bytes */
+
+ u8 cluster_bits;
+ u8 l2_bits;
+ u32 crypt_method;
+
+ u64 l1_table_offset;
+};
+
+struct disk_image *qcow_probe(int fd);
+
+#endif /* KVM__QCOW_H */
diff --git a/tools/kvm/include/linux/byteorder.h
b/tools/kvm/include/linux/byteorder.h
new file mode 100644
index 000..c490de8
--- /dev/null
+++ b/tools/kvm/include/linux/byteorder.h
@@ -0,0 +1,7 @@
+#ifndef __BYTE_ORDER_H__
+#define __BYTE_ORDER_H__
+
+#include asm/byteorder.h
+#include linux/byteorder/generic.h
+
+#endif
diff --git a/tools/kvm/include/linux/types.h b/tools/kvm/include/linux/types.h
index 8b608e7..efd8519 100644
--- a/tools/kvm/include/linux/types.h
+++ b/tools/kvm/include/linux/types.h
@@ -27,4 +27,23 @@ typedef __s16 s16;
typedef __u8 u8;
typedef __s8 s8;
+#ifdef __CHECKER__
+#define __bitwise__ __attribute__((bitwise))
+#else
+#define __bitwise__
+#endif
+#ifdef __CHECK_ENDIAN__
+#define __bitwise __bitwise__
+#else
+#define __bitwise
+#endif
+
+
+typedef __u16 __bitwise __le16;
+typedef __u16 __bitwise __be16;
+typedef __u32 __bitwise __le32;
+typedef __u32 __bitwise __be32;
+typedef __u64 __bitwise __le64;
+typedef __u64 __bitwise __be64;
+
#endif /* LINUX_TYPES_H */
diff --git a/tools/kvm/qcow.c b/tools/kvm/qcow.c
new file mode 100644
index 000..c4e3e48
--- /dev/null
+++ b/tools/kvm/qcow.c
@@ -0,0 +1,227 @@
+#include kvm/qcow.h
+
+#include kvm/disk-image.h
+#include kvm/read-write.h
+#include kvm/util.h
+
+#include sys/types.h
+#include sys/stat.h
+#include stdbool.h
+#include stdlib.h
+#include string.h
+#include unistd.h
+#include fcntl.h
+
+#include linux/byteorder.h
+#include linux/types.h
+
+static inline
[RFT/PATCH v2] kvm tools: Add read-only support for block devices
Add support for booting guests to host block devices in read-only mode.
Cc: Asias He asias.he...@gmail.com
Cc: Cyrill Gorcunov gorcu...@gmail.com
Cc: Prasad Joshi prasadjoshi...@gmail.com
Cc: Sasha Levin levinsasha...@gmail.com
Cc: Ingo Molnar mi...@elte.hu
Signed-off-by: Pekka Enberg penb...@kernel.org
---
tools/kvm/disk-image.c | 39 +++
1 files changed, 31 insertions(+), 8 deletions(-)
diff --git a/tools/kvm/disk-image.c b/tools/kvm/disk-image.c
index fff71b4..f5e11b9 100644
--- a/tools/kvm/disk-image.c
+++ b/tools/kvm/disk-image.c
@@ -4,6 +4,9 @@
#include kvm/qcow.h
#include kvm/util.h
+#include linux/fs.h /* for BLKGETSIZE64 */
+
+#include sys/ioctl.h
#include sys/types.h
#include inttypes.h
#include sys/mman.h
@@ -110,24 +113,44 @@ static struct disk_image_operations raw_image_ro_mmap_ops
= {
.close = raw_image__close_sector_ro_mmap,
};
-static struct disk_image *raw_image__probe(int fd, bool readonly)
+static struct disk_image *raw_image__probe(int fd, struct stat *st, bool
readonly)
{
- struct stat st;
+ if (readonly)
+ return disk_image__new_readonly(fd, st-st_size,
raw_image_ro_mmap_ops);
+ else
+ return disk_image__new(fd, st-st_size, raw_image_ops);
+}
- if (fstat(fd, st) 0)
+static struct disk_image *blkdev__probe(const char *filename, struct stat *st)
+{
+ uint64_t size;
+ int fd;
+
+ if (!S_ISBLK(st-st_mode))
return NULL;
- if (readonly)
- return disk_image__new_readonly(fd, st.st_size,
raw_image_ro_mmap_ops);
- else
- return disk_image__new(fd, st.st_size, raw_image_ops);
+ fd = open(filename, O_RDONLY);
+ if (fd 0)
+ return NULL;
+
+ if (ioctl(fd, BLKGETSIZE64, size) 0)
+ return NULL;
+
+ return disk_image__new_readonly(fd, size, raw_image_ro_mmap_ops);
}
struct disk_image *disk_image__open(const char *filename, bool readonly)
{
struct disk_image *self;
+ struct stat st;
int fd;
+ if (stat(filename, st) 0)
+ return NULL;
+
+ if (S_ISBLK(st.st_mode))
+ return blkdev__probe(filename, st);
+
fd = open(filename, readonly ? O_RDONLY : O_RDWR);
if (fd 0)
return NULL;
@@ -136,7 +159,7 @@ struct disk_image *disk_image__open(const char *filename,
bool readonly)
if (self)
return self;
- self = raw_image__probe(fd, readonly);
+ self = raw_image__probe(fd, st, readonly);
if (self)
return self;
--
1.7.0.4
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] kvm tool: check the cluster boundary in the qcow read code.
Changed the function names from sect_to_l1_offset(), sect_to_l2_offset() to
get_l1_index(), get_l2_index() as they return index into their respective table.
Signed-off-by: Prasad Joshi prasadjoshi...@gmail.com
---
tools/kvm/qcow.c | 103 ++
1 files changed, 57 insertions(+), 46 deletions(-)
diff --git a/tools/kvm/qcow.c b/tools/kvm/qcow.c
index c4e3e48..3afd3fb 100644
--- a/tools/kvm/qcow.c
+++ b/tools/kvm/qcow.c
@@ -15,14 +15,14 @@
#include linux/byteorder.h
#include linux/types.h
-static inline uint64_t sect_to_l1_offset(struct qcow *q, uint64_t offset)
+static inline uint64_t get_l1_index(struct qcow *q, uint64_t offset)
{
struct qcow1_header *header = q-header;
return offset (header-l2_bits + header-cluster_bits);
}
-static inline uint64_t sect_to_l2_offset(struct qcow *q, uint64_t offset)
+static inline uint64_t get_l2_index(struct qcow *q, uint64_t offset)
{
struct qcow1_header *header = q-header;
@@ -44,54 +44,65 @@ static int qcow1_read_sector(struct disk_image *self,
uint64_t sector, void *dst
uint64_t l2_table_size;
uint64_t clust_offset;
uint64_t clust_start;
+ uint64_t clust_size;
uint64_t *l2_table;
uint64_t l1_idx;
uint64_t l2_idx;
uint64_t offset;
-
- offset = sector SECTOR_SHIFT;
- if (offset = header-size)
- goto out_error;
-
- l1_idx = sect_to_l1_offset(self-priv, offset);
-
- if (l1_idx = q-table.table_size)
- goto out_error;
-
- l2_table_offset = be64_to_cpu(q-table.l1_table[l1_idx]);
- if (!l2_table_offset)
- goto zero_sector;
-
- l2_table_size = 1 header-l2_bits;
-
- l2_table= calloc(l2_table_size, sizeof(uint64_t));
- if (!l2_table)
- goto out_error;
-
- if (pread_in_full(q-fd, l2_table, sizeof(uint64_t) * l2_table_size,
l2_table_offset) 0)
- goto out_error_free_l2;
-
- l2_idx = sect_to_l2_offset(self-priv, offset);
-
- if (l2_idx = l2_table_size)
- goto out_error_free_l2;
-
- clust_start = be64_to_cpu(l2_table[l2_idx]);
-
- if (!clust_start)
- goto zero_sector;
-
- clust_offset= sect_to_cluster_offset(self-priv, offset);
-
- if (pread_in_full(q-fd, dst, dst_len, clust_start + clust_offset) 0)
- goto out_error_free_l2;
-
- free(l2_table);
-
- return 0;
-
-zero_sector:
- memset(dst, 0, dst_len);
+ uint32_t length;
+ uint32_t tmp;
+ char *buf = dst;
+
+ clust_size = 1 header-cluster_bits;
+ length = 0;
+
+ while (length dst_len) {
+ offset = sector SECTOR_SHIFT;
+ if (offset = header-size)
+ goto out_error;
+
+ l1_idx = get_l1_index(self-priv, offset);
+ if (l1_idx = q-table.table_size)
+ goto out_error;
+
+ l2_table_offset = be64_to_cpu(q-table.l1_table[l1_idx]);
+ if (!l2_table_offset) {
+ tmp = clust_size;
+ memset(buf, 0, tmp);
+ goto next_cluster;
+ }
+
+ l2_table_size = 1 header-l2_bits;
+
+ l2_table = calloc(l2_table_size, sizeof(uint64_t));
+ if (!l2_table)
+ goto out_error;
+
+ if (pread_in_full(q-fd, l2_table, sizeof(uint64_t) *
l2_table_size, l2_table_offset) 0)
+ goto out_error_free_l2;
+
+ l2_idx = get_l2_index(self-priv, offset);
+ if (l2_idx = l2_table_size)
+ goto out_error_free_l2;
+
+ clust_start = be64_to_cpu(l2_table[l2_idx]);
+ free(l2_table);
+ if (!clust_start) {
+ tmp = clust_size;
+ memset(buf, 0, tmp);
+ } else {
+ clust_offset = sect_to_cluster_offset(self-priv,
offset);
+ tmp = clust_size - clust_offset;
+
+ if (pread_in_full(q-fd, buf, tmp, clust_start +
clust_offset) 0)
+ goto out_error;
+ }
+
+next_cluster:
+ buf += tmp;
+ sector += (tmp SECTOR_SHIFT);
+ length += tmp;
+ }
return 0;
--
1.7.1
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvm tool: check the cluster boundary in the qcow read code.
On Thu, 2011-04-14 at 20:23 +0100, Prasad Joshi wrote:
Changed the function names from sect_to_l1_offset(), sect_to_l2_offset() to
get_l1_index(), get_l2_index() as they return index into their respective
table.
This patch does a lot more than what's described above. Please split the
changes in two separate patches.
+ uint32_t length;
+ uint32_t tmp;
+ char *buf = dst;
+
+ clust_size = 1 header-cluster_bits;
+ length = 0;
+
+ while (length dst_len) {
+ offset = sector SECTOR_SHIFT;
+ if (offset = header-size)
+ goto out_error;
+
+ l1_idx = get_l1_index(self-priv, offset);
+ if (l1_idx = q-table.table_size)
+ goto out_error;
+
+ l2_table_offset = be64_to_cpu(q-table.l1_table[l1_idx]);
+ if (!l2_table_offset) {
+ tmp = clust_size;
+ memset(buf, 0, tmp);
+ goto next_cluster;
+ }
+
+ l2_table_size = 1 header-l2_bits;
+
+ l2_table = calloc(l2_table_size, sizeof(uint64_t));
+ if (!l2_table)
+ goto out_error;
+
+ if (pread_in_full(q-fd, l2_table, sizeof(uint64_t) *
l2_table_size, l2_table_offset) 0)
+ goto out_error_free_l2;
+
+ l2_idx = get_l2_index(self-priv, offset);
+ if (l2_idx = l2_table_size)
+ goto out_error_free_l2;
+
+ clust_start = be64_to_cpu(l2_table[l2_idx]);
+ free(l2_table);
+ if (!clust_start) {
+ tmp = clust_size;
+ memset(buf, 0, tmp);
+ } else {
+ clust_offset = sect_to_cluster_offset(self-priv,
offset);
+ tmp = clust_size - clust_offset;
+
+ if (pread_in_full(q-fd, buf, tmp, clust_start +
clust_offset) 0)
+ goto out_error;
+ }
+
+next_cluster:
+ buf += tmp;
+ sector += (tmp SECTOR_SHIFT);
+ length += tmp;
+ }
Well, the loop is not exactly making the code any better. If you're
worried about reads that cross over a single cluster, it's probably
better to rename the current function to qcow1_read_cluster() or
something and use that in a loop that makes sure we never cross over a
cluster.
Btw, I think our -read_sector and -write_sector functions need
renaming to -read_sectors and -write_sectors with little bit
commentary on what they can expect to receive.
Pekka
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvm tool: check the cluster boundary in the qcow read code.
On Thu, Apr 14, 2011 at 8:59 PM, Pekka Enberg penb...@kernel.org wrote:
On Thu, 2011-04-14 at 20:23 +0100, Prasad Joshi wrote:
Changed the function names from sect_to_l1_offset(), sect_to_l2_offset() to
get_l1_index(), get_l2_index() as they return index into their respective
table.
This patch does a lot more than what's described above. Please split the
changes in two separate patches.
Okay.
+ uint32_t length;
+ uint32_t tmp;
+ char *buf = dst;
+
+ clust_size = 1 header-cluster_bits;
+ length = 0;
+
+ while (length dst_len) {
+ offset = sector SECTOR_SHIFT;
+ if (offset = header-size)
+ goto out_error;
+
+ l1_idx = get_l1_index(self-priv, offset);
+ if (l1_idx = q-table.table_size)
+ goto out_error;
+
+ l2_table_offset = be64_to_cpu(q-table.l1_table[l1_idx]);
+ if (!l2_table_offset) {
+ tmp = clust_size;
+ memset(buf, 0, tmp);
+ goto next_cluster;
+ }
+
+ l2_table_size = 1 header-l2_bits;
+
+ l2_table = calloc(l2_table_size, sizeof(uint64_t));
+ if (!l2_table)
+ goto out_error;
+
+ if (pread_in_full(q-fd, l2_table, sizeof(uint64_t) *
l2_table_size, l2_table_offset) 0)
+ goto out_error_free_l2;
+
+ l2_idx = get_l2_index(self-priv, offset);
+ if (l2_idx = l2_table_size)
+ goto out_error_free_l2;
+
+ clust_start = be64_to_cpu(l2_table[l2_idx]);
+ free(l2_table);
+ if (!clust_start) {
+ tmp = clust_size;
+ memset(buf, 0, tmp);
+ } else {
+ clust_offset = sect_to_cluster_offset(self-priv,
offset);
+ tmp = clust_size - clust_offset;
+
+ if (pread_in_full(q-fd, buf, tmp, clust_start +
clust_offset) 0)
+ goto out_error;
+ }
+
+next_cluster:
+ buf += tmp;
+ sector += (tmp SECTOR_SHIFT);
+ length += tmp;
+ }
Well, the loop is not exactly making the code any better.
More than clarity it is mandatory. The consecutive sectors are not
always guaranteed to be assigned to the same file. If fact, a cluster
might be holding a level 2 table.
If you're
worried about reads that cross over a single cluster, it's probably
better to rename the current function to qcow1_read_cluster() or
something and use that in a loop that makes sure we never cross over a
cluster.
Okay.
Btw, I think our -read_sector and -write_sector functions need
renaming to -read_sectors and -write_sectors with little bit
commentary on what they can expect to receive.
Okay.
Thanks and Regards,
Prasad
Pekka
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvm tool: add QCOW verions 1 read/write support
On Wed, Apr 13, 2011 at 08:01:58PM +0100, Prasad Joshi wrote: The patch only implements the basic read write support for QCOW version 1 images. Many of the QCOW features are not implmented, for example What's the point? Qcow1 has been deprecated for a long time. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] KVM: emulator: Use linearize() when fetching instructions.
This means that the truncation behavior in linearize needs to grow an additional
slight piece of complexity: when fetching, truncation is dependent on the
execution mode, instead of the current address size.
Signed-off-by: Nelson Elhage nelh...@ksplice.com
---
arch/x86/include/asm/kvm_emulate.h |1 -
arch/x86/kvm/emulate.c | 23 ---
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/arch/x86/include/asm/kvm_emulate.h
b/arch/x86/include/asm/kvm_emulate.h
index 0818448..9b760c8 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -265,7 +265,6 @@ struct x86_emulate_ctxt {
unsigned long eip; /* eip before instruction emulation */
/* Emulated execution mode, represented by an X86EMUL_MODE value. */
int mode;
- u32 cs_base;
/* interruptibility state, as a result of execution of STI or MOV SS */
int interruptibility;
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index a5f63d4..d3d43a7 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -542,7 +542,7 @@ static int emulate_nm(struct x86_emulate_ctxt *ctxt)
static int linearize(struct x86_emulate_ctxt *ctxt,
struct segmented_address addr,
-unsigned size, bool write,
+unsigned size, bool write, bool fetch,
ulong *linear)
{
struct decode_cache *c = ctxt-decode;
@@ -602,7 +602,7 @@ static int linearize(struct x86_emulate_ctxt *ctxt,
}
break;
}
- if (c-ad_bytes != 8)
+ if (fetch ? ctxt-mode != X86EMUL_MODE_PROT64 : c-ad_bytes != 8)
la = (u32)-1;
*linear = la;
return X86EMUL_CONTINUE;
@@ -621,7 +621,7 @@ static int segmented_read_std(struct x86_emulate_ctxt *ctxt,
int rc;
ulong linear;
- rc = linearize(ctxt, addr, size, false, linear);
+ rc = linearize(ctxt, addr, size, false, false, linear);
if (rc != X86EMUL_CONTINUE)
return rc;
return ctxt-ops-read_std(linear, data, size, ctxt-vcpu,
@@ -637,11 +637,13 @@ static int do_fetch_insn_byte(struct x86_emulate_ctxt
*ctxt,
int size, cur_size;
if (eip == fc-end) {
- unsigned long linear = eip + ctxt-cs_base;
- if (ctxt-mode != X86EMUL_MODE_PROT64)
- linear = (u32)-1;
+ unsigned long linear;
+ struct segmented_address addr = {VCPU_SREG_CS, eip};
cur_size = fc-end - fc-start;
size = min(15UL - cur_size, PAGE_SIZE - offset_in_page(eip));
+ rc = linearize(ctxt, addr, size, false, true, linear);
+ f (rc != X86EMUL_CONTINUE)
+ return rc;
rc = ops-fetch(linear, fc-data + cur_size,
size, ctxt-vcpu, ctxt-exception);
if (rc != X86EMUL_CONTINUE)
@@ -1047,7 +1049,7 @@ static int segmented_read(struct x86_emulate_ctxt *ctxt,
int rc;
ulong linear;
- rc = linearize(ctxt, addr, size, false, linear);
+ rc = linearize(ctxt, addr, size, false, false, linear);
if (rc != X86EMUL_CONTINUE)
return rc;
return read_emulated(ctxt, ctxt-ops, linear, data, size);
@@ -1061,7 +1063,7 @@ static int segmented_write(struct x86_emulate_ctxt *ctxt,
int rc;
ulong linear;
- rc = linearize(ctxt, addr, size, true, linear);
+ rc = linearize(ctxt, addr, size, true, false, linear);
if (rc != X86EMUL_CONTINUE)
return rc;
return ctxt-ops-write_emulated(linear, data, size,
@@ -1076,7 +1078,7 @@ static int segmented_cmpxchg(struct x86_emulate_ctxt
*ctxt,
int rc;
ulong linear;
- rc = linearize(ctxt, addr, size, true, linear);
+ rc = linearize(ctxt, addr, size, true, false, linear);
if (rc != X86EMUL_CONTINUE)
return rc;
return ctxt-ops-cmpxchg_emulated(linear, orig_data, data,
@@ -2576,7 +2578,7 @@ static int em_invlpg(struct x86_emulate_ctxt *ctxt)
int rc;
ulong linear;
- rc = linearize(ctxt, c-src.addr.mem, 1, false, linear);
+ rc = linearize(ctxt, c-src.addr.mem, 1, false, false, linear);
if (rc == X86EMUL_CONTINUE)
emulate_invlpg(ctxt-vcpu, linear);
/* Disable writeback. */
@@ -3154,7 +3156,6 @@ x86_decode_insn(struct x86_emulate_ctxt *ctxt, void
*insn, int insn_len)
c-fetch.end = c-fetch.start + insn_len;
if (insn_len 0)
memcpy(c-fetch.data, insn, insn_len);
- ctxt-cs_base = seg_base(ctxt, ops, VCPU_SREG_CS);
switch (mode) {
case X86EMUL_MODE_REAL:
--
1.7.2.43.g36c08.dirty
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at
