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@xxxxxxxxx>
> >
> > 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.
[1] https://github.com/qemu/qemu/blob/master/hw/ide/piix.c#L285
> > diff --git a/docs/schemas/domaincommon.rng
> > b/docs/schemas/domaincommon.rng
> > index 4dbda6932..c3f1557f0 100644
> > --- a/docs/schemas/domaincommon.rng
> > +++ b/docs/schemas/domaincommon.rng
> > @@ -1927,12 +1927,11 @@
> > <ref name="address"/>
> > </optional>
> > <choice>
> > - <!-- fdc/ide/sata/ccid have only the common attributes
> > -->
> > + <!-- fdc/sata/ccid have only the common attributes -->
> > <group>
> > <attribute name="type">
> > <choice>
> > <value>fdc</value>
> > - <value>ide</value>
> > <value>sata</value>
> > <value>ccid</value>
> > </choice>
> > @@ -1993,6 +1992,21 @@
> > </attribute>
> > </optional>
> > </group>
> > + <!-- ide has an optional attribute "model" -->
> > + <group>
> > + <attribute name="type">
> > + <value>ide</value>
> > + </attribute>
> > + <optional>
> > + <attribute name="model">
> > + <choice>
> > + <value>piix3</value>
> > + <value>piix4</value>
> > + <value>ich6</value>
> > + </choice>
> > + </attribute>
> > + </optional>
> > + </group>
> > <!-- pci has an optional attribute "model" -->
> > <group>
> > <attribute name="type">
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 54be9028d..493bf83ff 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -378,6 +378,11 @@ VIR_ENUM_IMPL(virDomainControllerModelUSB,
> > VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST,
> > "qemu-xhci",
> > "none")
> >
> > +VIR_ENUM_IMPL(virDomainControllerModelIDE,
> > VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST,
> > + "piix3",
> > + "piix4",
> > + "ich6")
> > +
> > VIR_ENUM_IMPL(virDomainFS, VIR_DOMAIN_FS_TYPE_LAST,
> > "mount",
> > "block",
> > @@ -9467,6 +9472,8 @@ virDomainControllerModelTypeFromString(const
> > virDomainControllerDef *def,
> > return virDomainControllerModelUSBTypeFromString(model);
> > else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
> > return virDomainControllerModelPCITypeFromString(model);
> > + else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
> > + return virDomainControllerModelIDETypeFromString(model);
> >
> > return -1;
> > }
> > @@ -9482,6 +9489,8 @@
> > virDomainControllerModelTypeToString(virDomainControllerDefPtr def,
> > return virDomainControllerModelUSBTypeToString(model);
> > else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
> > return virDomainControllerModelPCITypeToString(model);
> > + else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
> > + return virDomainControllerModelIDETypeToString(model);
> >
> > return NULL;
> > }
> > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> > index a42efcfa6..d7f4c3f1e 100644
> > --- a/src/conf/domain_conf.h
> > +++ b/src/conf/domain_conf.h
> > @@ -748,6 +748,14 @@ typedef enum {
> > VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST
> > } virDomainControllerModelUSB;
> >
> > +typedef enum {
> > + VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX3,
> > + VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX4,
> > + VIR_DOMAIN_CONTROLLER_MODEL_IDE_ICH6,
> > +
> > + VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST
> > +} virDomainControllerModelIDE;
> > +
> > # define IS_USB2_CONTROLLER(ctrl) \
> > (((ctrl)->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) && \
> > ((ctrl)->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1
> > || \
> > @@ -3231,6 +3239,7 @@ VIR_ENUM_DECL(virDomainControllerModelPCI)
> > VIR_ENUM_DECL(virDomainControllerPCIModelName)
> > VIR_ENUM_DECL(virDomainControllerModelSCSI)
> > VIR_ENUM_DECL(virDomainControllerModelUSB)
> > +VIR_ENUM_DECL(virDomainControllerModelIDE)
> > VIR_ENUM_DECL(virDomainFS)
> > VIR_ENUM_DECL(virDomainFSDriver)
> > VIR_ENUM_DECL(virDomainFSAccessMode)
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index 9243c5591..616b14f82 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -231,6 +231,8 @@ virDomainControllerFindUnusedIndex;
> > virDomainControllerInsert;
> > virDomainControllerInsertPreAlloced;
> > virDomainControllerIsPSeriesPHB;
> > +virDomainControllerModelIDETypeFromString;
> > +virDomainControllerModelIDETypeToString;
> > virDomainControllerModelPCITypeToString;
> > virDomainControllerModelSCSITypeFromString;
> > virDomainControllerModelSCSITypeToString;
> >
>
> "Currently" you probably don't need to export the *IDEType{To|From}*
> functions since domain_conf is the only consumer; however, seeing how
> the *SCSIType{To|From}* has multiple/external consumers makes me
> wonder
> if more consumers are needed for IDE.
>
> In particular, in qemuBuildControllerDevStr and some assumptions in
> src/qemu/qemu_domain_address.c related to where controllers live on
> the
> bus for piix3 (FWIW: This was written before the above last two
> paragraphs - it's what prompted me to consider the needs).
>
> John
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
