| Message ID | 20100331182031.GA5200@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 03/31/2010 01:20 PM, Michael S. Tsirkin wrote: > From: David L Stevens<dlstevens@us.ibm.com> > > vhost driver in qemu didn't ack features, and this happens > to work because we don't really require any features. However, > it's better not to rely on this. This patch passes features to > vhost as guest acks them. > > Signed-off-by: David L Stevens<dlstevens@us.ibm.com> > Signed-off-by: Michael S. Tsirkin<mst@redhat.com> > --- > > Anthony, here's a fixup patch to address an issue in vhost > patches. Incidentially, what's the status of the vhost patchset? > http://repo.or.cz/w/qemu/aliguori-queue.git vhost Is what I'm currently testing. With vhost disabled, the following seg faults: qemu-system-x86_64 -hda ~/images/linux.img -net tap -net nic,model=virtio -enable-kvm But not when using TCG. I'm not sure that it's your patches at fault and I'm attempting to bisect now to figure that out. Regards, Anthony Liguori > hw/virtio-net.c | 8 ++++++++ > 1 files changed, 8 insertions(+), 0 deletions(-) > > diff --git a/hw/virtio-net.c b/hw/virtio-net.c > index 9ddd58c..4c7c46e 100644 > --- a/hw/virtio-net.c > +++ b/hw/virtio-net.c > @@ -218,6 +218,14 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features) > (features>> VIRTIO_NET_F_GUEST_ECN)& 1, > (features>> VIRTIO_NET_F_GUEST_UFO)& 1); > } > + if (!n->nic->nc.peer || > + n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) { > + return; > + } > + if (!tap_get_vhost_net(n->nic->nc.peer)) { > + return; > + } > + return vhost_net_ack_features(tap_get_vhost_net(n->nic->nc.peer), features); > } > > static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd, >
On Wed, 31 Mar 2010 13:26:23 -0500 Anthony Liguori <anthony@codemonkey.ws> wrote: > On 03/31/2010 01:20 PM, Michael S. Tsirkin wrote: > > From: David L Stevens<dlstevens@us.ibm.com> > > > > vhost driver in qemu didn't ack features, and this happens > > to work because we don't really require any features. However, > > it's better not to rely on this. This patch passes features to > > vhost as guest acks them. > > > > Signed-off-by: David L Stevens<dlstevens@us.ibm.com> > > Signed-off-by: Michael S. Tsirkin<mst@redhat.com> > > --- > > > > Anthony, here's a fixup patch to address an issue in vhost > > patches. Incidentially, what's the status of the vhost patchset? > > > > http://repo.or.cz/w/qemu/aliguori-queue.git vhost > > Is what I'm currently testing. With vhost disabled, the following seg > faults: > > qemu-system-x86_64 -hda ~/images/linux.img -net tap -net > nic,model=virtio -enable-kvm > > But not when using TCG. I'm not sure that it's your patches at fault > and I'm attempting to bisect now to figure that out. Probably this is the same segfault I'm getting right now in master, bisect says it's: """ commit ad96090a01d848df67d70c5259ed8aa321fa8716 Author: Blue Swirl <blauwirbel@gmail.com> Date: Mon Mar 29 19:23:52 2010 +0000 Refactor target specific handling, compile vl.c only once """ > > Regards, > > Anthony Liguori > > > hw/virtio-net.c | 8 ++++++++ > > 1 files changed, 8 insertions(+), 0 deletions(-) > > > > diff --git a/hw/virtio-net.c b/hw/virtio-net.c > > index 9ddd58c..4c7c46e 100644 > > --- a/hw/virtio-net.c > > +++ b/hw/virtio-net.c > > @@ -218,6 +218,14 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features) > > (features>> VIRTIO_NET_F_GUEST_ECN)& 1, > > (features>> VIRTIO_NET_F_GUEST_UFO)& 1); > > } > > + if (!n->nic->nc.peer || > > + n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) { > > + return; > > + } > > + if (!tap_get_vhost_net(n->nic->nc.peer)) { > > + return; > > + } > > + return vhost_net_ack_features(tap_get_vhost_net(n->nic->nc.peer), features); > > } > > > > static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd, > > > > >
On Wed, Mar 31, 2010 at 03:38:05PM -0300, Luiz Capitulino wrote: > On Wed, 31 Mar 2010 13:26:23 -0500 > Anthony Liguori <anthony@codemonkey.ws> wrote: > > > On 03/31/2010 01:20 PM, Michael S. Tsirkin wrote: > > > From: David L Stevens<dlstevens@us.ibm.com> > > > > > > vhost driver in qemu didn't ack features, and this happens > > > to work because we don't really require any features. However, > > > it's better not to rely on this. This patch passes features to > > > vhost as guest acks them. > > > > > > Signed-off-by: David L Stevens<dlstevens@us.ibm.com> > > > Signed-off-by: Michael S. Tsirkin<mst@redhat.com> > > > --- > > > > > > Anthony, here's a fixup patch to address an issue in vhost > > > patches. Incidentially, what's the status of the vhost patchset? > > > > > > > http://repo.or.cz/w/qemu/aliguori-queue.git vhost > > > > Is what I'm currently testing. With vhost disabled, the following seg > > faults: > > > > qemu-system-x86_64 -hda ~/images/linux.img -net tap -net > > nic,model=virtio -enable-kvm > > > > But not when using TCG. I'm not sure that it's your patches at fault > > and I'm attempting to bisect now to figure that out. > > Probably this is the same segfault I'm getting right now in master, > bisect says it's: > > """ > commit ad96090a01d848df67d70c5259ed8aa321fa8716 > Author: Blue Swirl <blauwirbel@gmail.com> > Date: Mon Mar 29 19:23:52 2010 +0000 > > Refactor target specific handling, compile vl.c only once > """ Why are the compile once patches helpful? They seem to introduce churn and bugs, they actively make it harder to extend qemu as you can't use target-specific code in code that is compiled once, they might have performance penalty - and what do we gain? Any given user is unlikely to need to build on more than one target, distros have enough computing power to build in parallel. Maybe it makes sense to revert the compile once patches, and discuss these issues before re-commit?
On 03/31/2010 02:07 PM, Michael S. Tsirkin wrote: > On Wed, Mar 31, 2010 at 03:38:05PM -0300, Luiz Capitulino wrote: > >> On Wed, 31 Mar 2010 13:26:23 -0500 >> Anthony Liguori<anthony@codemonkey.ws> wrote: >> >> >>> On 03/31/2010 01:20 PM, Michael S. Tsirkin wrote: >>> >>>> From: David L Stevens<dlstevens@us.ibm.com> >>>> >>>> vhost driver in qemu didn't ack features, and this happens >>>> to work because we don't really require any features. However, >>>> it's better not to rely on this. This patch passes features to >>>> vhost as guest acks them. >>>> >>>> Signed-off-by: David L Stevens<dlstevens@us.ibm.com> >>>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com> >>>> --- >>>> >>>> Anthony, here's a fixup patch to address an issue in vhost >>>> patches. Incidentially, what's the status of the vhost patchset? >>>> >>>> >>> http://repo.or.cz/w/qemu/aliguori-queue.git vhost >>> >>> Is what I'm currently testing. With vhost disabled, the following seg >>> faults: >>> >>> qemu-system-x86_64 -hda ~/images/linux.img -net tap -net >>> nic,model=virtio -enable-kvm >>> >>> But not when using TCG. I'm not sure that it's your patches at fault >>> and I'm attempting to bisect now to figure that out. >>> >> Probably this is the same segfault I'm getting right now in master, >> bisect says it's: >> >> """ >> commit ad96090a01d848df67d70c5259ed8aa321fa8716 >> Author: Blue Swirl<blauwirbel@gmail.com> >> Date: Mon Mar 29 19:23:52 2010 +0000 >> >> Refactor target specific handling, compile vl.c only once >> """ >> > Why are the compile once patches helpful? They seem to introduce > churn and bugs, they actively make it harder to extend qemu as you can't use > target-specific code in code that is compiled once, they might have > performance penalty - and what do we gain? Any given user is unlikely to > need to build on more than one target, distros have enough computing > power to build in parallel. > > Maybe it makes sense to revert the compile once patches, and discuss > these issues before re-commit? > Compiling objects once is certainly useful. Long term, I think most of us want to see a single qemu executable that works for all architectures and compiling once is an important step in that direction. With respect to regressions, it might make sense to slow down these refactorings a bit and increase the amount of regression testing that is happening during them. Regards, Anthony Liguori
On Wed, Mar 31, 2010 at 02:24:51PM -0500, Anthony Liguori wrote: > Long term, I think most of us want to see a single qemu executable > that works for all architectures and compiling once is an important > step in that direction. I'm not so sure. It's pretty low on my list of priorities. Most users only need one target, speed of execution and/or features is likely much more important for them, and these refactorings make code more generic and harder to extend.s
On 03/31/2010 02:25 PM, Michael S. Tsirkin wrote: > On Wed, Mar 31, 2010 at 02:24:51PM -0500, Anthony Liguori wrote: > >> Long term, I think most of us want to see a single qemu executable >> that works for all architectures and compiling once is an important >> step in that direction. >> > I'm not so sure. It's pretty low on my list of priorities. Most users only need > one target, speed of execution and/or features is likely much more important for them, > and these refactorings make code more generic and harder to extend.s > We ought to have a set of device models that are compiled once, with well defined interfaces that model the actual way the various buses communicate. This should all roll into a generic CPU API. Then we should have a set of CPU implementations with choices including various TCG targets and KVM targets. You can still compile out TCG targets that you don't care about but the key point is to get all of these interfaces correct. This refactoring effort isn't really paying attention to improving interfaces which I think is a bit problematic. Regards, Anthony Liguori
On 3/31/10, Michael S. Tsirkin <mst@redhat.com> wrote: > On Wed, Mar 31, 2010 at 03:38:05PM -0300, Luiz Capitulino wrote: > > On Wed, 31 Mar 2010 13:26:23 -0500 > > Anthony Liguori <anthony@codemonkey.ws> wrote: > > > > > On 03/31/2010 01:20 PM, Michael S. Tsirkin wrote: > > > > From: David L Stevens<dlstevens@us.ibm.com> > > > > > > > > vhost driver in qemu didn't ack features, and this happens > > > > to work because we don't really require any features. However, > > > > it's better not to rely on this. This patch passes features to > > > > vhost as guest acks them. > > > > > > > > Signed-off-by: David L Stevens<dlstevens@us.ibm.com> > > > > Signed-off-by: Michael S. Tsirkin<mst@redhat.com> > > > > --- > > > > > > > > Anthony, here's a fixup patch to address an issue in vhost > > > > patches. Incidentially, what's the status of the vhost patchset? > > > > > > > > > > http://repo.or.cz/w/qemu/aliguori-queue.git vhost > > > > > > Is what I'm currently testing. With vhost disabled, the following seg > > > faults: > > > > > > qemu-system-x86_64 -hda ~/images/linux.img -net tap -net > > > nic,model=virtio -enable-kvm > > > > > > But not when using TCG. I'm not sure that it's your patches at fault > > > and I'm attempting to bisect now to figure that out. > > > > Probably this is the same segfault I'm getting right now in master, > > bisect says it's: > > > > """ > > commit ad96090a01d848df67d70c5259ed8aa321fa8716 > > Author: Blue Swirl <blauwirbel@gmail.com> > > Date: Mon Mar 29 19:23:52 2010 +0000 > > > > Refactor target specific handling, compile vl.c only once > > """ > > Why are the compile once patches helpful? They seem to introduce > churn and bugs, they actively make it harder to extend qemu as you can't use > target-specific code in code that is compiled once, they might have > performance penalty - and what do we gain? Any given user is unlikely to > need to build on more than one target, distros have enough computing > power to build in parallel. As has been explained many times, knowledge about CPU specific features has no place in devices. Actively removing CPU specifics from devices is good but preventing new bad code to be committed is better. I don't have distro computing powers (unless some distro's compute center only has two low power machines) and I guess some other developers don't have either. All developers and patch submitters are expected to compile all targets. This patch set has decreased the number of files compiled by about 20%. > Maybe it makes sense to revert the compile once patches, and discuss > these issues before re-commit? Maybe I'll try to remember this as your favourite problem solving approach if I happen to dislike the changes your patches may cause in the future. ;-)
On 3/31/10, Anthony Liguori <anthony@codemonkey.ws> wrote: > On 03/31/2010 02:07 PM, Michael S. Tsirkin wrote: > > > On Wed, Mar 31, 2010 at 03:38:05PM -0300, Luiz Capitulino wrote: > > > > > > > On Wed, 31 Mar 2010 13:26:23 -0500 > > > Anthony Liguori<anthony@codemonkey.ws> wrote: > > > > > > > > > > > > > On 03/31/2010 01:20 PM, Michael S. Tsirkin wrote: > > > > > > > > > > > > > From: David L Stevens<dlstevens@us.ibm.com> > > > > > > > > > > vhost driver in qemu didn't ack features, and this happens > > > > > to work because we don't really require any features. However, > > > > > it's better not to rely on this. This patch passes features to > > > > > vhost as guest acks them. > > > > > > > > > > Signed-off-by: David L Stevens<dlstevens@us.ibm.com> > > > > > Signed-off-by: Michael S. Tsirkin<mst@redhat.com> > > > > > --- > > > > > > > > > > Anthony, here's a fixup patch to address an issue in vhost > > > > > patches. Incidentially, what's the status of the vhost patchset? > > > > > > > > > > > > > > > > > > > > http://repo.or.cz/w/qemu/aliguori-queue.git > vhost > > > > > > > > Is what I'm currently testing. With vhost disabled, the following > seg > > > > faults: > > > > > > > > qemu-system-x86_64 -hda ~/images/linux.img -net tap -net > > > > nic,model=virtio -enable-kvm > > > > > > > > But not when using TCG. I'm not sure that it's your patches at fault > > > > and I'm attempting to bisect now to figure that out. > > > > > > > > > > > Probably this is the same segfault I'm getting right > now in master, > > > bisect says it's: > > > > > > """ > > > commit ad96090a01d848df67d70c5259ed8aa321fa8716 > > > Author: Blue Swirl<blauwirbel@gmail.com> > > > Date: Mon Mar 29 19:23:52 2010 +0000 > > > > > > Refactor target specific handling, compile vl.c only once > > > """ > > > > > > > > Why are the compile once patches helpful? They seem to > introduce > > churn and bugs, they actively make it harder to extend qemu as you can't > use > > target-specific code in code that is compiled once, they might have > > performance penalty - and what do we gain? Any given user is unlikely to > > need to build on more than one target, distros have enough computing > > power to build in parallel. > > > > Maybe it makes sense to revert the compile once patches, and discuss > > these issues before re-commit? > > > > > > Compiling objects once is certainly useful. Long term, I think most of us > want to see a single qemu executable that works for all architectures and > compiling once is an important step in that direction. > > With respect to regressions, it might make sense to slow down these > refactorings a bit and increase the amount of regression testing that is > happening during them. I think there are only a few useful refactorings left. MIPS was interesting because of fourfold savings, likewise triple savings with PPC. Refactoring i386/x86_64 devices may be worthwhile, the rest not.
On 3/31/10, Anthony Liguori <anthony@codemonkey.ws> wrote: > On 03/31/2010 02:25 PM, Michael S. Tsirkin wrote: > > > On Wed, Mar 31, 2010 at 02:24:51PM -0500, Anthony Liguori wrote: > > > > > > > Long term, I think most of us want to see a single qemu executable > > > that works for all architectures and compiling once is an important > > > step in that direction. > > > > > > > > I'm not so sure. It's pretty low on my list of priorities. Most users only > need > > one target, speed of execution and/or features is likely much more > important for them, > > and these refactorings make code more generic and harder to extend.s > > > > > > We ought to have a set of device models that are compiled once, with well > defined interfaces that model the actual way the various buses communicate. > This should all roll into a generic CPU API. Then we should have a set of > CPU implementations with choices including various TCG targets and KVM > targets. > > You can still compile out TCG targets that you don't care about but the key > point is to get all of these interfaces correct. > > This refactoring effort isn't really paying attention to improving > interfaces which I think is a bit problematic. I agree, but with improved memory API, the questionable byte swapping could be eliminated from devices. Do you plan to finish your earlier RFC patches? Were there problems with them?
On 03/31/2010 02:54 PM, Blue Swirl wrote: > >> This refactoring effort isn't really paying attention to improving >> interfaces which I think is a bit problematic. >> > I agree, but with improved memory API, the questionable byte swapping > could be eliminated from devices. Do you plan to finish your earlier > RFC patches? Were there problems with them? > It's a matter of figuring out a way to do it that makes everyone happy :-) Regards, Anthony Liguori
On Wed, Mar 31, 2010 at 10:25:53PM +0300, Michael S. Tsirkin wrote: > On Wed, Mar 31, 2010 at 02:24:51PM -0500, Anthony Liguori wrote: > > Long term, I think most of us want to see a single qemu executable > > that works for all architectures and compiling once is an important > > step in that direction. > > I'm not so sure. It's pretty low on my list of priorities. Most users only need > one target, speed of execution and/or features is likely much more important for them, > and these refactorings make code more generic and harder to extend.s Depends on the user, I suppose. Having 32-bit and 64-bit variants of the same architecture in a single binary would be useful. Having big-endian and little-endian variants of an architecture supported in a single binary would be useful (bonus points if you don't duplicate target-specific code excessively). Having a gigantic all-singing, all-dancing QEMU is perhaps not so useful, but between there and where we are now, there are definitely useful points. -Nathan
On Wed, Mar 31, 2010 at 02:24:51PM -0500, Anthony Liguori wrote: > On 03/31/2010 02:07 PM, Michael S. Tsirkin wrote: >> On Wed, Mar 31, 2010 at 03:38:05PM -0300, Luiz Capitulino wrote: >> >>> On Wed, 31 Mar 2010 13:26:23 -0500 >>> Anthony Liguori<anthony@codemonkey.ws> wrote: >>> >>> >>>> On 03/31/2010 01:20 PM, Michael S. Tsirkin wrote: >>>> >>>>> From: David L Stevens<dlstevens@us.ibm.com> >>>>> >>>>> vhost driver in qemu didn't ack features, and this happens >>>>> to work because we don't really require any features. However, >>>>> it's better not to rely on this. This patch passes features to >>>>> vhost as guest acks them. >>>>> >>>>> Signed-off-by: David L Stevens<dlstevens@us.ibm.com> >>>>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com> >>>>> --- >>>>> >>>>> Anthony, here's a fixup patch to address an issue in vhost >>>>> patches. Incidentially, what's the status of the vhost patchset? >>>>> >>>>> >>>> http://repo.or.cz/w/qemu/aliguori-queue.git vhost >>>> >>>> Is what I'm currently testing. With vhost disabled, the following seg >>>> faults: >>>> >>>> qemu-system-x86_64 -hda ~/images/linux.img -net tap -net >>>> nic,model=virtio -enable-kvm >>>> >>>> But not when using TCG. I'm not sure that it's your patches at fault >>>> and I'm attempting to bisect now to figure that out. >>>> >>> Probably this is the same segfault I'm getting right now in master, >>> bisect says it's: >>> >>> """ >>> commit ad96090a01d848df67d70c5259ed8aa321fa8716 >>> Author: Blue Swirl<blauwirbel@gmail.com> >>> Date: Mon Mar 29 19:23:52 2010 +0000 >>> >>> Refactor target specific handling, compile vl.c only once >>> """ >>> >> Why are the compile once patches helpful? They seem to introduce >> churn and bugs, they actively make it harder to extend qemu as you can't use >> target-specific code in code that is compiled once, they might have >> performance penalty - and what do we gain? Any given user is unlikely to >> need to build on more than one target, distros have enough computing >> power to build in parallel. >> >> Maybe it makes sense to revert the compile once patches, and discuss >> these issues before re-commit? >> > > Compiling objects once is certainly useful. Long term, I think most of > us want to see a single qemu executable that works for all architectures > and compiling once is an important step in that direction. > While it probably make sense to achieve this goal, it doesn't mean it should be done the dirty way. For example it is known for a lot of time that the solution for the bswap in the device code is to add a bus model doing the byteswapping. Removing the #ifdef by deciding "this device will only be big/little endian" doesn't seem to go in the right direction.
On 03/31/2010 05:45 PM, Aurelien Jarno wrote: > While it probably make sense to achieve this goal, it doesn't mean it > should be done the dirty way. > > For example it is known for a lot of time that the solution for the > bswap in the device code is to add a bus model doing the byteswapping. > Removing the #ifdef by deciding "this device will only be big/little > endian" doesn't seem to go in the right direction. > Yeah, I'm having real trouble with the KVM regression. I thought I had it fixed but linux-user really made a mess of things. There's no simple solution that doesn't require quite a bit of refactoring which I'd rather do in a less ugly way. We've already been discussing getting rid of all the kvm_enabled() stuff and I think doing that properly is going to be needed to handle this correctly. I'm thinking we should back out the vl.c changes and try to clean up the KVM bits first. Does that sound reasonable blueswirl or can you think of a cleaner way to deal with kvm? Regards, Anthony Liguori
On 03/31/2010 09:45 PM, Anthony Liguori wrote: > On 03/31/2010 05:45 PM, Aurelien Jarno wrote: >> While it probably make sense to achieve this goal, it doesn't mean it >> should be done the dirty way. >> >> For example it is known for a lot of time that the solution for the >> bswap in the device code is to add a bus model doing the byteswapping. >> Removing the #ifdef by deciding "this device will only be big/little >> endian" doesn't seem to go in the right direction. > > Yeah, I'm having real trouble with the KVM regression. I thought I > had it fixed but linux-user really made a mess of things. There's no > simple solution that doesn't require quite a bit of refactoring which > I'd rather do in a less ugly way. We've already been discussing > getting rid of all the kvm_enabled() stuff and I think doing that > properly is going to be needed to handle this correctly. > > I'm thinking we should back out the vl.c changes and try to clean up > the KVM bits first. Does that sound reasonable blueswirl or can you > think of a cleaner way to deal with kvm? And back out can just mean making vl.c compile per-target (along with the thinko fix in acpi.c). That would be a nice temporary solution until we worked out the kvm_enabled() bits correctly. Regards, Anthony Liguori > Regards, > > Anthony Liguori >
On Wed, Mar 31, 2010 at 10:38:47PM +0300, Blue Swirl wrote: > > Maybe it makes sense to revert the compile once patches, and discuss > > these issues before re-commit? > > Maybe I'll try to remember this as your favourite problem solving > approach if I happen to dislike the changes your patches may cause in > the future. ;-) My patches are usually posted on list for a week or two before commit to the main tree. So if there's anything to dislike, you just need to post your objections, no reason to go through commit/revert churn.
On 4/1/10, Anthony Liguori <anthony@codemonkey.ws> wrote: > On 03/31/2010 05:45 PM, Aurelien Jarno wrote: > > > While it probably make sense to achieve this goal, it doesn't mean it > > should be done the dirty way. > > > > For example it is known for a lot of time that the solution for the > > bswap in the device code is to add a bus model doing the byteswapping. > > Removing the #ifdef by deciding "this device will only be big/little > > endian" doesn't seem to go in the right direction. > > > > > > Yeah, I'm having real trouble with the KVM regression. I thought I had it > fixed but linux-user really made a mess of things. There's no simple > solution that doesn't require quite a bit of refactoring which I'd rather do > in a less ugly way. We've already been discussing getting rid of all the > kvm_enabled() stuff and I think doing that properly is going to be needed to > handle this correctly. Strange, does linux-user use kvm.h (indirectly)?
On 04/01/2010 10:54 AM, Blue Swirl wrote: > On 4/1/10, Anthony Liguori<anthony@codemonkey.ws> wrote: > >> On 03/31/2010 05:45 PM, Aurelien Jarno wrote: >> >> >>> While it probably make sense to achieve this goal, it doesn't mean it >>> should be done the dirty way. >>> >>> For example it is known for a lot of time that the solution for the >>> bswap in the device code is to add a bus model doing the byteswapping. >>> Removing the #ifdef by deciding "this device will only be big/little >>> endian" doesn't seem to go in the right direction. >>> >>> >>> >> Yeah, I'm having real trouble with the KVM regression. I thought I had it >> fixed but linux-user really made a mess of things. There's no simple >> solution that doesn't require quite a bit of refactoring which I'd rather do >> in a less ugly way. We've already been discussing getting rid of all the >> kvm_enabled() stuff and I think doing that properly is going to be needed to >> handle this correctly. >> > Strange, does linux-user use kvm.h (indirectly)? > Yes, because various things in target-i386 use kvm.h. Regards, Anthony Liguori
On 4/1/10, Aurelien Jarno <aurelien@aurel32.net> wrote: > On Wed, Mar 31, 2010 at 02:24:51PM -0500, Anthony Liguori wrote: > > On 03/31/2010 02:07 PM, Michael S. Tsirkin wrote: > >> On Wed, Mar 31, 2010 at 03:38:05PM -0300, Luiz Capitulino wrote: > >> > >>> On Wed, 31 Mar 2010 13:26:23 -0500 > >>> Anthony Liguori<anthony@codemonkey.ws> wrote: > >>> > >>> > >>>> On 03/31/2010 01:20 PM, Michael S. Tsirkin wrote: > >>>> > >>>>> From: David L Stevens<dlstevens@us.ibm.com> > >>>>> > >>>>> vhost driver in qemu didn't ack features, and this happens > >>>>> to work because we don't really require any features. However, > >>>>> it's better not to rely on this. This patch passes features to > >>>>> vhost as guest acks them. > >>>>> > >>>>> Signed-off-by: David L Stevens<dlstevens@us.ibm.com> > >>>>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com> > >>>>> --- > >>>>> > >>>>> Anthony, here's a fixup patch to address an issue in vhost > >>>>> patches. Incidentially, what's the status of the vhost patchset? > >>>>> > >>>>> > >>>> http://repo.or.cz/w/qemu/aliguori-queue.git vhost > >>>> > >>>> Is what I'm currently testing. With vhost disabled, the following seg > >>>> faults: > >>>> > >>>> qemu-system-x86_64 -hda ~/images/linux.img -net tap -net > >>>> nic,model=virtio -enable-kvm > >>>> > >>>> But not when using TCG. I'm not sure that it's your patches at fault > >>>> and I'm attempting to bisect now to figure that out. > >>>> > >>> Probably this is the same segfault I'm getting right now in master, > >>> bisect says it's: > >>> > >>> """ > >>> commit ad96090a01d848df67d70c5259ed8aa321fa8716 > >>> Author: Blue Swirl<blauwirbel@gmail.com> > >>> Date: Mon Mar 29 19:23:52 2010 +0000 > >>> > >>> Refactor target specific handling, compile vl.c only once > >>> """ > >>> > >> Why are the compile once patches helpful? They seem to introduce > >> churn and bugs, they actively make it harder to extend qemu as you can't use > >> target-specific code in code that is compiled once, they might have > >> performance penalty - and what do we gain? Any given user is unlikely to > >> need to build on more than one target, distros have enough computing > >> power to build in parallel. > >> > >> Maybe it makes sense to revert the compile once patches, and discuss > >> these issues before re-commit? > >> > > > > Compiling objects once is certainly useful. Long term, I think most of > > us want to see a single qemu executable that works for all architectures > > and compiling once is an important step in that direction. > > > > > While it probably make sense to achieve this goal, it doesn't mean it > should be done the dirty way. > > For example it is known for a lot of time that the solution for the > bswap in the device code is to add a bus model doing the byteswapping. > Removing the #ifdef by deciding "this device will only be big/little > endian" doesn't seem to go in the right direction. That was not the goal of the patch, so it doesn't go in the right direction with regards to bus model. The bus model will need a lot of changes for the devices, but since we are not there yet, we don't know what would be the right changes either. My guess is that the need for byte swapping just disappears for devices. Then the changes done now may or may not be useful. Where I made separate byte swapping and non-swapping versions, perhaps passing the endianness flag and the swapping version will be deleted. The endianness flag may still be useful at board level to decide whether the byte twisting bus will be installed or not. In PPC cases, the bswap lines will be deleted. For the devices that I haven't touched, there are a few #ifdef lines more to be deleted.
On Wed, Mar 31, 2010 at 09:20:31PM +0300, Michael S. Tsirkin wrote: > From: David L Stevens <dlstevens@us.ibm.com> > > vhost driver in qemu didn't ack features, and this happens > to work because we don't really require any features. However, > it's better not to rely on this. This patch passes features to > vhost as guest acks them. > > Signed-off-by: David L Stevens <dlstevens@us.ibm.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > Anthony, here's a fixup patch to address an issue in vhost > patches. Incidentially, what's the status of the vhost patchset? > > > hw/virtio-net.c | 8 ++++++++ > 1 files changed, 8 insertions(+), 0 deletions(-) Just making sure this patch does not get lost in the noise: now that vhost net support is merged, please apply this patch on top. Thanks! > diff --git a/hw/virtio-net.c b/hw/virtio-net.c > index 9ddd58c..4c7c46e 100644 > --- a/hw/virtio-net.c > +++ b/hw/virtio-net.c > @@ -218,6 +218,14 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features) > (features >> VIRTIO_NET_F_GUEST_ECN) & 1, > (features >> VIRTIO_NET_F_GUEST_UFO) & 1); > } > + if (!n->nic->nc.peer || > + n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) { > + return; > + } > + if (!tap_get_vhost_net(n->nic->nc.peer)) { > + return; > + } > + return vhost_net_ack_features(tap_get_vhost_net(n->nic->nc.peer), features); > } > > static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd, > -- > 1.7.0.2.280.gc6f05
On Wed, Mar 31, 2010 at 09:20:31PM +0300, Michael S. Tsirkin wrote: > From: David L Stevens <dlstevens@us.ibm.com> > > vhost driver in qemu didn't ack features, and this happens > to work because we don't really require any features. However, > it's better not to rely on this. This patch passes features to > vhost as guest acks them. > > Signed-off-by: David L Stevens <dlstevens@us.ibm.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > Anthony, here's a fixup patch to address an issue in vhost > patches. Incidentially, what's the status of the vhost patchset? > Thanks, applied. > hw/virtio-net.c | 8 ++++++++ > 1 files changed, 8 insertions(+), 0 deletions(-) > > diff --git a/hw/virtio-net.c b/hw/virtio-net.c > index 9ddd58c..4c7c46e 100644 > --- a/hw/virtio-net.c > +++ b/hw/virtio-net.c > @@ -218,6 +218,14 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features) > (features >> VIRTIO_NET_F_GUEST_ECN) & 1, > (features >> VIRTIO_NET_F_GUEST_UFO) & 1); > } > + if (!n->nic->nc.peer || > + n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) { > + return; > + } > + if (!tap_get_vhost_net(n->nic->nc.peer)) { > + return; > + } > + return vhost_net_ack_features(tap_get_vhost_net(n->nic->nc.peer), features); > } > > static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd, > -- > 1.7.0.2.280.gc6f05 > > >
diff --git a/hw/virtio-net.c b/hw/virtio-net.c index 9ddd58c..4c7c46e 100644 --- a/hw/virtio-net.c +++ b/hw/virtio-net.c @@ -218,6 +218,14 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features) (features >> VIRTIO_NET_F_GUEST_ECN) & 1, (features >> VIRTIO_NET_F_GUEST_UFO) & 1); } + if (!n->nic->nc.peer || + n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) { + return; + } + if (!tap_get_vhost_net(n->nic->nc.peer)) { + return; + } + return vhost_net_ack_features(tap_get_vhost_net(n->nic->nc.peer), features); } static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,