[libvirt] [PATCHv3 00/13] Add new PCIe controllers

[John Ferlan]

On 08/09/2015 01:39 PM, Laine Stump wrote:
> On 08/03/2015 08:33 AM, John Ferlan wrote:
>> Like Martin noted, I've been looking through this series as well. I
>> too haven't had a ton of time to devote to it over the last week or so
>> since I'm in the middle of a move. Also being so close to freeze - it
>> just didn't feel right to shimmy it in there late. This should go in
>> as soon as possible for 1.2.19 to hopefully weed out any weird issues!
>> Some more specific/general comments... The 1.2.18 -> 1.2.19 everywhere
>> will need to be done. The commit message in 2/13 seems to have
>> incurred a disjoint thought or perhaps a forgotten edit in the
>> paragraph that starts "The defaults are set..." - the last sentance
>> just doesn't read well. I had some concern over confusion with
>> 'chassis' and 'chassisNr'...
> 
> I've had problems in the past when I made the assumption that two
> apparently mutually exclusive things could share the same space. One day
> you get to a place where that isn't true, then you're screwed. Kind of
> like when you're time traveling and see a former/future version of
> yourself, and then time-space asplodes and there's no big sloppy mess to
> clean up because the universe has collapsed into a black hole or
> something... Kind of.
> 
>>  I
>> also had concern over whether the parser could confuse chassis values
>> depending on order seen - as in was a parse of 'chassis' an exact match
>> or a prefix match...
> 
> 
> I don't get this one - the comparison is on the entire string, not on a
> prefix of the string, so there can be no confusion.
> 

It was my own personal oh sh*! moment. Since chassisNr was first and
chassis second, I had some time/space memory thing that popped up and
caused me to go look and make sure the comparison algorithm was right.
Something from a former job where we had one particularly odd bug where
the comparison was not on the length of the constant string we were
comparing rather it was on the one read...

> 
>>  I think it's ok especially since one is used for
>> pci-bridge and the other for pcie-root-port...
>>
>> I'm still not sure if it's "best" to set chassisNr or make it some sort
>> of tristate where if we know it wasn't read in from the XML, then we
>> default or use the 'cont->idx' internally when formulating the qemu
>> command.
> 
> The whole point of auto-setting it in the xml is to assure that it will
> never change "by accident" in the future. If we imply some value when it
> isn't explicitly set in the xml, then at some later date decide to
> change how we derive the implicit value, then we will have broken ABI of
> the guest (albeit in a probably inconsequential way, since my
> understanding is that essentially nobody uses these values :-/), and we
> don't want to do that.
> 

I'm OK with the current implementation - just waiting for QA to claim
they can cause some sort of failure though ;-)

>>   That way at least we don't run into the situation where if
>> "index" changes, then we don't run into some issue since we don't follow
>> on with the chassisNr change (because we cannot be sure who set it).
> 
> Well, there are two conflicting situations we want to avoid: 1) having
> the chassisNr changed in the guest because libvirt internally changed
> the way it implicitly arrived at the value to use for chassisNr, and 2)
> having the chassisNr be in conflict with the chassisNr of some other pci
> controller because the user modified their config by both changing the
> index of this controller *and* adding a new controller of the same
> type/model in the old index.
> 
> The possibility of either is, quite frankly, rather remote. But in case
> (1) this "something bad" would happen with no conscious action on the
> part of the user (other than upgrading libvirt), so if it caused
> problems it would confuse/confound them. In case (2), at least it would
> be happening after they messed with the config (in a manner that almost
> nobody will ever do), so they might at least have some kind of clue what
> caused the problem.
> 
> Note that what we're doing here follows exactly the same behavior that
> we follow with network device model types - if no model type is given
> when the interface is added to the domain, rlt8139 is automatically
> added. We used to just leave it empty and set it to "whatever was qemu's
> default", but decided this was too dangerous - we really need to spell
> out every last detail in the XML and not rely on defaults, otherwise we
> run into problems when we want to change the defaults.
> 
>> This is patch 4/13 stuff.  I think it's fine as a followup to adjust
>> that logic if necessary - hopefully something that can be done in the
>> 1.2.19 timeframe though.  This would change the logic in
>> qemuBuildControllerDevStr in the def->model switch for
>> VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE to perhaps set a local chassisNr
>> value based on opts.pciopts.chassisNr == -1, then use index, else use
>> what was set.
> 
> That would be nullifying the entire purpose of storing chassisNr.
> 

OK - I see... That's just me following through on a thought - current
impl is fine.  I agree people should be messing with these values and
IIRC you've documented that (more or less).

>>
>> Beyond that and the duplicate 'chassis' parse in patch 6, things were
>> find and ready to push with a few adjustments.
> 
> Derp. Yeah, that must have snuck in during one of the dozens of rebases.
> I removed the one that incorrectly checked chassisNr, of course.
> 
> 

I know you've pushed already, but just wanted to be sure to follow-up on
my thoughts.

John