Patchwork Re: [PATCH] piix: use pci_config_set_prog_interface()

login
register
mail settings
Submitter Sebastian Herbszt
Date Dec. 28, 2010, 6:48 p.m.
Message ID <DA6C395899B14F2B8FD31E37B7EA0D8A@FSCPC>
Download mbox | patch
Permalink /patch/76855/
State New
Headers show

Comments

Sebastian Herbszt - Dec. 28, 2010, 6:48 p.m.
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
Michael S. Tsirkin - Dec. 28, 2010, 9:08 p.m.
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,
>
Sebastian Herbszt - Dec. 29, 2010, 10:26 a.m.
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
Michael S. Tsirkin - Dec. 29, 2010, 11:21 a.m.
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
Michael S. Tsirkin - Dec. 29, 2010, 2:55 p.m.
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.

Patch

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,