[Qemu-devel] [PATCH] pcnet: make subsystem vendor id match hardware

[Qemu-devel] [PATCH] pcnet: make subsystem vendor id match hardware

[Michael S. Tsirkin]

Real pcnet device (AT2450) apparently has subsystem
device and vendor id set to 0, this is out of spec
(which requires that vendor id is obtained from PCI SIG)
but windows xp driver seems to need this in order
to associate.

qemu sets pci subsystem id to qumranet/qemu
since d350d97d196a632b6c7493acf07a061017fc6f7d,
debian does not yet have this patch.

https://bugzilla.redhat.com/show_bug.cgi?id=521247

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/pcnet.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/hw/pcnet.c b/hw/pcnet.c
index 44b5b31..12260be 100644
--- a/hw/pcnet.c
+++ b/hw/pcnet.c
@@ -1997,6 +1997,9 @@ static int pci_pcnet_init(PCIDevice *pci_dev)
     pci_set_long(pci_conf + PCI_BASE_ADDRESS_0 + 4,
                  PCI_BASE_ADDRESS_SPACE_MEMORY);
 
+    pci_set_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID, 0x0);
+    pci_set_word(pci_conf + PCI_SUBSYSTEM_ID, 0x0);
+
     /* TODO: value must be 0 at RST# */
     pci_conf[PCI_INTERRUPT_PIN] = 1; // interrupt pin 0
     pci_conf[PCI_MIN_GNT] = 0x06;
-- 
1.7.0.18.g0d53a5

[Qemu-devel] Re: [PATCH] pcnet: make subsystem vendor id match hardware

[Jan Kiszka]

Michael S. Tsirkin wrote:
> Real pcnet device (AT2450) apparently has subsystem
> device and vendor id set to 0, this is out of spec
> (which requires that vendor id is obtained from PCI SIG)
> but windows xp driver seems to need this in order
> to associate.
> 
> qemu sets pci subsystem id to qumranet/qemu
> since d350d97d196a632b6c7493acf07a061017fc6f7d,
> debian does not yet have this patch.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=521247
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/pcnet.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pcnet.c b/hw/pcnet.c
> index 44b5b31..12260be 100644
> --- a/hw/pcnet.c
> +++ b/hw/pcnet.c
> @@ -1997,6 +1997,9 @@ static int pci_pcnet_init(PCIDevice *pci_dev)
>      pci_set_long(pci_conf + PCI_BASE_ADDRESS_0 + 4,
>                   PCI_BASE_ADDRESS_SPACE_MEMORY);
>  
> +    pci_set_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID, 0x0);
> +    pci_set_word(pci_conf + PCI_SUBSYSTEM_ID, 0x0);
> +
>      /* TODO: value must be 0 at RST# */
>      pci_conf[PCI_INTERRUPT_PIN] = 1; // interrupt pin 0
>      pci_conf[PCI_MIN_GNT] = 0x06;

No concerns from my side, still works here.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] pcnet: make subsystem vendor id match hardware

[Stefan Weil]

Michael S. Tsirkin schrieb:
> Real pcnet device (AT2450) apparently has subsystem
> device and vendor id set to 0, this is out of spec
> (which requires that vendor id is obtained from PCI SIG)
> but windows xp driver seems to need this in order
> to associate.
>
> qemu sets pci subsystem id to qumranet/qemu
> since d350d97d196a632b6c7493acf07a061017fc6f7d,
> debian does not yet have this patch.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=521247
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/pcnet.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/hw/pcnet.c b/hw/pcnet.c
> index 44b5b31..12260be 100644
> --- a/hw/pcnet.c
> +++ b/hw/pcnet.c
> @@ -1997,6 +1997,9 @@ static int pci_pcnet_init(PCIDevice *pci_dev)
>      pci_set_long(pci_conf + PCI_BASE_ADDRESS_0 + 4,
>                   PCI_BASE_ADDRESS_SPACE_MEMORY);
>  
> +    pci_set_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID, 0x0);
> +    pci_set_word(pci_conf + PCI_SUBSYSTEM_ID, 0x0);
> +
>      /* TODO: value must be 0 at RST# */
>      pci_conf[PCI_INTERRUPT_PIN] = 1; // interrupt pin 0
>      pci_conf[PCI_MIN_GNT] = 0x06;
>   

Don't you think that a comment in the code
(with the same explanation which you wrote above)
would be helpful in this special case?

Of course one can always call git blame, but
a comment is easier to read.

Regards
Stefan

Re: [Qemu-devel] [PATCH] pcnet: make subsystem vendor id match hardware

[Michael S. Tsirkin]

On Tue, Mar 16, 2010 at 10:29:43PM +0100, Stefan Weil wrote:
> Michael S. Tsirkin schrieb:
> > Real pcnet device (AT2450) apparently has subsystem
> > device and vendor id set to 0, this is out of spec
> > (which requires that vendor id is obtained from PCI SIG)
> > but windows xp driver seems to need this in order
> > to associate.
> >
> > qemu sets pci subsystem id to qumranet/qemu
> > since d350d97d196a632b6c7493acf07a061017fc6f7d,
> > debian does not yet have this patch.
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=521247
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Anthony Liguori <aliguori@us.ibm.com>
> > ---
> >  hw/pcnet.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> >
> > diff --git a/hw/pcnet.c b/hw/pcnet.c
> > index 44b5b31..12260be 100644
> > --- a/hw/pcnet.c
> > +++ b/hw/pcnet.c
> > @@ -1997,6 +1997,9 @@ static int pci_pcnet_init(PCIDevice *pci_dev)
> >      pci_set_long(pci_conf + PCI_BASE_ADDRESS_0 + 4,
> >                   PCI_BASE_ADDRESS_SPACE_MEMORY);
> >  
> > +    pci_set_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID, 0x0);
> > +    pci_set_word(pci_conf + PCI_SUBSYSTEM_ID, 0x0);
> > +
> >      /* TODO: value must be 0 at RST# */
> >      pci_conf[PCI_INTERRUPT_PIN] = 1; // interrupt pin 0
> >      pci_conf[PCI_MIN_GNT] = 0x06;
> >   
> 
> Don't you think that a comment in the code
> (with the same explanation which you wrote above)
> would be helpful in this special case?
> Of course one can always call git blame, but
> a comment is easier to read.
> 
> Regards
> Stefan

Yea, ok. Send a patch.

One thing that I'm still converned about is what pci.c should do with
subsystem id, since setting it to redhat/qumranet by default is not
really the best thing to do: it clearly is unlikely to match any real
hardware, and the heuristic it uses to detect when to set subsystem id
is also kind of off: the spec ties it with class not with header type.

Maybe I'll just add a TODO for now :)

-- 
MST