On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
From: Dawid Zamirski <dzamirski@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). 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. 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.
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