[Qemu-devel] [PATCH] adding helper pci functions

[Qemu-devel] [PATCH] adding helper pci functions

[Gerd Hoffmann]

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;
-- 
1.6.6

[Qemu-devel] Re: [PATCH] adding helper pci functions

[Juan Quintela]

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.

[Qemu-devel] Re: [PATCH] adding helper pci functions

[Michael S. Tsirkin]

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.


-- 
MST

[Qemu-devel] Re: [PATCH] adding helper pci functions

[Izik Eidus]

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>
> ---
>    

Re: [Qemu-devel] [PATCH] adding helper pci functions

[Anthony Liguori]

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;
>    

Re: [Qemu-devel] [PATCH] adding helper pci functions

[Anthony Liguori]

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;
>