[libvirt] [PATCH 2/6] domain: Allow 'model' attribute for ide controller.

[Daniel P. Berrange]

On Wed, Oct 18, 2017 at 12:23:24PM -0400, Dawid Zamirski wrote:
> On Tue, 2017-10-17 at 15:46 -0400, John Ferlan wrote:
> > 
> > On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
> > > From: Dawid Zamirski <dzamirski at datto.com>
> > > 
> > > The optional values are 'piix3', 'piix4' or 'ich6'. Those will be
> > > needed to allow setting IDE controller model in VirtualBox driver.
> > > ---
> > >  docs/schemas/domaincommon.rng | 18 ++++++++++++++++--
> > >  src/conf/domain_conf.c        |  9 +++++++++
> > >  src/conf/domain_conf.h        |  9 +++++++++
> > >  src/libvirt_private.syms      |  2 ++
> > >  4 files changed, 36 insertions(+), 2 deletions(-)
> > > 
> > 
> > Patches 2 & 3 should be combined and you'll need to provide an
> > xml2xml
> > example here, modifying tests/qemuxml2xmltest.c in order to add your
> > test. Search for controller type='scsi' and model= being set in any
> > of
> > the tests/*/*.xml files.  Then generate your test that would have
> > controller type='ide' ... model='xxx' (just one example is fine).
> > 
> 
> I understand that the patch with test cases should be separated,
> corrent?
> 
> > You may also need to alter virDomainControllerDefValidate to ensure
> > perhaps some existing assumptions on VIR_DOMAIN_CONTROLLER_TYPE_IDE
> > aren't lost if ->model is filled in.
> > 
> 
> That's clear, no problem...
> 
> > I am far from being an expert on IDE controllers and their existing
> > assumptions, but if you search on VIR_DOMAIN_CONTROLLER_TYPE_IDE
> > references you will find the existing ones.  My assumption has been
> > that
> > the current model assumption is piix3 - so by adding piix4 and ich6
> > you'll need to alter code in order to handle any assumptions those
> > models have; otherwise, once code finds IDE it assumes piix3 and
> > those
> > assumptions may not work well for piix4 and ich6.
> > 
> 
> ...though, I'm a little concerned with this part. While I'm somewhat
> familiar with libvirt qemu driver and can confirm that it implies PIIX3
> for IDE (at least libvirt does by checking if emulated machine is 440FX
> although qemu itself seems to support PIIX4 as well [1]), I'm not so
> sure I could easily determine "defaults" for other drivers like ESX
> (especially this one being closed-source) - AFAIK those drivers
> basically allow to attach "an IDE controller" without any specifics on
> what particular model of the southbridge the hypervisor should emulate

QEMU 'pc' machine type provides a piix3  IDE controller built-in,
and we can't (at time this) override this default.

You can then add extra IDE controllers which can be piix3 or piix4,
but we don't have that wired up for QEMU driver yet. It looks like
your proposed XML changes would let us do that though, which is nice.

> - I guess it's likely determined by the "machine" type or hwVersion
> (ESX)  and I suppose vbox is an "oddball" here allowing to set IDE
> controller model independently. Would it be acceptable to update each 
> driver to check that ->model == -1 and error out if it's not? In other
> words, qemu would allow model {-1,piix3}, vbox {-1,piix3,piix4,ich6}
> everything else would accept just -1 - guess those could be enforced
> by implementing virDomainDeviceDefValidateCallback in each driver.

vbox isn't really an odd-ball actually. vbox has as machine type of
piix3 by default if you look at the 'chipset' docs here:

  https://www.virtualbox.org/manual/ch03.html#settings-motherboard

the IDE controller model setting looks like it overrides the model
of this built-in IDE controller. This is slightly more flexible
than what QEMU does, in that you can overrde the default. Both
QEMU and VBox ways work.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|