* [Qemu-devel] [PATCH] piix: use pci_config_set_prog_interface() @ 2010-12-20 21:18 Sebastian Herbszt 2010-12-27 14:01 ` [Qemu-devel] " Michael S. Tsirkin 0 siblings, 1 reply; 9+ messages in thread From: Sebastian Herbszt @ 2010-12-20 21:18 UTC (permalink / raw) To: qemu-devel; +Cc: Sebastian Herbszt Use pci_config_set_prog_interface(). Signed-off-by: Sebastian Herbszt <herbszt@gmx.de> diff --git a/hw/ide/piix.c b/hw/ide/piix.c index 1cad906..2772e08 100644 --- a/hw/ide/piix.c +++ b/hw/ide/piix.c @@ -135,7 +135,7 @@ static int pci_piix_ide_initfn(PCIIDEState *d) { uint8_t *pci_conf = d->dev.config; - pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode + pci_config_set_prog_interface(pci_conf, 0x80); // legacy ATA mode pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_IDE); qemu_register_reset(piix3_reset, d); ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] Re: [PATCH] piix: use pci_config_set_prog_interface() 2010-12-20 21:18 [Qemu-devel] [PATCH] piix: use pci_config_set_prog_interface() Sebastian Herbszt @ 2010-12-27 14:01 ` Michael S. Tsirkin 2010-12-28 16:24 ` Sebastian Herbszt 0 siblings, 1 reply; 9+ messages in thread From: Michael S. Tsirkin @ 2010-12-27 14:01 UTC (permalink / raw) To: Sebastian Herbszt; +Cc: qemu-devel On Mon, Dec 20, 2010 at 10:18:01PM +0100, Sebastian Herbszt wrote: > Use pci_config_set_prog_interface(). > > Signed-off-by: Sebastian Herbszt <herbszt@gmx.de> Since I was asked explicitly - I don't have a problem with these changes: both class and prog interface. However, they aren't all that useful in themselves. For class, what I would like to see is a system where the device class is put in the qdev info table, and where -device ? (and hopefully the legacy -help, -nic etc as well) use this information. pci_config_set_prog_interface can then have an assert to verify that value. Maybe we can even make e.g. -device nic work. In a similar way, pci_config_set_prog_interface would really become useful if we put the handling for the legacy classes in a central place (e.g. pci_class.c) Any chance of doing something like the above? I'd be happy to apply such patches. > diff --git a/hw/ide/piix.c b/hw/ide/piix.c > index 1cad906..2772e08 100644 > --- a/hw/ide/piix.c > +++ b/hw/ide/piix.c > @@ -135,7 +135,7 @@ static int pci_piix_ide_initfn(PCIIDEState *d) > { > uint8_t *pci_conf = d->dev.config; > > - pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode > + pci_config_set_prog_interface(pci_conf, 0x80); // legacy ATA mode > pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_IDE); > > qemu_register_reset(piix3_reset, d); > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] Re: [PATCH] piix: use pci_config_set_prog_interface() 2010-12-27 14:01 ` [Qemu-devel] " Michael S. Tsirkin @ 2010-12-28 16:24 ` Sebastian Herbszt 2010-12-28 17:19 ` Michael S. Tsirkin 0 siblings, 1 reply; 9+ messages in thread From: Sebastian Herbszt @ 2010-12-28 16:24 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel Michael S. Tsirkin wrote: > On Mon, Dec 20, 2010 at 10:18:01PM +0100, Sebastian Herbszt wrote: >> Use pci_config_set_prog_interface(). >> >> Signed-off-by: Sebastian Herbszt <herbszt@gmx.de> > > Since I was asked explicitly - I don't have a problem > with these changes: both class and prog interface. > However, they aren't all that useful in themselves. > > For class, what I would like to see is a system where > the device class is put in the qdev info table, > and where -device ? > (and hopefully the legacy -help, -nic etc as well) > use this information. I am not sure if you mean something like the patch below. > pci_config_set_prog_interface can then have an assert to > verify that value. > Maybe we can even make e.g. -device nic work. > > In a similar way, pci_config_set_prog_interface > would really become useful if we put the handling > for the legacy classes in a central place > (e.g. pci_class.c) Which are "legacy classes"? > Any chance of doing something like the above? > I'd be happy to apply such patches. Sebastian diff --git a/hw/e1000.c b/hw/e1000.c index af101bd..88ace8f 100644 --- a/hw/e1000.c +++ b/hw/e1000.c @@ -1117,7 +1117,6 @@ static int pci_e1000_init(PCIDevice *pci_dev) /* TODO: we have no capabilities, so why is this bit set? */ pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_CAP_LIST); pci_conf[PCI_REVISION_ID] = 0x03; - pci_config_set_class(pci_conf, PCI_CLASS_NETWORK_ETHERNET); /* TODO: RST# value should be 0, PCI spec 6.2.4 */ pci_conf[PCI_CACHE_LINE_SIZE] = 0x10; @@ -1169,6 +1168,7 @@ static PCIDeviceInfo e1000_info = { .init = pci_e1000_init, .exit = pci_e1000_uninit, .romfile = "pxe-e1000.bin", + .class = PCI_CLASS_NETWORK_ETHERNET, .qdev.props = (Property[]) { DEFINE_NIC_PROPERTIES(E1000State, conf), DEFINE_PROP_END_OF_LIST(), diff --git a/hw/pci.c b/hw/pci.c index ef00d20..de0038c 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -1620,6 +1620,11 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base) info->is_bridge); if (pci_dev == NULL) return -1; + + if (info->class) { + pci_config_set_class(pci_dev->config, info->class); + } + rc = info->init(pci_dev); if (rc != 0) { do_pci_unregister_device(pci_dev); diff --git a/hw/pci.h b/hw/pci.h index 17744dc..29b9280 100644 --- a/hw/pci.h +++ b/hw/pci.h @@ -436,6 +436,9 @@ typedef struct { /* rom bar */ const char *romfile; + + /* class */ + uint16_t class; } PCIDeviceInfo; void pci_qdev_register(PCIDeviceInfo *info); ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] Re: [PATCH] piix: use pci_config_set_prog_interface() 2010-12-28 16:24 ` Sebastian Herbszt @ 2010-12-28 17:19 ` Michael S. Tsirkin 2010-12-28 18:48 ` Sebastian Herbszt 0 siblings, 1 reply; 9+ messages in thread From: Michael S. Tsirkin @ 2010-12-28 17:19 UTC (permalink / raw) To: Sebastian Herbszt; +Cc: qemu-devel On Tue, Dec 28, 2010 at 05:24:06PM +0100, Sebastian Herbszt wrote: > Michael S. Tsirkin wrote: > >On Mon, Dec 20, 2010 at 10:18:01PM +0100, Sebastian Herbszt wrote: > >>Use pci_config_set_prog_interface(). > >> > >>Signed-off-by: Sebastian Herbszt <herbszt@gmx.de> > > > >Since I was asked explicitly - I don't have a problem > >with these changes: both class and prog interface. > >However, they aren't all that useful in themselves. > > > >For class, what I would like to see is a system where > >the device class is put in the qdev info table, > >and where -device ? > >(and hopefully the legacy -help, -nic etc as well) > >use this information. > > I am not sure if you mean something like the patch below. Not exactly - I'd like to keep the pci_config_set_class in the devices, just make it do an assert. - Nics already have DEFINE_NIC_PROPERTIES - can this be used somehow? Same for other devices ... - This is still just moving bits around. The main point would be to report device type in -device ? and friends. > >pci_config_set_prog_interface can then have an assert to > >verify that value. > >Maybe we can even make e.g. -device nic work. > > > >In a similar way, pci_config_set_prog_interface > >would really become useful if we put the handling > >for the legacy classes in a central place > >(e.g. pci_class.c) > > Which are "legacy classes"? Some class/programming interface combinations let devices claim IO addresses not requested using a BAR. This is currently hard-coded in each such device. > >Any chance of doing something like the above? > >I'd be happy to apply such patches. > > Sebastian > > diff --git a/hw/e1000.c b/hw/e1000.c > index af101bd..88ace8f 100644 > --- a/hw/e1000.c > +++ b/hw/e1000.c > @@ -1117,7 +1117,6 @@ static int pci_e1000_init(PCIDevice *pci_dev) > /* TODO: we have no capabilities, so why is this bit set? */ > pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_CAP_LIST); > pci_conf[PCI_REVISION_ID] = 0x03; > - pci_config_set_class(pci_conf, PCI_CLASS_NETWORK_ETHERNET); > /* TODO: RST# value should be 0, PCI spec 6.2.4 */ > pci_conf[PCI_CACHE_LINE_SIZE] = 0x10; > > @@ -1169,6 +1168,7 @@ static PCIDeviceInfo e1000_info = { > .init = pci_e1000_init, > .exit = pci_e1000_uninit, > .romfile = "pxe-e1000.bin", > + .class = PCI_CLASS_NETWORK_ETHERNET, > .qdev.props = (Property[]) { > DEFINE_NIC_PROPERTIES(E1000State, conf), > DEFINE_PROP_END_OF_LIST(), > diff --git a/hw/pci.c b/hw/pci.c > index ef00d20..de0038c 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -1620,6 +1620,11 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base) > info->is_bridge); > if (pci_dev == NULL) > return -1; > + > + if (info->class) { > + pci_config_set_class(pci_dev->config, info->class); > + } > + > rc = info->init(pci_dev); > if (rc != 0) { > do_pci_unregister_device(pci_dev); > diff --git a/hw/pci.h b/hw/pci.h > index 17744dc..29b9280 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -436,6 +436,9 @@ typedef struct { > > /* rom bar */ > const char *romfile; > + > + /* class */ > + uint16_t class; > } PCIDeviceInfo; > > void pci_qdev_register(PCIDeviceInfo *info); ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] Re: [PATCH] piix: use pci_config_set_prog_interface() 2010-12-28 17:19 ` Michael S. Tsirkin @ 2010-12-28 18:48 ` Sebastian Herbszt 2010-12-28 21:08 ` Michael S. Tsirkin 0 siblings, 1 reply; 9+ messages in thread From: Sebastian Herbszt @ 2010-12-28 18:48 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel Michael S. Tsirkin wrote: > On Tue, Dec 28, 2010 at 05:24:06PM +0100, Sebastian Herbszt wrote: >> Michael S. Tsirkin wrote: >> >On Mon, Dec 20, 2010 at 10:18:01PM +0100, Sebastian Herbszt wrote: >> >>Use pci_config_set_prog_interface(). >> >> >> >>Signed-off-by: Sebastian Herbszt <herbszt@gmx.de> >> > >> >Since I was asked explicitly - I don't have a problem >> >with these changes: both class and prog interface. >> >However, they aren't all that useful in themselves. >> > >> >For class, what I would like to see is a system where >> >the device class is put in the qdev info table, >> >and where -device ? >> >(and hopefully the legacy -help, -nic etc as well) >> >use this information. >> >> I am not sure if you mean something like the patch below. > > Not exactly > > - I'd like to keep the pci_config_set_class in the devices, > just make it do an assert. Assert on which condition? > - Nics already have DEFINE_NIC_PROPERTIES - can this be used somehow? > Same for other devices ... We got DEFINE_NIC_PROPERTIES and DEFINE_BLOCK_PROPERTIES. Something like DEFINE_PCI_PROPERTIES could be introduced, but i am not sure which device properties it should hold (vendor_id, device_id, class, etc?). Those will then be user-modifiable with e.g. -device e1000,pci_class=1234. Sebastian diff --git a/hw/e1000.c b/hw/e1000.c index af101bd..e302703 100644 --- a/hw/e1000.c +++ b/hw/e1000.c @@ -1171,6 +1171,7 @@ static PCIDeviceInfo e1000_info = { .romfile = "pxe-e1000.bin", .qdev.props = (Property[]) { DEFINE_NIC_PROPERTIES(E1000State, conf), + DEFINE_PROP_UINT16("pci_class", E1000State, dev.class, PCI_CLASS_NETWORK_ETHERNET), DEFINE_PROP_END_OF_LIST(), } }; diff --git a/hw/pci.c b/hw/pci.c index ef00d20..06bbf04 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -1620,6 +1620,9 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base) info->is_bridge); if (pci_dev == NULL) return -1; + + pci_config_set_class(pci_dev->config, pci_dev->class); + rc = info->init(pci_dev); if (rc != 0) { do_pci_unregister_device(pci_dev); diff --git a/hw/pci.h b/hw/pci.h index 17744dc..5dc9053 100644 --- a/hw/pci.h +++ b/hw/pci.h @@ -189,6 +189,9 @@ struct PCIDevice { char *romfile; ram_addr_t rom_offset; uint32_t rom_bar; + + /* class */ + uint16_t class; }; PCIDevice *pci_register_device(PCIBus *bus, const char *name, ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] Re: [PATCH] piix: use pci_config_set_prog_interface() 2010-12-28 18:48 ` Sebastian Herbszt @ 2010-12-28 21:08 ` Michael S. Tsirkin 2010-12-29 10:26 ` Sebastian Herbszt 0 siblings, 1 reply; 9+ messages in thread From: Michael S. Tsirkin @ 2010-12-28 21:08 UTC (permalink / raw) To: Sebastian Herbszt; +Cc: qemu-devel On Tue, Dec 28, 2010 at 07:48:23PM +0100, Sebastian Herbszt wrote: > Michael S. Tsirkin wrote: > >On Tue, Dec 28, 2010 at 05:24:06PM +0100, Sebastian Herbszt wrote: > >>Michael S. Tsirkin wrote: > >>>On Mon, Dec 20, 2010 at 10:18:01PM +0100, Sebastian Herbszt wrote: > >>>>Use pci_config_set_prog_interface(). > >>>> > >>>>Signed-off-by: Sebastian Herbszt <herbszt@gmx.de> > >>> > >>>Since I was asked explicitly - I don't have a problem > >>>with these changes: both class and prog interface. > >>>However, they aren't all that useful in themselves. > >>> > >>>For class, what I would like to see is a system where > >>>the device class is put in the qdev info table, > >>>and where -device ? > >>>(and hopefully the legacy -help, -nic etc as well) > >>>use this information. > >> > >>I am not sure if you mean something like the patch below. > > > >Not exactly > > > >- I'd like to keep the pci_config_set_class in the devices, > > just make it do an assert. > > Assert on which condition? That PCI class matches device type defined in qdev. This will serve to verify that all devices are converted properly. > >- Nics already have DEFINE_NIC_PROPERTIES - can this be used somehow? > > Same for other devices ... > > We got DEFINE_NIC_PROPERTIES and DEFINE_BLOCK_PROPERTIES. > Something like DEFINE_PCI_PROPERTIES could be introduced, but i am not sure This shouldn't have to do anything with PCI. We should define the device type as NIC, PCI class can be derived from that. > which device properties it should hold (vendor_id, device_id, class, etc?). > Those will then be user-modifiable with e.g. -device e1000,pci_class=1234. > > Sebastian Why is this helpful? - tweaking class will just break guests - binary representation is unfriendly, let's not require users to read pci spec just to run qemu. I can imagine some useful features implemented by sticking the type in qdev: - -device ? would sort devices by type and print the type, - something like this for qtree etc - -help would get all e.g. nic models from qdev, - allow easy to remember aliases with per-type scope (e.g. -device virtio,type=nic to be equivalent to -device virtio-net-pci) - filter (for device ?, qtree etc) system devices that user has no business tweaking (could be type=system or something like that) > diff --git a/hw/e1000.c b/hw/e1000.c > index af101bd..e302703 100644 > --- a/hw/e1000.c > +++ b/hw/e1000.c > @@ -1171,6 +1171,7 @@ static PCIDeviceInfo e1000_info = { > .romfile = "pxe-e1000.bin", > .qdev.props = (Property[]) { > DEFINE_NIC_PROPERTIES(E1000State, conf), > + DEFINE_PROP_UINT16("pci_class", E1000State, dev.class, PCI_CLASS_NETWORK_ETHERNET), > DEFINE_PROP_END_OF_LIST(), > } > }; > diff --git a/hw/pci.c b/hw/pci.c > index ef00d20..06bbf04 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -1620,6 +1620,9 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base) > info->is_bridge); > if (pci_dev == NULL) > return -1; > + > + pci_config_set_class(pci_dev->config, pci_dev->class); > + > rc = info->init(pci_dev); > if (rc != 0) { > do_pci_unregister_device(pci_dev); > diff --git a/hw/pci.h b/hw/pci.h > index 17744dc..5dc9053 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -189,6 +189,9 @@ struct PCIDevice { > char *romfile; > ram_addr_t rom_offset; > uint32_t rom_bar; > + > + /* class */ Please don't copy code into comments verbatim. > + uint16_t class; Ugh. We have class in config already. > }; > > PCIDevice *pci_register_device(PCIBus *bus, const char *name, > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] Re: [PATCH] piix: use pci_config_set_prog_interface() 2010-12-28 21:08 ` Michael S. Tsirkin @ 2010-12-29 10:26 ` Sebastian Herbszt 2010-12-29 11:21 ` Michael S. Tsirkin 2010-12-29 14:55 ` Michael S. Tsirkin 0 siblings, 2 replies; 9+ messages in thread From: Sebastian Herbszt @ 2010-12-29 10:26 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel Michael S. Tsirkin wrote: > On Tue, Dec 28, 2010 at 07:48:23PM +0100, Sebastian Herbszt wrote: >> Michael S. Tsirkin wrote: >> >On Tue, Dec 28, 2010 at 05:24:06PM +0100, Sebastian Herbszt wrote: >> >>Michael S. Tsirkin wrote: >> >>>On Mon, Dec 20, 2010 at 10:18:01PM +0100, Sebastian Herbszt wrote: >> >>>>Use pci_config_set_prog_interface(). >> >>>> >> >>>>Signed-off-by: Sebastian Herbszt <herbszt@gmx.de> >> >>> >> >>>Since I was asked explicitly - I don't have a problem >> >>>with these changes: both class and prog interface. >> >>>However, they aren't all that useful in themselves. >> >>> >> >>>For class, what I would like to see is a system where >> >>>the device class is put in the qdev info table, >> >>>and where -device ? >> >>>(and hopefully the legacy -help, -nic etc as well) >> >>>use this information. >> >> >> >>I am not sure if you mean something like the patch below. >> > >> >Not exactly >> > >> >- I'd like to keep the pci_config_set_class in the devices, >> > just make it do an assert. >> >> Assert on which condition? > > That PCI class matches device type defined in qdev. > This will serve to verify that all devices are converted properly. > >> >- Nics already have DEFINE_NIC_PROPERTIES - can this be used somehow? >> > Same for other devices ... >> >> We got DEFINE_NIC_PROPERTIES and DEFINE_BLOCK_PROPERTIES. >> Something like DEFINE_PCI_PROPERTIES could be introduced, but i am not sure > > This shouldn't have to do anything with PCI. > We should define the device type as NIC, > PCI class can be derived from that. So you want to introduce a device property like "device_type" and set it to "NIC" and then translate "NIC" to the correct PCI class code. Are you sure this is unambiguous? "NIC" could be ethernet or token ring. A generic name like "HBA" could translate to even more codes (scsi, ide, raid, sata, ...). >> which device properties it should hold (vendor_id, device_id, class, etc?). >> Those will then be user-modifiable with e.g. -device e1000,pci_class=1234. >> >> Sebastian > > Why is this helpful? > - tweaking class will just break guests > - binary representation is unfriendly, let's not require users to read > pci spec just to run qemu. "pci_class" is optional. I just wanted to show that qdev device properties are user-modifiable and i didn't find a way to prevent this. > I can imagine some useful features implemented by sticking the type in qdev: > - -device ? would sort devices by type and print the type, > - something like this for qtree etc > - -help would get all e.g. nic models from qdev, > - allow easy to remember aliases with per-type scope (e.g. > -device virtio,type=nic to be equivalent to -device virtio-net-pci) Is this just a virtio problem? > - filter (for device ?, qtree etc) system devices that user has no business > tweaking (could be type=system or something like that) > >> diff --git a/hw/e1000.c b/hw/e1000.c >> index af101bd..e302703 100644 >> --- a/hw/e1000.c >> +++ b/hw/e1000.c >> @@ -1171,6 +1171,7 @@ static PCIDeviceInfo e1000_info = { >> .romfile = "pxe-e1000.bin", >> .qdev.props = (Property[]) { >> DEFINE_NIC_PROPERTIES(E1000State, conf), >> + DEFINE_PROP_UINT16("pci_class", E1000State, dev.class, PCI_CLASS_NETWORK_ETHERNET), >> DEFINE_PROP_END_OF_LIST(), >> } >> }; >> diff --git a/hw/pci.c b/hw/pci.c >> index ef00d20..06bbf04 100644 >> --- a/hw/pci.c >> +++ b/hw/pci.c >> @@ -1620,6 +1620,9 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base) >> info->is_bridge); >> if (pci_dev == NULL) >> return -1; >> + >> + pci_config_set_class(pci_dev->config, pci_dev->class); >> + >> rc = info->init(pci_dev); >> if (rc != 0) { >> do_pci_unregister_device(pci_dev); >> diff --git a/hw/pci.h b/hw/pci.h >> index 17744dc..5dc9053 100644 >> --- a/hw/pci.h >> +++ b/hw/pci.h >> @@ -189,6 +189,9 @@ struct PCIDevice { >> char *romfile; >> ram_addr_t rom_offset; >> uint32_t rom_bar; >> + >> + /* class */ > > Please don't copy code into comments verbatim. > >> + uint16_t class; > > Ugh. We have class in config already. But i could not get qdev to use it. >> }; >> >> PCIDevice *pci_register_device(PCIBus *bus, const char *name, >> Sebastian ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] Re: [PATCH] piix: use pci_config_set_prog_interface() 2010-12-29 10:26 ` Sebastian Herbszt @ 2010-12-29 11:21 ` Michael S. Tsirkin 2010-12-29 14:55 ` Michael S. Tsirkin 1 sibling, 0 replies; 9+ messages in thread From: Michael S. Tsirkin @ 2010-12-29 11:21 UTC (permalink / raw) To: Sebastian Herbszt; +Cc: qemu-devel On Wed, Dec 29, 2010 at 11:26:32AM +0100, Sebastian Herbszt wrote: > Michael S. Tsirkin wrote: > >On Tue, Dec 28, 2010 at 07:48:23PM +0100, Sebastian Herbszt wrote: > >>Michael S. Tsirkin wrote: > >>>On Tue, Dec 28, 2010 at 05:24:06PM +0100, Sebastian Herbszt wrote: > >>>>Michael S. Tsirkin wrote: > >>>>>On Mon, Dec 20, 2010 at 10:18:01PM +0100, Sebastian Herbszt wrote: > >>>>>>Use pci_config_set_prog_interface(). > >>>>>> > >>>>>>Signed-off-by: Sebastian Herbszt <herbszt@gmx.de> > >>>>> > >>>>>Since I was asked explicitly - I don't have a problem > >>>>>with these changes: both class and prog interface. > >>>>>However, they aren't all that useful in themselves. > >>>>> > >>>>>For class, what I would like to see is a system where > >>>>>the device class is put in the qdev info table, > >>>>>and where -device ? > >>>>>(and hopefully the legacy -help, -nic etc as well) > >>>>>use this information. > >>>> > >>>>I am not sure if you mean something like the patch below. > >>> > >>>Not exactly > >>> > >>>- I'd like to keep the pci_config_set_class in the devices, > >>> just make it do an assert. > >> > >>Assert on which condition? > > > >That PCI class matches device type defined in qdev. > >This will serve to verify that all devices are converted properly. > > > >>>- Nics already have DEFINE_NIC_PROPERTIES - can this be used somehow? > >>> Same for other devices ... > >> > >>We got DEFINE_NIC_PROPERTIES and DEFINE_BLOCK_PROPERTIES. > >>Something like DEFINE_PCI_PROPERTIES could be introduced, but i am not sure > > > >This shouldn't have to do anything with PCI. > >We should define the device type as NIC, > >PCI class can be derived from that. > > So you want to introduce a device property like "device_type" and set it to "NIC" and > then translate "NIC" to the correct PCI class code. Are you sure this is unambiguous? It does not need to be, that's why I would like to keep the code in device init that sets the PCI class. We would just check that it matches. > "NIC" could be ethernet or token ring. A generic name like "HBA" could translate to even > more codes (scsi, ide, raid, sata, ...). I don't insist on NIC. Maybe ethernet is a good thing. Maybe pci_class_descriptions is a good starting point. > > >>which device properties it should hold (vendor_id, device_id, class, etc?). > >>Those will then be user-modifiable with e.g. -device e1000,pci_class=1234. > >> > >>Sebastian > > > >Why is this helpful? > >- tweaking class will just break guests > >- binary representation is unfriendly, let's not require users to read > > pci spec just to run qemu. > > "pci_class" is optional. I just wanted to show that qdev device properties are > user-modifiable and i didn't find a way to prevent this. > > >I can imagine some useful features implemented by sticking the type in qdev: > >- -device ? would sort devices by type and print the type, > >- something like this for qtree etc > >- -help would get all e.g. nic models from qdev, > >- allow easy to remember aliases with per-type scope (e.g. > > -device virtio,type=nic to be equivalent to -device virtio-net-pci) > > Is this just a virtio problem? Not sure. I think usb has this, I think there are others. > >- filter (for device ?, qtree etc) system devices that user has no business > > tweaking (could be type=system or something like that) > > > >>diff --git a/hw/e1000.c b/hw/e1000.c > >>index af101bd..e302703 100644 > >>--- a/hw/e1000.c > >>+++ b/hw/e1000.c > >>@@ -1171,6 +1171,7 @@ static PCIDeviceInfo e1000_info = { > >> .romfile = "pxe-e1000.bin", > >> .qdev.props = (Property[]) { > >> DEFINE_NIC_PROPERTIES(E1000State, conf), > >>+ DEFINE_PROP_UINT16("pci_class", E1000State, dev.class, PCI_CLASS_NETWORK_ETHERNET), > >> DEFINE_PROP_END_OF_LIST(), > >> } > >>}; > >>diff --git a/hw/pci.c b/hw/pci.c > >>index ef00d20..06bbf04 100644 > >>--- a/hw/pci.c > >>+++ b/hw/pci.c > >>@@ -1620,6 +1620,9 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base) > >> info->is_bridge); > >> if (pci_dev == NULL) > >> return -1; > >>+ > >>+ pci_config_set_class(pci_dev->config, pci_dev->class); > >>+ > >> rc = info->init(pci_dev); > >> if (rc != 0) { > >> do_pci_unregister_device(pci_dev); > >>diff --git a/hw/pci.h b/hw/pci.h > >>index 17744dc..5dc9053 100644 > >>--- a/hw/pci.h > >>+++ b/hw/pci.h > >>@@ -189,6 +189,9 @@ struct PCIDevice { > >> char *romfile; > >> ram_addr_t rom_offset; > >> uint32_t rom_bar; > >>+ > >>+ /* class */ > > > >Please don't copy code into comments verbatim. > > > >>+ uint16_t class; > > > >Ugh. We have class in config already. > > But i could not get qdev to use it. That's a problem for qdev to solve :) > >>}; > >> > >>PCIDevice *pci_register_device(PCIBus *bus, const char *name, > >> > > Sebastian ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] Re: [PATCH] piix: use pci_config_set_prog_interface() 2010-12-29 10:26 ` Sebastian Herbszt 2010-12-29 11:21 ` Michael S. Tsirkin @ 2010-12-29 14:55 ` Michael S. Tsirkin 1 sibling, 0 replies; 9+ messages in thread From: Michael S. Tsirkin @ 2010-12-29 14:55 UTC (permalink / raw) To: Sebastian Herbszt; +Cc: qemu-devel On Wed, Dec 29, 2010 at 11:26:32AM +0100, Sebastian Herbszt wrote: > Michael S. Tsirkin wrote: > >On Tue, Dec 28, 2010 at 07:48:23PM +0100, Sebastian Herbszt wrote: > >>Michael S. Tsirkin wrote: > >>>On Tue, Dec 28, 2010 at 05:24:06PM +0100, Sebastian Herbszt wrote: > >>>>Michael S. Tsirkin wrote: > >>>>>On Mon, Dec 20, 2010 at 10:18:01PM +0100, Sebastian Herbszt wrote: > >>>>>>Use pci_config_set_prog_interface(). > >>>>>> > >>>>>>Signed-off-by: Sebastian Herbszt <herbszt@gmx.de> > >>>>> > >>>>>Since I was asked explicitly - I don't have a problem > >>>>>with these changes: both class and prog interface. > >>>>>However, they aren't all that useful in themselves. > >>>>> > >>>>>For class, what I would like to see is a system where > >>>>>the device class is put in the qdev info table, > >>>>>and where -device ? > >>>>>(and hopefully the legacy -help, -nic etc as well) > >>>>>use this information. > >>>> > >>>>I am not sure if you mean something like the patch below. > >>> > >>>Not exactly > >>> > >>>- I'd like to keep the pci_config_set_class in the devices, > >>> just make it do an assert. > >> > >>Assert on which condition? > > > >That PCI class matches device type defined in qdev. > >This will serve to verify that all devices are converted properly. > > > >>>- Nics already have DEFINE_NIC_PROPERTIES - can this be used somehow? > >>> Same for other devices ... > >> > >>We got DEFINE_NIC_PROPERTIES and DEFINE_BLOCK_PROPERTIES. > >>Something like DEFINE_PCI_PROPERTIES could be introduced, but i am not sure > > > >This shouldn't have to do anything with PCI. > >We should define the device type as NIC, > >PCI class can be derived from that. > > So you want to introduce a device property like "device_type" and set it to "NIC" and > then translate "NIC" to the correct PCI class code. Are you sure this is unambiguous? > "NIC" could be ethernet or token ring. A generic name like "HBA" could translate to even > more codes (scsi, ide, raid, sata, ...). > > >>which device properties it should hold (vendor_id, device_id, class, etc?). > >>Those will then be user-modifiable with e.g. -device e1000,pci_class=1234. > >> > >>Sebastian > > > >Why is this helpful? > >- tweaking class will just break guests > >- binary representation is unfriendly, let's not require users to read > > pci spec just to run qemu. > > "pci_class" is optional. > I just wanted to show that qdev device properties are > user-modifiable and i didn't find a way to prevent this. Ah, ok. I think properties starting with x- are internal. It's a kind of undocumented thing. -- MST ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2010-12-29 14:55 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-12-20 21:18 [Qemu-devel] [PATCH] piix: use pci_config_set_prog_interface() Sebastian Herbszt 2010-12-27 14:01 ` [Qemu-devel] " Michael S. Tsirkin 2010-12-28 16:24 ` Sebastian Herbszt 2010-12-28 17:19 ` Michael S. Tsirkin 2010-12-28 18:48 ` Sebastian Herbszt 2010-12-28 21:08 ` Michael S. Tsirkin 2010-12-29 10:26 ` Sebastian Herbszt 2010-12-29 11:21 ` Michael S. Tsirkin 2010-12-29 14:55 ` Michael S. Tsirkin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.