[libvirt] [PATCH v2] qemu: sound: Support intel 'ich6' model
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@redhat.com> --- 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); + } + VIR_FREE(str); } } @@ -3762,13 +3770,19 @@ qemuBuildCommandLine(virConnectPtr conn, for (i = 0 ; i < def->nsounds && size > 0 ; i++) { virDomainSoundDefPtr sound = def->sounds[i]; - const char *model = virDomainSoundModelTypeToString(sound->model); - if (!model) { - VIR_FREE(modstr); + const char *model = "hda"; + + if (sound->model != VIR_DOMAIN_SOUND_MODEL_ICH6 && + !(model = virDomainSoundModelTypeToString(sound->model))) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("invalid sound model")); + } + + if (!model) { + VIR_FREE(modstr); goto error; } + strncat(modstr, model, size); size -= strlen(model); if (i < (def->nsounds - 1)) @@ -5530,6 +5544,8 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, type = VIR_DOMAIN_SOUND_MODEL_ES1370; } else if (STRPREFIX(start, "ac97")) { type = VIR_DOMAIN_SOUND_MODEL_AC97; + } else if (STRPREFIX(start, "hda")) { + type = VIR_DOMAIN_SOUND_MODEL_ICH6; } if (type != -1) { diff --git a/tests/qemuxml2argvdata/qemuxml2argv-sound-device.args b/tests/qemuxml2argvdata/qemuxml2argv-sound-device.args index 6b2e697..1b20dfc 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-sound-device.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-sound-device.args @@ -1 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -usb -soundhw pcspk -device ES1370,id=sound1,bus=pci.0,addr=0x2 -device sb16,id=sound2 -device AC97,id=sound3,bus=pci.0,addr=0x3 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -usb -soundhw pcspk -device ES1370,id=sound1,bus=pci.0,addr=0x2 -device sb16,id=sound2 -device AC97,id=sound3,bus=pci.0,addr=0x3 -device intel-hda,id=sound4,bus=pci.0,addr=0x4 -device hda-duplex -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-sound-device.xml b/tests/qemuxml2argvdata/qemuxml2argv-sound-device.xml index c725346..fbca4fe 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-sound-device.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-sound-device.xml @@ -22,6 +22,7 @@ <sound model='es1370'/> <sound model='sb16'/> <sound model='ac97'/> + <sound model='ich6'/> <memballoon model='virtio'/> </devices> </domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-sound.args b/tests/qemuxml2argvdata/qemuxml2argv-sound.args index 8cb2da2..0db62c3 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-sound.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-sound.args @@ -1 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb -soundhw pcspk,es1370,sb16,ac97 +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb -soundhw pcspk,es1370,sb16,ac97,hda diff --git a/tests/qemuxml2argvdata/qemuxml2argv-sound.xml b/tests/qemuxml2argvdata/qemuxml2argv-sound.xml index d34e0b3..78bfe3d 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-sound.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-sound.xml @@ -24,6 +24,7 @@ <sound model='es1370'/> <sound model='sb16'/> <sound model='ac97'/> + <sound model='ich6'/> <memballoon model='virtio'/> </devices> </domain> -- 1.7.3.3
On 01/13/2011 08:45 AM, 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.
Reasonable for a first round of patches.
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
Agree that those are good steps forward, and that they do not hold up this patch.
+++ 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 */
As long as you're touching the comment: s/wierdly/weirdly/
@@ -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); + } +
Should this come with a qemu_capabilities.[ch] check that device hda-duplex is available? Or are we relying on the fact that qemu will exit with a sane error message if an unsupported device is requested? The patch looks fine to me once you fix the spelling nit, but I'd rather get an answer to whether qemu_capabilities should be changed before giving ack. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 01/13/2011 03:21 PM, Eric Blake wrote:
On 01/13/2011 08:45 AM, 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.
Reasonable for a first round of patches.
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
Agree that those are good steps forward, and that they do not hold up this patch.
+++ 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 */
As long as you're touching the comment: s/wierdly/weirdly/
Will fix.
@@ -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); + } +
Should this come with a qemu_capabilities.[ch] check that device hda-duplex is available? Or are we relying on the fact that qemu will exit with a sane error message if an unsupported device is requested?
The patch looks fine to me once you fix the spelling nit, but I'd rather get an answer to whether qemu_capabilities should be changed before giving ack.
Ideally we would just rely on qemu to report a useful error in this case, but instead it gives us: $ x86_64-softmmu/qemu-system-x86_64 -device foobar qemu-system-x86_64: -device foobar: Parameter 'driver' expects a driver name Try with argument '?' for a list. I consider that a qemu bug though. I've filed a report against RHEL6: https://bugzilla.redhat.com/show_bug.cgi?id=669524 I'd rather error out than just ignore the unsupported device, so I don't think a capabilities check buys us much besides working around a suboptimal error message (which will hopefully be fixed soon anyways) Thanks, Cole
On 01/13/2011 02:45 PM, Cole Robinson wrote:
@@ -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); + } +
Should this come with a qemu_capabilities.[ch] check that device hda-duplex is available? Or are we relying on the fact that qemu will exit with a sane error message if an unsupported device is requested?
Ideally we would just rely on qemu to report a useful error in this case, but instead it gives us:
$ x86_64-softmmu/qemu-system-x86_64 -device foobar qemu-system-x86_64: -device foobar: Parameter 'driver' expects a driver name Try with argument '?' for a list.
I consider that a qemu bug though. I've filed a report against RHEL6:
But even if that gets patched, your proposed error message is still rather weak: qemu-system-x86_64: -device foobar: unknown device 'foobar' Whereas my recent patch to make qemu device parsing much easier could let us do: diff --git i/src/qemu/qemu_capabilities.c w/src/qemu/qemu_capabilities.c index f967255..b70c583 100644 --- i/src/qemu/qemu_capabilities.c +++ w/src/qemu/qemu_capabilities.c @@ -1046,6 +1046,7 @@ qemuCapsExtractDeviceStr(const char *qemu, * isolation, but are silently ignored in combination with * '-device ?'. */ cmd = virCommandNewArgList(qemu, + "-device", "?", "-device", "pci-assign,?", NULL); virCommandAddEnvPassCommon(cmd); @@ -1068,6 +1069,11 @@ cleanup: int qemuCapsParseDeviceStr(const char *str, unsigned long long *flags) { + /* Which devices exist. */ + if (strstr(str, "name \"hda-duplex\"")) + *flags |= QEMUD_CMD_FLAG_HDA_DUPLEX; + + /* Features of given devices. */ if (strstr(str, "pci-assign.configfd")) *flags |= QEMUD_CMD_FLAG_PCI_CONFIGFD; diff --git i/src/qemu/qemu_capabilities.h w/src/qemu/qemu_capabilities.h index 8057479..a4c9cfd 100644 --- i/src/qemu/qemu_capabilities.h +++ w/src/qemu/qemu_capabilities.h @@ -83,6 +83,7 @@ enum qemuCapsFlags { QEMUD_CMD_FLAG_SPICE = (1LL << 46), /* Is -spice avail */ QEMUD_CMD_FLAG_VGA_NONE = (1LL << 47), /* The 'none' arg for '-vga' */ QEMUD_CMD_FLAG_MIGRATE_QEMU_FD = (1LL << 48), /* -incoming fd:n */ + QEMUD_CMD_FLAG_HDA_DUPLEX = (1LL << 49), /* -device hda-duplex */ }; virCapsPtr qemuCapsInit(virCapsPtr old_caps); and issue a much nicer + if (!(qemuCmdFlags & QEMUD_CMD_FLAG_HDA_DUPLEX)) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this QEMU binary lacks hda support")); + goto error;
I'd rather error out than just ignore the unsupported device, so I don't think a capabilities check buys us much besides working around a suboptimal error message (which will hopefully be fixed soon anyways)
I agree with erroring out, but the question is whether it is nicer to rely on qemu erroring out or doing it ourselves, when it is not that much more work to do it ourselves. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 01/14/2011 11:20 AM, Eric Blake wrote:
On 01/13/2011 02:45 PM, Cole Robinson wrote:
@@ -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); + } +
Should this come with a qemu_capabilities.[ch] check that device hda-duplex is available? Or are we relying on the fact that qemu will exit with a sane error message if an unsupported device is requested?
Ideally we would just rely on qemu to report a useful error in this case, but instead it gives us:
$ x86_64-softmmu/qemu-system-x86_64 -device foobar qemu-system-x86_64: -device foobar: Parameter 'driver' expects a driver name Try with argument '?' for a list.
I consider that a qemu bug though. I've filed a report against RHEL6:
But even if that gets patched, your proposed error message is still rather weak:
qemu-system-x86_64: -device foobar: unknown device 'foobar'
Whereas my recent patch to make qemu device parsing much easier could let us do:
diff --git i/src/qemu/qemu_capabilities.c w/src/qemu/qemu_capabilities.c index f967255..b70c583 100644 --- i/src/qemu/qemu_capabilities.c +++ w/src/qemu/qemu_capabilities.c @@ -1046,6 +1046,7 @@ qemuCapsExtractDeviceStr(const char *qemu, * isolation, but are silently ignored in combination with * '-device ?'. */ cmd = virCommandNewArgList(qemu, + "-device", "?", "-device", "pci-assign,?", NULL); virCommandAddEnvPassCommon(cmd); @@ -1068,6 +1069,11 @@ cleanup: int qemuCapsParseDeviceStr(const char *str, unsigned long long *flags) { + /* Which devices exist. */ + if (strstr(str, "name \"hda-duplex\"")) + *flags |= QEMUD_CMD_FLAG_HDA_DUPLEX; + + /* Features of given devices. */ if (strstr(str, "pci-assign.configfd")) *flags |= QEMUD_CMD_FLAG_PCI_CONFIGFD;
diff --git i/src/qemu/qemu_capabilities.h w/src/qemu/qemu_capabilities.h index 8057479..a4c9cfd 100644 --- i/src/qemu/qemu_capabilities.h +++ w/src/qemu/qemu_capabilities.h @@ -83,6 +83,7 @@ enum qemuCapsFlags { QEMUD_CMD_FLAG_SPICE = (1LL << 46), /* Is -spice avail */ QEMUD_CMD_FLAG_VGA_NONE = (1LL << 47), /* The 'none' arg for '-vga' */ QEMUD_CMD_FLAG_MIGRATE_QEMU_FD = (1LL << 48), /* -incoming fd:n */ + QEMUD_CMD_FLAG_HDA_DUPLEX = (1LL << 49), /* -device hda-duplex */ };
virCapsPtr qemuCapsInit(virCapsPtr old_caps);
and issue a much nicer
+ if (!(qemuCmdFlags & QEMUD_CMD_FLAG_HDA_DUPLEX)) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this QEMU binary lacks hda support")); + goto error;
I'd rather error out than just ignore the unsupported device, so I don't think a capabilities check buys us much besides working around a suboptimal error message (which will hopefully be fixed soon anyways)
I agree with erroring out, but the question is whether it is nicer to rely on qemu erroring out or doing it ourselves, when it is not that much more work to do it ourselves.
You just won't take 'lazy' for an answer will you? :) Thanks for the code, I'll fold this into the next posting. - Cole
On 01/13/2011 08:45 AM, Cole Robinson wrote:
@@ -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);
Suppose I want a guest with two sound cards, both using model='ich6'. Am I correct that the qemu command line needs exactly one "-device hda-duplex" to enable sound from either card to get back to the host, but two "-device intel-hda,..." to implement the two cards in the guest? If so, this doesn't quite do the right thing (you instantiate the hda-duplex device twice); you'd need a bool variable set after the first time you emit this extra device. I noticed this because it is similar to my smartcard patch, where -device usb-ccid is needed only once, but -device ccid-card-* is needed per <smartcard>. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Jan 14, 2011 at 11:13:06AM -0700, Eric Blake wrote:
On 01/13/2011 08:45 AM, Cole Robinson wrote:
@@ -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);
Suppose I want a guest with two sound cards, both using model='ich6'. Am I correct that the qemu command line needs exactly one "-device hda-duplex" to enable sound from either card to get back to the host, but two "-device intel-hda,..." to implement the two cards in the guest? If so, this doesn't quite do the right thing (you instantiate the hda-duplex device twice); you'd need a bool variable set after the first time you emit this extra device.
The 'intel-hda' device should have an 'id' set on the command line. The codec devices should then use that id value to associate themselves with the intel-hda device. QEMU generally allows you to omit all these ids and does some 'sensible' behaviour, but libvirt must never rely on that because we need to guarentee a stable guest ABI. Everything must thus be fully specified Regards, Daniel
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@redhat.com> --- 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. Regards, Daniel
On 01/14/2011 02:05 PM, Daniel P. Berrange wrote:
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@redhat.com> --- 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.
Thanks, I'll address this in the next posting. - Cole
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Eric Blake