[Qemu-devel] Re: [PATCH] piix: use pci_config_set_prog_interface()

[Sebastian Herbszt]

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 <address@hidden>
> > >>>
> > >>>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