[Qemu-devel] [PATCH 1.0] ac97: don't override the pci subsystem id

[Qemu-devel] [PATCH 1.0] ac97: don't override the pci subsystem id

[Gerd Hoffmann]

This patch removes the code lines which set the subsystem id for the
emulated ac97 card to 8086:0000.  Due to the device id being zero the
subsystem id isn't vaild anyway.  With the patch applied the sound card
gets the default qemu subsystem id (1af4:1100) instead.

Cc: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/ac97.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/hw/ac97.c b/hw/ac97.c
index 6800af4..b43e435 100644
--- a/hw/ac97.c
+++ b/hw/ac97.c
@@ -1305,12 +1305,6 @@ static int ac97_initfn (PCIDevice *dev)
     c[PCI_BASE_ADDRESS_0 + 6] = 0x00;
     c[PCI_BASE_ADDRESS_0 + 7] = 0x00;
 
-    c[PCI_SUBSYSTEM_VENDOR_ID] = 0x86;      /* svid subsystem vendor id rwo */
-    c[PCI_SUBSYSTEM_VENDOR_ID + 1] = 0x80;
-
-    c[PCI_SUBSYSTEM_ID] = 0x00;      /* sid subsystem id rwo */
-    c[PCI_SUBSYSTEM_ID + 1] = 0x00;
-
     c[PCI_INTERRUPT_LINE] = 0x00;      /* intr_ln interrupt line rw */
     c[PCI_INTERRUPT_PIN] = 0x01;      /* intr_pn interrupt pin ro */
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1.0] ac97: don't override the pci subsystem id

[Avi Kivity]

On 11/07/2011 01:00 PM, Gerd Hoffmann wrote:
> This patch removes the code lines which set the subsystem id for the
> emulated ac97 card to 8086:0000.  Due to the device id being zero the
> subsystem id isn't vaild anyway.  With the patch applied the sound card
> gets the default qemu subsystem id (1af4:1100) instead.
>
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/ac97.c |    6 ------
>  1 files changed, 0 insertions(+), 6 deletions(-)
>
> diff --git a/hw/ac97.c b/hw/ac97.c
> index 6800af4..b43e435 100644
> --- a/hw/ac97.c
> +++ b/hw/ac97.c
> @@ -1305,12 +1305,6 @@ static int ac97_initfn (PCIDevice *dev)
>      c[PCI_BASE_ADDRESS_0 + 6] = 0x00;
>      c[PCI_BASE_ADDRESS_0 + 7] = 0x00;
>  
> -    c[PCI_SUBSYSTEM_VENDOR_ID] = 0x86;      /* svid subsystem vendor id rwo */
> -    c[PCI_SUBSYSTEM_VENDOR_ID + 1] = 0x80;
> -
> -    c[PCI_SUBSYSTEM_ID] = 0x00;      /* sid subsystem id rwo */
> -    c[PCI_SUBSYSTEM_ID + 1] = 0x00;
> -

This a guest ABI change.  Do we want -M support for it?

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 1.0] ac97: don't override the pci subsystem id

[Gerd Hoffmann]

On 11/07/11 15:17, Avi Kivity wrote:
> On 11/07/2011 01:00 PM, Gerd Hoffmann wrote:
>> This patch removes the code lines which set the subsystem id for the
>> emulated ac97 card to 8086:0000.  Due to the device id being zero the
>> subsystem id isn't vaild anyway.  With the patch applied the sound card
>> gets the default qemu subsystem id (1af4:1100) instead.
> 
> This a guest ABI change.  Do we want -M support for it?

Given that the old subsystem id isn't valid I'd say no unless someone
comes up with a good reason.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1.0] ac97: don't override the pci subsystem id

[Anthony Liguori]

On 11/07/2011 08:33 AM, Gerd Hoffmann wrote:
> On 11/07/11 15:17, Avi Kivity wrote:
>> On 11/07/2011 01:00 PM, Gerd Hoffmann wrote:
>>> This patch removes the code lines which set the subsystem id for the
>>> emulated ac97 card to 8086:0000.  Due to the device id being zero the
>>> subsystem id isn't vaild anyway.  With the patch applied the sound card
>>> gets the default qemu subsystem id (1af4:1100) instead.
>>
>> This a guest ABI change.  Do we want -M support for it?
>
> Given that the old subsystem id isn't valid I'd say no unless someone
> comes up with a good reason.

Agreed.

Regards,

Anthony Liguori

>
> cheers,
>    Gerd
>
>

Re: [Qemu-devel] [PATCH 1.0] ac97: don't override the pci subsystem id

[Avi Kivity]

On 11/07/2011 04:33 PM, Gerd Hoffmann wrote:
> On 11/07/11 15:17, Avi Kivity wrote:
> > On 11/07/2011 01:00 PM, Gerd Hoffmann wrote:
> >> This patch removes the code lines which set the subsystem id for the
> >> emulated ac97 card to 8086:0000.  Due to the device id being zero the
> >> subsystem id isn't vaild anyway.  With the patch applied the sound card
> >> gets the default qemu subsystem id (1af4:1100) instead.
> > 
> > This a guest ABI change.  Do we want -M support for it?
>
> Given that the old subsystem id isn't valid I'd say no unless someone
> comes up with a good reason.

Do we know that Windows won't complain about it?

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 1.0] ac97: don't override the pci subsystem id

