Re: Why QCOW1? (was: [PATCH v2] kvm tool: add QCOW verions 1 read/write support)
On Fri, Apr 15, 2011 at 9:41 AM, Markus Armbruster wrote: > What hasn't been discussed much is the other half of Kevin's remark: why > QCOW1? QCOW1 was simpler to implement as the first non-raw image format. 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
Why QCOW1? (was: [PATCH v2] kvm tool: add QCOW verions 1 read/write support)
Kevin Wolf writes: > 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. As discussed already, reimplementing rather than sharing can be excused, because the existing code is not ready for sharing. What hasn't been discussed much is the other half of Kevin's remark: why QCOW1? Does anyone still use that old hag in anger? The format people actually use is QCOW2, and it is properly maintained. Even for little-used QOW formats, there are more interesting choices: QED and FVD. -- 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
Hi Christoph, 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 On Fri, Apr 15, 2011 at 6:24 AM, Christoph Hellwig wrote: > What's the point? Qcow1 has been deprecated for a long time. We're going to add QCOW2 too. Do you think there's no value in supporting QCOW1 and we should drop it completely? I have no objections in being QCOW2-only. 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
[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
---
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 m
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
Re: [PATCH] kvm tool: check the cluster boundary in the qcow read code.
On Thu, Apr 14, 2011 at 8:59 PM, Pekka Enberg 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: 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
[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
---
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
#include
-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
[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
Cc: Cyrill Gorcunov
Cc: Prasad Joshi
Cc: Sasha Levin
Cc: Ingo Molnar
Signed-off-by: Pekka Enberg
---
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 /* for BLKGETSIZE64 */
+
+#include
#include
#include
#include
@@ -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 tools: Add QCOW version 1 read-only support
From: Prasad Joshi
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
Cc: Cyrill Gorcunov
Cc: Ingo Molnar
Cc: Kevin Wolf
Cc: Sasha Levin
Cc: Stefan Hajnoczi
Signed-off-by: Prasad Joshi
Signed-off-by: Pekka Enberg
---
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
@@ -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
+
+#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
+#include
+
+#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
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+static inline uint64_t sect_to_l1_offset(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)
+{
+ struct qcow1_header *header = q->header;
+
+
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" > 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
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" > 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. Correct. I wonder whether we need to pass the relevant flag to vring_size. If we do we'll need to add a new function for that though as vring_size is exported to userspace. -- 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
Re: [PATCH 2/2 V2] kvm tools: Make host side IP configurable
On 04/14/2011 01:24 PM, Sasha Levin wrote:
> Add --host-ip-addr parameter to allow changing the host-side IP address.
> Add a networking group to the cmdline menu.
>
> Signed-off-by: Sasha Levin
> ---
> tools/kvm/include/kvm/virtio-net.h |2 +-
> tools/kvm/kvm-run.c| 15 ---
> tools/kvm/virtio-net.c |9 -
> 3 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/tools/kvm/include/kvm/virtio-net.h
> b/tools/kvm/include/kvm/virtio-net.h
> index a1cab15..9800a41 100644
> --- a/tools/kvm/include/kvm/virtio-net.h
> +++ b/tools/kvm/include/kvm/virtio-net.h
> @@ -2,6 +2,6 @@
> #define KVM__VIRTIO_NET_H
>
> struct kvm;
> -void virtio_net__init(struct kvm *self);
> +void virtio_net__init(struct kvm *self, const char *host_ip_addr);
>
> #endif /* KVM__VIRTIO_NET_H */
> diff --git a/tools/kvm/kvm-run.c b/tools/kvm/kvm-run.c
> index 6046a0a..d71057c 100644
> --- a/tools/kvm/kvm-run.c
> +++ b/tools/kvm/kvm-run.c
> @@ -31,6 +31,7 @@
> #define DEFAULT_KVM_DEV "/dev/kvm"
> #define DEFAULT_CONSOLE "serial"
> #define DEFAULT_NETWORK "none"
> +#define DEFAULT_HOST_ADDR"192.168.33.2"
>
> #define MB_SHIFT (20)
> #define MIN_RAM_SIZE_MB (64ULL)
> @@ -66,6 +67,7 @@ static const char *image_filename;
> static const char *console;
> static const char *kvm_dev;
> static const char *network;
> +static const char *host_ip_addr;
> static bool single_step;
> static bool readonly_image;
> extern bool ioport_debug;
> @@ -87,8 +89,6 @@ static const struct option options[] = {
> "Don't write changes back to disk image"),
> OPT_STRING('c', "console", &console, "serial or virtio",
> "Console to use"),
> - OPT_STRING('n', "network", &network, "virtio",
> - "Network to use"),
>
> OPT_GROUP("Kernel options:"),
> OPT_STRING('k', "kernel", &kernel_filename, "kernel",
> @@ -98,6 +98,12 @@ static const struct option options[] = {
> OPT_STRING('p', "params", &kernel_cmdline, "params",
> "Kernel command line arguments"),
>
> + OPT_GROUP("Networking options:"),
> + OPT_STRING('n', "network", &network, "virtio",
> + "Network to use"),
> + OPT_STRING('\0', "host-ip-addr", &host_ip_addr, "a.b.c.d",
> + "Assign this address to the host side networking"),
> +
> OPT_GROUP("Debug options:"),
> OPT_STRING('d', "kvm-dev", &kvm_dev, "kvm-dev", "KVM device file"),
> OPT_BOOLEAN('s', "single-step", &single_step,
> @@ -218,6 +224,9 @@ int kvm_cmd_run(int argc, const char **argv, const char
> *prefix)
> else
> active_console = CONSOLE_8250;
>
> + if (!host_ip_addr)
> + host_ip_addr = DEFAULT_HOST_ADDR;
> +
> term_init();
>
> kvm = kvm__init(kvm_dev, ram_size);
> @@ -259,7 +268,7 @@ int kvm_cmd_run(int argc, const char **argv, const char
> *prefix)
> network = DEFAULT_NETWORK;
>
> if (!strncmp(network, "virtio", 6))
> - virtio_net__init(kvm);
> + virtio_net__init(kvm, host_ip_addr);
>
> kvm__start_timer(kvm);
>
> diff --git a/tools/kvm/virtio-net.c b/tools/kvm/virtio-net.c
> index 622cfc6..90a6a17 100644
> --- a/tools/kvm/virtio-net.c
> +++ b/tools/kvm/virtio-net.c
> @@ -276,7 +276,7 @@ static struct pci_device_header virtio_net_pci_device = {
> .irq_line = VIRTIO_NET_IRQ,
> };
>
> -static void virtio_net__tap_init(void)
> +static void virtio_net__tap_init(const char *host_ip_addr)
> {
> struct ifreq ifr;
> int sock = socket(AF_INET, SOCK_STREAM, 0);
> @@ -301,8 +301,7 @@ static void virtio_net__tap_init(void)
>
> strncpy(ifr.ifr_name, net_device.tap_name, sizeof(net_device.tap_name));
>
> - /*FIXME: Remove this after user can specify ip address and netmask*/
> - sin.sin_addr.s_addr = inet_addr("192.168.33.2");
> + sin.sin_addr.s_addr = inet_addr(host_ip_addr);
> memcpy(&(ifr.ifr_addr), &sin, sizeof(ifr.ifr_addr));
> ifr.ifr_addr.sa_family = AF_INET;
>
> @@ -331,11 +330,11 @@ static void virtio_net__io_thread_init(struct kvm *self)
> pthread_create(&net_device.io_tx_thread, NULL, virtio_net_tx_thread,
> (void *)self);
> }
>
> -void virtio_net__init(struct kvm *self)
> +void virtio_net__init(struct kvm *self, const char *host_ip_addr)
> {
> pci__register(&virtio_net_pci_device, PCI_VIRTIO_NET_DEVNUM);
> ioport__register(IOPORT_VIRTIO_NET, &virtio_net_io_ops,
> IOPORT_VIRTIO_NET_SIZE);
>
> - virtio_net__tap_init();
> + virtio_net__tap_init(host_ip_addr);
> virtio_net__io_thread_init(self);
> }
Looks good to me.
Reviewed-by: Asias He
--
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 h
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
> ---
> 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
> #include
> #include
> +#include
> +#include
> +#include
>
> #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
--
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: Network performance with small packets
On Tue, 12 Apr 2011 23:01:12 +0300, "Michael S. Tsirkin" 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] 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: [PATCH v2] kvm tool: add QCOW verions 1 read/write support
Kevin Wolf writes: > Am 14.04.2011 11:26, schrieb Markus Armbruster: >> Kevin Wolf 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 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 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: [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 > wrote: > > On Tue, 12 Apr 2011 21:31:18 +0300 > > Blue Swirl wrote: > > > >> On Tue, Apr 12, 2011 at 10:52 AM, Avi Kivity wrote: > >> > On 04/11/2011 08:15 PM, Blue Swirl wrote: > >> >> > >> >> On Mon, Apr 11, 2011 at 10:01 AM, Markus Armbruster > >> >> wrote: > >> >> > Avi Kivity 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] 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) 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: [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 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 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 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 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
Am 14.04.2011 11:26, schrieb Markus Armbruster: > Kevin Wolf 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 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 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
* Prasad Joshi 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
Kevin Wolf 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 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 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
On Thu, Apr 14, 2011 at 9:27 AM, Ingo Molnar wrote: > > * Kevin Wolf wrote: > > > Am 14.04.2011 10:15, schrieb Pekka Enberg: > > > * Kevin Wolf 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 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
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 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 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 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 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 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
Hi Kevin! Am 14.04.2011 10:21, schrieb Pekka Enberg: >> On Thu, Apr 14, 2011 at 11:02 AM, Kevin Wolf 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 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
Am 14.04.2011 10:21, schrieb Pekka Enberg: > On Thu, Apr 14, 2011 at 11:02 AM, Kevin Wolf 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
* Kevin Wolf wrote: > Am 14.04.2011 10:15, schrieb Pekka Enberg: > > * Kevin Wolf 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 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
* Kevin Wolf wrote: > Am 14.04.2011 10:07, schrieb Ingo Molnar: > > > > * Kevin Wolf 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
Am 14.04.2011 10:15, schrieb Pekka Enberg: > * Kevin Wolf 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 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
On Thu, Apr 14, 2011 at 11:02 AM, Kevin Wolf 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 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 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
* Kevin Wolf 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 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
Am 14.04.2011 10:07, schrieb Ingo Molnar: > > * Kevin Wolf 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 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 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] 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: [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 > --- > 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
