[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH] piix: use pci_config_set_prog_interface()
From: |
Michael S. Tsirkin |
Subject: |
[Qemu-devel] Re: [PATCH] piix: use pci_config_set_prog_interface() |
Date: |
Wed, 29 Dec 2010 13:21:27 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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 <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?
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
- [Qemu-devel] [PATCH] piix: use pci_config_set_prog_interface(), Sebastian Herbszt, 2010/12/20
- [Qemu-devel] Re: [PATCH] piix: use pci_config_set_prog_interface(), Michael S. Tsirkin, 2010/12/27
- [Qemu-devel] Re: [PATCH] piix: use pci_config_set_prog_interface(), Sebastian Herbszt, 2010/12/28
- [Qemu-devel] Re: [PATCH] piix: use pci_config_set_prog_interface(), Michael S. Tsirkin, 2010/12/28
- [Qemu-devel] Re: [PATCH] piix: use pci_config_set_prog_interface(), Sebastian Herbszt, 2010/12/28
- [Qemu-devel] Re: [PATCH] piix: use pci_config_set_prog_interface(), Michael S. Tsirkin, 2010/12/28
- [Qemu-devel] Re: [PATCH] piix: use pci_config_set_prog_interface(), Sebastian Herbszt, 2010/12/29
- [Qemu-devel] Re: [PATCH] piix: use pci_config_set_prog_interface(),
Michael S. Tsirkin <=
- [Qemu-devel] Re: [PATCH] piix: use pci_config_set_prog_interface(), Michael S. Tsirkin, 2010/12/29