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

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(a)datto.com&gt; > > > > 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

- 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.