| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-05-14 | |||
| 14:14:42 | sean-k-mooney | so before we decided on this chagne we shoudl have that disucssion | |
| 14:15:14 | kashyap | sean-k-mooney: Note: at 'traits point of view", it is impossibel to have "divergent attributes". See my last comment | |
| 14:15:21 | kashyap | I carefully discussed it with DanPB yesterday, and posted my discussion | |
| 14:16:28 | sean-k-mooney | sure but just because danpb sates an opion does not mean we will alway follow it but im reading the comments now :) | |
| 14:17:30 | efried | point is, we need HW_CPU_X86_INTEL_FOO and HW_CPU_X86_AMD_BAR as well as HW_CPU_X86_BAZ. I'm just not sure whether that happens with | |
| 14:17:30 | efried | hw/cpu/x86.py: BAZ | |
| 14:17:30 | efried | or | |
| 14:17:30 | efried | hw/cpu/x86/intel.py: FOO | |
| 14:17:30 | efried | hw/cpu/x86/amd.py: BAR | |
| 14:17:31 | efried | hw/cpu/x86/__init__.py: BAZ | |
| 14:17:31 | efried | or we could even leave the existing structure and do | |
| 14:17:32 | efried | hw/cpu/x86.py: INTEL_FOO | |
| 14:17:32 | efried | hw/cpu/x86.py: AMD_BAR | |
| 14:17:33 | efried | hw/cpu/x86.py: BAZ | |
| 14:17:50 | efried | edleafe: Do you have an opinion on this? | |
| 14:18:18 | edleafe | They all come out to the same trait string, right? | |
| 14:18:21 | sean-k-mooney | you can do it with all of the above | |
| 14:18:38 | kashyap | sean-k-mooney: No, you misunderstood me. It's not "because" DanPB said. | |
| 14:18:41 | kashyap | sean-k-mooney: See these two points: | |
| 14:18:48 | aspiers | I'm with sean-k-mooney in that I don't like putting stuff in __init__.py | |
| 14:18:48 | kashyap | * The benefit of HW_CPU_HAS_SPECTRE_CURE is that it glosses over the | |
| 14:18:48 | kashyap | difference between the AMD and Intel flag names for the fix ('IBRS' vs | |
| 14:18:51 | kashyap | 'IBPB'). However, we likely going to want feature- based traits for | |
| 14:18:54 | kashyap | other things: AES, PCID, etc. | |
| 14:18:57 | kashyap | * The HW_CPU_HAS_SPECTRE_CURE is ill-defined :-( since there are many | |
| 14:18:59 | kashyap | Spectre-related bugs. | |
| 14:19:06 | kashyap | sean-k-mooney: Frankly, we're over-thinking on the generic "traits" thing. | |
| 14:19:21 | kashyap | I want to tackle that separately. And first get the granular traits in, without discussing this to the ends of the worlds. | |
| 14:19:25 | sean-k-mooney | kashyap: so traits shoudl not may to qemu feature flags directly | |
| 14:19:44 | mdbooth | stephenfin: I know you called it a nit, but given that I suspect it's a bug it wasn't caught by flake8 I'm going to respin anyway. Thanks for looking! | |
| 14:20:08 | sean-k-mooney | there is no reas an IBRS trait cant map to the IBPB feature on amd and IBRS on intel | |
| 14:20:14 | kashyap | sean-k-mooney: I disagree. | |
| 14:20:29 | stephenfin | mdbooth: Go for it. fwiw, I do have a series to update the hacking version we use but I've WIP'd that til the removal of cells v1 is complete | |
| 14:20:31 | sean-k-mooney | part of os-tratis is to provide a vendor independ set of common traits that can be normalised | |
| 14:20:37 | edleafe | efried: if we're going to be having _INTEL_* and _AMD_* traits, it would make more sense to have them in their own modules | |
| 14:20:38 | stephenfin | There's enough merge conflicts as it is | |
| 14:20:49 | sean-k-mooney | and them mapped to plathform specific thing in the virt dirvers | |
| 14:21:02 | kashyap | efried: Yeah, I'd like to know the answer to your earlier question | |
| 14:21:07 | kashyap | (On the structure) | |
| 14:21:56 | sean-k-mooney | edleafe: we dont really want to add tratis to the __init__.py file in general but we do allow it | |
| 14:22:11 | kashyap | Why we "don't really want"? | |
| 14:22:17 | kashyap | If efried says there's a precedence | |
| 14:22:22 | edleafe | sean-k-mooney: yeah, that's messier, but it does work | |
| 14:22:29 | edleafe | Separate modules is cleaner | |
| 14:22:41 | efried | sean-k-mooney: That's happening all over os-traits. In typical projects I agree it may be best avoided, but when in rome... | |
| 14:22:43 | sean-k-mooney | kashyap: because its condiered an anti pattern in python | |
| 14:22:58 | kashyap | Hm | |
| 14:23:27 | efried | of nine __init__.pyZ in os-traits, five have content. | |
| 14:23:38 | sean-k-mooney | efried: yes i know | |
| 14:23:49 | kashyap | sean-k-mooney: Also, look at: compute/__init__.py | |
| 14:24:14 | sean-k-mooney | although when i suggested adding namespaces in os-traits intially i was hoping to not use __init__.py | |
| 14:24:19 | edleafe | If you're creating a directory hierarchy, and need directory-level traits, the only way to do that is in __init__.py | |
| 14:24:46 | sean-k-mooney | edleafe: yes which is why we use it | |
| 14:25:12 | openstackgerrit | Matthew Booth proposed openstack/nova master: Fix retry of instance_update_and_get_original https://review.opendev.org/658845 | |
| 14:25:26 | efried | edleafe: you can't have x86.py and x86/ ? | |
| 14:25:28 | sean-k-mooney | but do we need hw/cpu/x86/intel.py or just hw/cpu/intel.py | |
| 14:25:40 | sean-k-mooney | if its an intel specific trait why add x86 | |
| 14:25:43 | efried | edleafe: that seems horrible to me because you wouldn't know where to look for your stuff, but is it even legal python? | |
| 14:25:45 | mdbooth | bauzas stephenfin: ^^^ Only change is whitespace for stephenfin's nit. | |
| 14:25:47 | kashyap | efried: edleafe: So, does this structure make sense: | |
| 14:25:49 | kashyap | hw/cpu/x86/amd.py (AMD-specific) | |
| 14:25:49 | kashyap | hw/cpu/x86/intel.py (Intel-specific) | |
| 14:25:49 | kashyap | hw/cpu/x86/__init__.py (Common for both AMD and Intel) | |
| 14:25:49 | kashyap | --- | |
| 14:25:51 | kashyap | hw/cpu/amd.py (Deprecate it with a comment) | |
| 14:26:04 | efried | kashyap: Yes, I think that's the right way to go. | |
| 14:26:14 | sean-k-mooney | i think the opisite | |
| 14:26:14 | edleafe | efried: that would create HW_CPU_X86_X86_FOO, no? | |
| 14:26:26 | kashyap | sean-k-mooney: Also note that there are other vulns. _besides_ Spectre/Meltdown: E.g. "L1TF". | |
| 14:26:26 | efried | kashyap: And remove x86.py. Current contents go to x86/__init__.py | |
| 14:26:27 | sean-k-mooney | i would prefer to add an intel.py beside the amd.py | |
| 14:26:37 | edleafe | efried: oh, you mean at the level above? | |
| 14:26:42 | stephenfin | This feels like a lot of ado about nothing. What's wrong with keeping everything in x86.py again? | |
| 14:26:46 | kashyap | efried: Right (on removing x86.py) | |
| 14:26:49 | edleafe | efried: yeah, that could work | |
| 14:26:54 | sean-k-mooney | i mean just add an intel.py here https://github.com/openstack/os-traits/tree/master/os_traits/hw/cpu | |
| 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 | 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:29 | aspiers | efried: I guess stephenfin's point was about which file they go in, not so much how they're namespaced | |
| 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 | |