[Qemu-devel] Wierd hack to sound/pci/intel8x0.c

[Qemu-devel] Wierd hack to sound/pci/intel8x0.c

[Avi Kivity]

The recently merged 228cf79376f1 ("ALSA: intel8x0: Improve performance
in virtual environment") is hacky and somewhat wrong.

First, the detection code

+       if (inside_vm < 0) {
+               /* detect KVM and Parallels virtual environments */
+               inside_vm = kvm_para_available();
+#if defined(__i386__) || defined(__x86_64__)
+               inside_vm = inside_vm ||
boot_cpu_has(X86_FEATURE_HYPERVISOR);
+#endif
+       }
+
 
is incorrect.  It detects that you're running in a guest, but that
doesn't imply that the device you're accessing is emulated.  It may be a
host device assigned to the guest; presumably the optimization you apply
doesn't work for real devices.

Second, the optimization itself looks fishy:

        spin_lock(&chip->reg_lock);
        do {
                civ = igetbyte(chip, ichdev->reg_offset + ICH_REG_OFF_CIV);
                ptr1 = igetword(chip, ichdev->reg_offset +
ichdev->roff_picb);
                position = ichdev->position;
                if (ptr1 == 0) {
                        udelay(10);
                        continue;
                }
-               if (civ == igetbyte(chip, ichdev->reg_offset +
ICH_REG_OFF_CIV) &&
-                   ptr1 == igetword(chip, ichdev->reg_offset +
ichdev->roff_picb))
+               if (civ != igetbyte(chip, ichdev->reg_offset +
ICH_REG_OFF_CIV))
+                       continue;
+               if (chip->inside_vm)
+                       break;
+               if (ptr1 == igetword(chip, ichdev->reg_offset +
ichdev->roff_picb))
                        break;
        } while (timeout--);


Why is the emulated device timing out?  Can't the emulation be fixed to
behave like real hardware?

Last, please copy kvm@vger.kernel.org on such issues.

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

Re: [Qemu-devel] Wierd hack to sound/pci/intel8x0.c

[Denis V. Lunev]

On 11/6/11 6:51 PM, Avi Kivity wrote:
> The recently merged 228cf79376f1 ("ALSA: intel8x0: Improve performance
> in virtual environment") is hacky and somewhat wrong.
>
> First, the detection code
>
> +       if (inside_vm<  0) {
> +               /* detect KVM and Parallels virtual environments */
> +               inside_vm = kvm_para_available();
> +#if defined(__i386__) || defined(__x86_64__)
> +               inside_vm = inside_vm ||
> boot_cpu_has(X86_FEATURE_HYPERVISOR);
> +#endif
> +       }
> +
>
> is incorrect.  It detects that you're running in a guest, but that
> doesn't imply that the device you're accessing is emulated.  It may be a
> host device assigned to the guest; presumably the optimization you apply
> doesn't work for real devices.
>
> Second, the optimization itself looks fishy:
>
>          spin_lock(&chip->reg_lock);
>          do {
>                  civ = igetbyte(chip, ichdev->reg_offset + ICH_REG_OFF_CIV);
>                  ptr1 = igetword(chip, ichdev->reg_offset +
> ichdev->roff_picb);
>                  position = ichdev->position;
>                  if (ptr1 == 0) {
>                          udelay(10);
>                          continue;
>                  }
> -               if (civ == igetbyte(chip, ichdev->reg_offset +
> ICH_REG_OFF_CIV)&&
> -                   ptr1 == igetword(chip, ichdev->reg_offset +
> ichdev->roff_picb))
> +               if (civ != igetbyte(chip, ichdev->reg_offset +
> ICH_REG_OFF_CIV))
> +                       continue;
> +               if (chip->inside_vm)
> +                       break;
> +               if (ptr1 == igetword(chip, ichdev->reg_offset +
> ichdev->roff_picb))
>                          break;
>          } while (timeout--);
>
>
> Why is the emulated device timing out?  Can't the emulation be fixed to
> behave like real hardware?
>
> Last, please copy kvm@vger.kernel.org on such issues.
>
The problem is that emulation can not be fixed.

How this is working for real hardware? You get data from real sound card 
register.
The scheduling is off at the moment thus you can not be re-scheduled.

In the virtual environment the situation is different. Any IO emulation 
is expensive.
The processor is switched from guest to hypervisor and further to 
emulation process
takes a lot of time.  This time is enough to obtain different value on 
next register read.
That's why this code is really timed out. Please also note that host 
scheduler also
plays his games and could schedule out VCPU thread.

The problem could be potentially fixed reducing precision of PICB emulation,
but this results in lower sound quality.

This kludge has been written this way in order not to break legacy card 
for which we
do not have an access. The code reading PICB/CIV registers second time 
was added
to fix issues on unknown for now platform and it looks not possible how 
to find/test
against this platform now. We have checked Windows drivers written by 
Intel/AMD
(32/64 bit) and MacOS ones. There is no second reading of CIV/PICB 
inside. We
hope that this is relay needed only for some rare hadware devices.

The only thing we can is to improve detection code. Suggestions are welcome.

Regards,
     Den

Re: [Qemu-devel] Wierd hack to sound/pci/intel8x0.c

[Avi Kivity]

On 11/06/2011 06:15 PM, Denis V. Lunev wrote:
> On 11/6/11 6:51 PM, Avi Kivity wrote:
>> The recently merged 228cf79376f1 ("ALSA: intel8x0: Improve performance
>> in virtual environment") is hacky and somewhat wrong.
>>
>> First, the detection code
>>
>> +       if (inside_vm<  0) {
>> +               /* detect KVM and Parallels virtual environments */
>> +               inside_vm = kvm_para_available();
>> +#if defined(__i386__) || defined(__x86_64__)
>> +               inside_vm = inside_vm ||
>> boot_cpu_has(X86_FEATURE_HYPERVISOR);
>> +#endif
>> +       }
>> +
>>
>> is incorrect.  It detects that you're running in a guest, but that
>> doesn't imply that the device you're accessing is emulated.  It may be a
>> host device assigned to the guest; presumably the optimization you apply
>> doesn't work for real devices.
>>
>> Second, the optimization itself looks fishy:
>>
>>          spin_lock(&chip->reg_lock);
>>          do {
>>                  civ = igetbyte(chip, ichdev->reg_offset +
>> ICH_REG_OFF_CIV);
>>                  ptr1 = igetword(chip, ichdev->reg_offset +
>> ichdev->roff_picb);
>>                  position = ichdev->position;
>>                  if (ptr1 == 0) {
>>                          udelay(10);
>>                          continue;
>>                  }
>> -               if (civ == igetbyte(chip, ichdev->reg_offset +
>> ICH_REG_OFF_CIV)&&
>> -                   ptr1 == igetword(chip, ichdev->reg_offset +
>> ichdev->roff_picb))
>> +               if (civ != igetbyte(chip, ichdev->reg_offset +
>> ICH_REG_OFF_CIV))
>> +                       continue;
>> +               if (chip->inside_vm)
>> +                       break;
>> +               if (ptr1 == igetword(chip, ichdev->reg_offset +
>> ichdev->roff_picb))
>>                          break;
>>          } while (timeout--);
>>
>>
>> Why is the emulated device timing out?  Can't the emulation be fixed to
>> behave like real hardware?
>>
>> Last, please copy kvm@vger.kernel.org on such issues.
>>
> The problem is that emulation can not be fixed.
>
> How this is working for real hardware? You get data from real sound
> card register.
> The scheduling is off at the moment thus you can not be re-scheduled.
>
> In the virtual environment the situation is different. Any IO
> emulation is expensive.
> The processor is switched from guest to hypervisor and further to
> emulation process
> takes a lot of time.  This time is enough to obtain different value on
> next register read.
> That's why this code is really timed out. Please also note that host
> scheduler also
> plays his games and could schedule out VCPU thread.
>

Note on kvm this is rare, since the guest thread and the emulator thread
are the same.

> The problem could be potentially fixed reducing precision of PICB
> emulation,
> but this results in lower sound quality.
>
> This kludge has been written this way in order not to break legacy
> card for which we
> do not have an access. The code reading PICB/CIV registers second time
> was added
> to fix issues on unknown for now platform and it looks not possible
> how to find/test
> against this platform now. We have checked Windows drivers written by
> Intel/AMD
> (32/64 bit) and MacOS ones. There is no second reading of CIV/PICB
> inside. We
> hope that this is relay needed only for some rare hadware devices.
>

Ok, so if I understand correctly, this loop is a hack for broken
hardware, and this patch basically unhacks it back, assuming that the
emulated (or assigned) hardware is not broken.

> The only thing we can is to improve detection code. Suggestions are
> welcome.

I think it's fine to assume that you're not assigning a 2004 era sound
card to a guest.  So I think the code is fine as it is, and can only
suggest to add a comment explaining the mess.

Thanks for explaining.

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

Re: [Qemu-devel] Wierd hack to sound/pci/intel8x0.c

[Takashi Iwai]

At Sun, 6 Nov 2011 20:15:01 +0400,
Denis V. Lunev wrote:
> 
> On 11/6/11 6:51 PM, Avi Kivity wrote:
> > The recently merged 228cf79376f1 ("ALSA: intel8x0: Improve performance
> > in virtual environment") is hacky and somewhat wrong.
> >
> > First, the detection code
> >
> > +       if (inside_vm<  0) {
> > +               /* detect KVM and Parallels virtual environments */
> > +               inside_vm = kvm_para_available();
> > +#if defined(__i386__) || defined(__x86_64__)
> > +               inside_vm = inside_vm ||
> > boot_cpu_has(X86_FEATURE_HYPERVISOR);
> > +#endif
> > +       }
> > +
> >
> > is incorrect.  It detects that you're running in a guest, but that
> > doesn't imply that the device you're accessing is emulated.  It may be a
> > host device assigned to the guest; presumably the optimization you apply
> > doesn't work for real devices.
> >
> > Second, the optimization itself looks fishy:
> >
> >          spin_lock(&chip->reg_lock);
> >          do {
> >                  civ = igetbyte(chip, ichdev->reg_offset + ICH_REG_OFF_CIV);
> >                  ptr1 = igetword(chip, ichdev->reg_offset +
> > ichdev->roff_picb);
> >                  position = ichdev->position;
> >                  if (ptr1 == 0) {
> >                          udelay(10);
> >                          continue;
> >                  }
> > -               if (civ == igetbyte(chip, ichdev->reg_offset +
> > ICH_REG_OFF_CIV)&&
> > -                   ptr1 == igetword(chip, ichdev->reg_offset +
> > ichdev->roff_picb))
> > +               if (civ != igetbyte(chip, ichdev->reg_offset +
> > ICH_REG_OFF_CIV))
> > +                       continue;
> > +               if (chip->inside_vm)
> > +                       break;
> > +               if (ptr1 == igetword(chip, ichdev->reg_offset +
> > ichdev->roff_picb))
> >                          break;
> >          } while (timeout--);
> >
> >
> > Why is the emulated device timing out?  Can't the emulation be fixed to
> > behave like real hardware?
> >
> > Last, please copy kvm@vger.kernel.org on such issues.
> >
> The problem is that emulation can not be fixed.
> 
> How this is working for real hardware? You get data from real sound card 
> register.
> The scheduling is off at the moment thus you can not be re-scheduled.
> 
> In the virtual environment the situation is different. Any IO emulation 
> is expensive.
> The processor is switched from guest to hypervisor and further to 
> emulation process
> takes a lot of time.  This time is enough to obtain different value on 
> next register read.
> That's why this code is really timed out. Please also note that host 
> scheduler also
> plays his games and could schedule out VCPU thread.
> 
> The problem could be potentially fixed reducing precision of PICB emulation,
> but this results in lower sound quality.
> 
> This kludge has been written this way in order not to break legacy card 
> for which we
> do not have an access. The code reading PICB/CIV registers second time 
> was added
> to fix issues on unknown for now platform and it looks not possible how 
> to find/test
> against this platform now. We have checked Windows drivers written by 
> Intel/AMD
> (32/64 bit) and MacOS ones. There is no second reading of CIV/PICB 
> inside. We
> hope that this is relay needed only for some rare hadware devices.
> 
> The only thing we can is to improve detection code. Suggestions are welcome.

Agreed.  If we can know the virtual device for KVM in a better way
(e.g. any specific PCI SSID or such), we can narrow the condition more
safely.


thanks,

Takashi

Re: [Qemu-devel] Wierd hack to sound/pci/intel8x0.c

[Takashi Iwai]

At Sun, 06 Nov 2011 18:31:42 +0200,
Avi Kivity wrote:
> 
> On 11/06/2011 06:15 PM, Denis V. Lunev wrote:
> > On 11/6/11 6:51 PM, Avi Kivity wrote:
> >> The recently merged 228cf79376f1 ("ALSA: intel8x0: Improve performance
> >> in virtual environment") is hacky and somewhat wrong.
> >>
> >> First, the detection code
> >>
> >> +       if (inside_vm<  0) {
> >> +               /* detect KVM and Parallels virtual environments */
> >> +               inside_vm = kvm_para_available();
> >> +#if defined(__i386__) || defined(__x86_64__)
> >> +               inside_vm = inside_vm ||
> >> boot_cpu_has(X86_FEATURE_HYPERVISOR);
> >> +#endif
> >> +       }
> >> +
> >>
> >> is incorrect.  It detects that you're running in a guest, but that
> >> doesn't imply that the device you're accessing is emulated.  It may be a
> >> host device assigned to the guest; presumably the optimization you apply
> >> doesn't work for real devices.
> >>
> >> Second, the optimization itself looks fishy:
> >>
> >>          spin_lock(&chip->reg_lock);
> >>          do {
> >>                  civ = igetbyte(chip, ichdev->reg_offset +
> >> ICH_REG_OFF_CIV);
> >>                  ptr1 = igetword(chip, ichdev->reg_offset +
> >> ichdev->roff_picb);
> >>                  position = ichdev->position;
> >>                  if (ptr1 == 0) {
> >>                          udelay(10);
> >>                          continue;
> >>                  }
> >> -               if (civ == igetbyte(chip, ichdev->reg_offset +
> >> ICH_REG_OFF_CIV)&&
> >> -                   ptr1 == igetword(chip, ichdev->reg_offset +
> >> ichdev->roff_picb))
> >> +               if (civ != igetbyte(chip, ichdev->reg_offset +
> >> ICH_REG_OFF_CIV))
> >> +                       continue;
> >> +               if (chip->inside_vm)
> >> +                       break;
> >> +               if (ptr1 == igetword(chip, ichdev->reg_offset +
> >> ichdev->roff_picb))
> >>                          break;
> >>          } while (timeout--);
> >>
> >>
> >> Why is the emulated device timing out?  Can't the emulation be fixed to
> >> behave like real hardware?
> >>
> >> Last, please copy kvm@vger.kernel.org on such issues.
> >>
> > The problem is that emulation can not be fixed.
> >
> > How this is working for real hardware? You get data from real sound
> > card register.
> > The scheduling is off at the moment thus you can not be re-scheduled.
> >
> > In the virtual environment the situation is different. Any IO
> > emulation is expensive.
> > The processor is switched from guest to hypervisor and further to
> > emulation process
> > takes a lot of time.  This time is enough to obtain different value on
> > next register read.
> > That's why this code is really timed out. Please also note that host
> > scheduler also
> > plays his games and could schedule out VCPU thread.
> >
> 
> Note on kvm this is rare, since the guest thread and the emulator thread
> are the same.
> 
> > The problem could be potentially fixed reducing precision of PICB
> > emulation,
> > but this results in lower sound quality.
> >
> > This kludge has been written this way in order not to break legacy
> > card for which we
> > do not have an access. The code reading PICB/CIV registers second time
> > was added
> > to fix issues on unknown for now platform and it looks not possible
> > how to find/test
> > against this platform now. We have checked Windows drivers written by
> > Intel/AMD
> > (32/64 bit) and MacOS ones. There is no second reading of CIV/PICB
> > inside. We
> > hope that this is relay needed only for some rare hadware devices.
> >
> 
> Ok, so if I understand correctly, this loop is a hack for broken
> hardware, and this patch basically unhacks it back, assuming that the
> emulated (or assigned) hardware is not broken.

Exactly.  The loop itself is still needed, but the check can be
less restrictive.

> > The only thing we can is to improve detection code. Suggestions are
> > welcome.
> 
> I think it's fine to assume that you're not assigning a 2004 era sound
> card to a guest.  So I think the code is fine as it is, and can only
> suggest to add a comment explaining the mess.

True.  Denis, care to submit a patch?


thanks,

Takashi

Re: [Qemu-devel] Wierd hack to sound/pci/intel8x0.c

[Denis V. Lunev]

On 11/6/11 8:31 PM, Avi Kivity wrote:
> On 11/06/2011 06:15 PM, Denis V. Lunev wrote:
>> On 11/6/11 6:51 PM, Avi Kivity wrote:
>>> The recently merged 228cf79376f1 ("ALSA: intel8x0: Improve performance
>>> in virtual environment") is hacky and somewhat wrong.
>>>
>>> First, the detection code
>>>
>>> +       if (inside_vm<   0) {
>>> +               /* detect KVM and Parallels virtual environments */
>>> +               inside_vm = kvm_para_available();
>>> +#if defined(__i386__) || defined(__x86_64__)
>>> +               inside_vm = inside_vm ||
>>> boot_cpu_has(X86_FEATURE_HYPERVISOR);
>>> +#endif
>>> +       }
>>> +
>>>
>>> is incorrect.  It detects that you're running in a guest, but that
>>> doesn't imply that the device you're accessing is emulated.  It may be a
>>> host device assigned to the guest; presumably the optimization you apply
>>> doesn't work for real devices.
>>>
>>> Second, the optimization itself looks fishy:
>>>
>>>           spin_lock(&chip->reg_lock);
>>>           do {
>>>                   civ = igetbyte(chip, ichdev->reg_offset +
>>> ICH_REG_OFF_CIV);
>>>                   ptr1 = igetword(chip, ichdev->reg_offset +
>>> ichdev->roff_picb);
>>>                   position = ichdev->position;
>>>                   if (ptr1 == 0) {
>>>                           udelay(10);
>>>                           continue;
>>>                   }
>>> -               if (civ == igetbyte(chip, ichdev->reg_offset +
>>> ICH_REG_OFF_CIV)&&
>>> -                   ptr1 == igetword(chip, ichdev->reg_offset +
>>> ichdev->roff_picb))
>>> +               if (civ != igetbyte(chip, ichdev->reg_offset +
>>> ICH_REG_OFF_CIV))
>>> +                       continue;
>>> +               if (chip->inside_vm)
>>> +                       break;
>>> +               if (ptr1 == igetword(chip, ichdev->reg_offset +
>>> ichdev->roff_picb))
>>>                           break;
>>>           } while (timeout--);
>>>
>>>
>>> Why is the emulated device timing out?  Can't the emulation be fixed to
>>> behave like real hardware?
>>>
>>> Last, please copy kvm@vger.kernel.org on such issues.
>>>
>> The problem is that emulation can not be fixed.
>>
>> How this is working for real hardware? You get data from real sound
>> card register.
>> The scheduling is off at the moment thus you can not be re-scheduled.
>>
>> In the virtual environment the situation is different. Any IO
>> emulation is expensive.
>> The processor is switched from guest to hypervisor and further to
>> emulation process
>> takes a lot of time.  This time is enough to obtain different value on
>> next register read.
>> That's why this code is really timed out. Please also note that host
>> scheduler also
>> plays his games and could schedule out VCPU thread.
>>
> Note on kvm this is rare, since the guest thread and the emulator thread
> are the same.

It is the same for Parallels too, but it can be scheduled out anyway.
The most important part here is context switches cost, which is actually 
high and enough to result
in different PICB value.

>> The problem could be potentially fixed reducing precision of PICB
>> emulation,
>> but this results in lower sound quality.
>>
>> This kludge has been written this way in order not to break legacy
>> card for which we
>> do not have an access. The code reading PICB/CIV registers second time
>> was added
>> to fix issues on unknown for now platform and it looks not possible
>> how to find/test
>> against this platform now. We have checked Windows drivers written by
>> Intel/AMD
>> (32/64 bit) and MacOS ones. There is no second reading of CIV/PICB
>> inside. We
>> hope that this is relay needed only for some rare hadware devices.
>>
> Ok, so if I understand correctly, this loop is a hack for broken
> hardware, and this patch basically unhacks it back, assuming that the
> emulated (or assigned) hardware is not broken.
>
>> The only thing we can is to improve detection code. Suggestions are
>> welcome.
> I think it's fine to assume that you're not assigning a 2004 era sound
> card to a guest.  So I think the code is fine as it is, and can only
> suggest to add a comment explaining the mess.
>
> Thanks for explaining.
>
no prob

Regard,
     Den

Re: [Qemu-devel] Wierd hack to sound/pci/intel8x0.c

[Denis V. Lunev]

On 11/6/11 8:47 PM, Takashi Iwai wrote:
> At Sun, 06 Nov 2011 18:31:42 +0200,
> Avi Kivity wrote:
>> On 11/06/2011 06:15 PM, Denis V. Lunev wrote:
>>> On 11/6/11 6:51 PM, Avi Kivity wrote:
>>>> The recently merged 228cf79376f1 ("ALSA: intel8x0: Improve performance
>>>> in virtual environment") is hacky and somewhat wrong.
>>>>
>>>> First, the detection code
>>>>
>>>> +       if (inside_vm<   0) {
>>>> +               /* detect KVM and Parallels virtual environments */
>>>> +               inside_vm = kvm_para_available();
>>>> +#if defined(__i386__) || defined(__x86_64__)
>>>> +               inside_vm = inside_vm ||
>>>> boot_cpu_has(X86_FEATURE_HYPERVISOR);
>>>> +#endif
>>>> +       }
>>>> +
>>>>
>>>> is incorrect.  It detects that you're running in a guest, but that
>>>> doesn't imply that the device you're accessing is emulated.  It may be a
>>>> host device assigned to the guest; presumably the optimization you apply
>>>> doesn't work for real devices.
>>>>
>>>> Second, the optimization itself looks fishy:
>>>>
>>>>           spin_lock(&chip->reg_lock);
>>>>           do {
>>>>                   civ = igetbyte(chip, ichdev->reg_offset +
>>>> ICH_REG_OFF_CIV);
>>>>                   ptr1 = igetword(chip, ichdev->reg_offset +
>>>> ichdev->roff_picb);
>>>>                   position = ichdev->position;
>>>>                   if (ptr1 == 0) {
>>>>                           udelay(10);
>>>>                           continue;
>>>>                   }
>>>> -               if (civ == igetbyte(chip, ichdev->reg_offset +
>>>> ICH_REG_OFF_CIV)&&
>>>> -                   ptr1 == igetword(chip, ichdev->reg_offset +
>>>> ichdev->roff_picb))
>>>> +               if (civ != igetbyte(chip, ichdev->reg_offset +
>>>> ICH_REG_OFF_CIV))
>>>> +                       continue;
>>>> +               if (chip->inside_vm)
>>>> +                       break;
>>>> +               if (ptr1 == igetword(chip, ichdev->reg_offset +
>>>> ichdev->roff_picb))
>>>>                           break;
>>>>           } while (timeout--);
>>>>
>>>>
>>>> Why is the emulated device timing out?  Can't the emulation be fixed to
>>>> behave like real hardware?
>>>>
>>>> Last, please copy kvm@vger.kernel.org on such issues.
>>>>
>>> The problem is that emulation can not be fixed.
>>>
>>> How this is working for real hardware? You get data from real sound
>>> card register.
>>> The scheduling is off at the moment thus you can not be re-scheduled.
>>>
>>> In the virtual environment the situation is different. Any IO
>>> emulation is expensive.
>>> The processor is switched from guest to hypervisor and further to
>>> emulation process
>>> takes a lot of time.  This time is enough to obtain different value on
>>> next register read.
>>> That's why this code is really timed out. Please also note that host
>>> scheduler also
>>> plays his games and could schedule out VCPU thread.
>>>
>> Note on kvm this is rare, since the guest thread and the emulator thread
>> are the same.
>>
>>> The problem could be potentially fixed reducing precision of PICB
>>> emulation,
>>> but this results in lower sound quality.
>>>
>>> This kludge has been written this way in order not to break legacy
>>> card for which we
>>> do not have an access. The code reading PICB/CIV registers second time
>>> was added
>>> to fix issues on unknown for now platform and it looks not possible
>>> how to find/test
>>> against this platform now. We have checked Windows drivers written by
>>> Intel/AMD
>>> (32/64 bit) and MacOS ones. There is no second reading of CIV/PICB
>>> inside. We
>>> hope that this is relay needed only for some rare hadware devices.
>>>
>> Ok, so if I understand correctly, this loop is a hack for broken
>> hardware, and this patch basically unhacks it back, assuming that the
>> emulated (or assigned) hardware is not broken.
> Exactly.  The loop itself is still needed, but the check can be
> less restrictive.
>
>>> The only thing we can is to improve detection code. Suggestions are
>>> welcome.
>> I think it's fine to assume that you're not assigning a 2004 era sound
>> card to a guest.  So I think the code is fine as it is, and can only
>> suggest to add a comment explaining the mess.
> True.  Denis, care to submit a patch?
sure! I'll do that tomorrow

Re: [Qemu-devel] Wierd hack to sound/pci/intel8x0.c

[Gerd Hoffmann]

  Hi,

> Agreed.  If we can know the virtual device for KVM in a better way
> (e.g. any specific PCI SSID or such), we can narrow the condition more
> safely.

PCI Subsystem ID 1af4:1100 is qemu/kvm (not only Intel HDA, most other
emulated pci devices have it too). Complete entry:

00:04.0 0403: 8086:2668 (rev 01)
        Subsystem: 1af4:1100
        Physical Slot: 4
        Flags: bus master, fast devsel, latency 0, IRQ 11
        Memory at e2030000 (32-bit, non-prefetchable) [size=16K]
        Kernel driver in use: HDA Intel
        Kernel modules: snd-hda-intel

cheers,
  Gerd

Re: [Qemu-devel] Wierd hack to sound/pci/intel8x0.c

[Konstantin Ozerkov]

On 07.11.11 12:25, Gerd Hoffmann wrote:
>    Hi,
>
>> Agreed.  If we can know the virtual device for KVM in a better way
>> (e.g. any specific PCI SSID or such), we can narrow the condition more
>> safely.
> PCI Subsystem ID 1af4:1100 is qemu/kvm (not only Intel HDA, most other
> emulated pci devices have it too). Complete entry:
>
> 00:04.0 0403: 8086:2668 (rev 01)
>          Subsystem: 1af4:1100
>          Physical Slot: 4
>          Flags: bus master, fast devsel, latency 0, IRQ 11
>          Memory at e2030000 (32-bit, non-prefetchable) [size=16K]
>          Kernel driver in use: HDA Intel
>          Kernel modules: snd-hda-intel
>
> cheers,
>    Gerd
We discuss Intel ICH/AC'97 (snd-intel8x0) here, but I hope that PCI SSID is same.

 From Parallels VM side we also have specific PCI SSID (1ab8:0400):
00:1f.4 Multimedia audio controller: Intel Corporation 82801BA/BAM AC'97 Audio Controller (rev 02)
     Subsystem: Device 1ab8:0400
     Flags: bus master, medium devsel, latency 0, IRQ 17
     I/O ports at ee00 [size=256]
     I/O ports at f000 [size=256]
     Kernel driver in use: Intel ICH
     Kernel modules: snd-intel8x0

Now we can make detection code more adequate. I will make patch today.

Re: [Qemu-devel] Wierd hack to sound/pci/intel8x0.c

[Gerd Hoffmann]

  Hi,

> We discuss Intel ICH/AC'97 (snd-intel8x0) here, but I hope that PCI SSID
> is same.

Hmm, it's not, the ac97 emulation doesn't use the default qemu subsystem
id for some reason.  Here is the entry:

[root@fedora ~]# lspci -vns6
00:06.0 0401: 8086:2415 (rev 01)
        Subsystem: 8086:0000
        Physical Slot: 6
        Flags: bus master, medium devsel, latency 0, IRQ 10
        I/O ports at c000 [size=1K]
        I/O ports at c400 [size=256]
        Kernel driver in use: Intel ICH
        Kernel modules: snd-intel8x0

I guess that should be fixed.  8086:0000 isn't valid anyway and I doubt
it serves any useful purpose other than avoiding a guest complaining
about the subsystem id being unset.

/me goes preparing a patch.

cheers,
  Gerd

Re: [Qemu-devel] Wierd hack to sound/pci/intel8x0.c

[Takashi Iwai]

At Mon, 07 Nov 2011 10:25:24 +0100,
Gerd Hoffmann wrote:
> 
>   Hi,
> 
> > Agreed.  If we can know the virtual device for KVM in a better way
> > (e.g. any specific PCI SSID or such), we can narrow the condition more
> > safely.
> 
> PCI Subsystem ID 1af4:1100 is qemu/kvm (not only Intel HDA, most other
> emulated pci devices have it too). Complete entry:
> 
> 00:04.0 0403: 8086:2668 (rev 01)
>         Subsystem: 1af4:1100
>         Physical Slot: 4
>         Flags: bus master, fast devsel, latency 0, IRQ 11
>         Memory at e2030000 (32-bit, non-prefetchable) [size=16K]
>         Kernel driver in use: HDA Intel
>         Kernel modules: snd-hda-intel

Thanks.

BTW, this reminds me of a possible improvement in the current HD-audio
driver.  It has some workaround for the delayed DMA-position update
(the DMA-position isn't completely updated when IRQ is issued) by
offsetting the buffer position.  This is controlled by bdl_pos_adj
option of snd-hda-intel driver, and this could be zero for VMs.


Takashi

Re: [Qemu-devel] Wierd hack to sound/pci/intel8x0.c

[Takashi Iwai]

At Mon, 07 Nov 2011 11:35:49 +0100,
Gerd Hoffmann wrote:
> 
>   Hi,
> 
> > We discuss Intel ICH/AC'97 (snd-intel8x0) here, but I hope that PCI SSID
> > is same.
> 
> Hmm, it's not, the ac97 emulation doesn't use the default qemu subsystem
> id for some reason.  Here is the entry:
> 
> [root@fedora ~]# lspci -vns6
> 00:06.0 0401: 8086:2415 (rev 01)
>         Subsystem: 8086:0000
>         Physical Slot: 6
>         Flags: bus master, medium devsel, latency 0, IRQ 10
>         I/O ports at c000 [size=1K]
>         I/O ports at c400 [size=256]
>         Kernel driver in use: Intel ICH
>         Kernel modules: snd-intel8x0
> 
> I guess that should be fixed.  8086:0000 isn't valid anyway and I doubt
> it serves any useful purpose other than avoiding a guest complaining
> about the subsystem id being unset.

Yeah, especially the value 0 looks pretty bad.

> /me goes preparing a patch.

Thanks!


Takashi