* [PATCH] kvm tools: Ninja out support for VIRTIO_F_FEATURES_HIGH
@ 2011-12-06 8:45 Sasha Levin
2011-12-06 9:47 ` Pekka Enberg
0 siblings, 1 reply; 4+ messages in thread
From: Sasha Levin @ 2011-12-06 8:45 UTC (permalink / raw)
To: penberg; +Cc: mingo, gorcunov, asias.hejun, kvm, Sasha Levin, Rusty Russell
Rusty has just removed it out of the spec. Since we probably the only ones
who implemented support for it, we should remove it out of our code as well.
There is no issue with breaking anything since nothing else worked with it,
so it's fully backwards compatible.
Cc: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
tools/kvm/include/kvm/virtio.h | 3 +--
tools/kvm/virtio/core.c | 9 +--------
tools/kvm/virtio/pci.c | 4 ++--
3 files changed, 4 insertions(+), 12 deletions(-)
diff --git a/tools/kvm/include/kvm/virtio.h b/tools/kvm/include/kvm/virtio.h
index a7aa020..610f4d0 100644
--- a/tools/kvm/include/kvm/virtio.h
+++ b/tools/kvm/include/kvm/virtio.h
@@ -14,7 +14,6 @@
#define VIRTIO_PCI_O_CONFIG 0
#define VIRTIO_PCI_O_MSIX 1
-#define VIRTIO_PCI_O_FEATURES 2
struct virt_queue {
struct vring vring;
@@ -63,6 +62,6 @@ u16 virt_queue__get_head_iov(struct virt_queue *vq, struct iovec iov[], u16 *out
u16 virt_queue__get_inout_iov(struct kvm *kvm, struct virt_queue *queue,
struct iovec in_iov[], struct iovec out_iov[],
u16 *in, u16 *out);
-int virtio__get_dev_specific_field(int offset, bool msix, bool features_hi, u32 *config_off);
+int virtio__get_dev_specific_field(int offset, bool msix, u32 *config_off);
#endif /* KVM__VIRTIO_H */
diff --git a/tools/kvm/virtio/core.c b/tools/kvm/virtio/core.c
index fe9d588..5dc767a 100644
--- a/tools/kvm/virtio/core.c
+++ b/tools/kvm/virtio/core.c
@@ -128,7 +128,7 @@ u16 virt_queue__get_inout_iov(struct kvm *kvm, struct virt_queue *queue,
return head;
}
-int virtio__get_dev_specific_field(int offset, bool msix, bool features_hi, u32 *config_off)
+int virtio__get_dev_specific_field(int offset, bool msix, u32 *config_off)
{
if (msix) {
if (offset < 4)
@@ -137,13 +137,6 @@ int virtio__get_dev_specific_field(int offset, bool msix, bool features_hi, u32
offset -= 4;
}
- if (features_hi) {
- if (offset < 4)
- return VIRTIO_PCI_O_FEATURES;
- else
- offset -= 4;
- }
-
*config_off = offset;
return VIRTIO_PCI_O_CONFIG;
diff --git a/tools/kvm/virtio/pci.c b/tools/kvm/virtio/pci.c
index 0737ae7..3d9a7f8 100644
--- a/tools/kvm/virtio/pci.c
+++ b/tools/kvm/virtio/pci.c
@@ -69,7 +69,7 @@ static bool virtio_pci__specific_io_in(struct kvm *kvm, struct virtio_trans *vtr
struct virtio_pci *vpci = vtrans->virtio;
int type = virtio__get_dev_specific_field(offset - 20,
virtio_pci__msix_enabled(vpci),
- 0, &config_offset);
+ &config_offset);
if (type == VIRTIO_PCI_O_MSIX) {
switch (offset) {
case VIRTIO_MSI_CONFIG_VECTOR:
@@ -140,7 +140,7 @@ static bool virtio_pci__specific_io_out(struct kvm *kvm, struct virtio_trans *vt
struct virtio_pci *vpci = vtrans->virtio;
u32 config_offset, gsi, vec;
int type = virtio__get_dev_specific_field(offset - 20, virtio_pci__msix_enabled(vpci),
- 0, &config_offset);
+ &config_offset);
if (type == VIRTIO_PCI_O_MSIX) {
switch (offset) {
case VIRTIO_MSI_CONFIG_VECTOR:
--
1.7.8
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] kvm tools: Ninja out support for VIRTIO_F_FEATURES_HIGH
2011-12-06 8:45 [PATCH] kvm tools: Ninja out support for VIRTIO_F_FEATURES_HIGH Sasha Levin
@ 2011-12-06 9:47 ` Pekka Enberg
2011-12-06 9:57 ` Sasha Levin
0 siblings, 1 reply; 4+ messages in thread
From: Pekka Enberg @ 2011-12-06 9:47 UTC (permalink / raw)
To: Sasha Levin; +Cc: mingo, gorcunov, asias.hejun, kvm, Rusty Russell
On Tue, Dec 6, 2011 at 10:45 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
> Rusty has just removed it out of the spec. Since we probably the only ones
> who implemented support for it, we should remove it out of our code as well.
>
> There is no issue with breaking anything since nothing else worked with it,
> so it's fully backwards compatible.
>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
Applied, thanks!
How is this going to work going forward? Should I ask Rusty for an ACK
before merging code that implements some (new) part of the virtio
spec? I like the fact that we're bleeding edge but it's pointless for
everyone involved if we implement something that's known to be
half-baked in the spec.
Pekka
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] kvm tools: Ninja out support for VIRTIO_F_FEATURES_HIGH
2011-12-06 9:47 ` Pekka Enberg
@ 2011-12-06 9:57 ` Sasha Levin
2011-12-07 0:31 ` Rusty Russell
0 siblings, 1 reply; 4+ messages in thread
From: Sasha Levin @ 2011-12-06 9:57 UTC (permalink / raw)
To: Pekka Enberg; +Cc: mingo, gorcunov, asias.hejun, kvm, Rusty Russell
On Tue, 2011-12-06 at 11:47 +0200, Pekka Enberg wrote:
> On Tue, Dec 6, 2011 at 10:45 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
> > Rusty has just removed it out of the spec. Since we probably the only ones
> > who implemented support for it, we should remove it out of our code as well.
> >
> > There is no issue with breaking anything since nothing else worked with it,
> > so it's fully backwards compatible.
> >
> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
>
> Applied, thanks!
>
> How is this going to work going forward? Should I ask Rusty for an ACK
> before merging code that implements some (new) part of the virtio
> spec? I like the fact that we're bleeding edge but it's pointless for
> everyone involved if we implement something that's known to be
> half-baked in the spec.
There was a little cheating involved here since the spec basically broke
backwards compatibility by removing 64 bit features, so it's a special
case and probably won't happen (too many times) again.
Half baked features usually don't go into the spec as well, Rusty
usually insists on having a working code besides just spec updates, so
again - this was one of those rare special cases.
If Rusty or Michael could ACK our virtio patches it would be awesome.
--
Sasha.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] kvm tools: Ninja out support for VIRTIO_F_FEATURES_HIGH
2011-12-06 9:57 ` Sasha Levin
@ 2011-12-07 0:31 ` Rusty Russell
0 siblings, 0 replies; 4+ messages in thread
From: Rusty Russell @ 2011-12-07 0:31 UTC (permalink / raw)
To: Sasha Levin, Pekka Enberg; +Cc: mingo, gorcunov, asias.hejun, kvm
On Tue, 06 Dec 2011 11:57:20 +0200, Sasha Levin <levinsasha928@gmail.com> wrote:
> On Tue, 2011-12-06 at 11:47 +0200, Pekka Enberg wrote:
> > On Tue, Dec 6, 2011 at 10:45 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
> > > Rusty has just removed it out of the spec. Since we probably the only ones
> > > who implemented support for it, we should remove it out of our code as well.
> > >
> > > There is no issue with breaking anything since nothing else worked with it,
> > > so it's fully backwards compatible.
> > >
> > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> >
> > Applied, thanks!
> >
> > How is this going to work going forward? Should I ask Rusty for an ACK
> > before merging code that implements some (new) part of the virtio
> > spec? I like the fact that we're bleeding edge but it's pointless for
> > everyone involved if we implement something that's known to be
> > half-baked in the spec.
>
> There was a little cheating involved here since the spec basically broke
> backwards compatibility by removing 64 bit features, so it's a special
> case and probably won't happen (too many times) again.
>
> Half baked features usually don't go into the spec as well, Rusty
> usually insists on having a working code besides just spec updates, so
> again - this was one of those rare special cases.
>
> If Rusty or Michael could ACK our virtio patches it would be awesome.
Acked-by: Rusty Russell <rusty@rustcorp.com.au>
TLDR version: I shouldn't have put it in the spec until we needed it.
The spec revert broke no guests, because no guest ever implemented high
feature bits (both because it wasn't in the draft spec long, and no
device defined > 32 features yet, so no guest needed to).
It also broke no hosts, since it was up to the host to offer the high
feature bits. No host did, since no device needed those high bits.
kvmtool guys are just enthusiasts, so they put in the infrastructure,
but didn't use it.
If I'm wrong, then when we use bit 31 for something else, a new guest
may break. It may happen, but we're going to 64 bits in a different
manner, so we can avoid using bit 31 for a long time.
It's very unusual for me to un-spec things, but in this very limited
case it fairly safe.
Cheers,
Rusty.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-12-07 1:09 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-12-06 8:45 [PATCH] kvm tools: Ninja out support for VIRTIO_F_FEATURES_HIGH Sasha Levin
2011-12-06 9:47 ` Pekka Enberg
2011-12-06 9:57 ` Sasha Levin
2011-12-07 0:31 ` Rusty Russell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).