* [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.