[Anthony Liguori]

On 11/07/2011 08:42 AM, Avi Kivity wrote:
> On 11/07/2011 04:33 PM, Gerd Hoffmann wrote:
>> On 11/07/11 15:17, Avi Kivity wrote:
>>> On 11/07/2011 01:00 PM, Gerd Hoffmann wrote:
>>>> This patch removes the code lines which set the subsystem id for the
>>>> emulated ac97 card to 8086:0000.  Due to the device id being zero the
>>>> subsystem id isn't vaild anyway.  With the patch applied the sound card
>>>> gets the default qemu subsystem id (1af4:1100) instead.
>>>
>>> This a guest ABI change.  Do we want -M support for it?
>>
>> Given that the old subsystem id isn't valid I'd say no unless someone
>> comes up with a good reason.
>
> Do we know that Windows won't complain about it?

I thought the original motivation for the default subsystem ids was that some 
Windows test suite was explicitly complaining about having invalid subsystem ids?

Regards,

Anthony Liguori

>

Re: [Qemu-devel] [PATCH 1.0] ac97: don't override the pci subsystem id

[Avi Kivity]

On 11/07/2011 04:44 PM, Anthony Liguori wrote:
>>>> This a guest ABI change.  Do we want -M support for it?
>>>
>>> Given that the old subsystem id isn't valid I'd say no unless someone
>>> comes up with a good reason.
>>
>> Do we know that Windows won't complain about it?
>
>
> I thought the original motivation for the default subsystem ids was
> that some Windows test suite was explicitly complaining about having
> invalid subsystem ids?

I think so, but that's unrelated.  The worry is that some DRM code
checksums your hardware and complains if it changed too much.  Nothing
to do with the test suite.

The sense of Gerd's comment is reversed.  We should preserve the ABI
unless there is a strong reason not to.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 1.0] ac97: don't override the pci subsystem id

[Anthony Liguori]

On 11/07/2011 08:50 AM, Avi Kivity wrote:
> On 11/07/2011 04:44 PM, Anthony Liguori wrote:
>>>>> This a guest ABI change.  Do we want -M support for it?
>>>>
>>>> Given that the old subsystem id isn't valid I'd say no unless someone
>>>> comes up with a good reason.
>>>
>>> Do we know that Windows won't complain about it?
>>
>>
>> I thought the original motivation for the default subsystem ids was
>> that some Windows test suite was explicitly complaining about having
>> invalid subsystem ids?
>
> I think so, but that's unrelated.  The worry is that some DRM code
> checksums your hardware and complains if it changed too much.  Nothing
> to do with the test suite.
>
> The sense of Gerd's comment is reversed.  We should preserve the ABI
> unless there is a strong reason not to.

Yes, I understand where you're coming from and I agree except when it comes to 
bug fixes.

My view toward bug fixes is the opposite--unless we know that the bug fix breaks 
something, we should fix the bug. If it's a bug, we have to assume it's breaking 
something.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1.0] ac97: don't override the pci subsystem id

[Avi Kivity]

On 11/07/2011 04:53 PM, Anthony Liguori wrote:
>> I think so, but that's unrelated.  The worry is that some DRM code
>> checksums your hardware and complains if it changed too much.  Nothing
>> to do with the test suite.
>>
>> The sense of Gerd's comment is reversed.  We should preserve the ABI
>> unless there is a strong reason not to.
>
>
> Yes, I understand where you're coming from and I agree except when it
> comes to bug fixes.
>
> My view toward bug fixes is the opposite--unless we know that the bug
> fix breaks something, we should fix the bug. If it's a bug, we have to
> assume it's breaking something.

You're right in that not every bug fix deserves an entry in our quirk
table.  We don't want -M pc-0.15 to reintroduce a data corrupting bug
just because it is guest visible!

This is more of an edge case however, since we know that hardware tools
rely on PCI IDs.  For example our hypothetical ABI signature tool will
certainly include lspci like functionality and detect this as a change.

I now agree with both sides of the argument and can continue the
discussion unaided for at least 30-40 emails.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 1.0] ac97: don't override the pci subsystem id

[Gerd Hoffmann]

  Hi,

> This is more of an edge case however, since we know that hardware tools
> rely on PCI IDs.

The ID is invalid, you can't do anything useful with it ...

> For example our hypothetical ABI signature tool will
> certainly include lspci like functionality and detect this as a change.

... except maybe recording it somewhere to notice when changes.

So I guess your point is Windows guests might think they got a new sound
card, record that has hardware change and may require re-activation
because of that?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1.0] ac97: don't override the pci subsystem id

[Avi Kivity]

On 11/07/2011 05:10 PM, Gerd Hoffmann wrote:
>   Hi,
>
> > This is more of an edge case however, since we know that hardware tools
> > rely on PCI IDs.
>
> The ID is invalid, you can't do anything useful with it ...
>
> > For example our hypothetical ABI signature tool will
> > certainly include lspci like functionality and detect this as a change.
>
> ... except maybe recording it somewhere to notice when changes.
>
> So I guess your point is Windows guests might think they got a new sound
> card, record that has hardware change and may require re-activation
> because of that?

Yes.  Or some hardware inventory tool we know nothing about, which was
constructed specifically in order to make our lives miserable.

-- 
error compiling committee.c: too many arguments to function