Re: [Qemu-devel] Re: [PATCH v5] virtio-9p: fix build on !CONFIG_UTIMENSAT
Ping. Maintainers, please tell me if still something is required for this patch before applying it. Thanks, H.Seto (2010/11/24 17:01), Jes Sorensen wrote: > On 11/24/10 03:38, Hidetoshi Seto wrote: >> This patch introduce a fallback mechanism for old systems that do not >> support utimensat(). This fix build failure with following warnings: >> >> hw/virtio-9p-local.c: In function 'local_utimensat': >> hw/virtio-9p-local.c:479: warning: implicit declaration of function >> 'utimensat' >> hw/virtio-9p-local.c:479: warning: nested extern declaration of 'utimensat' >> >> and: >> >> hw/virtio-9p.c: In function 'v9fs_setattr_post_chmod': >> hw/virtio-9p.c:1410: error: 'UTIME_NOW' undeclared (first use in this >> function) >> hw/virtio-9p.c:1410: error: (Each undeclared identifier is reported only once >> hw/virtio-9p.c:1410: error: for each function it appears in.) >> hw/virtio-9p.c:1413: error: 'UTIME_OMIT' undeclared (first use in this >> function) >> hw/virtio-9p.c: In function 'v9fs_wstat_post_chmod': >> hw/virtio-9p.c:2905: error: 'UTIME_OMIT' undeclared (first use in this >> function) >> >> [NOTE: At this time virtio-9p is only user of utimensat(), and is available >>only when host is linux and CONFIG_VIRTFS is defined. So there are >>no similar warning for win32. Please provide a wrapper for win32 in >>oslib-win32.c if new user really requires it.] >> >> v5: >> - Allow fallback on runtime >> - Move qemu_utimensat() to oslib-posix.c >> - Rebased on latest qemu.git >> v4: >> - Use tv_now.tv_usec >> v3: >> - Use better alternative handling for UTIME_NOW/OMIT >> - Move qemu_utimensat() to cutils.c >> V2: >> - Introduce qemu_utimensat() >> >> Acked-by: Chris Wright >> Acked-by: M. Mohan Kumar >> Signed-off-by: Hidetoshi Seto >> --- >> hw/virtio-9p-local.c |4 ++-- >> oslib-posix.c| 48 >> qemu-os-posix.h | 12 >> 3 files changed, 62 insertions(+), 2 deletions(-) > > Hi Hidetoshi, > > This looks good to me! > > Acked-by: Jes Sorensen > > Cheers, > Jes -- 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 v5] virtio-9p: fix build on !CONFIG_UTIMENSAT
This patch introduce a fallback mechanism for old systems that do not
support utimensat(). This fix build failure with following warnings:
hw/virtio-9p-local.c: In function 'local_utimensat':
hw/virtio-9p-local.c:479: warning: implicit declaration of function 'utimensat'
hw/virtio-9p-local.c:479: warning: nested extern declaration of 'utimensat'
and:
hw/virtio-9p.c: In function 'v9fs_setattr_post_chmod':
hw/virtio-9p.c:1410: error: 'UTIME_NOW' undeclared (first use in this function)
hw/virtio-9p.c:1410: error: (Each undeclared identifier is reported only once
hw/virtio-9p.c:1410: error: for each function it appears in.)
hw/virtio-9p.c:1413: error: 'UTIME_OMIT' undeclared (first use in this function)
hw/virtio-9p.c: In function 'v9fs_wstat_post_chmod':
hw/virtio-9p.c:2905: error: 'UTIME_OMIT' undeclared (first use in this function)
[NOTE: At this time virtio-9p is only user of utimensat(), and is available
only when host is linux and CONFIG_VIRTFS is defined. So there are
no similar warning for win32. Please provide a wrapper for win32 in
oslib-win32.c if new user really requires it.]
v5:
- Allow fallback on runtime
- Move qemu_utimensat() to oslib-posix.c
- Rebased on latest qemu.git
v4:
- Use tv_now.tv_usec
v3:
- Use better alternative handling for UTIME_NOW/OMIT
- Move qemu_utimensat() to cutils.c
V2:
- Introduce qemu_utimensat()
Acked-by: Chris Wright
Acked-by: M. Mohan Kumar
Signed-off-by: Hidetoshi Seto
---
hw/virtio-9p-local.c |4 ++--
oslib-posix.c| 48
qemu-os-posix.h | 12
3 files changed, 62 insertions(+), 2 deletions(-)
diff --git a/hw/virtio-9p-local.c b/hw/virtio-9p-local.c
index 0d52020..41603ea 100644
--- a/hw/virtio-9p-local.c
+++ b/hw/virtio-9p-local.c
@@ -480,9 +480,9 @@ static int local_chown(FsContext *fs_ctx, const char *path,
FsCred *credp)
}
static int local_utimensat(FsContext *s, const char *path,
- const struct timespec *buf)
+ const struct timespec *buf)
{
-return utimensat(AT_FDCWD, rpath(s, path), buf, AT_SYMLINK_NOFOLLOW);
+return qemu_utimensat(AT_FDCWD, rpath(s, path), buf, AT_SYMLINK_NOFOLLOW);
}
static int local_remove(FsContext *ctx, const char *path)
diff --git a/oslib-posix.c b/oslib-posix.c
index 727dee1..edd4ddf 100644
--- a/oslib-posix.c
+++ b/oslib-posix.c
@@ -111,3 +111,51 @@ int qemu_pipe(int pipefd[2])
return ret;
}
+
+int qemu_utimensat(int dirfd, const char *path, const struct timespec *times,
+ int flags)
+{
+struct timeval tv[2], tv_now;
+struct stat st;
+int i;
+#ifdef CONFIG_UTIMENSAT
+int ret;
+
+ret = utimensat(dirfd, path, times, flags);
+if (ret != -1 || errno != ENOSYS) {
+return ret;
+}
+#endif
+/* Fallback: use utimes() instead of utimensat() */
+
+/* happy if special cases */
+if (times[0].tv_nsec == UTIME_OMIT && times[1].tv_nsec == UTIME_OMIT) {
+return 0;
+}
+if (times[0].tv_nsec == UTIME_NOW && times[1].tv_nsec == UTIME_NOW) {
+return utimes(path, NULL);
+}
+
+/* prepare for hard cases */
+if (times[0].tv_nsec == UTIME_NOW || times[1].tv_nsec == UTIME_NOW) {
+gettimeofday(&tv_now, NULL);
+}
+if (times[0].tv_nsec == UTIME_OMIT || times[1].tv_nsec == UTIME_OMIT) {
+stat(path, &st);
+}
+
+for (i = 0; i < 2; i++) {
+if (times[i].tv_nsec == UTIME_NOW) {
+tv[i].tv_sec = tv_now.tv_sec;
+tv[i].tv_usec = tv_now.tv_usec;
+} else if (times[i].tv_nsec == UTIME_OMIT) {
+tv[i].tv_sec = (i == 0) ? st.st_atime : st.st_mtime;
+tv[i].tv_usec = 0;
+} else {
+tv[i].tv_sec = times[i].tv_sec;
+tv[i].tv_usec = times[i].tv_nsec / 1000;
+}
+}
+
+return utimes(path, &tv[0]);
+}
diff --git a/qemu-os-posix.h b/qemu-os-posix.h
index 353f878..81fd9ab 100644
--- a/qemu-os-posix.h
+++ b/qemu-os-posix.h
@@ -39,4 +39,16 @@ void os_setup_post(void);
typedef struct timeval qemu_timeval;
#define qemu_gettimeofday(tp) gettimeofday(tp, NULL)
+#ifndef CONFIG_UTIMENSAT
+#ifndef UTIME_NOW
+# define UTIME_NOW ((1l << 30) - 1l)
+#endif
+#ifndef UTIME_OMIT
+# define UTIME_OMIT((1l << 30) - 2l)
+#endif
+#endif
+typedef struct timespec qemu_timespec;
+int qemu_utimensat(int dirfd, const char *path, const qemu_timespec *times,
+int flags);
+
#endif
--
1.6.5.2
--
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 v4] virtio-9p: fix build on !CONFIG_UTIMENSAT
(2010/11/18 17:28), Philipp Hahn wrote:
> Hello,
>
> Am Donnerstag 18 November 2010 01:41:39 schrieb Hidetoshi Seto:
>> This patch introduce a fallback mechanism for old systems that do not
>> support utimensat(). This fix build failure with following warnings:
>
>> +#ifdef CONFIG_UTIMENSAT
>> +return utimensat(dirfd, path, times, flags);
>> +#else
>> +/* Fallback: use utimes() instead of utimensat() */
>
> Since we also had a problem with utimestat() some time ago with Samba
> <http://lists.samba.org/archive/samba-technical/2010-November/074613.html>
> I'd like to comment on that:
>
> Your have to be careful about compile-time-detection and runtime-detection:
> If
> you later run your utimestat()-enabled binary on an older kernel not
> supporting that syscall, you get -1 as the return-value and errno=ENOSYS. So
> even if you detected utimesatat() during compile-time, please always provide
> a fallback for run-time.
> This is less important for people compiling there own version of kvm, but is
> essential for Linux distributions, since people often run newer kvm versions
> on older kernels.
Hum, you have a good point.
Well, then I'll change it like:
-#ifdef CONFIG_UTIMENSAT
-return utimensat(dirfd, path, times, flags);
-#else
+{
+int ret = utimensat(dirfd, path, times, flags);
+if (ret != -1 || errno != ENOSYS) {
+return ret;
+}
+}
Thanks,
H.Seto
--
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 v4] virtio-9p: fix build on !CONFIG_UTIMENSAT
(2010/11/18 17:02), Jes Sorensen wrote: > On 11/18/10 01:41, Hidetoshi Seto wrote: >> This patch introduce a fallback mechanism for old systems that do not >> support utimensat(). This fix build failure with following warnings: >> >> hw/virtio-9p-local.c: In function 'local_utimensat': >> hw/virtio-9p-local.c:479: warning: implicit declaration of function >> 'utimensat' >> hw/virtio-9p-local.c:479: warning: nested extern declaration of 'utimensat' >> >> and: >> >> hw/virtio-9p.c: In function 'v9fs_setattr_post_chmod': >> hw/virtio-9p.c:1410: error: 'UTIME_NOW' undeclared (first use in this >> function) >> hw/virtio-9p.c:1410: error: (Each undeclared identifier is reported only once >> hw/virtio-9p.c:1410: error: for each function it appears in.) >> hw/virtio-9p.c:1413: error: 'UTIME_OMIT' undeclared (first use in this >> function) >> hw/virtio-9p.c: In function 'v9fs_wstat_post_chmod': >> hw/virtio-9p.c:2905: error: 'UTIME_OMIT' undeclared (first use in this >> function) >> >> v4: >> - Use tv_now.tv_usec >> - Rebased on latest qemu.git >> v3: >> - Use better alternative handling for UTIME_NOW/OMIT >> - Move qemu_utimensat() to cutils.c >> V2: >> - Introduce qemu_utimensat() >> >> Acked-by: Chris Wright >> Acked-by: M. Mohan Kumar >> Signed-off-by: Hidetoshi Seto > > Hi Hidetoshi, > > I think the idea of the patch is good, but please move qemu_utimensat() > to oslib-posix.c and provide a wrapper for oslib-win32.c. It is > emulation for a system library function, so it doesn't belong in > cutils.c, but rather in the oslib group. Unfortunately one fact is that I'm not familiar with win32 codes so I don't have any idea how the wrapper for win32 will be... If someone could kindly tell me about the win32 part, I could update this patch to v5, but even though I have no test environment for the new part :-< Could we wait an incremental patch on this v4? Can somebody help me? Volunteers? Thanks, H.Seto -- 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 v4] virtio-9p: fix build on !CONFIG_UTIMENSAT
This patch introduce a fallback mechanism for old systems that do not
support utimensat(). This fix build failure with following warnings:
hw/virtio-9p-local.c: In function 'local_utimensat':
hw/virtio-9p-local.c:479: warning: implicit declaration of function 'utimensat'
hw/virtio-9p-local.c:479: warning: nested extern declaration of 'utimensat'
and:
hw/virtio-9p.c: In function 'v9fs_setattr_post_chmod':
hw/virtio-9p.c:1410: error: 'UTIME_NOW' undeclared (first use in this function)
hw/virtio-9p.c:1410: error: (Each undeclared identifier is reported only once
hw/virtio-9p.c:1410: error: for each function it appears in.)
hw/virtio-9p.c:1413: error: 'UTIME_OMIT' undeclared (first use in this function)
hw/virtio-9p.c: In function 'v9fs_wstat_post_chmod':
hw/virtio-9p.c:2905: error: 'UTIME_OMIT' undeclared (first use in this function)
v4:
- Use tv_now.tv_usec
- Rebased on latest qemu.git
v3:
- Use better alternative handling for UTIME_NOW/OMIT
- Move qemu_utimensat() to cutils.c
V2:
- Introduce qemu_utimensat()
Acked-by: Chris Wright
Acked-by: M. Mohan Kumar
Signed-off-by: Hidetoshi Seto
---
cutils.c | 44
hw/virtio-9p-local.c |4 ++--
qemu-common.h| 10 ++
3 files changed, 56 insertions(+), 2 deletions(-)
diff --git a/cutils.c b/cutils.c
index 28089aa..dbeb3f2 100644
--- a/cutils.c
+++ b/cutils.c
@@ -371,3 +371,47 @@ fail:
return retval;
}
+
+int qemu_utimensat(int dirfd, const char *path, const struct timespec *times,
+ int flags)
+{
+#ifdef CONFIG_UTIMENSAT
+return utimensat(dirfd, path, times, flags);
+#else
+/* Fallback: use utimes() instead of utimensat() */
+struct timeval tv[2], tv_now;
+struct stat st;
+int i;
+
+/* happy if special cases */
+if (times[0].tv_nsec == UTIME_OMIT && times[1].tv_nsec == UTIME_OMIT) {
+return 0;
+}
+if (times[0].tv_nsec == UTIME_NOW && times[1].tv_nsec == UTIME_NOW) {
+return utimes(path, NULL);
+}
+
+/* prepare for hard cases */
+if (times[0].tv_nsec == UTIME_NOW || times[1].tv_nsec == UTIME_NOW) {
+gettimeofday(&tv_now, NULL);
+}
+if (times[0].tv_nsec == UTIME_OMIT || times[1].tv_nsec == UTIME_OMIT) {
+stat(path, &st);
+}
+
+for (i = 0; i < 2; i++) {
+if (times[i].tv_nsec == UTIME_NOW) {
+tv[i].tv_sec = tv_now.tv_sec;
+tv[i].tv_usec = tv_now.tv_usec;
+} else if (times[i].tv_nsec == UTIME_OMIT) {
+tv[i].tv_sec = (i == 0) ? st.st_atime : st.st_mtime;
+tv[i].tv_usec = 0;
+} else {
+tv[i].tv_sec = times[i].tv_sec;
+tv[i].tv_usec = times[i].tv_nsec / 1000;
+}
+}
+
+return utimes(path, &tv[0]);
+#endif
+}
diff --git a/hw/virtio-9p-local.c b/hw/virtio-9p-local.c
index 0d52020..41603ea 100644
--- a/hw/virtio-9p-local.c
+++ b/hw/virtio-9p-local.c
@@ -480,9 +480,9 @@ static int local_chown(FsContext *fs_ctx, const char *path,
FsCred *credp)
}
static int local_utimensat(FsContext *s, const char *path,
- const struct timespec *buf)
+ const struct timespec *buf)
{
-return utimensat(AT_FDCWD, rpath(s, path), buf, AT_SYMLINK_NOFOLLOW);
+return qemu_utimensat(AT_FDCWD, rpath(s, path), buf, AT_SYMLINK_NOFOLLOW);
}
static int local_remove(FsContext *ctx, const char *path)
diff --git a/qemu-common.h b/qemu-common.h
index b3957f1..f0b2c9d 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -150,6 +150,16 @@ int qemu_fls(int i);
int qemu_fdatasync(int fd);
int fcntl_setfl(int fd, int flag);
ssize_t strtosz(const char *nptr, char **end);
+#ifndef CONFIG_UTIMENSAT
+#ifndef UTIME_NOW
+# define UTIME_NOW ((1l << 30) - 1l)
+#endif
+#ifndef UTIME_OMIT
+# define UTIME_OMIT((1l << 30) - 2l)
+#endif
+#endif
+int qemu_utimensat(int dirfd, const char *path, const struct timespec *times,
+int flags);
/* path.c */
void init_paths(const char *prefix);
--
1.7.3.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
[PATCH v3] virtio-9p: fix build on !CONFIG_UTIMENSAT
This patch introduce a fallback mechanism for old systems that do not
support utimensat(). This fix build failure with following warnings:
hw/virtio-9p-local.c: In function 'local_utimensat':
hw/virtio-9p-local.c:479: warning: implicit declaration of function 'utimensat'
hw/virtio-9p-local.c:479: warning: nested extern declaration of 'utimensat'
and:
hw/virtio-9p.c: In function 'v9fs_setattr_post_chmod':
hw/virtio-9p.c:1410: error: 'UTIME_NOW' undeclared (first use in this function)
hw/virtio-9p.c:1410: error: (Each undeclared identifier is reported only once
hw/virtio-9p.c:1410: error: for each function it appears in.)
hw/virtio-9p.c:1413: error: 'UTIME_OMIT' undeclared (first use in this function)
hw/virtio-9p.c: In function 'v9fs_wstat_post_chmod':
hw/virtio-9p.c:2905: error: 'UTIME_OMIT' undeclared (first use in this function)
v3:
- Use better alternative handling for UTIME_NOW/OMIT
- Move qemu_utimensat() to cutils.c
V2:
- Introduce qemu_utimensat()
Signed-off-by: Hidetoshi Seto
---
cutils.c | 43 +++
hw/virtio-9p-local.c |4 ++--
qemu-common.h| 10 ++
3 files changed, 55 insertions(+), 2 deletions(-)
diff --git a/cutils.c b/cutils.c
index 536ee93..3c18941 100644
--- a/cutils.c
+++ b/cutils.c
@@ -288,3 +288,46 @@ int fcntl_setfl(int fd, int flag)
}
#endif
+int qemu_utimensat(int dirfd, const char *path, const struct timespec *times,
+ int flags)
+{
+#ifdef CONFIG_UTIMENSAT
+return utimensat(dirfd, path, times, flags);
+#else
+/* Fallback: use utimes() instead of utimensat() */
+struct timeval tv[2], tv_now;
+struct stat st;
+int i;
+
+/* happy if special cases */
+if (times[0].tv_nsec == UTIME_OMIT && times[1].tv_nsec == UTIME_OMIT) {
+return 0;
+}
+if (times[0].tv_nsec == UTIME_NOW && times[1].tv_nsec == UTIME_NOW) {
+return utimes(path, NULL);
+}
+
+/* prepare for hard cases */
+if (times[0].tv_nsec == UTIME_NOW || times[1].tv_nsec == UTIME_NOW) {
+gettimeofday(&tv_now, NULL);
+}
+if (times[0].tv_nsec == UTIME_OMIT || times[1].tv_nsec == UTIME_OMIT) {
+stat(path, &st);
+}
+
+for (i = 0; i < 2; i++) {
+if (times[i].tv_nsec == UTIME_NOW) {
+tv[i].tv_sec = tv_now.tv_sec;
+tv[i].tv_usec = 0;
+} else if (times[i].tv_nsec == UTIME_OMIT) {
+tv[i].tv_sec = (i == 0) ? st.st_atime : st.st_mtime;
+tv[i].tv_usec = 0;
+} else {
+tv[i].tv_sec = times[i].tv_sec;
+tv[i].tv_usec = times[i].tv_nsec / 1000;
+}
+}
+
+return utimes(path, &tv[0]);
+#endif
+}
diff --git a/hw/virtio-9p-local.c b/hw/virtio-9p-local.c
index 0d52020..41603ea 100644
--- a/hw/virtio-9p-local.c
+++ b/hw/virtio-9p-local.c
@@ -480,9 +480,9 @@ static int local_chown(FsContext *fs_ctx, const char *path,
FsCred *credp)
}
static int local_utimensat(FsContext *s, const char *path,
- const struct timespec *buf)
+ const struct timespec *buf)
{
-return utimensat(AT_FDCWD, rpath(s, path), buf, AT_SYMLINK_NOFOLLOW);
+return qemu_utimensat(AT_FDCWD, rpath(s, path), buf, AT_SYMLINK_NOFOLLOW);
}
static int local_remove(FsContext *ctx, const char *path)
diff --git a/qemu-common.h b/qemu-common.h
index 2fbc27f..7fe4c16 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -146,6 +146,16 @@ time_t mktimegm(struct tm *tm);
int qemu_fls(int i);
int qemu_fdatasync(int fd);
int fcntl_setfl(int fd, int flag);
+#ifndef CONFIG_UTIMENSAT
+#ifndef UTIME_NOW
+# define UTIME_NOW ((1l << 30) - 1l)
+#endif
+#ifndef UTIME_OMIT
+# define UTIME_OMIT((1l << 30) - 2l)
+#endif
+#endif
+int qemu_utimensat(int dirfd, const char *path, const struct timespec *times,
+int flags);
/* path.c */
void init_paths(const char *prefix);
--
1.7.3.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: [Qemu-devel] Re: [PATCH] virtio-9p: fix build on !CONFIG_UTIMENSAT v2
(2010/11/14 14:58), Chris Wright wrote:
> * Hidetoshi Seto (seto.hideto...@jp.fujitsu.com) wrote:
>> +/*
>> + * Fallback: use utimes() instead of utimensat().
>> + * See commit 74bc02b2d2272dc88fb98d43e631eb154717f517 for known
>> problem.
>> + */
>> +struct timeval tv[2];
>> +int i;
>> +
>> +for (i = 0; i < 2; i++) {
>> +if (times[i].tv_nsec == UTIME_OMIT || times[i].tv_nsec ==
>> UTIME_NOW) {
>> +tv[i].tv_sec = 0;
>> +tv[i].tv_usec = 0;
>
> I don't think this is accurate in either case. It will set the
> atime, mtime, or both to 0.
>
> For UTIME_NOW (in both) we'd simply pass NULL to utimes(2). For
> UTIME_OMIT (in both) we'd simply skip the call to utimes(2) altogether.
>
> The harder part is a mixed mode (i.e. the truncate fix mentioned in the
> above commit). I think the only way to handle UTIME_NOW in one is to
> call gettimeofday (or clock_gettime for better resolution) to find out
> what current time is. And for UTIME_OMIT call stat to find out what the
> current setting is and reset to that value. Both of those cases can
> possibly zero out the extra precision (providing only seconds
> resolution).
Thank you for comments!
I'll post an updated one soon.
Thanks,
H.Seto
--
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] virtio-9p: fix build on !CONFIG_UTIMENSAT v2
This patch introduce a fallback mechanism for old systems that do not
support utimensat. This will fix build failure with following warnings:
hw/virtio-9p-local.c: In function 'local_utimensat':
hw/virtio-9p-local.c:479: warning: implicit declaration of function 'utimensat'
hw/virtio-9p-local.c:479: warning: nested extern declaration of 'utimensat'
and
hw/virtio-9p.c: In function 'v9fs_setattr_post_chmod':
hw/virtio-9p.c:1410: error: 'UTIME_NOW' undeclared (first use in this function)
hw/virtio-9p.c:1410: error: (Each undeclared identifier is reported only once
hw/virtio-9p.c:1410: error: for each function it appears in.)
hw/virtio-9p.c:1413: error: 'UTIME_OMIT' undeclared (first use in this function)
hw/virtio-9p.c: In function 'v9fs_wstat_post_chmod':
hw/virtio-9p.c:2905: error: 'UTIME_OMIT' undeclared (first use in this function)
Signed-off-by: Hidetoshi Seto
---
hw/virtio-9p-local.c | 32 ++--
hw/virtio-9p.h | 10 ++
2 files changed, 40 insertions(+), 2 deletions(-)
diff --git a/hw/virtio-9p-local.c b/hw/virtio-9p-local.c
index 0d52020..7811d2c 100644
--- a/hw/virtio-9p-local.c
+++ b/hw/virtio-9p-local.c
@@ -479,10 +479,38 @@ static int local_chown(FsContext *fs_ctx, const char
*path, FsCred *credp)
return -1;
}
+/* TODO: relocate this to proper file, and make it more generic */
+static int qemu_utimensat(int dirfd, const char *path,
+ const struct timespec *times, int flags)
+{
+#ifdef CONFIG_UTIMENSAT
+return utimensat(dirfd, path, times, flags);
+#else
+/*
+ * Fallback: use utimes() instead of utimensat().
+ * See commit 74bc02b2d2272dc88fb98d43e631eb154717f517 for known problem.
+ */
+struct timeval tv[2];
+int i;
+
+for (i = 0; i < 2; i++) {
+if (times[i].tv_nsec == UTIME_OMIT || times[i].tv_nsec == UTIME_NOW) {
+tv[i].tv_sec = 0;
+tv[i].tv_usec = 0;
+} else {
+tv[i].tv_sec = times[i].tv_sec;
+tv[i].tv_usec = times[i].tv_nsec / 1000;
+}
+}
+
+return utimes(path, &tv[0]);
+#endif
+}
+
static int local_utimensat(FsContext *s, const char *path,
- const struct timespec *buf)
+ const struct timespec *buf)
{
-return utimensat(AT_FDCWD, rpath(s, path), buf, AT_SYMLINK_NOFOLLOW);
+return qemu_utimensat(AT_FDCWD, rpath(s, path), buf, AT_SYMLINK_NOFOLLOW);
}
static int local_remove(FsContext *ctx, const char *path)
diff --git a/hw/virtio-9p.h b/hw/virtio-9p.h
index 6c23319..d448d8a 100644
--- a/hw/virtio-9p.h
+++ b/hw/virtio-9p.h
@@ -8,6 +8,16 @@
#include "file-op-9p.h"
+/* TODO: relocate this to proper file */
+#ifndef CONFIG_UTIMENSAT
+#ifndef UTIME_NOW
+# define UTIME_NOW ((1l << 30) - 1l)
+#endif
+#ifndef UTIME_OMIT
+# define UTIME_OMIT((1l << 30) - 2l)
+#endif
+#endif
+
/* The feature bitmap for virtio 9P */
/* The mount point is specified in a config variable */
#define VIRTIO_9P_MOUNT_TAG 0
--
1.7.3.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: [Qemu-devel] qemu-kvm build issue on RHEL5.1
(2010/11/05 2:03), Chris Wright wrote: > * Hidetoshi Seto (seto.hideto...@jp.fujitsu.com) wrote: >> (2010/10/14 4:11), Blue Swirl wrote: >>> On Wed, Oct 13, 2010 at 8:00 AM, Hidetoshi Seto >>> wrote: >>>> (Add CC to k...@vger) >>>> >>>> (2010/10/12 10:52), Hao, Xudong wrote: >>>>> Hi, >>>>> Currently qemu-kvm build fail on RHEL5 with gcc 4.1.2, build can pass on >>>>> Fedora11 with gcc 4.4.1, can anybody look on RHEL5 system? >>>>> >>>>> Gcc: 4.1.2 >>>>> system: RHEL5.1 >>>>> qemu-kvm: 85566812a4f8cae721fea0224e05a7e75c08c5dd >>>>> >>>>> ... >>>>> LINK qemu-img >>>>> LINK qemu-io >>>>> CClibhw64/virtio-9p-local.o >>>>> cc1: warnings being treated as errors >>>>> /home/source/qemu-kvm/hw/virtio-9p-local.c: In function 'local_utimensat': >>>>> /home/source/qemu-kvm/hw/virtio-9p-local.c:479: warning: implicit >>>>> declaration of function 'utimensat' >>>>> /home/source/qemu-kvm/hw/virtio-9p-local.c:479: warning: nested extern >>>>> declaration of 'utimensat' >>>>> make[1]: *** [virtio-9p-local.o] Error 1 >>>>> make: *** [subdir-libhw64] Error 2 >>>>> >>>>> >>>>> Best Regards, >>>>> Xudong Hao >>>> >>>> It seems that this issue is caused by the old glibc. >>>> Though I don't know well about virtio-9p and suppose there >>>> should be better fix, I confirmed that following change >>>> removed the warnings. >>> >>> But then the system call will be made blindly without checking if the >>> kernel supports utimensat(). At the minimum, there should be a sane >>> response to ENOSYS error. >> >> Yes. But I'm not sure how this virtio-9p should behave if there is >> no utimensat. I think it will be better to fix this warning first >> to allow fellows using RHEL5 to restart contribute on qemu-kvm, >> and change this issue to virtio-9p specific problem to allow >> specialists of virtio-9p to have discussion for fix without >> bothering other developers. > > One way to workaround this is to simply not install libattr-devel > (effecitvely disabling virtio-9p). > > But I agree with Blue Swirl, need a better fallback plan. A qemu local > implementation of something like qemu_utimensat() that simply uses > glibc/kernel interface when available and falls back to using utimes() > makes sense to me. Then the worst case is loss of resolution from ns to > us. According to the commit 74bc02b2d2272dc88fb98d43e631eb154717f517, the title "Do not reset atime" can tell us that the original motivation to use utimensat() is not for the resolution. Anyway, I agree to have something like qemu_utimensat(). I made a patch for the first step, and will post it next to this reply. Thanks, H.Seto -- 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/5] PCI: Add mask bit definition for MSI-X table
(2010/11/04 15:15), Sheng Yang wrote:
> Then we can use it instead of magic number 1.
>
> Cc: Matthew Wilcox
> Cc: Jesse Barnes
> Cc: linux-...@vger.kernel.org
> Signed-off-by: Sheng Yang
> ---
> drivers/pci/msi.c|4 ++--
> include/linux/pci_regs.h |1 +
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 69b7be3..673e7dc 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -158,7 +158,7 @@ static u32 __msix_mask_irq(struct msi_desc *desc, u32
> flag)
> u32 mask_bits = desc->masked;
> unsigned offset = desc->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE +
> PCI_MSIX_ENTRY_VECTOR_CTRL;
> - mask_bits &= ~1;
> + mask_bits &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
> mask_bits |= flag;
> writel(mask_bits, desc->mask_base + offset);
>
> @@ -185,7 +185,7 @@ static void msi_set_mask_bit(unsigned irq, u32 flag)
>
> void mask_msi_irq(unsigned int irq)
> {
> - msi_set_mask_bit(irq, 1);
> + msi_set_mask_bit(irq, PCI_MSIX_ENTRY_CTRL_MASKBIT);
> }
>
> void unmask_msi_irq(unsigned int irq)
This hunk is not good, because the function msi_set_mask_bit() is
not only for MSI-X but also for MSI, so the number 0/1 here works
as like as false/true:
static void msi_set_mask_bit(struct irq_data *data, u32 flag)
{
struct msi_desc *desc = irq_data_get_msi(data);
if (desc->msi_attrib.is_msix) {
msix_mask_irq(desc, flag);
readl(desc->mask_base); /* Flush write to device */
} else {
unsigned offset = data->irq - desc->dev->irq;
msi_mask_irq(desc, 1 << offset, flag << offset);
}
}
> diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
> index acfc224..ff51632 100644
> --- a/include/linux/pci_regs.h
> +++ b/include/linux/pci_regs.h
> @@ -313,6 +313,7 @@
> #define PCI_MSIX_ENTRY_UPPER_ADDR 4
> #define PCI_MSIX_ENTRY_DATA 8
> #define PCI_MSIX_ENTRY_VECTOR_CTRL 12
> +#define PCI_MSIX_ENTRY_CTRL_MASKBIT1
>
> /* CompactPCI Hotswap Register */
>
So I recommend you to have a patch with the above hunk for header
plus a change like:
- mask_bits &= ~1;
- mask_bits |= flag;
+ mask_bits &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
+ mask_bits |= PCI_MSIX_ENTRY_CTRL_MASKBIT & !!flag;
Anyway thank you very much for doing this work.
Reviewed-by: Hidetoshi Seto
Thanks,
H.Seto
--
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/5] PCI: MSI: Move MSI-X entry definition to pci_regs.h
Yeah, I think there are many virtualization stuff awaiting this change in these days. Reviewed-by: Hidetoshi Seto (2010/11/04 15:15), Sheng Yang wrote: > Then it can be used by others. > > Reviewed-by: Matthew Wilcox > Cc: Jesse Barnes > Cc: linux-...@vger.kernel.org > Signed-off-by: Sheng Yang > --- > drivers/pci/msi.h|6 -- > include/linux/pci_regs.h |7 +++ > 2 files changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/msi.h b/drivers/pci/msi.h > index de27c1c..28a3c52 100644 > --- a/drivers/pci/msi.h > +++ b/drivers/pci/msi.h > @@ -6,12 +6,6 @@ > #ifndef MSI_H > #define MSI_H > > -#define PCI_MSIX_ENTRY_SIZE 16 > -#define PCI_MSIX_ENTRY_LOWER_ADDR 0 > -#define PCI_MSIX_ENTRY_UPPER_ADDR 4 > -#define PCI_MSIX_ENTRY_DATA 8 > -#define PCI_MSIX_ENTRY_VECTOR_CTRL 12 > - > #define msi_control_reg(base)(base + PCI_MSI_FLAGS) > #define msi_lower_address_reg(base) (base + PCI_MSI_ADDRESS_LO) > #define msi_upper_address_reg(base) (base + PCI_MSI_ADDRESS_HI) > diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h > index 455b9cc..acfc224 100644 > --- a/include/linux/pci_regs.h > +++ b/include/linux/pci_regs.h > @@ -307,6 +307,13 @@ > #define PCI_MSIX_FLAGS_MASKALL (1 << 14) > #define PCI_MSIX_FLAGS_BIRMASK (7 << 0) > > +/* MSI-X entry's format */ > +#define PCI_MSIX_ENTRY_SIZE 16 > +#define PCI_MSIX_ENTRY_LOWER_ADDR 0 > +#define PCI_MSIX_ENTRY_UPPER_ADDR 4 > +#define PCI_MSIX_ENTRY_DATA 8 > +#define PCI_MSIX_ENTRY_VECTOR_CTRL 12 > + > /* CompactPCI Hotswap Register */ > > #define PCI_CHSWP_CSR2 /* Control and Status Register > */ -- 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] x86, mce: broadcast mce depending on the cpu version
There is no reason why SRAO event received by the main thread
is the only one that being broadcasted.
According to the x86 ASDM vol.3A 15.10.4.1,
MCE signal is broadcast on processor version 06H_EH or later.
This change is required to handle SRAR in smp guests.
Signed-off-by: Hidetoshi Seto
---
target-i386/kvm.c | 29 -
1 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index a0d0603..00bb083 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1637,6 +1637,28 @@ static void hardware_memory_error(void)
exit(1);
}
+#ifdef KVM_CAP_MCE
+static void kvm_mce_broadcast_rest(CPUState *env)
+{
+CPUState *cenv;
+int family, model, cpuver = env->cpuid_version;
+
+family = (cpuver >> 8) & 0xf;
+model = ((cpuver >> 12) & 0xf0) + ((cpuver >> 4) & 0xf);
+
+/* Broadcast MCA signal for processor version 06H_EH and above */
+if ((family == 6 && model >= 14) || family > 6) {
+if (cenv == env) {
+continue;
+}
+for (cenv = first_cpu; cenv != NULL; cenv = cenv->next_cpu) {
+kvm_inject_x86_mce(cenv, 1, MCI_STATUS_VAL | MCI_STATUS_UC,
+ MCG_STATUS_MCIP | MCG_STATUS_RIPV, 0, 0, 1);
+}
+}
+}
+#endif
+
int kvm_on_sigbus_vcpu(CPUState *env, int code, void *addr)
{
#if defined(KVM_CAP_MCE)
@@ -1694,6 +1716,7 @@ int kvm_on_sigbus_vcpu(CPUState *env, int code, void
*addr)
fprintf(stderr, "kvm_set_mce: %s\n", strerror(errno));
abort();
}
+kvm_mce_broadcast_rest(env);
} else
#endif
{
@@ -1716,7 +1739,6 @@ int kvm_on_sigbus(int code, void *addr)
void *vaddr;
ram_addr_t ram_addr;
target_phys_addr_t paddr;
-CPUState *cenv;
/* Hope we are lucky for AO MCE */
vaddr = addr;
@@ -1732,10 +1754,7 @@ int kvm_on_sigbus(int code, void *addr)
kvm_inject_x86_mce(first_cpu, 9, status,
MCG_STATUS_MCIP | MCG_STATUS_RIPV, paddr,
(MCM_ADDR_PHYS << 6) | 0xc, 1);
-for (cenv = first_cpu->next_cpu; cenv != NULL; cenv = cenv->next_cpu) {
-kvm_inject_x86_mce(cenv, 1, MCI_STATUS_VAL | MCI_STATUS_UC,
- MCG_STATUS_MCIP | MCG_STATUS_RIPV, 0, 0, 1);
-}
+kvm_mce_broadcast_rest(first_cpu);
} else
#endif
{
--
1.6.5.2
--
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] x86, mce: ignore SRAO only when MCG_SER_P is available
And restruct this block to call kvm_mce_in_exception() only when it is
required.
Signed-off-by: Hidetoshi Seto
---
target-i386/kvm.c | 16 ++--
1 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 786eeeb..a0d0603 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -239,12 +239,16 @@ static void kvm_do_inject_x86_mce(void *_data)
struct kvm_x86_mce_data *data = _data;
int r;
-/* If there is an MCE excpetion being processed, ignore this SRAO MCE */
-r = kvm_mce_in_exception(data->env);
-if (r == -1)
-fprintf(stderr, "Failed to get MCE status\n");
-else if (r && !(data->mce->status & MCI_STATUS_AR))
-return;
+/* If there is an MCE exception being processed, ignore this SRAO MCE */
+if ((data->env->mcg_cap & MCG_SER_P) &&
+!(data->mce->status & MCI_STATUS_AR)) {
+r = kvm_mce_in_exception(data->env);
+if (r == -1) {
+fprintf(stderr, "Failed to get MCE status\n");
+} else if (r) {
+return;
+}
+}
r = kvm_set_mce(data->env, data->mce);
if (r < 0) {
--
1.6.5.2
--
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] Fix build on !KVM_CAP_MCE
This patch removes following warnings:
target-i386/kvm.c: In function 'kvm_put_msrs':
target-i386/kvm.c:782: error: unused variable 'i'
target-i386/kvm.c: In function 'kvm_get_msrs':
target-i386/kvm.c:1083: error: label at end of compound statement
Signed-off-by: Hidetoshi Seto
---
target-i386/kvm.c |5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 512d533..786eeeb 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -779,7 +779,7 @@ static int kvm_put_msrs(CPUState *env, int level)
struct kvm_msr_entry entries[100];
} msr_data;
struct kvm_msr_entry *msrs = msr_data.entries;
-int i, n = 0;
+int n = 0;
kvm_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_CS, env->sysenter_cs);
kvm_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_ESP, env->sysenter_esp);
@@ -801,6 +801,7 @@ static int kvm_put_msrs(CPUState *env, int level)
}
#ifdef KVM_CAP_MCE
if (env->mcg_cap) {
+int i;
if (level == KVM_PUT_RESET_STATE)
kvm_msr_entry_set(&msrs[n++], MSR_MCG_STATUS, env->mcg_status);
else if (level == KVM_PUT_FULL_STATE) {
@@ -1085,9 +1086,9 @@ static int kvm_get_msrs(CPUState *env)
if (msrs[i].index >= MSR_MC0_CTL &&
msrs[i].index < MSR_MC0_CTL + (env->mcg_cap & 0xff) * 4) {
env->mce_banks[msrs[i].index - MSR_MC0_CTL] = msrs[i].data;
-break;
}
#endif
+break;
}
}
--
1.6.5.2
--
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 uq/master 2/2] kvm, x86: broadcast mce depending on the cpu version
There is no reason why SRAO event received by the main thread
is the only one that being broadcasted.
According to the x86 ASDM vol.3A 15.10.4.1,
MCE signal is broadcast on processor version 06H_EH or later.
This change is required to handle SRAR in smp guests.
Signed-off-by: Hidetoshi Seto
---
target-i386/kvm.c | 28
1 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 98a0505..e97fbd3 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1631,6 +1631,28 @@ static void hardware_memory_error(void)
exit(1);
}
+#ifdef KVM_CAP_MCE
+static void kvm_mce_broadcast_rest(CPUState *env)
+{
+CPUState *cenv;
+int family, model, cpuver = env->cpuid_version;
+
+family = (cpuver >> 8) & 0xf;
+model = ((cpuver >> 12) & 0xf0) + ((cpuver >> 4) & 0xf);
+
+/* Broadcast MCA signal for processor version 06H_EH and above */
+if ((family == 6 && model >= 14) || family > 6) {
+if (cenv == env) {
+continue;
+}
+for (cenv = first_cpu; cenv != NULL; cenv = cenv->next_cpu) {
+kvm_inject_x86_mce(cenv, 1, MCI_STATUS_VAL | MCI_STATUS_UC,
+ MCG_STATUS_MCIP | MCG_STATUS_RIPV, 0, 0, 1);
+}
+}
+}
+#endif
+
int kvm_on_sigbus_vcpu(CPUState *env, int code, void *addr)
{
#if defined(KVM_CAP_MCE)
@@ -1688,6 +1710,7 @@ int kvm_on_sigbus_vcpu(CPUState *env, int code, void
*addr)
fprintf(stderr, "kvm_set_mce: %s\n", strerror(errno));
abort();
}
+kvm_mce_broadcast_rest(env);
} else
#endif
{
@@ -1726,10 +1749,7 @@ int kvm_on_sigbus(int code, void *addr)
kvm_inject_x86_mce(first_cpu, 9, status,
MCG_STATUS_MCIP | MCG_STATUS_RIPV, paddr,
(MCM_ADDR_PHYS << 6) | 0xc, 1);
-for (cenv = first_cpu->next_cpu; cenv != NULL; cenv = cenv->next_cpu) {
-kvm_inject_x86_mce(cenv, 1, MCI_STATUS_VAL | MCI_STATUS_UC,
- MCG_STATUS_MCIP | MCG_STATUS_RIPV, 0, 0, 1);
-}
+kvm_mce_broadcast_rest(first_cpu);
} else
#endif
{
--
1.7.3.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
[PATCH uq/master 1/2] kvm, x86: ignore SRAO only when MCG_SER_P is available
And restruct this block to call kvm_mce_in_exception() only when it is
required.
Signed-off-by: Hidetoshi Seto
---
target-i386/kvm.c | 16 ++--
1 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index d940175..98a0505 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -239,12 +239,16 @@ static void kvm_do_inject_x86_mce(void *_data)
struct kvm_x86_mce_data *data = _data;
int r;
-/* If there is an MCE excpetion being processed, ignore this SRAO MCE */
-r = kvm_mce_in_exception(data->env);
-if (r == -1)
-fprintf(stderr, "Failed to get MCE status\n");
-else if (r && !(data->mce->status & MCI_STATUS_AR))
-return;
+/* If there is an MCE exception being processed, ignore this SRAO MCE */
+if ((data->env->mcg_cap & MCG_SER_P) &&
+!(data->mce->status & MCI_STATUS_AR)) {
+r = kvm_mce_in_exception(data->env);
+if (r == -1) {
+fprintf(stderr, "Failed to get MCE status\n");
+} else if (r) {
+return;
+}
+}
r = kvm_set_mce(data->env, data->mce);
if (r < 0) {
--
1.7.3.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: [Qemu-devel] Re: [PATCH 11/11] kvm, x86: broadcast mce depending on the cpu version
(2010/10/15 22:30), Marcelo Tosatti wrote: > On Fri, Oct 15, 2010 at 10:52:05AM +0900, Hidetoshi Seto wrote: >> (2010/10/15 10:06), Marcelo Tosatti wrote: >>> On Thu, Oct 14, 2010 at 05:55:28PM +0900, Jin Dongming wrote: >>>> There is no reason why SRAO event received by the main thread >>>> is the only one that being broadcasted. >>>> >>>> According to the x86 ASDM vol.3A 15.10.4.1, >>>> MCE signal is broadcast on processor version 06H_EH or later. >>>> >>>> This change is required to handle SRAR in the guest. >>>> >>>> Signed-off-by: Hidetoshi Seto >>>> Tested-by: Jin Dongming >>>> --- >>>> qemu-kvm.c | 63 >>>> +-- >>>> 1 files changed, 31 insertions(+), 32 deletions(-) >>> >>> Why is this necessary? _AO SIGBUS should be sent to all vcpu threads and >>> main thread. >> >> Humm? If you are right, vcpu threads will receive same SRAO event twice, >> one is that received by itself and another is that received by main thread >> and forwarded by the broadcast. >> >> My understanding is (Jin, please correct me if something wrong): >> - _AO SIGBUS is sent to main thread only, and then SRAO event is >>broadcasted to all vcpu threads. >> - _AR SIGBUS is sent to a vcpu thread that tried to touch the >>unmapped poisoned page, and SRAR event is posted to the vcpu. >> >> One problem here is that SRAR is not broadcasted. >> The guest might observe the event differently, like "some cpus >> don't enter machine check." > > Right. > >>> Please separate bug fixes from cleanups. Very nice, thanks. >> >> Maybe this set is considered as 10 cleanups + 1 fix. >> I think this fix will be complicated one without preceding cleanups. > > Why? All you need is to broadcast from vcpu context. No, it is not correct. What I really need is reliable QEMU and maintainable source codes with open community. Anyway, since I found it could be simpler than what I expected, I rebased 2 "functional change" pieces in this set to today's uq/master. But these are not tested on the tree yet since I could not build the uq/master due to many warnings on it (even without my fixes). > Please do a minimal fix separately so it can be backported, and the > cleanups can be done later once its merged upstream. When it will be merged? Thanks, H.Seto -- 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] Re: [PATCH 11/11] kvm, x86: broadcast mce depending on the cpu version
(2010/10/15 10:06), Marcelo Tosatti wrote: > On Thu, Oct 14, 2010 at 05:55:28PM +0900, Jin Dongming wrote: >> There is no reason why SRAO event received by the main thread >> is the only one that being broadcasted. >> >> According to the x86 ASDM vol.3A 15.10.4.1, >> MCE signal is broadcast on processor version 06H_EH or later. >> >> This change is required to handle SRAR in the guest. >> >> Signed-off-by: Hidetoshi Seto >> Tested-by: Jin Dongming >> --- >> qemu-kvm.c | 63 >> +-- >> 1 files changed, 31 insertions(+), 32 deletions(-) > > Why is this necessary? _AO SIGBUS should be sent to all vcpu threads and > main thread. Humm? If you are right, vcpu threads will receive same SRAO event twice, one is that received by itself and another is that received by main thread and forwarded by the broadcast. My understanding is (Jin, please correct me if something wrong): - _AO SIGBUS is sent to main thread only, and then SRAO event is broadcasted to all vcpu threads. - _AR SIGBUS is sent to a vcpu thread that tried to touch the unmapped poisoned page, and SRAR event is posted to the vcpu. One problem here is that SRAR is not broadcasted. The guest might observe the event differently, like "some cpus don't enter machine check." > Please separate bug fixes from cleanups. Very nice, thanks. Maybe this set is considered as 10 cleanups + 1 fix. I think this fix will be complicated one without preceding cleanups. Thanks, H.Seto -- 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] Re: [PATCH 07/11] kvm, x86: unify sigbus handling, prep
(2010/10/15 9:36), Marcelo Tosatti wrote: > On Thu, Oct 14, 2010 at 05:49:43PM +0900, Jin Dongming wrote: >> There are 2 similar functions to handle SIGBUS: >> sigbus_handler(int n, struct qemu_signalfd_siginfo *siginfo, >> void *ctx) >> kvm_on_sigbus(CPUState *env, siginfo_t *siginfo) >> >> The former is used when main thread receives SIGBUS via signalfd, >> while latter is used when vcpu thread receives SIGBUS. >> These 2 take different siginfo, but in both case required parameters >> are common, the code and the addr in the info. >> >> Restruct functions to take the code and the addr explicitly. >> >> Signed-off-by: Hidetoshi Seto >> Tested-by: Jin Dongming >> --- >> qemu-kvm.c | 41 - >> 1 files changed, 20 insertions(+), 21 deletions(-) > > Don't see the benefit, separate functions are cleaner. I think this is good for maintainability. If you want to fix a bug in this area, you might have to change 2 separate functions in completely same way. See 6c85786 and a05684e for examples. Thanks, H.Seto -- 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] qemu-kvm build issue on RHEL5.1
(2010/10/14 4:11), Blue Swirl wrote: > On Wed, Oct 13, 2010 at 8:00 AM, Hidetoshi Seto > wrote: >> (Add CC to k...@vger) >> >> (2010/10/12 10:52), Hao, Xudong wrote: >>> Hi, >>> Currently qemu-kvm build fail on RHEL5 with gcc 4.1.2, build can pass on >>> Fedora11 with gcc 4.4.1, can anybody look on RHEL5 system? >>> >>> Gcc: 4.1.2 >>> system: RHEL5.1 >>> qemu-kvm: 85566812a4f8cae721fea0224e05a7e75c08c5dd >>> >>> ... >>> LINK qemu-img >>> LINK qemu-io >>> CClibhw64/virtio-9p-local.o >>> cc1: warnings being treated as errors >>> /home/source/qemu-kvm/hw/virtio-9p-local.c: In function 'local_utimensat': >>> /home/source/qemu-kvm/hw/virtio-9p-local.c:479: warning: implicit >>> declaration of function 'utimensat' >>> /home/source/qemu-kvm/hw/virtio-9p-local.c:479: warning: nested extern >>> declaration of 'utimensat' >>> make[1]: *** [virtio-9p-local.o] Error 1 >>> make: *** [subdir-libhw64] Error 2 >>> >>> >>> Best Regards, >>> Xudong Hao >> >> It seems that this issue is caused by the old glibc. >> Though I don't know well about virtio-9p and suppose there >> should be better fix, I confirmed that following change >> removed the warnings. > > But then the system call will be made blindly without checking if the > kernel supports utimensat(). At the minimum, there should be a sane > response to ENOSYS error. Yes. But I'm not sure how this virtio-9p should behave if there is no utimensat. I think it will be better to fix this warning first to allow fellows using RHEL5 to restart contribute on qemu-kvm, and change this issue to virtio-9p specific problem to allow specialists of virtio-9p to have discussion for fix without bothering other developers. ... Or is it better to put abort() here instead of syscall? > > What happens if the system headers do not define SYS_utimensat? I suppose build will fail, say, we might need incremental patch named "fix build on RHEL4" or so. But I have no idea, whether there could be a workaround if there is no utimensat, whether we could provide something like wrapper named compat_utimensat or qemu_utimensat, and/or whether it makes sense if virtio-9p have dependency with presence of utimensat. Thanks, H.Seto -- 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] device-assignment: introduce get_assigned_dev_id
Stop repeating same code.
Signed-off-by: Hidetoshi Seto
---
hw/device-assignment.c | 25 +++--
1 files changed, 11 insertions(+), 14 deletions(-)
diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 975bf29..a2fa902 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -823,6 +823,11 @@ static uint32_t calc_assigned_dev_id(uint16_t seg, uint8_t
bus, uint8_t devfn)
return (uint32_t)seg << 16 | (uint32_t)bus << 8 | (uint32_t)devfn;
}
+static uint32_t get_assigned_dev_id(AssignedDevice *dev)
+{
+return calc_assigned_dev_id(dev->h_segnr, dev->h_busnr, dev->h_devfn);
+}
+
static void assign_failed_examine(AssignedDevice *dev)
{
char name[PATH_MAX], dir[PATH_MAX], driver[PATH_MAX] = {}, *ns;
@@ -886,8 +891,7 @@ static int assign_device(AssignedDevice *dev)
#endif
memset(&assigned_dev_data, 0, sizeof(assigned_dev_data));
-assigned_dev_data.assigned_dev_id =
- calc_assigned_dev_id(dev->h_segnr, dev->h_busnr, dev->h_devfn);
+assigned_dev_data.assigned_dev_id = get_assigned_dev_id(dev);
#ifdef KVM_CAP_PCI_SEGMENT
assigned_dev_data.segnr = dev->h_segnr;
#endif
@@ -944,8 +948,7 @@ static int assign_irq(AssignedDevice *dev)
return r;
memset(&assigned_irq_data, 0, sizeof(assigned_irq_data));
-assigned_irq_data.assigned_dev_id =
-calc_assigned_dev_id(dev->h_segnr, dev->h_busnr, dev->h_devfn);
+assigned_irq_data.assigned_dev_id = get_assigned_dev_id(dev);
assigned_irq_data.guest_irq = irq;
assigned_irq_data.host_irq = dev->real_device.irq;
#ifdef KVM_CAP_ASSIGN_DEV_IRQ
@@ -985,8 +988,7 @@ static void deassign_device(AssignedDevice *dev)
int r;
memset(&assigned_dev_data, 0, sizeof(assigned_dev_data));
-assigned_dev_data.assigned_dev_id =
- calc_assigned_dev_id(dev->h_segnr, dev->h_busnr, dev->h_devfn);
+assigned_dev_data.assigned_dev_id = get_assigned_dev_id(dev);
r = kvm_deassign_pci_device(kvm_context, &assigned_dev_data);
if (r < 0)
@@ -1041,9 +1043,7 @@ static void assigned_dev_update_msi(PCIDevice *pci_dev,
unsigned int ctrl_pos)
int r;
memset(&assigned_irq_data, 0, sizeof assigned_irq_data);
-assigned_irq_data.assigned_dev_id =
-calc_assigned_dev_id(assigned_dev->h_segnr, assigned_dev->h_busnr,
-(uint8_t)assigned_dev->h_devfn);
+assigned_irq_data.assigned_dev_id = get_assigned_dev_id(assigned_dev);
/* Some guests gratuitously disable MSI even if they're not using it,
* try to catch this by only deassigning irqs if the guest is using
@@ -1133,8 +1133,7 @@ static int assigned_dev_update_msix_mmio(PCIDevice
*pci_dev)
fprintf(stderr, "MSI-X entry number is zero!\n");
return -EINVAL;
}
-msix_nr.assigned_dev_id = calc_assigned_dev_id(adev->h_segnr,
adev->h_busnr,
- (uint8_t)adev->h_devfn);
+msix_nr.assigned_dev_id = get_assigned_dev_id(adev);
msix_nr.entry_nr = entries_nr;
r = kvm_assign_set_msix_nr(kvm_context, &msix_nr);
if (r != 0) {
@@ -1205,9 +1204,7 @@ static void assigned_dev_update_msix(PCIDevice *pci_dev,
unsigned int ctrl_pos)
int r;
memset(&assigned_irq_data, 0, sizeof assigned_irq_data);
-assigned_irq_data.assigned_dev_id =
-calc_assigned_dev_id(assigned_dev->h_segnr, assigned_dev->h_busnr,
-(uint8_t)assigned_dev->h_devfn);
+assigned_irq_data.assigned_dev_id = get_assigned_dev_id(assigned_dev);
/* Some guests gratuitously disable MSIX even if they're not using it,
* try to catch this by only deassigning irqs if the guest is using
--
1.7.3.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
[PATCH] device-assignment: cleanup assigned_dev_ioport_map
Here we already have: AssignedDevRegion *region = &r_dev->v_addrs[region_num]; Signed-off-by: Hidetoshi Seto --- hw/device-assignment.c | 18 ++ 1 files changed, 6 insertions(+), 12 deletions(-) diff --git a/hw/device-assignment.c b/hw/device-assignment.c index 26cb797..975bf29 100644 --- a/hw/device-assignment.c +++ b/hw/device-assignment.c @@ -324,18 +324,12 @@ static void assigned_dev_ioport_map(PCIDevice *pci_dev, int region_num, kvm_ioperm(env, data); } -register_ioport_read(addr, size, 1, assigned_dev_ioport_readb, - (r_dev->v_addrs + region_num)); -register_ioport_read(addr, size, 2, assigned_dev_ioport_readw, - (r_dev->v_addrs + region_num)); -register_ioport_read(addr, size, 4, assigned_dev_ioport_readl, - (r_dev->v_addrs + region_num)); -register_ioport_write(addr, size, 1, assigned_dev_ioport_writeb, - (r_dev->v_addrs + region_num)); -register_ioport_write(addr, size, 2, assigned_dev_ioport_writew, - (r_dev->v_addrs + region_num)); -register_ioport_write(addr, size, 4, assigned_dev_ioport_writel, - (r_dev->v_addrs + region_num)); +register_ioport_read(addr, size, 1, assigned_dev_ioport_readb, region); +register_ioport_read(addr, size, 2, assigned_dev_ioport_readw, region); +register_ioport_read(addr, size, 4, assigned_dev_ioport_readl, region); +register_ioport_write(addr, size, 1, assigned_dev_ioport_writeb, region); +register_ioport_write(addr, size, 2, assigned_dev_ioport_writew, region); +register_ioport_write(addr, size, 4, assigned_dev_ioport_writel, region); } static uint32_t assigned_dev_pci_read(PCIDevice *d, int pos, int len) -- 1.7.3.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: [Qemu-devel] qemu-kvm build issue on RHEL5.1
(Add CC to k...@vger)
(2010/10/12 10:52), Hao, Xudong wrote:
> Hi,
> Currently qemu-kvm build fail on RHEL5 with gcc 4.1.2, build can pass on
> Fedora11 with gcc 4.4.1, can anybody look on RHEL5 system?
>
> Gcc: 4.1.2
> system: RHEL5.1
> qemu-kvm: 85566812a4f8cae721fea0224e05a7e75c08c5dd
>
> ...
> LINK qemu-img
> LINK qemu-io
> CClibhw64/virtio-9p-local.o
> cc1: warnings being treated as errors
> /home/source/qemu-kvm/hw/virtio-9p-local.c: In function 'local_utimensat':
> /home/source/qemu-kvm/hw/virtio-9p-local.c:479: warning: implicit declaration
> of function 'utimensat'
> /home/source/qemu-kvm/hw/virtio-9p-local.c:479: warning: nested extern
> declaration of 'utimensat'
> make[1]: *** [virtio-9p-local.o] Error 1
> make: *** [subdir-libhw64] Error 2
>
>
> Best Regards,
> Xudong Hao
It seems that this issue is caused by the old glibc.
Though I don't know well about virtio-9p and suppose there
should be better fix, I confirmed that following change
removed the warnings.
Thanks,
H.Seto
=
[PATCH] virtio-9p: fix build on !CONFIG_UTIMENSAT
This removes following warnings on RHEL5, which has utimensat syscall
but has old glibc that doesn't have support for it:
hw/virtio-9p-local.c: In function 'local_utimensat':
hw/virtio-9p-local.c:479: warning: implicit declaration of function 'utimensat'
hw/virtio-9p-local.c:479: warning: nested extern declaration of 'utimensat'
and
hw/virtio-9p.c: In function 'v9fs_setattr_post_chmod':
hw/virtio-9p.c:1410: error: 'UTIME_NOW' undeclared (first use in this function)
hw/virtio-9p.c:1410: error: (Each undeclared identifier is reported only once
hw/virtio-9p.c:1410: error: for each function it appears in.)
hw/virtio-9p.c:1413: error: 'UTIME_OMIT' undeclared (first use in this function)
hw/virtio-9p.c: In function 'v9fs_wstat_post_chmod':
hw/virtio-9p.c:2905: error: 'UTIME_OMIT' undeclared (first use in this function)
Signed-off-by: Hidetoshi Seto
---
hw/virtio-9p-local.c |8
hw/virtio-9p.c |9 +
2 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/hw/virtio-9p-local.c b/hw/virtio-9p-local.c
index 57f9243..e075c27 100644
--- a/hw/virtio-9p-local.c
+++ b/hw/virtio-9p-local.c
@@ -18,6 +18,9 @@
#include
#include
#include
+#ifndef CONFIG_UTIMENSAT
+#include
+#endif
static const char *rpath(FsContext *ctx, const char *path)
{
@@ -476,7 +479,12 @@ static int local_chown(FsContext *fs_ctx, const char
*path, FsCred *credp)
static int local_utimensat(FsContext *s, const char *path,
const struct timespec *buf)
{
+#ifndef CONFIG_UTIMENSAT
+return syscall(SYS_utimensat, AT_FDCWD, rpath(s, path), buf,
+ AT_SYMLINK_NOFOLLOW);
+#else
return utimensat(AT_FDCWD, rpath(s, path), buf, AT_SYMLINK_NOFOLLOW);
+#endif
}
static int local_remove(FsContext *ctx, const char *path)
diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
index 32fa3bc..efe5c51 100644
--- a/hw/virtio-9p.c
+++ b/hw/virtio-9p.c
@@ -1393,6 +1393,15 @@ out:
qemu_free(vs);
}
+#ifndef CONFIG_UTIMENSAT
+#ifndef UTIME_NOW
+# define UTIME_NOW ((1l << 30) - 1l)
+#endif
+#ifndef UTIME_OMIT
+# define UTIME_OMIT((1l << 30) - 2l)
+#endif
+#endif
+
static void v9fs_setattr_post_chmod(V9fsState *s, V9fsSetattrState *vs, int
err)
{
if (err == -1) {
--
1.7.3.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] Fix SRAO/SRAR MCE injecting on guest without MCG_SER_P
(2010/10/08 17:25), Huang Ying wrote:
> On real machine, if MCG_SER_P in MSR_MCG_CAP is not set, SRAO/SRAR MCE
> should not be raised by hardware. This patch makes QEMU-KVM to follow
> the same rule.
>
> Reported-by: Hidetoshi Seto
> Signed-off-by: Huang Ying
> ---
> qemu-kvm.c |6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> --- a/qemu-kvm.c
> +++ b/qemu-kvm.c
> @@ -1134,7 +1134,7 @@ static void sigbus_handler(int n, struct
> void *ctx)
> {
> #if defined(KVM_CAP_MCE) && defined(TARGET_I386)
> -if (first_cpu->mcg_cap && siginfo->ssi_addr
> +if ((first_cpu->mcg_cap & MCG_SER_P) && siginfo->ssi_addr
> && siginfo->ssi_code == BUS_MCEERR_AO) {
> uint64_t status;
> void *vaddr;
> @@ -1324,7 +1324,7 @@ static void kvm_on_sigbus(CPUState *env,
> unsigned long paddr;
> int r;
>
> -if (env->mcg_cap && siginfo->si_addr
> +if ((env->mcg_cap & MCG_SER_P) && siginfo->si_addr
> && (siginfo->si_code == BUS_MCEERR_AR
> || siginfo->si_code == BUS_MCEERR_AO)) {
> if (siginfo->si_code == BUS_MCEERR_AR) {
> @@ -1356,7 +1356,7 @@ static void kvm_on_sigbus(CPUState *env,
> if (do_qemu_ram_addr_from_host(vaddr, &ram_addr) ||
> !kvm_physical_memory_addr_from_ram(kvm_state, ram_addr,
> (target_phys_addr_t *)&paddr)) {
> fprintf(stderr, "Hardware memory error for memory used by "
> -"QEMU itself instaed of guest system!\n");
> +"QEMU itself instead of guest system!\n");
> /* Hope we are lucky for AO MCE */
> if (siginfo->si_code == BUS_MCEERR_AO) {
> return;
Reviewed-by: Hidetoshi Seto
Thanks,
H.Seto
--
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 uq/master 7/8] MCE: Relay UCR MCE to guest
Hi, Huang-san, (2010/10/08 12:15), Huang Ying wrote: > Hi, Seto, > > On Thu, 2010-10-07 at 11:41 +0800, Hidetoshi Seto wrote: >> (2010/10/07 3:10), Dean Nelson wrote: >>> On 10/06/2010 11:05 AM, Marcelo Tosatti wrote: >>>> On Wed, Oct 06, 2010 at 10:58:36AM +0900, Hidetoshi Seto wrote: >>>>> I got some more question: >>>>> >>>>> (2010/10/05 3:54), Marcelo Tosatti wrote: >>>>>> Index: qemu/target-i386/cpu.h >>>>>> === >>>>>> --- qemu.orig/target-i386/cpu.h >>>>>> +++ qemu/target-i386/cpu.h >>>>>> @@ -250,16 +250,32 @@ >>>>>> #define PG_ERROR_RSVD_MASK 0x08 >>>>>> #define PG_ERROR_I_D_MASK 0x10 >>>>>> >>>>>> -#define MCG_CTL_P(1UL<<8) /* MCG_CAP register available */ >>>>>> +#define MCG_CTL_P(1ULL<<8) /* MCG_CAP register available */ >>>>>> +#define MCG_SER_P(1ULL<<24) /* MCA recovery/new status bits */ >>>>>> >>>>>> -#define MCE_CAP_DEFMCG_CTL_P >>>>>> +#define MCE_CAP_DEF(MCG_CTL_P|MCG_SER_P) >>>>>> #define MCE_BANKS_DEF10 >>>>>> >>>>> >>>>> It seems that current kvm doesn't support SER_P, so injecting SRAO >>>>> to guest will mean that guest receives VAL|UC|!PCC and RIPV event >>>>> from virtual processor that doesn't have SER_P. >>>> >>>> Dean also noted this. I don't think it was deliberate choice to not >>>> expose SER_P. Huang? >>> >>> In my testing, I found that MCG_SER_P was not being set (and I was >>> running on a Nehalem-EX system). Injecting a MCE resulted in the >>> guest entering into panic() from mce_panic(). If crash_kexec() >>> finds a kexec_crash_image the system ends up rebooting, otherwise, >>> what happens next requires operator intervention. >> >> Good to know. >> What I'm concerning is that if memory scrubbing SRAO event is >> injected when !SER_P, linux guest with certain mce tolerant level >> might grade it as "UC" severity and continue running with none of >> panicking, killing and poisoning because of !PCC and RIPV. >> >> Could you provide the panic message of the guest in your test? >> I think it can tell me why the mce handler decided to go panic. > > That is a bug that the SER_P is not in KVM_MCE_CAP_SUPPORTED in kernel. > I will fix it as soon as possible. And SRAO MCE should not be sent > when !SER_P, we should add that condition in qemu-kvm. That makes sense. I think it is qemu's responsibility for what follows the AO-SIGBUS, what action should be taken depends on the KVM's capability. >>> When I applied a patch to the guest's kernel which forces mce_ser to be >>> set, as if MCG_SER_P was set (see __mcheck_cpu_cap_init()), I found >>> that when the memory page was 'owned' by a guest process, the process >>> would be killed (if the page was dirty), and the guest would stay >>> running. The HWPoisoned page would be sidelined and not cause any more >>> issues. >> >> Excellent. >> So while guest kernel knows which page is poisoned, guest processes >> are controlled not to touch the page. >> >> ... Therefore rebooting the vm and renewing kernel will lost the >> information where is poisoned. > > Yes. That is an issue. Dean suggests that make qemu-kvm to refuse reboot > the guest if there is poisoned page and ask for user to intervention. I > have another idea to replace the poison pages with good pages when > reboot, that is, recover without user intervention. Sounds good. I think it may be worth something to reserve pages for the replacement before reboot is requested; at least we really don't want to fail rebooting with 'no memory'. >>>>> I think most OSes don't expect that it can receives MCE with !PCC >>>>> on traditional x86 processor without SER_P. >>>>> >>>>> Q1: Is it safe to expect that guests can handle such !PCC event? >>> >>> This might be best answered by Huang, but as I mentioned above, without >>> MCG_SER_P being set, the result was an orderly system panic on the >>> guest. >> >> Though I'll wait Huang (I think he is on holiday), I believe that >> system panic is just a possible option for AO (Action Optional) >> event, no matter how the SER_P is. > > W
Re: [patch uq/master 7/8] MCE: Relay UCR MCE to guest
(2010/10/07 3:10), Dean Nelson wrote: > On 10/06/2010 11:05 AM, Marcelo Tosatti wrote: >> On Wed, Oct 06, 2010 at 10:58:36AM +0900, Hidetoshi Seto wrote: >>> I got some more question: >>> >>> (2010/10/05 3:54), Marcelo Tosatti wrote: >>>> Index: qemu/target-i386/cpu.h >>>> === >>>> --- qemu.orig/target-i386/cpu.h >>>> +++ qemu/target-i386/cpu.h >>>> @@ -250,16 +250,32 @@ >>>> #define PG_ERROR_RSVD_MASK 0x08 >>>> #define PG_ERROR_I_D_MASK 0x10 >>>> >>>> -#define MCG_CTL_P(1UL<<8) /* MCG_CAP register available */ >>>> +#define MCG_CTL_P(1ULL<<8) /* MCG_CAP register available */ >>>> +#define MCG_SER_P(1ULL<<24) /* MCA recovery/new status bits */ >>>> >>>> -#define MCE_CAP_DEFMCG_CTL_P >>>> +#define MCE_CAP_DEF(MCG_CTL_P|MCG_SER_P) >>>> #define MCE_BANKS_DEF10 >>>> >>> >>> It seems that current kvm doesn't support SER_P, so injecting SRAO >>> to guest will mean that guest receives VAL|UC|!PCC and RIPV event >>> from virtual processor that doesn't have SER_P. >> >> Dean also noted this. I don't think it was deliberate choice to not >> expose SER_P. Huang? > > In my testing, I found that MCG_SER_P was not being set (and I was > running on a Nehalem-EX system). Injecting a MCE resulted in the > guest entering into panic() from mce_panic(). If crash_kexec() > finds a kexec_crash_image the system ends up rebooting, otherwise, > what happens next requires operator intervention. Good to know. What I'm concerning is that if memory scrubbing SRAO event is injected when !SER_P, linux guest with certain mce tolerant level might grade it as "UC" severity and continue running with none of panicking, killing and poisoning because of !PCC and RIPV. Could you provide the panic message of the guest in your test? I think it can tell me why the mce handler decided to go panic. > When I applied a patch to the guest's kernel which forces mce_ser to be > set, as if MCG_SER_P was set (see __mcheck_cpu_cap_init()), I found > that when the memory page was 'owned' by a guest process, the process > would be killed (if the page was dirty), and the guest would stay > running. The HWPoisoned page would be sidelined and not cause any more > issues. Excellent. So while guest kernel knows which page is poisoned, guest processes are controlled not to touch the page. ... Therefore rebooting the vm and renewing kernel will lost the information where is poisoned. >>> I think most OSes don't expect that it can receives MCE with !PCC >>> on traditional x86 processor without SER_P. >>> >>> Q1: Is it safe to expect that guests can handle such !PCC event? > > This might be best answered by Huang, but as I mentioned above, without > MCG_SER_P being set, the result was an orderly system panic on the > guest. Though I'll wait Huang (I think he is on holiday), I believe that system panic is just a possible option for AO (Action Optional) event, no matter how the SER_P is. >>> Q2: What is the expected behavior on the guest? > > I think I answered this above. Yeah, thanks. > >>> Q3: What happen if guest reboots itself in response to the MCE? > > That depends... > > And the following issue also holds for a guest that is rebooted at > some point having successfully sidelined the bad page. > > After the guest has panic'd, a system_reset of the guest or a restart > initiated by crash_kexec() (called by panic() on the guest), usually > results in the guest hanging because the bad page still belongs > to qemu-kvm and is now being referenced by the new guest in some way. Yes. In other words my concern about reboot is that new guest kernel including kdump kernel might try to read the bad page. If there is no AR-SIGBUS etc., we need some tricks to inhibit such accesses. > (It actually may not hang, but successfully reboot and be runnable, > with the bad page lurking in the background. It all seems to depend on > where the bad page ends up, and whether it's ever referenced.) I know some tough guys using their PC with buggy DIMMs :-) > > I believe there was an attempt to deal with this in kvm on the host. > See kvm_handle_bad_page(). This function was suppose to result in the > sending of a BUS_MCEERR_AR flavored SIGBUS by do_sigbus() to qemu-kvm > which in theory would result in the right thing happening. But commit > 96054569190bdec375fe824e48ca1f4e3b53dd36 prevents the signal from being > sent. So this mechanism n
Re: [patch uq/master 7/8] MCE: Relay UCR MCE to guest
I got some more question: (2010/10/05 3:54), Marcelo Tosatti wrote: > Index: qemu/target-i386/cpu.h > === > --- qemu.orig/target-i386/cpu.h > +++ qemu/target-i386/cpu.h > @@ -250,16 +250,32 @@ > #define PG_ERROR_RSVD_MASK 0x08 > #define PG_ERROR_I_D_MASK 0x10 > > -#define MCG_CTL_P(1UL<<8) /* MCG_CAP register available */ > +#define MCG_CTL_P(1ULL<<8) /* MCG_CAP register available */ > +#define MCG_SER_P(1ULL<<24) /* MCA recovery/new status bits */ > > -#define MCE_CAP_DEF MCG_CTL_P > +#define MCE_CAP_DEF (MCG_CTL_P|MCG_SER_P) > #define MCE_BANKS_DEF10 > It seems that current kvm doesn't support SER_P, so injecting SRAO to guest will mean that guest receives VAL|UC|!PCC and RIPV event from virtual processor that doesn't have SER_P. I think most OSes don't expect that it can receives MCE with !PCC on traditional x86 processor without SER_P. Q1: Is it safe to expect that guests can handle such !PCC event? Q2: What is the expected behavior on the guest? Q3: What happen if guest reboots itself in response to the MCE? Thanks, H.Seto -- 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 uq/master 7/8] MCE: Relay UCR MCE to guest
(2010/10/05 3:54), Marcelo Tosatti wrote:
> Port qemu-kvm's
>
> commit 4b62fff1101a7ad77553147717a8bd3bf79df7ef
> Author: Huang Ying
> Date: Mon Sep 21 10:43:25 2009 +0800
>
> MCE: Relay UCR MCE to guest
>
> UCR (uncorrected recovery) MCE is supported in recent Intel CPUs,
> where some hardware error such as some memory error can be reported
> without PCC (processor context corrupted). To recover from such MCE,
> the corresponding memory will be unmapped, and all processes accessing
> the memory will be killed via SIGBUS.
>
> For KVM, if QEMU/KVM is killed, all guest processes will be killed
> too. So we relay SIGBUS from host OS to guest system via a UCR MCE
> injection. Then guest OS can isolate corresponding memory and kill
> necessary guest processes only. SIGBUS sent to main thread (not VCPU
> threads) will be broadcast to all VCPU threads as UCR MCE.
>
> Signed-off-by: Marcelo Tosatti
>
(snip)
> +static void sigbus_handler(int n, struct qemu_signalfd_siginfo *siginfo,
> + void *ctx)
> +{
> +#if defined(TARGET_I386)
> +if (kvm_on_sigbus_vcpu(siginfo->ssi_code, (void
> *)(intptr_t)siginfo->ssi_addr))
> +#endif
> +sigbus_reraise();
> +}
> +
> static void qemu_kvm_eat_signal(CPUState *env, int timeout)
> {
> struct timespec ts;
> int r, e;
> siginfo_t siginfo;
> sigset_t waitset;
> +sigset_t chkset;
>
> ts.tv_sec = timeout / 1000;
> ts.tv_nsec = (timeout % 1000) * 100;
>
> sigemptyset(&waitset);
> sigaddset(&waitset, SIG_IPI);
> +sigaddset(&waitset, SIGBUS);
>
> -qemu_mutex_unlock(&qemu_global_mutex);
> -r = sigtimedwait(&waitset, &siginfo, &ts);
> -e = errno;
> -qemu_mutex_lock(&qemu_global_mutex);
> +do {
> +qemu_mutex_unlock(&qemu_global_mutex);
>
> -if (r == -1 && !(e == EAGAIN || e == EINTR)) {
> -fprintf(stderr, "sigtimedwait: %s\n", strerror(e));
> -exit(1);
> -}
> +r = sigtimedwait(&waitset, &siginfo, &ts);
> +e = errno;
> +
> +qemu_mutex_lock(&qemu_global_mutex);
> +
> +if (r == -1 && !(e == EAGAIN || e == EINTR)) {
> +fprintf(stderr, "sigtimedwait: %s\n", strerror(e));
> +exit(1);
> +}
> +
> +switch (r) {
> +case SIGBUS:
> +#ifdef TARGET_I386
> +if (kvm_on_sigbus(env, siginfo.si_code, siginfo.si_addr))
> +#endif
> +sigbus_reraise();
> +break;
> +default:
> +break;
> +}
> +
> +r = sigpending(&chkset);
> +if (r == -1) {
> +fprintf(stderr, "sigpending: %s\n", strerror(e));
> +exit(1);
> +}
> +} while (sigismember(&chkset, SIG_IPI) || sigismember(&chkset, SIGBUS));
> }
>
> static void qemu_kvm_wait_io_event(CPUState *env)
(snip)
> Index: qemu/kvm.h
> ===
> --- qemu.orig/kvm.h
> +++ qemu/kvm.h
> @@ -110,6 +110,9 @@ int kvm_arch_init_vcpu(CPUState *env);
>
> void kvm_arch_reset_vcpu(CPUState *env);
>
> +int kvm_on_sigbus(CPUState *env, int code, void *addr);
> +int kvm_on_sigbus_vcpu(int code, void *addr);
> +
> struct kvm_guest_debug;
> struct kvm_debug_exit_arch;
>
So kvm_on_sigbus() is called from qemu_kvm_eat_signal() that is
called on vcpu thread, while kvm_on_sigbus_vcpu() is called via
sigbus_handler that invoked on iothread using signalfd.
... Inverse naming?
Thanks,
H.Seto
--
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: PCI passthru issue
(2010/09/17 8:01), Anirban Chakraborty wrote: > Hi All, > > I noticed that the PCI registers are not all mapped to the > guest for a PCI pass through device. For example, I was trying > to pass through a PCI function (NIC) to a guest and in the > guest the MSI-X table offset register is not mapped properly. > It was showing all 0s. This is in RHEL 6.0 beta and the guest > was a SLES11 64 bit. > > In the RHEL 6.0 host I see following for the PCI function: > - > 05:00.0 Ethernet controller: QLogic Corp. cLOM8214 1/10GbE Controller (rev 54) > Subsystem: QLogic Corp. 8200 Series Dual Port 10GbE Converged Network > Adapter (TCP/IP Networking) > Control: I/O- Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr+ > Stepping- SERR+ FastB2B- DisINTx+ > Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- SERR-Interrupt: pin A routed to IRQ 74 > Region 0: Memory at df20 (64-bit, non-prefetchable) [size=2M] > Region 4: Memory at df1b (64-bit, non-prefetchable) [size=64K] > Expansion ROM at df1c [disabled] [size=256K] > Capabilities: [40] MSI-X: Enable- Count=32 Masked- > Vector table: BAR=0 offset=0009 > PBA: BAR=0 offset=00090800 > ... > Kernel driver in use: pci-stub > 00: 77 10 20 80 42 05 10 00 54 00 00 02 40 00 80 00 > 10: 04 00 20 df 00 00 00 00 00 00 00 00 00 00 00 00 > 20: 04 00 1b df 00 00 00 00 00 00 00 00 77 10 07 02 > 30: 00 00 1c df 40 00 00 00 00 00 00 00 0a 01 00 00 > 40: 11 80 1f 00 00 00 09 00 00 08 09 00 00 00 00 00 > -- > And in the SLES11 guest, I see following for the same function: > > 00:07.0 Ethernet controller: QLogic Corp. Device 8020 (rev 54) > Subsystem: QLogic Corp. Device 0207 > Control: I/O- Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr+ > Stepping- SERR+ FastB2B- DisINTx+ > Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- SERR-Interrupt: pin A routed to IRQ 11 > Region 0: Memory at f220 (32-bit, non-prefetchable) [size=2M] > Region 4: Memory at f240 (32-bit, non-prefetchable) [size=64K] > Expansion ROM at f244 [disabled] [size=256K] > Capabilities: [40] Message Signalled Interrupts: Mask- 64bit- Count=1/1 > Enable- > Address: Data: > Capabilities: [50] MSI-X: Enable- Mask- TabSize=32 > Vector table: BAR=0 offset=0009 > PBA: BAR=0 offset= > 00: 77 10 20 80 42 05 10 00 54 00 00 02 40 00 80 00 > 10: 00 00 20 f2 00 00 00 00 00 00 00 00 00 00 00 00 > 20: 00 00 40 f2 00 00 00 00 00 00 00 00 77 10 07 02 > 30: 08 00 44 f2 40 00 00 00 00 00 00 00 0b 01 00 00 > 40: 05 50 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > > Note that the data at byte offset 40h is different for the > host and the guest. Is this expected or is it a bug in KVM? This is expected, since MSIs of PCI pass through device need some emulation to route them to the guest instead of the host. So what we can see on the guest's PCI pass through device is modified capability list that contains emulated capabilities. > > Is there any spec out there that specifies what PCI registers > are accessible from guest and how much of the PCI config space > is mapped to the guest? This is helpful to know, especially > in the context of SR-IOV. If a VF is pass throughed to a guest, > are all the VF registers visible in host system will be mapped > (and accessible) to the guest OS? AFAIK the QEMU source code, hw/device-assignment.c, will help you to know what are going on there. I'm not sure what interesting registers can the VF have. But I think you should be careful because the register in the PCI config space might not work like that on the host even the register is mapped and visible from the guest. Thanks, H.Seto -- 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] ivshmem: remove unnecessary checks for unsigned
(2010/09/03 10:54), Hidetoshi Seto wrote:
> fixes gcc 4.1 warning:
> In function 'ivshmem_io_writel':
> 202: warning: comparison is always false due to limited range of data type
> 208: warning: comparison is always true due to limited range of data type
>
> Signed-off-by: Hidetoshi Seto
> ---
> hw/ivshmem.c |4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ivshmem.c b/hw/ivshmem.c
> index bbb5cba..afebbc3 100644
> --- a/hw/ivshmem.c
> +++ b/hw/ivshmem.c
> @@ -199,13 +199,13 @@ static void ivshmem_io_writel(void *opaque,
> target_phys_addr_t addr,
>
> case DOORBELL:
> /* check that dest VM ID is reasonable */
> -if ((dest < 0) || (dest > s->max_peer)) {
> +if (dest > s->max_peer) {
> IVSHMEM_DPRINTF("Invalid destination VM ID (%d)\n", dest);
> break;
> }
>
> /* check doorbell range */
> -if ((vector >= 0) && (vector < s->peers[dest].nb_eventfds)) {
> +if (vector < s->peers[dest].nb_eventfds) {
> IVSHMEM_DPRINTF("Writing %" PRId64 " to VM %d on vector
> %d\n",
> write_one, dest, vector);
> if (write(s->peers[dest].eventfds[vector],
Oops, since I've registered qemu-kvm ML but not qemu-devel ML, I could not
noticed that Jes have already posted same patch to qemu-devel.
Now build of ivshmem is enabled only on KVM systems, please apply this patch
to qemu-kvm.git asap.
Thanks,
H.Seto
--
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] ivshmem: remove unnecessary checks for unsigned
fixes gcc 4.1 warning:
In function 'ivshmem_io_writel':
202: warning: comparison is always false due to limited range of data type
208: warning: comparison is always true due to limited range of data type
Signed-off-by: Hidetoshi Seto
---
hw/ivshmem.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/ivshmem.c b/hw/ivshmem.c
index bbb5cba..afebbc3 100644
--- a/hw/ivshmem.c
+++ b/hw/ivshmem.c
@@ -199,13 +199,13 @@ static void ivshmem_io_writel(void *opaque,
target_phys_addr_t addr,
case DOORBELL:
/* check that dest VM ID is reasonable */
-if ((dest < 0) || (dest > s->max_peer)) {
+if (dest > s->max_peer) {
IVSHMEM_DPRINTF("Invalid destination VM ID (%d)\n", dest);
break;
}
/* check doorbell range */
-if ((vector >= 0) && (vector < s->peers[dest].nb_eventfds)) {
+if (vector < s->peers[dest].nb_eventfds) {
IVSHMEM_DPRINTF("Writing %" PRId64 " to VM %d on vector %d\n",
write_one, dest, vector);
if (write(s->peers[dest].eventfds[vector],
--
1.7.2.2
--
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] Use signed 16-bit values for ivshmem register writes
(2010/08/31 1:58), Cam Macdonell wrote: > fixes gcc 4.1 warning > > Signed-off-by: Cam Macdonell > --- > hw/ivshmem.c |4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/ivshmem.c b/hw/ivshmem.c > index bbb5cba..fa9c684 100644 > --- a/hw/ivshmem.c > +++ b/hw/ivshmem.c > @@ -181,8 +181,8 @@ static void ivshmem_io_writel(void *opaque, > target_phys_addr_t addr, > IVShmemState *s = opaque; > > uint64_t write_one = 1; > -uint16_t dest = val >> 16; > -uint16_t vector = val & 0xff; > +int16_t dest = val >> 16; > +int16_t vector = val & 0xff; > > addr &= 0xfc; > Since val is uint32_t, I think this change doesn't make sense. Thanks, H.Seto -- 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] QEMU: Update .gitignore
(2010/07/01 6:33), Aurelien Jarno wrote: > On Mon, Jun 21, 2010 at 06:14:17PM +0900, Hidetoshi Seto wrote: >> (2010/06/21 17:19), Avi Kivity wrote: >>> On 06/21/2010 08:24 AM, Hidetoshi Seto wrote: >>>> I think some people have noticed that: >>>> >>>>> $ ./configure >>>>> $ make >>>>> $ git status >>>>> # On branch master >>>>> # Untracked files: >>>>> # (use "git add ..." to include in what will be committed) >>>>> # >>>>> # QMP/qmp-commands.txt >>>>> # libdis-user/ >>>>> # libdis/ >>>>> # pc-bios/optionrom/vapic.bin >>>>> nothing added to commit but untracked files present (use "git add" to >>>>> track) >>>>> >>>> Please consider applying this patch to qemu-kvm.git. >>> >>> This is equally applicable to qemu.git, so please sent it to the qemu >>> mailing list, qemu-de...@nongnu.org. >> >> Thanks for your advice, Avi. >> >> Now this mail is sent to qemu ML, w/ above quotes as short history. >> Could someone pick this up? >> >> Thanks, >> H.Seto >> >> = > > This patch looks good, but doesn't apply anymore, as the libdis/ and > libdis-user/ part has already been applied from another patch. Care > to resend it? I've just posted. Please check it. Thanks. H.Seto -- 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] Add vapic.bin to .gitignore
# This patch is for qemu-kvm.git The vapic.bin is a generated binary file. Signed-off-by: Hidetoshi Seto --- .gitignore |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/.gitignore b/.gitignore index ddc248b..26eba20 100644 --- a/.gitignore +++ b/.gitignore @@ -53,4 +53,5 @@ pc-bios/optionrom/linuxboot.bin pc-bios/optionrom/multiboot.bin pc-bios/optionrom/multiboot.raw pc-bios/optionrom/extboot.bin +pc-bios/optionrom/vapic.bin .stgit-* -- 1.7.0 -- 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] Add QMP/qmp-commands.txt to .gitignore
QMP/qmp-commands.txt is a generated file. Signed-off-by: Hidetoshi Seto --- .gitignore |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/.gitignore b/.gitignore index ce66ed5..a32b7c4 100644 --- a/.gitignore +++ b/.gitignore @@ -28,6 +28,7 @@ qemu-img-cmds.texi qemu-img-cmds.h qemu-io qemu-monitor.texi +QMP/qmp-commands.txt .gdbinit *.a *.aux -- 1.7.0 -- 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] device-assignment: Rework "name" of assigned pci device
(2010/06/30 15:53), Markus Armbruster wrote:
> Summary: upstream qemu commit b560a9ab broke -pcidevice and pci_add host
> in two ways:
>
> * Use without options id and name is broken when option host contains
> ':'. That's because id defaults to host. I recommend to fix it
> incompatibly: don't default id to host. The alternative is to get
> upstream qemu to accept ':' in qdev IDs again.
>
> * Funny characters in option name no longer work. Same as funny
> characters in id options all over the place. Intentional change. I
> recommend to do nothing.
Thanks a lot.
I'm not a person in really need, so now I'm going to follow your
recommendation.
> Details inline.
>
> Hidetoshi Seto writes:
>
>> Thanks Markus,
>>
>> (2010/06/29 14:28), Markus Armbruster wrote:
>>> Hidetoshi Seto writes:
>>>
>>>> "Hao, Xudong" writes:
>>>>>> When assign one PCI device, qemu fail to parse the command line:
>>>>>> qemu-system_x86 -smp 2 -m 1024 -hda /path/to/img -pcidevice host=00:19.0
>>>>>> Error:
>>>>>> qemu-system-x86_64: Parameter 'id' expects an identifier
>>>>>> Identifiers consist of letters, digits, '-', '.', '_', starting with a
>>>>>> letter.
>>>>>> pcidevice argument parse error; please check the help text for usage
>>>>>> Could not add assigned device host=00:19.0
>>>>>>
>>>>>> https://bugs.launchpad.net/qemu/+bug/597932
>>>>>>
>>>>>> This issue caused by qemu-kvm commit
>>>>>> b560a9ab9be06afcbb78b3791ab836dad208a239.
>>>>
>>>> This patch is a response to the above report.
>>>>
>>>> Thanks,
>>>> H.Seto
>>>>
>>>> =
>>>>
>>>> Because use of some characters in "id" is restricted recently, assigned
>>>> device start to fail having implicit "id" that uses address string of
>>>> host device, like "00:19.0" which includes restricted character ':'.
>>>>
>>>> It seems that this implicit "id" is intended to be run as "name" that
>>>> could be passed with option "-pcidevice ... ,name=..." to specify a
>>>> string to be used in log outputs. In other words it seems that
>>>> dev->dev.qdev.id of assigned device had been used to have such
>>>> "name", that is user-defined string or address string of "host".
>>>
>>> As far as I can tell, option "name" is just a leftover from pre-qdev
>>> days, kept for compatibility.
>>
>> Yea, I see.
>> I don't know well about the history of such pre-qdev days...
>
> It's often useful to examine history to figure out what a piece of code
> attempts to accomplish. git-blame and git-log are your friends :)
I often play with git-log, however I have a little trouble here since
qemu tree is too young.
>>>> The problem is that "name" for specific use is not equal to "id" for
>>>> universal use. So it is better to remove this tricky mix up here.
>>>>
>>>> This patch introduces new function assigned_dev_name() that returns
>>>> proper name string for the device.
>>>> Now property "name" is explicitly defined in struct AssignedDevice.
>>>>
>>>> When if the device have neither "name" nor "id", address string like
>>>> ":00:19.0" will be created and passed instead. Once created, new
>>>> field r_name holds the string to be reused and to be released later.
> [...]
>>>> @@ -1520,6 +1545,7 @@ static PCIDeviceInfo assign_info = {
>>>> DEFINE_PROP("host", AssignedDevice, host, qdev_prop_hostaddr,
>>>> PCIHostDevice),
>>>> DEFINE_PROP_UINT32("iommu", AssignedDevice, use_iommu, 1),
>>>> DEFINE_PROP_STRING("configfd", AssignedDevice, configfd_name),
>>>> +DEFINE_PROP_STRING("name", AssignedDevice, u_name),
>>>> DEFINE_PROP_END_OF_LIST(),
>>>> },
>>>> };
>>>> @@ -1545,24 +1571,25 @@ device_init(assign_register_devices)
>>>> QemuOpts *add_assigned_device(const char *arg)
>>>> {
>>>> QemuOpts *opts = NULL;
>>>> -char host[64], id[64], dma[8];
>>>>
[PATCH v2] device-assignment: Rework "name" of assigned pci device
Because use of some characters in "id" is restricted recently,
assigned device start to fail having implicit "id" that uses
address string of host device, like "00:19.0" which includes
restricted character ':'.
It seems that this implicit "id" is intended to be run as "name"
that could be passed with option "-pcidevice ... ,name=..." to
specify a string to be used in log outputs. In other words it
seems that dev->dev.qdev.id of assigned device had been used to
point such "name", that is user-defined string or address string
of "host".
The problem is that "name" for specific use is not equal to "id"
for universal use. So it is better to remove this tricky mix up.
This patch introduces new function assigned_dev_name() that
returns proper name string for the device.
Now property "name" is explicitly defined in struct AssignedDevice.
When if the device have neither "name" nor "id", address string
like ":00:19.0" will be created and passed instead.
Once created, new field r_name holds the string to be reused and
to be released later.
v2: - Pass id to qemu_opts_create if "id=" is specified even it is
an undocumented option for -pcidevice and pci_add.
- Drop minor cleanup & unrelated changes.
Signed-off-by: Hidetoshi Seto
---
hw/device-assignment.c | 52
hw/device-assignment.h |2 +
2 files changed, 41 insertions(+), 13 deletions(-)
diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 585162b..cda6ee3 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -62,6 +62,25 @@ static void assigned_dev_load_option_rom(AssignedDevice
*dev);
static void assigned_dev_unregister_msix_mmio(AssignedDevice *dev);
+static const char *assigned_dev_name(AssignedDevice *dev)
+{
+/* use user-defined "name" if specified */
+if (dev->u_name)
+return dev->u_name;
+/* else use "id" if available */
+if (dev->dev.qdev.id)
+return dev->dev.qdev.id;
+/* otherwise use address of host device instead */
+if (!dev->r_name) {
+char buf[32];
+
+snprintf(buf, sizeof(buf), "%04x:%02x:%02x.%01x",
+ dev->host.seg, dev->host.bus, dev->host.dev, dev->host.func);
+dev->r_name = qemu_strdup(buf);
+}
+return dev->r_name;
+}
+
static uint32_t guest_to_host_ioport(AssignedDevRegion *region, uint32_t addr)
{
return region->u.r_baseport + (addr - region->e_physbase);
@@ -798,6 +817,10 @@ static void free_assigned_device(AssignedDevice *dev)
dev->real_device.config_fd = 0;
}
+if (dev->r_name) {
+qemu_free(dev->r_name);
+}
+
#ifdef KVM_CAP_IRQ_ROUTING
free_dev_irq_entries(dev);
#endif
@@ -885,7 +908,7 @@ static int assign_device(AssignedDevice *dev)
if (dev->use_iommu) {
if (!kvm_check_extension(kvm_state, KVM_CAP_IOMMU)) {
fprintf(stderr, "No IOMMU found. Unable to assign device
\"%s\"\n",
-dev->dev.qdev.id);
+assigned_dev_name(dev));
return -ENODEV;
}
assigned_dev_data.flags |= KVM_DEV_ASSIGN_ENABLE_IOMMU;
@@ -897,7 +920,7 @@ static int assign_device(AssignedDevice *dev)
r = kvm_assign_pci_device(kvm_context, &assigned_dev_data);
if (r < 0) {
fprintf(stderr, "Failed to assign device \"%s\" : %s\n",
-dev->dev.qdev.id, strerror(-r));
+assigned_dev_name(dev), strerror(-r));
switch (r) {
case -EBUSY:
@@ -953,7 +976,7 @@ static int assign_irq(AssignedDevice *dev)
r = kvm_assign_irq(kvm_context, &assigned_irq_data);
if (r < 0) {
fprintf(stderr, "Failed to assign irq for \"%s\": %s\n",
-dev->dev.qdev.id, strerror(-r));
+assigned_dev_name(dev), strerror(-r));
fprintf(stderr, "Perhaps you are assigning a device "
"that shares an IRQ with another device?\n");
return r;
@@ -977,7 +1000,7 @@ static void deassign_device(AssignedDevice *dev)
r = kvm_deassign_pci_device(kvm_context, &assigned_dev_data);
if (r < 0)
fprintf(stderr, "Failed to deassign device \"%s\" : %s\n",
-dev->dev.qdev.id, strerror(-r));
+assigned_dev_name(dev), strerror(-r));
#endif
}
@@ -1421,7 +1444,7 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
if (get_real_device(dev, dev->host.seg, dev->host.bus,
dev->host.dev, dev->host.func)) {
error_report("pci-assign: Error: Couldn't get re
Re: [PATCH] device-assignment: Rework "name" of assigned pci device
Thanks Markus,
(2010/06/29 14:28), Markus Armbruster wrote:
> Hidetoshi Seto writes:
>
>> "Hao, Xudong" writes:
>>>> When assign one PCI device, qemu fail to parse the command line:
>>>> qemu-system_x86 -smp 2 -m 1024 -hda /path/to/img -pcidevice host=00:19.0
>>>> Error:
>>>> qemu-system-x86_64: Parameter 'id' expects an identifier
>>>> Identifiers consist of letters, digits, '-', '.', '_', starting with a
>>>> letter.
>>>> pcidevice argument parse error; please check the help text for usage
>>>> Could not add assigned device host=00:19.0
>>>>
>>>> https://bugs.launchpad.net/qemu/+bug/597932
>>>>
>>>> This issue caused by qemu-kvm commit
>>>> b560a9ab9be06afcbb78b3791ab836dad208a239.
>>
>> This patch is a response to the above report.
>>
>> Thanks,
>> H.Seto
>>
>> =
>>
>> Because use of some characters in "id" is restricted recently, assigned
>> device start to fail having implicit "id" that uses address string of
>> host device, like "00:19.0" which includes restricted character ':'.
>>
>> It seems that this implicit "id" is intended to be run as "name" that
>> could be passed with option "-pcidevice ... ,name=..." to specify a
>> string to be used in log outputs. In other words it seems that
>> dev->dev.qdev.id of assigned device had been used to have such
>> "name", that is user-defined string or address string of "host".
>
> As far as I can tell, option "name" is just a leftover from pre-qdev
> days, kept for compatibility.
Yea, I see.
I don't know well about the history of such pre-qdev days...
>> The problem is that "name" for specific use is not equal to "id" for
>> universal use. So it is better to remove this tricky mix up here.
>>
>> This patch introduces new function assigned_dev_name() that returns
>> proper name string for the device.
>> Now property "name" is explicitly defined in struct AssignedDevice.
>>
>> When if the device have neither "name" nor "id", address string like
>> ":00:19.0" will be created and passed instead. Once created, new
>> field r_name holds the string to be reused and to be released later.
>>
>> Signed-off-by: Hidetoshi Seto
>
> Comments inline.
>
>> ---
>> hw/device-assignment.c | 59
>> ++-
>> hw/device-assignment.h |2 +
>> 2 files changed, 44 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
>> index 585162b..d73516f 100644
>> --- a/hw/device-assignment.c
>> +++ b/hw/device-assignment.c
>> @@ -62,6 +62,25 @@ static void assigned_dev_load_option_rom(AssignedDevice
>> *dev);
>>
>> static void assigned_dev_unregister_msix_mmio(AssignedDevice *dev);
>>
>> +static const char *assigned_dev_name(AssignedDevice *dev)
>> +{
>> +/* use user-defined "name" if specified */
>> +if (dev->u_name)
>> +return dev->u_name;
>> +/* else use "id" if available */
>> +if (dev->dev.qdev.id)
>> +return dev->dev.qdev.id;
>> +/* otherwise use address of host device instead */
>> +if (!dev->r_name) {
>> +char buf[32];
>> +
>> +snprintf(buf, sizeof(buf), "%04x:%02x:%02x.%01x",
>> + dev->host.seg, dev->host.bus, dev->host.dev,
>> dev->host.func);
>> +dev->r_name = qemu_strdup(buf);
>> +}
>> +return dev->r_name;
>> +}
>> +
>> static uint32_t guest_to_host_ioport(AssignedDevRegion *region, uint32_t
>> addr)
>> {
>> return region->u.r_baseport + (addr - region->e_physbase);
>> @@ -798,6 +817,10 @@ static void free_assigned_device(AssignedDevice *dev)
>> dev->real_device.config_fd = 0;
>> }
>>
>> +if (dev->r_name) {
>> +qemu_free(dev->r_name);
>> +}
>> +
>> #ifdef KVM_CAP_IRQ_ROUTING
>> free_dev_irq_entries(dev);
>> #endif
>> @@ -885,7 +908,7 @@ static int assign_device(AssignedDevice *dev)
>> if (dev->use_iommu) {
>> if (!kvm_check_extension(kvm_state, KVM_CAP_IOMMU)) {
>> fprintf(stderr, "No IOMMU found. Unable to assign dev
[PATCH] device-assignment: Rework "name" of assigned pci device
"Hao, Xudong" writes:
> > When assign one PCI device, qemu fail to parse the command line:
> > qemu-system_x86 -smp 2 -m 1024 -hda /path/to/img -pcidevice host=00:19.0
> > Error:
> > qemu-system-x86_64: Parameter 'id' expects an identifier
> > Identifiers consist of letters, digits, '-', '.', '_', starting with a
> > letter.
> > pcidevice argument parse error; please check the help text for usage
> > Could not add assigned device host=00:19.0
> >
> > https://bugs.launchpad.net/qemu/+bug/597932
> >
> > This issue caused by qemu-kvm commit
> > b560a9ab9be06afcbb78b3791ab836dad208a239.
This patch is a response to the above report.
Thanks,
H.Seto
=
Because use of some characters in "id" is restricted recently, assigned
device start to fail having implicit "id" that uses address string of
host device, like "00:19.0" which includes restricted character ':'.
It seems that this implicit "id" is intended to be run as "name" that
could be passed with option "-pcidevice ... ,name=..." to specify a
string to be used in log outputs. In other words it seems that
dev->dev.qdev.id of assigned device had been used to have such
"name", that is user-defined string or address string of "host".
The problem is that "name" for specific use is not equal to "id" for
universal use. So it is better to remove this tricky mix up here.
This patch introduces new function assigned_dev_name() that returns
proper name string for the device.
Now property "name" is explicitly defined in struct AssignedDevice.
When if the device have neither "name" nor "id", address string like
":00:19.0" will be created and passed instead. Once created, new
field r_name holds the string to be reused and to be released later.
Signed-off-by: Hidetoshi Seto
---
hw/device-assignment.c | 59 ++-
hw/device-assignment.h |2 +
2 files changed, 44 insertions(+), 17 deletions(-)
diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 585162b..d73516f 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -62,6 +62,25 @@ static void assigned_dev_load_option_rom(AssignedDevice
*dev);
static void assigned_dev_unregister_msix_mmio(AssignedDevice *dev);
+static const char *assigned_dev_name(AssignedDevice *dev)
+{
+/* use user-defined "name" if specified */
+if (dev->u_name)
+return dev->u_name;
+/* else use "id" if available */
+if (dev->dev.qdev.id)
+return dev->dev.qdev.id;
+/* otherwise use address of host device instead */
+if (!dev->r_name) {
+char buf[32];
+
+snprintf(buf, sizeof(buf), "%04x:%02x:%02x.%01x",
+ dev->host.seg, dev->host.bus, dev->host.dev, dev->host.func);
+dev->r_name = qemu_strdup(buf);
+}
+return dev->r_name;
+}
+
static uint32_t guest_to_host_ioport(AssignedDevRegion *region, uint32_t addr)
{
return region->u.r_baseport + (addr - region->e_physbase);
@@ -798,6 +817,10 @@ static void free_assigned_device(AssignedDevice *dev)
dev->real_device.config_fd = 0;
}
+if (dev->r_name) {
+qemu_free(dev->r_name);
+}
+
#ifdef KVM_CAP_IRQ_ROUTING
free_dev_irq_entries(dev);
#endif
@@ -885,7 +908,7 @@ static int assign_device(AssignedDevice *dev)
if (dev->use_iommu) {
if (!kvm_check_extension(kvm_state, KVM_CAP_IOMMU)) {
fprintf(stderr, "No IOMMU found. Unable to assign device
\"%s\"\n",
-dev->dev.qdev.id);
+assigned_dev_name(dev));
return -ENODEV;
}
assigned_dev_data.flags |= KVM_DEV_ASSIGN_ENABLE_IOMMU;
@@ -897,7 +920,7 @@ static int assign_device(AssignedDevice *dev)
r = kvm_assign_pci_device(kvm_context, &assigned_dev_data);
if (r < 0) {
fprintf(stderr, "Failed to assign device \"%s\" : %s\n",
-dev->dev.qdev.id, strerror(-r));
+assigned_dev_name(dev), strerror(-r));
switch (r) {
case -EBUSY:
@@ -953,7 +976,7 @@ static int assign_irq(AssignedDevice *dev)
r = kvm_assign_irq(kvm_context, &assigned_irq_data);
if (r < 0) {
fprintf(stderr, "Failed to assign irq for \"%s\": %s\n",
-dev->dev.qdev.id, strerror(-r));
+assigned_dev_name(dev), strerror(-r));
fprintf(stderr, "Perhaps you are assigning a device "
"that shares an IRQ with another device?\n");
return r;
@@ -977,7 +1000,7 @@ static void deass
[PATCH] Update .gitignore
Add some files/directories to .gitignore - vapic.bin A generated binary file - libdis/ and libdis-user/ Directories generated in ./configure - QMP/qmp-commands.txt A generated text - qemu-options.def A generated file used as a source of qemu-options.h Signed-off-by: Hidetoshi Seto --- .gitignore |5 + 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/.gitignore b/.gitignore index 2d7f439..81b1ba4 100644 --- a/.gitignore +++ b/.gitignore @@ -9,6 +9,8 @@ config-target.* libhw32 libhw64 libuser +libdis +libdis-user qemu-doc.html qemu-tech.html qemu-doc.info @@ -21,11 +23,13 @@ qemu-img qemu-nbd qemu-nbd.8 qemu-nbd.pod +qemu-options.def qemu-options.texi qemu-img-cmds.texi qemu-img-cmds.h qemu-io qemu-monitor.texi +QMP/qmp-commands.txt .gdbinit *.a *.aux @@ -50,4 +54,5 @@ pc-bios/optionrom/linuxboot.bin pc-bios/optionrom/multiboot.bin pc-bios/optionrom/multiboot.raw pc-bios/optionrom/extboot.bin +pc-bios/optionrom/vapic.bin .stgit-* -- 1.7.0 -- 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 fail to parse command line with "-pcidevice 00:19.0"
(2010/06/24 15:08), Markus Armbruster wrote: > Note to qemu-devel: this issue is qemu-kvm only. > > "Hao, Xudong" writes: > >> When assign one PCI device, qemu fail to parse the command line: >> qemu-system_x86 -smp 2 -m 1024 -hda /path/to/img -pcidevice host=00:19.0 >> Error: >> qemu-system-x86_64: Parameter 'id' expects an identifier >> Identifiers consist of letters, digits, '-', '.', '_', starting with a >> letter. >> pcidevice argument parse error; please check the help text for usage >> Could not add assigned device host=00:19.0 >> >> https://bugs.launchpad.net/qemu/+bug/597932 >> >> This issue caused by qemu-kvm commit >> b560a9ab9be06afcbb78b3791ab836dad208a239. > > The bug is in add_assigned_device(): > > r = get_param_value(id, sizeof(id), "id", arg); > if (!r) > r = get_param_value(id, sizeof(id), "name", arg); > if (!r) > r = get_param_value(id, sizeof(id), "host", arg); > > We end up with invalid ID "00:19.0". ... Are there any strong reason why we cannot use ':' in the identifier? Thanks, H.Seto -- 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] QEMU: Update .gitignore
(2010/06/21 17:19), Avi Kivity wrote: > On 06/21/2010 08:24 AM, Hidetoshi Seto wrote: >> I think some people have noticed that: >> >>> $ ./configure >>> $ make >>> $ git status >>> # On branch master >>> # Untracked files: >>> # (use "git add ..." to include in what will be committed) >>> # >>> # QMP/qmp-commands.txt >>> # libdis-user/ >>> # libdis/ >>> # pc-bios/optionrom/vapic.bin >>> nothing added to commit but untracked files present (use "git add" to track) >>> >> Please consider applying this patch to qemu-kvm.git. > > This is equally applicable to qemu.git, so please sent it to the qemu > mailing list, qemu-de...@nongnu.org. Thanks for your advice, Avi. Now this mail is sent to qemu ML, w/ above quotes as short history. Could someone pick this up? Thanks, H.Seto = Subject: [PATCH] QEMU: Update .gitignore Add some files/directories to .gitignore - vapic.bin A generated binary file. - libdis/ and libdis-user/ These are directories generated by ./configure. - QMP/qmp-commands.txt A generated text. Signed-off-by: Hidetoshi Seto --- .gitignore |4 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/.gitignore b/.gitignore index 2d7f439..fa4f241 100644 --- a/.gitignore +++ b/.gitignore @@ -9,6 +9,8 @@ config-target.* libhw32 libhw64 libuser +libdis +libdis-user qemu-doc.html qemu-tech.html qemu-doc.info @@ -26,6 +28,7 @@ qemu-img-cmds.texi qemu-img-cmds.h qemu-io qemu-monitor.texi +QMP/qmp-commands.txt .gdbinit *.a *.aux @@ -50,4 +53,5 @@ pc-bios/optionrom/linuxboot.bin pc-bios/optionrom/multiboot.bin pc-bios/optionrom/multiboot.raw pc-bios/optionrom/extboot.bin +pc-bios/optionrom/vapic.bin .stgit-* -- 1.7.0 -- 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] Update .gitignore
I think some people have noticed that: > $ ./configure > $ make > $ git status > # On branch master > # Untracked files: > # (use "git add ..." to include in what will be committed) > # > # QMP/qmp-commands.txt > # libdis-user/ > # libdis/ > # pc-bios/optionrom/vapic.bin > nothing added to commit but untracked files present (use "git add" to track) Please consider applying this patch to qemu-kvm.git. Thanks, H.Seto = Subject: [PATCH] Update .gitignore Add some files/directories to .gitignore - vapic.bin A generated binary file. - libdis/ and libdis-user/ These are directories generated by ./configure. - QMP/qmp-commands.txt A generated text. Signed-off-by: Hidetoshi Seto --- .gitignore |4 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/.gitignore b/.gitignore index 2d7f439..fa4f241 100644 --- a/.gitignore +++ b/.gitignore @@ -9,6 +9,8 @@ config-target.* libhw32 libhw64 libuser +libdis +libdis-user qemu-doc.html qemu-tech.html qemu-doc.info @@ -26,6 +28,7 @@ qemu-img-cmds.texi qemu-img-cmds.h qemu-io qemu-monitor.texi +QMP/qmp-commands.txt .gdbinit *.a *.aux @@ -50,4 +53,5 @@ pc-bios/optionrom/linuxboot.bin pc-bios/optionrom/multiboot.bin pc-bios/optionrom/multiboot.raw pc-bios/optionrom/extboot.bin +pc-bios/optionrom/vapic.bin .stgit-* -- 1.7.0 -- 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] device-assignment, msi: PBA is long
Accidentally a pci_read_long() was replaced with assigned_dev_pci_read_byte() by the commit: commit a81a1f0a7410976be7dbc9a81524a8640f446ab5 Author: Alex Williamson device-assignment: Don't use libpci Signed-off-by: Hidetoshi Seto --- hw/device-assignment.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/hw/device-assignment.c b/hw/device-assignment.c index e254203..e237bd3 100644 --- a/hw/device-assignment.c +++ b/hw/device-assignment.c @@ -1263,7 +1263,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev) pci_dev->cap.length + PCI_MSIX_TABLE) = msix_table_entry; *(uint32_t *)(pci_dev->config + pci_dev->cap.start + pci_dev->cap.length + PCI_MSIX_PBA) = -assigned_dev_pci_read_byte(pci_dev, pos + PCI_MSIX_PBA); +assigned_dev_pci_read_long(pci_dev, pos + PCI_MSIX_PBA); bar_nr = msix_table_entry & PCI_MSIX_BIR; msix_table_entry &= ~PCI_MSIX_BIR; dev->msix_table_addr = pci_region[bar_nr].base_addr + msix_table_entry; -- 1.7.0 -- 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
