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

[John Ferlan]

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

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