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

["Michael S. Tsirkin" <mst@redhat.com>]

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