| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-05-14 | |||
| 14:01:15 | kashyap | efried: When you're about, bike-shedding, for the file name in 'os-traits': how about the x86.py --> x86-common.py? (Or is it "implied"?) | |
| 14:03:26 | efried | kashyap: We can't change x86.py at this point. And yes, I think common is implied. I would just make an x86 subdirectory with intel.py and amd.py | |
| 14:03:39 | kashyap | (Oh, right.) | |
| 14:03:49 | kashyap | I keep forgetting that "can't ever change" aspect. | |
| 14:03:51 | kashyap | Thx. | |
| 14:04:29 | efried | kashyap: I'm actually not 100% sure of the mechanics. I think you may have to remove x86.py and move the contents to x86/__init__.py | |
| 14:04:47 | efried | It's the strings that get produced that aren't allowed to change. | |
| 14:05:05 | kashyap | Right, I'm not sure either. I'll duke around a bit. This is my first time tipping toes into this repo | |
| 14:05:32 | kashyap | (Trying not to boil any lakes as "first steps" :D) | |
| 14:07:13 | sean-k-mooney | why woudl you move them to __init__.py | |
| 14:07:31 | sean-k-mooney | just to not have to type x86 | |
| 14:08:13 | kashyap | sean-k-mooney: I don't know, having x86.py file and x86/ directory in the _same_ directory feels "clumsy". | |
| 14:08:29 | sean-k-mooney | from a general style point of view is strongly dislike haveing __init__.py contain any content ever | |
| 14:08:37 | kashyap | sean-k-mooney: I can live with it; but open to any approach that lets us avoid it | |
| 14:09:11 | sean-k-mooney | we dont currently have an x86 direcotry correct | |
| 14:09:31 | sean-k-mooney | https://github.com/openstack/os-traits/tree/master/os_traits/hw/cpu | |
| 14:09:42 | sean-k-mooney | we just have the x86.py | |
| 14:09:51 | sean-k-mooney | why are you creating a directory | |
| 14:10:09 | kashyap | sean-k-mooney: See the discussion on the change here: https://review.opendev.org/#/c/655193/ | |
| 14:11:14 | kashyap | sean-k-mooney: Specifically this remark from Eric on PS2: | |
| 14:11:15 | kashyap | hw/cpu/x86.py (common to both Intel and AMD) | |
| 14:11:15 | kashyap | hw/cpu/x86/amd.py (AMD-specific) <== new (but see below) | |
| 14:11:15 | kashyap | hw/cpu/x86/intel.py (Intel-specific) <== new | |
| 14:11:34 | efried | sean-k-mooney: os-traits has a precedent for having attributes in __init__.py | |
| 14:12:12 | sean-k-mooney | im looking but or you do hw/cpu/intel.py hw/cpu/amd.py and hw/cpu/x86.py | |
| 14:12:32 | sean-k-mooney | * or you could do hw/cpu/intel.py hw/cpu/amd.py and hw/cpu/x86.py | |
| 14:12:49 | efried | sean-k-mooney: Point is, both intel and amd are x86 | |
| 14:12:53 | sean-k-mooney | yes | |
| 14:12:58 | efried | but have divergent attributes | |
| 14:13:04 | efried | like for this spectre/meltdown stuff | |
| 14:13:13 | sean-k-mooney | well they really should not | |
| 14:13:17 | efried | heh | |
| 14:13:18 | sean-k-mooney | at the traits point of view | |
| 14:13:26 | efried | sean-k-mooney: I mentioned that | |
| 14:13:30 | efried | but kashyap shot me down | |
| 14:13:31 | sean-k-mooney | we shoudl have a common trait where posible | |
| 14:13:42 | efried | yup, read discussion in that patch. | |
| 14:14:14 | sean-k-mooney | kashyap: what was the outcome fo the discussion with openstack security on exposeing thes as traits | |
| 14:14:25 | kashyap | sean-k-mooney: I didn't write to the 'openstack-security' folks. | |
| 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) | |