| Message ID | 1267087285-15582-1-git-send-email-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
Gerd Hoffmann <kraxel@redhat.com> wrote: > From: Izik Eidus <ieidus@redhat.com> > > Signed-off-by: Izik Eidus <ieidus@redhat.com> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > hw/pci.h | 18 ++++++++++++++++++ > 1 files changed, 18 insertions(+), 0 deletions(-) > > diff --git a/hw/pci.h b/hw/pci.h > index 37ebdc4..20c670e 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -301,11 +301,29 @@ pci_config_set_device_id(uint8_t *pci_config, uint16_t val) > } > > static inline void > +pci_config_set_revision(uint8_t *pci_config, uint8_t val) > +{ > + pci_set_byte(&pci_config[PCI_REVISION_ID], val); > +} > + > +static inline void > pci_config_set_class(uint8_t *pci_config, uint16_t val) > { > pci_set_word(&pci_config[PCI_CLASS_DEVICE], val); > } > > +static inline void > +pci_config_set_prog_interface(uint8_t *pci_config, uint8_t val) > +{ > + pci_set_byte(&pci_config[PCI_CLASS_PROG], val); > +} > + > +static inline void > +pci_config_set_interrupt_pin(uint8_t *pci_config, uint8_t val) > +{ > + pci_set_byte(&pci_config[PCI_INTERRUPT_PIN], val); > +} > + > typedef int (*pci_qdev_initfn)(PCIDevice *dev); > typedef struct { > DeviceInfo qdev; mst had some reservations abotu this functions, but I have forgotten. mst can you comment again? Later, Juan.
On 02/25/2010 10:41 AM, Gerd Hoffmann wrote: > From: Izik Eidus<ieidus@redhat.com> > The true auther of this patch is Yaniv Kamay. Thanks. > Signed-off-by: Izik Eidus<ieidus@redhat.com> > Signed-off-by: Gerd Hoffmann<kraxel@redhat.com> > --- >
On Thu, Feb 25, 2010 at 11:55:25AM +0100, Juan Quintela wrote: > Gerd Hoffmann <kraxel@redhat.com> wrote: > > From: Izik Eidus <ieidus@redhat.com> > > > > Signed-off-by: Izik Eidus <ieidus@redhat.com> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > --- > > hw/pci.h | 18 ++++++++++++++++++ > > 1 files changed, 18 insertions(+), 0 deletions(-) > > > > diff --git a/hw/pci.h b/hw/pci.h > > index 37ebdc4..20c670e 100644 > > --- a/hw/pci.h > > +++ b/hw/pci.h > > @@ -301,11 +301,29 @@ pci_config_set_device_id(uint8_t *pci_config, uint16_t val) > > } > > > > static inline void > > +pci_config_set_revision(uint8_t *pci_config, uint8_t val) > > +{ > > + pci_set_byte(&pci_config[PCI_REVISION_ID], val); > > +} > > + > > +static inline void > > pci_config_set_class(uint8_t *pci_config, uint16_t val) > > { > > pci_set_word(&pci_config[PCI_CLASS_DEVICE], val); > > } > > > > +static inline void > > +pci_config_set_prog_interface(uint8_t *pci_config, uint8_t val) > > +{ > > + pci_set_byte(&pci_config[PCI_CLASS_PROG], val); > > +} > > + > > +static inline void > > +pci_config_set_interrupt_pin(uint8_t *pci_config, uint8_t val) > > +{ > > + pci_set_byte(&pci_config[PCI_INTERRUPT_PIN], val); > > +} > > + This one is wrong I think. Devices have no business touching interrupt pin register. > > typedef int (*pci_qdev_initfn)(PCIDevice *dev); > > typedef struct { > > DeviceInfo qdev; > > mst had some reservations abotu this functions, but I have forgotten. > > mst can you comment again? > > Later, Juan. Yes, I commented on this internally. By the last count, we have about 600 registers. Adding incline functions for all of them is just a source of bugs, using pci_regs directly is better in that sense. Even worse, this creates two legal ways to do the same thing. So no one thing you can grep for. We could convert all devices, but it's a lot of churn for very little benefit, and I don't see how such a change can be tested. So I think really only the common registers should have inline functions, and for most devices, class/revision/prog interface should just have default values. Finally, the APIs as proposed here just do not get us much. You still must remember which values to put in: pci_config_set_prog_interface(config, PCI_CLASS_SERIAL_USB) will happily compile, and you still must verify that value you put in does fit in. What I would really like to see is higher level APIs. So pci_init_legacy_vga_adapter might make sense, it would not just set class, but also register IO handlers for legacy ports.
On 02/25/2010 02:41 AM, Gerd Hoffmann wrote: > From: Izik Eidus<ieidus@redhat.com> > > Signed-off-by: Izik Eidus<ieidus@redhat.com> > Signed-off-by: Gerd Hoffmann<kraxel@redhat.com> > Applied. Thanks. Regards, Anthony Liguori > --- > hw/pci.h | 18 ++++++++++++++++++ > 1 files changed, 18 insertions(+), 0 deletions(-) > > diff --git a/hw/pci.h b/hw/pci.h > index 37ebdc4..20c670e 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -301,11 +301,29 @@ pci_config_set_device_id(uint8_t *pci_config, uint16_t val) > } > > static inline void > +pci_config_set_revision(uint8_t *pci_config, uint8_t val) > +{ > + pci_set_byte(&pci_config[PCI_REVISION_ID], val); > +} > + > +static inline void > pci_config_set_class(uint8_t *pci_config, uint16_t val) > { > pci_set_word(&pci_config[PCI_CLASS_DEVICE], val); > } > > +static inline void > +pci_config_set_prog_interface(uint8_t *pci_config, uint8_t val) > +{ > + pci_set_byte(&pci_config[PCI_CLASS_PROG], val); > +} > + > +static inline void > +pci_config_set_interrupt_pin(uint8_t *pci_config, uint8_t val) > +{ > + pci_set_byte(&pci_config[PCI_INTERRUPT_PIN], val); > +} > + > typedef int (*pci_qdev_initfn)(PCIDevice *dev); > typedef struct { > DeviceInfo qdev; >
On 02/25/2010 02:41 AM, Gerd Hoffmann wrote: > From: Izik Eidus<ieidus@redhat.com> > > Signed-off-by: Izik Eidus<ieidus@redhat.com> > Signed-off-by: Gerd Hoffmann<kraxel@redhat.com> > Applied. Thanks. Regards, Anthony Liguori > --- > hw/pci.h | 18 ++++++++++++++++++ > 1 files changed, 18 insertions(+), 0 deletions(-) > > diff --git a/hw/pci.h b/hw/pci.h > index 37ebdc4..20c670e 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -301,11 +301,29 @@ pci_config_set_device_id(uint8_t *pci_config, uint16_t val) > } > > static inline void > +pci_config_set_revision(uint8_t *pci_config, uint8_t val) > +{ > + pci_set_byte(&pci_config[PCI_REVISION_ID], val); > +} > + > +static inline void > pci_config_set_class(uint8_t *pci_config, uint16_t val) > { > pci_set_word(&pci_config[PCI_CLASS_DEVICE], val); > } > > +static inline void > +pci_config_set_prog_interface(uint8_t *pci_config, uint8_t val) > +{ > + pci_set_byte(&pci_config[PCI_CLASS_PROG], val); > +} > + > +static inline void > +pci_config_set_interrupt_pin(uint8_t *pci_config, uint8_t val) > +{ > + pci_set_byte(&pci_config[PCI_INTERRUPT_PIN], val); > +} > + > typedef int (*pci_qdev_initfn)(PCIDevice *dev); > typedef struct { > DeviceInfo qdev; >
diff --git a/hw/pci.h b/hw/pci.h index 37ebdc4..20c670e 100644 --- a/hw/pci.h +++ b/hw/pci.h @@ -301,11 +301,29 @@ pci_config_set_device_id(uint8_t *pci_config, uint16_t val) } static inline void +pci_config_set_revision(uint8_t *pci_config, uint8_t val) +{ + pci_set_byte(&pci_config[PCI_REVISION_ID], val); +} + +static inline void pci_config_set_class(uint8_t *pci_config, uint16_t val) { pci_set_word(&pci_config[PCI_CLASS_DEVICE], val); } +static inline void +pci_config_set_prog_interface(uint8_t *pci_config, uint8_t val) +{ + pci_set_byte(&pci_config[PCI_CLASS_PROG], val); +} + +static inline void +pci_config_set_interrupt_pin(uint8_t *pci_config, uint8_t val) +{ + pci_set_byte(&pci_config[PCI_INTERRUPT_PIN], val); +} + typedef int (*pci_qdev_initfn)(PCIDevice *dev); typedef struct { DeviceInfo qdev;