| Message ID | 54D85B3B.3040306@intel.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Feb 09, 2015 at 03:01:15PM +0800, Chen, Tiejun wrote: > On 2015/2/8 18:48, Michael S. Tsirkin wrote: > >On Fri, Feb 06, 2015 at 01:14:46PM +0100, Cornelia Huck wrote: > >>On Fri, 6 Feb 2015 13:41:26 +0800 > >>Tiejun Chen <tiejun.chen@intel.com> wrote: > >> > >>>Actually we define these device IDs in virtio standard, so > >>>we'd better put them into one common place to manage conveniently. > >>>Here I also add VIRTIO_ID_RESERVE according to virtio spec. > >>> > >>>Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> > > > >We really should just write a script to import the headers > >from the linux kernel. > >They will need some tweaks to avoid dependencies on > >linux/types, but this seems easy to do - better than > >trying to keep things in sync manually. > > I prefer Cornelia's comment since actually we're trying to define a little > bit according to a spec, so the following may be enough? > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index f24997d..4afb0b7 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -23,6 +23,22 @@ > #include "hw/virtio/virtio-9p.h" > #endif > > +/* Refer to VirtIO Spec 1.0. */ > + > +#define VIRTIO_ID_RESERVED 0 /* reserved (invalid)*/ > +#define VIRTIO_ID_NET 1 /* network card */ > +#define VIRTIO_ID_BLOCK 2 /* block device */ > +#define VIRTIO_ID_CONSOLE 3 /* console */ > +#define VIRTIO_ID_RNG 4 /* entropy source */ > +#define VIRTIO_ID_BALLOON 5 /* memory ballooning */ > +#define VIRTIO_ID_IOMEMORY 6 /* ioMemory */ > +#define VIRTIO_ID_RPMSG 7 /* rpmsg */ > +#define VIRTIO_ID_SCSI 8 /* SCSI host */ > +#define VIRTIO_ID_9P 9 /* 9P transport */ > +#define VIRTIO_ID_MAC80211_WALN 10 /* mac80211 wlan */ > +#define VIRTIO_ID_RPROC_SERIAL 11 /* rproc seria */ > +#define VIRTIO_ID_CAIF 12 /* virtio CAIF */ > + > /* from Linux's linux/virtio_config.h */ > > /* Status byte for guest to report progress, and synchronize features. */ > > Thanks > Tiejun This still means each change has to be done in two places. An automated script for copying headers would be much better IMHO.
On 2015/2/9 15:02, Michael S. Tsirkin wrote: > On Mon, Feb 09, 2015 at 03:01:15PM +0800, Chen, Tiejun wrote: >> On 2015/2/8 18:48, Michael S. Tsirkin wrote: >>> On Fri, Feb 06, 2015 at 01:14:46PM +0100, Cornelia Huck wrote: >>>> On Fri, 6 Feb 2015 13:41:26 +0800 >>>> Tiejun Chen <tiejun.chen@intel.com> wrote: >>>> >>>>> Actually we define these device IDs in virtio standard, so >>>>> we'd better put them into one common place to manage conveniently. >>>>> Here I also add VIRTIO_ID_RESERVE according to virtio spec. >>>>> >>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> >>> >>> We really should just write a script to import the headers >> >from the linux kernel. >>> They will need some tweaks to avoid dependencies on >>> linux/types, but this seems easy to do - better than >>> trying to keep things in sync manually. >> >> I prefer Cornelia's comment since actually we're trying to define a little >> bit according to a spec, so the following may be enough? >> >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h >> index f24997d..4afb0b7 100644 >> --- a/include/hw/virtio/virtio.h >> +++ b/include/hw/virtio/virtio.h >> @@ -23,6 +23,22 @@ >> #include "hw/virtio/virtio-9p.h" >> #endif >> >> +/* Refer to VirtIO Spec 1.0. */ >> + >> +#define VIRTIO_ID_RESERVED 0 /* reserved (invalid)*/ >> +#define VIRTIO_ID_NET 1 /* network card */ >> +#define VIRTIO_ID_BLOCK 2 /* block device */ >> +#define VIRTIO_ID_CONSOLE 3 /* console */ >> +#define VIRTIO_ID_RNG 4 /* entropy source */ >> +#define VIRTIO_ID_BALLOON 5 /* memory ballooning */ >> +#define VIRTIO_ID_IOMEMORY 6 /* ioMemory */ >> +#define VIRTIO_ID_RPMSG 7 /* rpmsg */ >> +#define VIRTIO_ID_SCSI 8 /* SCSI host */ >> +#define VIRTIO_ID_9P 9 /* 9P transport */ >> +#define VIRTIO_ID_MAC80211_WALN 10 /* mac80211 wlan */ >> +#define VIRTIO_ID_RPROC_SERIAL 11 /* rproc seria */ >> +#define VIRTIO_ID_CAIF 12 /* virtio CAIF */ >> + >> /* from Linux's linux/virtio_config.h */ >> >> /* Status byte for guest to report progress, and synchronize features. */ >> >> Thanks >> Tiejun > > This still means each change has to be done in two places. Are you saying another head file, pc-bios/s390-ccw/virtio.h? But seems Cornelia thought in case of s390-ccw, -quote- "Even though this one is incomplete; but we don't need anything but the block id anyway." So I'm not sure we really should do this :) Thanks Tiejun
On Mon, 09 Feb 2015 15:10:17 +0800 "Chen, Tiejun" <tiejun.chen@intel.com> wrote: > On 2015/2/9 15:02, Michael S. Tsirkin wrote: > > On Mon, Feb 09, 2015 at 03:01:15PM +0800, Chen, Tiejun wrote: > >> On 2015/2/8 18:48, Michael S. Tsirkin wrote: > >>> On Fri, Feb 06, 2015 at 01:14:46PM +0100, Cornelia Huck wrote: > >>>> On Fri, 6 Feb 2015 13:41:26 +0800 > >>>> Tiejun Chen <tiejun.chen@intel.com> wrote: > >>>> > >>>>> Actually we define these device IDs in virtio standard, so > >>>>> we'd better put them into one common place to manage conveniently. > >>>>> Here I also add VIRTIO_ID_RESERVE according to virtio spec. > >>>>> > >>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> > >>> > >>> We really should just write a script to import the headers > >> >from the linux kernel. > >>> They will need some tweaks to avoid dependencies on > >>> linux/types, but this seems easy to do - better than > >>> trying to keep things in sync manually. > >> > >> I prefer Cornelia's comment since actually we're trying to define a little > >> bit according to a spec, so the following may be enough? > >> > >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > >> index f24997d..4afb0b7 100644 > >> --- a/include/hw/virtio/virtio.h > >> +++ b/include/hw/virtio/virtio.h > >> @@ -23,6 +23,22 @@ > >> #include "hw/virtio/virtio-9p.h" > >> #endif > >> > >> +/* Refer to VirtIO Spec 1.0. */ > >> + > >> +#define VIRTIO_ID_RESERVED 0 /* reserved (invalid)*/ > >> +#define VIRTIO_ID_NET 1 /* network card */ > >> +#define VIRTIO_ID_BLOCK 2 /* block device */ > >> +#define VIRTIO_ID_CONSOLE 3 /* console */ > >> +#define VIRTIO_ID_RNG 4 /* entropy source */ > >> +#define VIRTIO_ID_BALLOON 5 /* memory ballooning */ > >> +#define VIRTIO_ID_IOMEMORY 6 /* ioMemory */ > >> +#define VIRTIO_ID_RPMSG 7 /* rpmsg */ > >> +#define VIRTIO_ID_SCSI 8 /* SCSI host */ > >> +#define VIRTIO_ID_9P 9 /* 9P transport */ > >> +#define VIRTIO_ID_MAC80211_WALN 10 /* mac80211 wlan */ > >> +#define VIRTIO_ID_RPROC_SERIAL 11 /* rproc seria */ > >> +#define VIRTIO_ID_CAIF 12 /* virtio CAIF */ I like that. > >> + > >> /* from Linux's linux/virtio_config.h */ > >> > >> /* Status byte for guest to report progress, and synchronize features. */ > >> > >> Thanks > >> Tiejun > > > > This still means each change has to be done in two places. Well, we do need the changes in way more than two places, as every host or guest has to collect the definitions on its own, no? (Granted, with Linux and qemu you get most of the users; but it feels a bit strange for a host implementation to collect information from one of its guests. I really think that we should go back to the common root. Didn't we have a BSD-licenced header in the spec?) > > Are you saying another head file, pc-bios/s390-ccw/virtio.h? > > But seems Cornelia thought in case of s390-ccw, -quote- > > "Even though this one is incomplete; but we don't need anything but the > block id anyway." Note that this is the s390-ccw _bios_, which is a very incomplete implementation only containing the bare minimum needed to access a virtio-blk root device for booting.
On Mon, Feb 09, 2015 at 09:47:20AM +0100, Cornelia Huck wrote: > On Mon, 09 Feb 2015 15:10:17 +0800 > "Chen, Tiejun" <tiejun.chen@intel.com> wrote: > > > On 2015/2/9 15:02, Michael S. Tsirkin wrote: > > > On Mon, Feb 09, 2015 at 03:01:15PM +0800, Chen, Tiejun wrote: > > >> On 2015/2/8 18:48, Michael S. Tsirkin wrote: > > >>> On Fri, Feb 06, 2015 at 01:14:46PM +0100, Cornelia Huck wrote: > > >>>> On Fri, 6 Feb 2015 13:41:26 +0800 > > >>>> Tiejun Chen <tiejun.chen@intel.com> wrote: > > >>>> > > >>>>> Actually we define these device IDs in virtio standard, so > > >>>>> we'd better put them into one common place to manage conveniently. > > >>>>> Here I also add VIRTIO_ID_RESERVE according to virtio spec. > > >>>>> > > >>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> > > >>> > > >>> We really should just write a script to import the headers > > >> >from the linux kernel. > > >>> They will need some tweaks to avoid dependencies on > > >>> linux/types, but this seems easy to do - better than > > >>> trying to keep things in sync manually. > > >> > > >> I prefer Cornelia's comment since actually we're trying to define a little > > >> bit according to a spec, so the following may be enough? > > >> > > >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > > >> index f24997d..4afb0b7 100644 > > >> --- a/include/hw/virtio/virtio.h > > >> +++ b/include/hw/virtio/virtio.h > > >> @@ -23,6 +23,22 @@ > > >> #include "hw/virtio/virtio-9p.h" > > >> #endif > > >> > > >> +/* Refer to VirtIO Spec 1.0. */ > > >> + > > >> +#define VIRTIO_ID_RESERVED 0 /* reserved (invalid)*/ > > >> +#define VIRTIO_ID_NET 1 /* network card */ > > >> +#define VIRTIO_ID_BLOCK 2 /* block device */ > > >> +#define VIRTIO_ID_CONSOLE 3 /* console */ > > >> +#define VIRTIO_ID_RNG 4 /* entropy source */ > > >> +#define VIRTIO_ID_BALLOON 5 /* memory ballooning */ > > >> +#define VIRTIO_ID_IOMEMORY 6 /* ioMemory */ > > >> +#define VIRTIO_ID_RPMSG 7 /* rpmsg */ > > >> +#define VIRTIO_ID_SCSI 8 /* SCSI host */ > > >> +#define VIRTIO_ID_9P 9 /* 9P transport */ > > >> +#define VIRTIO_ID_MAC80211_WALN 10 /* mac80211 wlan */ > > >> +#define VIRTIO_ID_RPROC_SERIAL 11 /* rproc seria */ > > >> +#define VIRTIO_ID_CAIF 12 /* virtio CAIF */ > > I like that. > > > >> + > > >> /* from Linux's linux/virtio_config.h */ > > >> > > >> /* Status byte for guest to report progress, and synchronize features. */ > > >> > > >> Thanks > > >> Tiejun > > > > > > This still means each change has to be done in two places. > > Well, we do need the changes in way more than two places, as every host > or guest has to collect the definitions on its own, no? This has nothing to do with host. It's just using linux source as main basis for the file simply because linux is a higher visibility project than qemu, they won't borrow code from us but we can borrow from them. > (Granted, with > Linux and qemu you get most of the users; but it feels a bit strange > for a host implementation to collect information from one of its > guests. I really think that we should go back to the common root. > Didn't we have a BSD-licenced header in the spec?) virtio linux headers are also BSD licensed intentionally for this purpose. * This header is BSD licensed so anyone can use the definitions to * implement compatible drivers/servers. > > > > Are you saying another head file, pc-bios/s390-ccw/virtio.h? > > > > But seems Cornelia thought in case of s390-ccw, -quote- > > > > "Even though this one is incomplete; but we don't need anything but the > > block id anyway." > > Note that this is the s390-ccw _bios_, which is a very incomplete > implementation only containing the bare minimum needed to access a > virtio-blk root device for booting. Let's worry about this one once we have fixed the duplication for the others.
On Wed, 11 Feb 2015 13:41:29 +0100 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Mon, Feb 09, 2015 at 09:47:20AM +0100, Cornelia Huck wrote: > > Well, we do need the changes in way more than two places, as every host > > or guest has to collect the definitions on its own, no? > > This has nothing to do with host. Hm, but all hosts and all guests need the ids, no? > It's just using linux source as main basis for the file > simply because linux is a higher visibility project than qemu, > they won't borrow code from us but we can borrow from them. It still seems restricting to use Linux as the ultimate source - and that has nothing to do with visibililty. I'd say pulling from any project is restricting: Why shouldn't we want to implement something in qemu that Linux is not (yet) interested in, but other guests are? > > (Granted, with > > Linux and qemu you get most of the users; but it feels a bit strange > > for a host implementation to collect information from one of its > > guests. I really think that we should go back to the common root. > > Didn't we have a BSD-licenced header in the spec?) > > virtio linux headers are also BSD licensed intentionally for this > purpose. > * This header is BSD licensed so anyone can use the definitions to > * implement compatible drivers/servers. Sure, but I always took this to mean "you can freely copy the definitions", not "this is the authorative source".
On Wed, Feb 11, 2015 at 07:10:22PM +0100, Cornelia Huck wrote: > On Wed, 11 Feb 2015 13:41:29 +0100 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Mon, Feb 09, 2015 at 09:47:20AM +0100, Cornelia Huck wrote: > > > > Well, we do need the changes in way more than two places, as every host > > > or guest has to collect the definitions on its own, no? > > > > This has nothing to do with host. > > Hm, but all hosts and all guests need the ids, no? > > > It's just using linux source as main basis for the file > > simply because linux is a higher visibility project than qemu, > > they won't borrow code from us but we can borrow from them. > > It still seems restricting to use Linux as the ultimate source - and > that has nothing to do with visibililty. I'd say pulling from any > project is restricting: Why shouldn't we want to implement something in > qemu that Linux is not (yet) interested in, but other guests are? We can always add such a hypothetical feature in a qemu specific headers. But most things are shared, and manual duplication is bad. > > > (Granted, with > > > Linux and qemu you get most of the users; but it feels a bit strange > > > for a host implementation to collect information from one of its > > > guests. I really think that we should go back to the common root. > > > Didn't we have a BSD-licenced header in the spec?) > > > > virtio linux headers are also BSD licensed intentionally for this > > purpose. > > * This header is BSD licensed so anyone can use the definitions to > > * implement compatible drivers/servers. > > Sure, but I always took this to mean "you can freely copy the > definitions", not "this is the authorative source". virtio spec only has a ring header. We could get it from there but it's easier to get everything from one place I think.
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index f24997d..4afb0b7 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -23,6 +23,22 @@ #include "hw/virtio/virtio-9p.h" #endif +/* Refer to VirtIO Spec 1.0. */ + +#define VIRTIO_ID_RESERVED 0 /* reserved (invalid)*/ +#define VIRTIO_ID_NET 1 /* network card */ +#define VIRTIO_ID_BLOCK 2 /* block device */ +#define VIRTIO_ID_CONSOLE 3 /* console */ +#define VIRTIO_ID_RNG 4 /* entropy source */ +#define VIRTIO_ID_BALLOON 5 /* memory ballooning */ +#define VIRTIO_ID_IOMEMORY 6 /* ioMemory */ +#define VIRTIO_ID_RPMSG 7 /* rpmsg */ +#define VIRTIO_ID_SCSI 8 /* SCSI host */ +#define VIRTIO_ID_9P 9 /* 9P transport */ +#define VIRTIO_ID_MAC80211_WALN 10 /* mac80211 wlan */ +#define VIRTIO_ID_RPROC_SERIAL 11 /* rproc seria */ +#define VIRTIO_ID_CAIF 12 /* virtio CAIF */ + /* from Linux's linux/virtio_config.h */ /* Status byte for guest to report progress, and synchronize features. */