Re: [PATCH 1/8] KVM: Add MSI_ACTION flag for assigned irq
On Wed, Dec 24, 2008 at 10:24:34AM +0800, Sheng Yang wrote:
On Wednesday 24 December 2008 01:32:24 Marcelo Tosatti wrote:
On Tue, Dec 23, 2008 at 04:00:24PM +0800, Sheng Yang wrote:
For MSI disable feature later.
Notice I changed ABI here, but due to no userspace patch, I think it's
OK.
Signed-off-by: Sheng Yang sh...@linux.intel.com
---
include/linux/kvm.h |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index ef7f98e..5b965f6 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -544,6 +544,7 @@ struct kvm_assigned_irq {
#define KVM_DEV_ASSIGN_ENABLE_IOMMU (1 0)
-#define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI(1 0)
+#define KVM_DEV_IRQ_ASSIGN_MSI_ACTION(1 0)
+#define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI(1 1)
This is a little confusing. KVM_DEV_IRQ_ASSIGN_MSI_ACTION is assigned
from userspace, and in the patchset used in conjunction with msi2intx
which is a module parameter.
Is there anything that blocks control of msi2intx translate behaviour
from userspace?
Perhaps add a KVM_DEV_IRQ_UNASSIGN ioctl, and pass the desired
guest/host irq types on the IRQ_ASSIGN ioctl, thus removing some of the
kernel complexity.
Marcelo, Thanks for reviewing.
msi2intx module parameter is mostly a debug assist rather than a real option,
so we won't want this to be a real option controlled by userspace program.
It seems msi2intx (as a module parameter) is not necessary.
You can achieve the same from userspace by configuring
KVM_DEV_IRQ_ASSIGN_ENABLE_MSI/MSI_ACTION, no? The module parameter seems
a weird interface to me (even if it is a debug one).
But its not a big deal, if you think it is worthwhile to keep it that
way.
--
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 4/8] KVM: Add a route layer to convert MSI message to GSI
On Thu, Dec 25, 2008 at 09:59:45AM +0800, Sheng Yang wrote:
+ found_msg = kvm_find_gsi_msg(kvm, gsi_msg-gsi);
+ if (found_msg)
+ *found_msg = *gsi_msg;
+ else {
+ gsi = find_first_zero_bit(kvm-gsi_msg_bitmap,
KVM_NR_GSI_MSG);
+ if (gsi = KVM_NR_GSI_MSG) {
+ r = -EFAULT;
ENOSPC?
OK. (Though I am confusing with all kinds of ERR all the time, ENOSPC show
No space left in the device... And last time somebody told me ENOTTY
means something is not available...)
Not sure what the most appropriate error is here.
+static int kvm_vm_ioctl_request_gsi_msg(struct kvm *kvm,
+ struct kvm_assigned_gsi_msg
*agsi_msg)
+{
+ struct kvm_gsi_msg gsi_msg;
+ int r;
+
+ gsi_msg.gsi = agsi_msg-gsi;
+ gsi_msg.msg.address_lo = agsi_msg-msg.addr_lo;
+ gsi_msg.msg.address_hi = agsi_msg-msg.addr_hi;
+ gsi_msg.msg.data = agsi_msg-msg.data;
+
+ r = kvm_update_gsi_msg(kvm, gsi_msg);
+ if (r == 0)
+ agsi_msg-gsi = gsi_msg.gsi;
+ return r;
+}
Can't see the purpose of this function. Why preserve the user-passed GSI
value in case of failure? It will return an error anyway...
Oh, we preserved the user-passed GSI in case of userspace want to update one
entry. :)
The structure is not copied back to userspace in case of failure anyway:
+ r = kvm_vm_ioctl_request_gsi_msg(kvm, agsi_msg);
+ if (r)
+ goto out;
+ r = -EFAULT;
+ if (copy_to_user(argp, agsi_msg, sizeof agsi_msg))
+ goto out;
So I don't see the point of doing this?
--
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
Fix compilation with -Werror=format-security
Mandriva is now using the -Werror=format-security CFLAG by default. To make kvm 82 compile with this option, I had to apply this patch: http://svn.mandriva.com/cgi-bin/viewvc.cgi/packages/cooker/kvm/current/SOURCES/kvm-82-format-string.patch?view=co By the way, is there any reason why the following patch was never applied? http://lists.gnu.org/archive/html/qemu-devel/2008-11/msg00116.html -- Frederik Himpe -- 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 15/15] KVM: Fix racy in kvm_free_assigned_irq
On Fri, Dec 26, 2008 at 10:30:07AM +0800, Sheng Yang wrote: Thanks to Marcelo's observation, The following code have potential issue: if (cancel_work_sync(assigned_dev-interrupt_work)) kvm_put_kvm(kvm); In fact, cancel_work_sync() would return true either work struct is only scheduled or the callback of work struct is executed. This code only consider the former situation. Why not simply drop the reference inc / dec from irq handler/work function? Just make sure that there is no queued/executing work left behind on vm shutdown. Don't think an additional reference is necessary. Or am I missing something? Also, we have a window between cancel_work_sync() and free_irq. This one looks OK. -- 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 15/15] KVM: Fix racy in kvm_free_assigned_irq
On Sat, Dec 27, 2008 at 06:06:26PM -0200, Marcelo Tosatti wrote: On Fri, Dec 26, 2008 at 10:30:07AM +0800, Sheng Yang wrote: Thanks to Marcelo's observation, The following code have potential issue: if (cancel_work_sync(assigned_dev-interrupt_work)) kvm_put_kvm(kvm); In fact, cancel_work_sync() would return true either work struct is only scheduled or the callback of work struct is executed. This code only consider the former situation. Why not simply drop the reference inc / dec from irq handler/work function? Just make sure that there is no queued/executing work left behind on vm shutdown. Don't think an additional reference is necessary. Or am I missing something? And maybe drop this issue from the patchset and fix it separately, since it is a bugfix. -- 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] Fix compilation with -Werror=format-security
On Sat, Dec 27, 2008 at 07:52:39PM +, Frederik Himpe wrote:
Mandriva is now using the -Werror=format-security CFLAG by default. To
make kvm 82 compile with this option, I had to apply this patch:
http://svn.mandriva.com/cgi-bin/viewvc.cgi/packages/cooker/kvm/current/SOURCES/kvm-82-format-string.patch?view=co
diff -ur kvm-82.orig/qemu/monitor.c kvm-82/qemu/monitor.c
--- kvm-82.orig/qemu/monitor.c 2008-12-24 15:24:58.0 +0100
+++ kvm-82/qemu/monitor.c 2008-12-27 20:19:32.0 +0100
@@ -1975,7 +1975,7 @@
static void expr_error(const char *fmt)
{
-term_printf(fmt);
+term_printf(%s, fmt);
term_printf(\n);
longjmp(expr_env, 1);
}
Why not make this term_printf(%s\n, fmt); while you're at it?
--
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 0/4] Remove interrupt stack table usage from x86_64 kernel (v2)
Ingo Molnar wrote: * Ingo Molnar mi...@elte.hu wrote: They have the following commit IDs, and they are also in tip/master: 921e521: x86: move NMI back to interrupt stack 36ef6c9: x86: make interrupt stack switching atomic dd64891: x86: consolidate irq stack switching to a single macro 955a368: x86: drop the use of the tss interrupt stack table (IST) I also started testing them in tip-qa. testing failed quickly, the attached config crashes. I've pushed out the bad kernel to the tip/tmp.master.bad branch: fe3aac9: Merge branch 'x86/irq' (no crashlog available - all i know that the box crashed and rebooted, when booted with the bzImage built out of the attached config.) I managed to reproduce this (after several hundred million interrupts and several dozen million NMIs - do you run anything special on your tests?) -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain. -- 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 0/5] for_each_shadow_entry
On Thu, Dec 25, 2008 at 03:23:34PM +0200, Avi Kivity wrote: This patchset replaces walk_shadow(), which calls a callback for each shadow pte that maps a guest virtal address, by an equivalent for_each style construct. Benefits are less thunks and smaller code. Please review. Looks good. -- 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
