Re: [libvirt] [PATCH v2] qemu: sound: Support intel 'ich6' model

On Thu, Jan 13, 2011 at 10:45:41AM -0500, Cole Robinson wrote: > In QEMU, the card itself is a PCI device, but it requires a codec > (either -device hda-output or -device hda-duplex) to actually output > sound. We set up an hda-duplex codec by default: I think it's important > that a simple <sound model='ich6'/> sets up a useful codec, to have > consistent behavior with all other sound cards. > > This is basically Dan's proposal of > > <sound model='ich6'> > <codec type='output' slot='0'/> > <codec type='duplex' slot='3'/> > </sound> > > without the codec bits implemented. > > The important thing is to keep a consistent API here, we don't want some > <sound> models to require tweaking codecs but not others. Steps I see to > accomplishing this when someone gets around to it: > > - every <sound> device has a <codec type='default'/> (unless codecs are > manually specified) > - <codec type='none'/> is required to specify 'no codecs' > - new audio settings like mic=on|off could then be exposed in > <sound> or <codec> in a consistent manner for all sound models > > Signed-off-by: Cole Robinson <crobinso(a)redhat.com&gt; > --- > docs/formatdomain.html.in | 5 ++- > docs/schemas/domain.rng | 1 + > src/conf/domain_conf.c | 3 +- > src/conf/domain_conf.h | 1 + > src/qemu/qemu_command.c | 24 ++++++++++++++++--- > .../qemuxml2argv-sound-device.args | 2 +- > .../qemuxml2argvdata/qemuxml2argv-sound-device.xml | 1 + > tests/qemuxml2argvdata/qemuxml2argv-sound.args | 2 +- > tests/qemuxml2argvdata/qemuxml2argv-sound.xml | 1 + > 9 files changed, 31 insertions(+), 9 deletions(-) > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index e9fcea1..b9b83c2 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -1663,8 +1663,9 @@ qemu-kvm -net nic,model=? /dev/null > The <code>sound</code> element has one mandatory attribute, > <code>model</code>, which specifies what real sound device is emulated. > Valid values are specific to the underlying hypervisor, though typical > - choices are 'es1370', 'sb16', and 'ac97' > - (<span class="since">'ac97' only since 0.6.0</span>) > + choices are 'es1370', 'sb16', 'ac97', and 'ich6' > + (<span class="since"> > + 'ac97' only since 0.6.0, 'ich6' only since 0.8.8</span>) > </dd> > </dl> > > diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng > index 9c1e9bb..ae0defe 100644 > --- a/docs/schemas/domain.rng > +++ b/docs/schemas/domain.rng > @@ -1496,6 +1496,7 @@ > <value>es1370</value> > <value>pcspk</value> > <value>ac97</value> > + <value>ich6</value> > </choice> > </attribute> > <optional> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index e5b89a2..3aabdf9 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -225,7 +225,8 @@ VIR_ENUM_IMPL(virDomainSoundModel, VIR_DOMAIN_SOUND_MODEL_LAST, > "sb16", > "es1370", > "pcspk", > - "ac97") > + "ac97", > + "ich6") > > VIR_ENUM_IMPL(virDomainMemballoonModel, VIR_DOMAIN_MEMBALLOON_MODEL_LAST, > "virtio", > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 81409f8..924ce89 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -458,6 +458,7 @@ enum virDomainSoundModel { > VIR_DOMAIN_SOUND_MODEL_ES1370, > VIR_DOMAIN_SOUND_MODEL_PCSPK, > VIR_DOMAIN_SOUND_MODEL_AC97, > + VIR_DOMAIN_SOUND_MODEL_ICH6, > > VIR_DOMAIN_SOUND_MODEL_LAST > }; > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index a0075a4..2024221 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -1727,11 +1727,13 @@ qemuBuildSoundDevStr(virDomainSoundDefPtr sound) > goto error; > } > > - /* Hack for 2 wierdly unusal devices name in QEMU */ > + /* Hack for wierdly unusal devices name in QEMU */ > if (STREQ(model, "es1370")) > model = "ES1370"; > else if (STREQ(model, "ac97")) > model = "AC97"; > + else if (STREQ(model, "ich6")) > + model = "intel-hda"; > > virBufferVSprintf(&buf, "%s", model); > virBufferVSprintf(&buf, ",id=%s", sound->info.alias); > @@ -3751,6 +3753,12 @@ qemuBuildCommandLine(virConnectPtr conn, > goto error; > > virCommandAddArg(cmd, str); > + > + if (sound->model == VIR_DOMAIN_SOUND_MODEL_ICH6) { > + virCommandAddArgList(cmd, > + "-device", "hda-duplex", NULL); > + } To allow us to later add hotplug, we need to include an 'id' on this device. We should also set the codec slot number and explicitly reference the intel-hda sound card so that you can give multiple ich6 cards in one guest. Something like. eg for a device with a codec in slot 3, we'd do -device intel-hda,id=foo -device hda-duplex,id=codecfoo3,bus=foo,cad=3 Obviously hardcode slot 0 for now. Even though we don't have hotplug now, we need all the IDs and cad properties wired up, so that if someone upgrades they can use hotplug without having to reboot all their guests.