On 10/18/2017 12:23 PM, 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@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?
>
Hmmm.... Typically when one makes .rng and domain_conf changes, then I
expect to also see *.html.in and tests/qemuxml2xmltest.c changes as well
as including the new .xml files in the input/output directories
(tests/qemuxml2argvdata and tests/qemuxml2xmloutdata). But this isn't
qemu.... So I guess I'm too used to reviewing qemu changes!
Since you're adding this for vbox and there isn't the same type xml
validation test for that (I see only tests/vboxsnapshotxmltest.c) that
just means you have less to do now. BTW: You will note there are
bhyvexml2xmltest and lxcxml2xmltests, but nothing for vbox, so I guess
it's a moot point since there's no way to test. Hey it is something to
add some day in the future if you or someone else is so inclined.
FWIW: If you made qemu driver changes, such as qemu_command.c - then one
would change the qemuxml2argvtest.c and create a .args output file.
>> 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...
>
Of course perhaps a moot point too ...
>> 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
>
Well, again, it's my qemu review bias clearly showing through. The qemu
code will know nothing about models so it'll just happily ignore the
model type (at least for now until someday someone decides differently).
So, how to handle this? Hmmm... Well it seems there's perhaps nothing
to do unless there is some need/desire to ensure the <address> has
specific values in which case you'd probably need that device def
callback specific to vbox similarly to the qemu device def callback.
One other thought - thinking about patch3 and formatdomain.html.in - you
should indicate there that model types are only supported and handled
for the vbox hypervisor only - there are other examples of that already.
For example,
<dd><span class="since">Since 3.9.0</span> for the vbox driver, the
<code>ide</code> controller has an optional attribute
<code>model</code>, which is one of "piix3", "piix4", or "ich6".</dd>
I assume if the model='xxx' isn't provided, then some default is chosen.
Sorry for the confusion -
John
>>> 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
