| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-05-14 | |||
| 14:27:00 | edleafe | But it's unnecesary | |
| 14:27:07 | efried | edleafe: I meant, is it legal to have: hw/cpu/x86.py and hw/cpu/x86/__init__.py | |
| 14:27:08 | aspiers | stephenfin: stop trying to spoil the bike-shedding fun ;-) | |
| 14:27:20 | stephenfin | :) | |
| 14:27:21 | kashyap | stephenfin: "Nothing"? It's because traits are "baked in forever", we're having this discussion. Otherwise, none would bother belaboring. | |
| 14:27:28 | edleafe | efried: yep | |
| 14:27:46 | kashyap | aspiers: Yeah, stephenfin is only aggravating :D | |
| 14:27:50 | aspiers | kashyap is right about that, traits much need more care than most stuff | |
| 14:27:53 | efried | stephenfin: because there are traits that exist only on amd and only on intel. | |
| 14:28:03 | sean-k-mooney | stephenfin: well until openstack security ways in on the security question my current stance is we should add none of these tratis | |
| 14:28:27 | efried | From now on we shall require everyone to read all comments on this patch and pass a quiz before being allowed to participate in the conversation. | |
| 14:28:29 | aspiers | efried: I guess stephenfin's point was about which file they go in, not so much how they're namespaced | |
| 14:28:29 | kashyap | stephenfin: I know you have more than 10 minutes of attention span: have fun reading: https://review.opendev.org/#/c/655193/4 | |
| 14:28:44 | aspiers | efried: haha good idea! | |
| 14:29:00 | stephenfin | kashyap: Not really. I mean, they're still for x86 architectures, only those from specific companies | |
| 14:29:22 | stephenfin | Throwing everything in x86.py sounds a lot easier than trying to categorize each flag, IMO :) | |
| 14:29:37 | sean-k-mooney | stephenfin: i agree | |
| 14:29:50 | kashyap | sean-k-mooney: I think your stance there is invalid, recall what we discussed before: | |
| 14:29:53 | kashyap | < efried> kashyap, sean-k-mooney: I'm going to say it's fine to add the traits to os-traits regardless; the vulnerability occurs when we expose them via compute | |
| 14:30:16 | kashyap | stephenfin: Did you read the discussion here: https://review.opendev.org/#/c/655193/ ? | |
| 14:30:18 | stephenfin | I mean, we're not going to have an amd_intel.py file if a flag is only supported by AMD and Intel and not whoever else has an x86 license | |
| 14:30:33 | kashyap | (If not, there's more nuance than just following the IRC discussion here) | |
| 14:30:34 | stephenfin | kashyap: I did and held my tongue because I wan't sure | |
| 14:30:41 | kashyap | Heh | |
| 14:30:54 | efried | we have to categorize the intel- and amd-specific ones anyway. The strings that come out in the end are already figured. It's a question of whether to have INTEL_FOO and AMD_BAR in x86.py or have separate namespace dirs. | |
| 14:30:59 | sean-k-mooney | kashyap: my full stance on this is we should not do it peiord but if we must add these i would prefer them to be generic not vender specific and only as a last resouce expose vendor specific tratits | |
| 14:31:04 | stephenfin | but now that I've seen this, I've decided my first impression made some sense at least | |
| 14:31:43 | stephenfin | sean-k-mooney: Aye, agreed on the latter part of that at least. Not sure about the basis for the former so can't comment | |
| 14:32:10 | stephenfin | efried: Do we? Why? | |
| 14:32:19 | stephenfin | (just for context) | |
| 14:32:21 | kashyap | sean-k-mooney: You seem to be too hung up on 'generic'. It _doesn't_ fully make sense in this case. | |
| 14:32:39 | efried | I'll go out on a limb and say that anything x86 that's supported by both AMD and Intel should go into the x86 namespace, and random manufacturer that owns 0.0000001% of the x86 market who doesn't support that feature can just not turn it on. | |
| 14:33:03 | sean-k-mooney | kashyap: i think this is a missue use of traits and im pushing back becasue i dont think you have mad the case of why we should be exposing this in a non generic way | |
| 14:33:18 | efried | stephenfin: Because some of them are mutually exclusive. | |
| 14:33:20 | kashyap | sean-k-mooney: The case is the vulnerabilities cannot be trivially mapped into 'generic' traits> | |
| 14:33:33 | sean-k-mooney | traits are ment to be an abstration over hardware and are not intened to enable specifric feature flags in the emulated cpu | |
| 14:33:52 | efried | well now I can get behind that ^ | |
| 14:33:54 | kashyap | sean-k-mooney: And some people may want to run _without_ "L1TF" (which is Intel-only) | |
| 14:34:12 | stephenfin | efried: OK. How would that manifest itself in a way we care about? | |
| 14:34:19 | sean-k-mooney | kashyap: sure but why do this need to be in placement | |
| 14:34:25 | sean-k-mooney | as a trait | |
| 14:34:26 | stephenfin | (again, just so I know. Genuine question) | |
| 14:35:25 | sean-k-mooney | kashyap: my concern is that you appear to be trying to use tratis to carry vm configuration in fomation by making the vendor specific when we should be able to provide an abstration here | |
| 14:35:59 | efried | stephenfin: Apparently there's "has a flag you can turn on to mitigate X vulnerability" with variants of "do it on the CPU" and "do it in virt" and then there's "is manufactured without the vulnerability in the first place so no flags needed". And different variants of those exist depending on amd vs intel. | |
| 14:36:26 | kashyap | sean-k-mooney: No, you're assuming far too much. This whole discussion "exploded" when I simply noticed a few weeks ago some missing CPU traits. | |
| 14:36:39 | efried | stephenfin: I had already questioned whether we really cared about those distinctions, as opposed to "vulnerable or not". | |
| 14:36:45 | efried | apparently we do. | |
| 14:36:54 | sean-k-mooney | efried: right but the request to turn on the flag shoudl either be in the nova.conf or as a flavor extra spec or as an image proerty | |
| 14:36:56 | kashyap | And digigng more, I then thought: "If you have generic CPU flags listed, what about those that provide mitigation for security flaws.) | |
| 14:37:17 | sean-k-mooney | that flag should not be enabled by addign a required tratit to the flavor or image | |
| 14:37:24 | efried | sean-k-mooney: Right, but how do you schedule (or avoid scheduling to) a system that is or is not able to enable those flags? | |
| 14:37:46 | stephenfin | efried: OK, I wasn't aware of that. Thanks | |
| 14:37:56 | sean-k-mooney | we normalise the vendor diference in the virt dirver to a standard trait that is vendor independed | |
| 14:38:29 | kashyap | stephenfin: Your earlier question is answered in the commit message: "Notes on the "SSB"/"SSBD" confusion" | |
| 14:38:43 | kashyap | (Here: https://review.opendev.org/#/c/655193/) | |
| 14:39:10 | kashyap | (It notes how Intel and AMD addressed the same issue differently.) | |
| 14:39:19 | dansmith | kashyap: are you suggesting traits for flaws, not just traits for cpu flags? | |
| 14:40:26 | kashyap | dansmith: On "traits for flaws", I'm not sure if it makes sense. For now, all I care about is: all the required CPU flags are captured as traits | |
| 14:40:48 | kashyap | Err, s/required/missing/ | |
| 14:41:26 | sean-k-mooney | kashyap: efried suggested generic HW_CPU_HAS_MELTDOWN_CURE, HW_CPU_HAS_SPECTRE_CURE style traits in teh review | |
| 14:41:46 | kashyap | sean-k-mooney: And did you see my response that I already pasted here? | |
| 14:41:48 | sean-k-mooney | personally i think that is a better solution then what you are suggesting | |
| 14:41:48 | dansmith | :( | |
| 14:41:51 | artom | MELTDOWN_CURE sounds like a thing all parents of toddlers need | |
| 14:42:31 | kashyap | sean-k-mooney: Huh, you haven't read the response, that I posted, have you: | |
| 14:42:34 | kashyap | * The benefit of HW_CPU_HAS_SPECTRE_CURE is that it glosses over the | |
| 14:42:37 | kashyap | difference between the AMD and Intel flag names for the fix ('IBRS' vs | |
| 14:42:40 | kashyap | 'IBPB'). However, we likely going to want feature-based traits for | |
| 14:42:40 | sean-k-mooney | althoght i woudl personally go with hw_CPU_VULNERABLITY_SPECTER | |
| 14:42:42 | kashyap | Given the above it doesn't make sense the "generic roll-up traits". | |
| 14:42:42 | kashyap | other things: AES, PCID, etc. | |
| 14:42:45 | kashyap | * The HW_CPU_HAS_SPECTRE_CURE is ill-defined :-( since there are many | |
| 14:42:48 | kashyap | Spectre-related bugs. | |
| 14:43:01 | kashyap | s/"sense the"/"sense to have"/ | |
| 14:43:24 | sean-k-mooney | yes i did see that comment | |
| 14:43:34 | kashyap | sean-k-mooney: If you have a _clear_ proposal, please write it as a (not too long) comment in the change, I'd appreciate it. | |
| 14:43:51 | kashyap | For now, I'll go with the structure I discussed with efried earlier. | |
| 14:44:57 | sean-k-mooney | ok i will. but to be clear adding a required trait should have 0 impact on the xml generation and should not enable a cpu feature flag in the libvirt xml for that cpu feature | |
| 14:45:53 | sean-k-mooney | if the host is configed in the nova.conf to enable that feature that is fine but reporting/requireing a trait should not change the behavior of the vm form another that was schduled to that host without requiring the traits | |
| 14:46:12 | dansmith | fwiw, I *hate* the "has the cure" trait | |
| 14:46:23 | kashyap | dansmith: Hehe, that's not going to happen :-) | |
| 14:46:57 | sean-k-mooney | dansmith: well what about the other fomulation of hw_cpu_vulnerablity_XYZ | |
| 14:47:01 | kashyap | dansmith: It was a thinking-out-loud point from Eric. | |
| 14:47:13 | edleafe | dansmith: it's as bad as "doesn't have the vulnerability" trait | |
| 14:47:38 | dansmith | sean-k-mooney: I don't like that either | |
| 14:47:42 | sean-k-mooney | which the cpu feature flags are in another vail | |
| 14:47:54 | dansmith | "is broken" or "was broken" or "has a software fix" are terrible traits to me | |
| 14:48:02 | sean-k-mooney | dansmith: ya i dont like tracking vulnerablity in placement period | |
| 14:48:07 | dansmith | sean-k-mooney: agreen | |
| 14:48:09 | dansmith | *agreed | |
| 14:48:30 | dansmith | I also don't think that tracking cpu flags in placement for non-feature flags makes sense | |
| 14:48:57 | dansmith | AVX2, yes... flags that just imply that a microcode fix has been applied, not so much | |
| 14:49:01 | sean-k-mooney | i stil am concerned that adding any of the security realted flag give me a vector to upload an image with a require/forbidn trait and target a vulnerable host | |
| 14:49:16 | dansmith | sean-k-mooney: for serious | |
| 14:49:17 | kashyap | dansmith: How about this idea (that I discussed here on PS:4 as "Another Idea" - https://review.opendev.org/#/c/655193/): | |
| 14:49:20 | kashyap | Make Nova check the 'sysfs' (/sys/devices/system/cpu/vulnerabilities) directory for vulnerabilities. And e.g. if it reports "Vulnerable" (instead of "Mitigation") for Meltdown (or other flaws), print a warning that the host is vulnerable, and on next release refuse to start the VMs | |
| 14:49:46 | sean-k-mooney | dansmith: :) | |
| 14:50:16 | dansmith | kashyap: I have a hard time understanding why this is a nova thing at all.. configs to turn on compatibility as needed makes sense, since we are controlling the definition of the VMs, | |
| 14:50:20 | sean-k-mooney | kashyap: i dont think its novas place to do that | |
| 14:50:28 | dansmith | but the management of the host-level patching is not our deal, IMHO | |