From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NZUf0-0006Rf-6Z for qemu-devel@nongnu.org; Mon, 25 Jan 2010 14:31:42 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NZUev-0006Po-Km for qemu-devel@nongnu.org; Mon, 25 Jan 2010 14:31:41 -0500 Received: from [199.232.76.173] (port=43185 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NZUev-0006Pi-C1 for qemu-devel@nongnu.org; Mon, 25 Jan 2010 14:31:37 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37028) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NZUeu-0000rH-SG for qemu-devel@nongnu.org; Mon, 25 Jan 2010 14:31:37 -0500 Date: Mon, 25 Jan 2010 21:28:27 +0200 From: "Michael S. Tsirkin" Message-ID: <20100125192827.GE11998@redhat.com> References: <20100111171703.GC11936@redhat.com> <4B4CF925.80602@codemonkey.ws> <20100125192015.GD11998@redhat.com> <4B5DF0F1.9080108@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4B5DF0F1.9080108@codemonkey.ws> Subject: [Qemu-devel] Re: [PATCH-RFC 02/13] kvm: add API to set ioeventfd List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: qemu-devel@nongnu.org On Mon, Jan 25, 2010 at 01:28:49PM -0600, Anthony Liguori wrote: > On 01/25/2010 01:20 PM, Michael S. Tsirkin wrote: >> On Tue, Jan 12, 2010 at 04:35:17PM -0600, Anthony Liguori wrote: >> >>> On 01/11/2010 11:17 AM, Michael S. Tsirkin wrote: >>> >>>> Signed-off-by: Michael S. Tsirkin >>>> --- >>>> kvm-all.c | 24 ++++++++++++++++++++++++ >>>> kvm.h | 3 +++ >>>> 2 files changed, 27 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/kvm-all.c b/kvm-all.c >>>> index a312654..aa00119 100644 >>>> --- a/kvm-all.c >>>> +++ b/kvm-all.c >>>> @@ -1113,3 +1113,27 @@ void kvm_remove_all_breakpoints(CPUState *current_env) >>>> { >>>> } >>>> #endif /* !KVM_CAP_SET_GUEST_DEBUG */ >>>> + >>>> +#ifdef KVM_IOEVENTFD >>>> +int kvm_set_ioeventfd(uint16_t addr, uint16_t data, int fd, bool assigned) >>>> +{ >>>> + struct kvm_ioeventfd kick = { >>>> + .datamatch = data, >>>> + .addr = addr, >>>> + .len = 2, >>>> + .flags = KVM_IOEVENTFD_FLAG_DATAMATCH | KVM_IOEVENTFD_FLAG_PIO, >>>> + .fd = fd, >>>> + }; >>>> + if (!assigned) >>>> + kick.flags |= KVM_IOEVENTFD_FLAG_DEASSIGN; >>>> + int r = kvm_vm_ioctl(kvm_state, KVM_IOEVENTFD,&kick); >>>> + if (r< 0) >>>> + return r; >>>> + return 0; >>>> +} >>>> >>>> >>> I think we really ought to try to avoid having global state used here. >>> That means either we need to pass a CPUState to this function or we need >>> to have a ioeventfd be allocated as a structure that is then passed >>> around that can store a copy of the kvm_state fetched through CPUState. >>> >>> I think the later is best. We really don't want ioeventfd scattered >>> throughout the code. >>> >>> Regards, >>> >>> Anthony Liguori >>> >> >> I don't really understand this comment. >> I have no idea where to get CPUState in >> a device and how to get kvm state from there. >> > > This function doesn't seem to get called in the current patches so it's > hard to evaluate what the right thing to do is. >> And all other kvm-all functions use kvm_state >> already, let's fix them first if we want to >> get rid of global state? >> > > Any time an operation is tied to a CPU in some way, we should not rely > on global state. Based on the name and the fact that it references PIO, > it strongly suggests to me that it's somehow related to a CPU but since > it's not used in the current code it's hard to validate that. > > Regards, > > Anthony Liguori It is called in the binding. Pls take a look. The function does not reference a specific CPU: it binds an eventfd descriptor to a specific address/data pair for all CPUs. > >> Avi, what's your take on this patch? >> >>