[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 03/18] qemu: Support disk model=virtio-{non-}transitional



On 01/22/2019 09:00 AM, Daniel P. Berrangé wrote:
> On Tue, Jan 22, 2019 at 10:44:23AM -0200, Eduardo Habkost wrote:
>> On Tue, Jan 22, 2019 at 12:45:59PM +0100, Andrea Bolognani wrote:
>>> On Tue, 2019-01-22 at 09:31 +0000, Daniel P. Berrangé wrote:
>>>> On Mon, Jan 21, 2019 at 05:41:47PM -0500, Cole Robinson wrote:
>>>>> On 01/18/2019 07:33 AM, Andrea Bolognani wrote:
>>>>>> On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:
>>>>>>> +    if (has_tmodel) {
>>>>>>> +        if (virQEMUCapsGet(qemuCaps, tmodel_cap))
>>>>>>> +            virBufferAddLit(buf, "-transitional");
>>>>>>> +
>>>>>>> +        /* No error for if -transitional is not supported: our address
>>>>>>> +         * allocation will force the device into plain PCI bus, which
>>>>>>> +         * is functionally identical to standard 'virtio-XXX' behavior
>>>>>>> +         */
>>>>>>> +    } else if (has_ntmodel) {
>>>>>>> +        if (virQEMUCapsGet(qemuCaps, ntmodel_cap)) {
>>>>>>> +            virBufferAddLit(buf, "-non-transitional");
>>>>>>> +        } else if (virQEMUCapsGet(qemuCaps,
>>>>>>> +                                  QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) {
>>>>>>> +            virBufferAddLit(buf, ",disable-legacy=on");
>>>>>>> +        } else {
>>>>>>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>>>>>> +                           _("virtio non-transitional model not supported "
>>>>>>> +                             "for this qemu"));
>>>>>>> +            return -1;
>>>>>>> +        }
>>>>>>> +    }
>>>>>>
>>>>>> Would it make sense to be more explicit here? Current versions of
>>>>>> QEMU default to disable-modern=off,disable-legacy=off for virtio-pci
>>>>>> devices plugged into conventional PCI slots, but unless I'm mistaken
>>>>>> that was not always the case, so it would perhaps be preferrable to
>>>>>> not rely on that behavior and always explicitly set both disable-*
>>>>>> options when the new devices are not available; if the options
>>>>>> themselves are not available, then we should error out.
>>>>>
>>>>> I don't know enough to say, CCing ehabkost and danpb for more eyes
>>>>
>>>> The QEMU code for the devices has
>>>>
>>>>  - Original devs:  disable-modern=off disable-legacy=auto
>>>>  - Transitional devs: disable-modern=off disable-legacy=off
>>>>  - Non-transitional devs: disable-modern=off disable-legacy=on
>>>>
>>>> IOW, in the case that -transitional is not available, we could
>>>> set disable-legacy=off. Provided that we always place -transitional
>>>> devices into PCI slots, never PCI-e slots, whether we set
>>>> disable-legacy=off or not doesn't have any effect. None the less it
>>>> would make sense to set it explicitly though as that would cause us
>>>> to catch bugs if we mistakenly had a -transitional dev in a PCI-e
>>>> slot.
>>>>
>>>> I don't see a need to set disable-modern at all, since it is
>>>> defaulting to "off" in all cases.
>>>
>>> Wasn't there some old QEMU release where disable-modern didn't
>>> default to off? I seem to remember that.
>>
>> Yes, disable-modern is "on" on 2.6 and older machine-types.
>>
>>>
>>> More generally, I don't see a reason *not* to specify disable-modern
>>> along with disable-legacy. Why be implicit when you can very easily
>>> be explicit instead?
>>
>> Agreed.
> 
> Ok, I've no objection to an explicit disable-modern=off based on this
> info.

Cool I'll make this change for the next round

Thanks,
Cole


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]