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 address@hidden 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