| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-06-20 | |||
| 14:03:39 | kashyap | On a long call; back in a few, folks | |
| 14:03:54 | aspiers | maybe better to keep each device's IOMMU decision local to the code which creates its config | |
| 14:04:11 | efried | We should never turn off requested encryption based on something discovered internally "later on". | |
| 14:04:18 | aspiers | efried: sure, that's not quite the scenario I mean though | |
| 14:04:22 | efried | okay | |
| 14:04:44 | aspiers | let's say some other extra spec requires a particular storage device to be virtio but have IOMMU disabled | |
| 14:04:54 | aspiers | and there's also an extra spec or image property requiring memory encryption | |
| 14:05:32 | aspiers | these are mutually incompatible since SEV requires IOMMU on all virtio devices | |
| 14:05:36 | aspiers | now ideally, this would be caught earlier on than at instance creation time | |
| 14:05:52 | aspiers | but as a last resort, the chunk of code building the XML for that device should spot the conflict | |
| 14:06:33 | aspiers | whereas if I followed my previous suggestion of moving the "turn IOMMU on for all virtio devices" code to a separate SEV method, it couldn't | |
| 14:07:25 | aspiers | so in summary, maybe the general approach in places like https://review.opendev.org/#/c/644565/9/nova/virt/libvirt/driver.py@1626 is fine | |
| 14:07:35 | aspiers | well | |
| 14:07:42 | aspiers | fine, except for your valid point about explicit turn-off | |
| 14:07:46 | aspiers | which I am still replying to | |
| 14:18:38 | openstackgerrit | Adam Spiers proposed openstack/nova master: Use |
|
| 14:18:38 | openstackgerrit | Adam Spiers proposed openstack/nova master: Detect that SEV is required and enable iommu for devices https://review.opendev.org/644565 | |
| 14:18:39 | openstackgerrit | Adam Spiers proposed openstack/nova master: Enable memory locking if SEV is requested https://review.opendev.org/662558 | |
| 14:22:56 | aspiers | efried: regarding the need for some kind of master switch: I don't think I can float the MEM_ENCRYPTION_CONTEXT patch to the top - at very least, that's the one which bumps os-resource-classes to 0.4.0 to provide the new class | |
| 14:23:43 | aspiers | efried: if I combined the top three patches, that would be the master switch really, since none of the others change the way guests are created | |
| 14:27:55 | openstackgerrit | Merged openstack/nova master: Log disk transfer stats in live migration monitor. https://review.opendev.org/619395 | |
| 14:33:04 | mriedem | melwitt: replied inline on https://review.opendev.org/#/c/633227/ | |
| 14:33:07 | mriedem | thanks for thinking through that | |
| 14:39:59 | kashyap | aspiers: Back here for 15 min. So on the approach to enable IOMMU for virtio devices 'if-SEV-is-requested': I need a fresher mind than the pear-shaped one I have now, afraid. Not to mention a chunky block of time. Let's work it out tomm? | |
| 14:40:18 | aspiers | kashyap: sure but it's simpler than you think | |
| 14:40:40 | aspiers | e.g. https://review.opendev.org/#/c/644565/11/nova/virt/libvirt/driver.py@1626 | |
| 14:40:56 | aspiers | the only question I'm asking is whether this is the right place to do it | |
| 14:40:57 | kashyap | aspiers: The "single method" thing does make sense to me | |
| 14:41:19 | aspiers | except the single method approach has the issue described above | |
| 14:41:31 | aspiers | (35 mins ago) | |
| 14:41:47 | kashyap | Hm, "maybe not, since other code might want to insist on (say) disabling IOMMU for a specific type of device" | |
| 14:42:43 | kashyap | aspiers: Did you audit what other code will insist disabling IOMMU? Let me quickly check with the RHT virt folks | |
| 14:43:02 | aspiers | kashyap: no, that was a potentially hypothetical future scenario | |
| 14:43:04 | kashyap | As in, for potential gotchas here | |
| 14:44:36 | kashyap | aspiers: So SEV requires IOMMU unconditionally on _all_ virtio devices? | |
| 14:46:12 | aspiers | kashyap: yes, http://specs.openstack.org/openstack/nova-specs/specs/train/approved/amd-sev-libvirt-support.html#proposed-change | |
| 14:46:43 | kashyap | aspiers: So, I looked at the "enable IOMMU at the time of each device configuration" code -- where the check is: *if* bus is 'virtio', then enable IOMMU... | |
| 14:53:13 | efried | aspiers: My problem with "none of the others change the way guests are created" is that as of patch #3: the inventory is being exposed, the flavor/image syntax is being accepted, and the allocation is being created. The only thing that's *not* happening is... the actual feature. That's bad. | |
| 14:53:42 | aspiers | efried: OK. Totally open to suggestions for how to fix that | |
| 14:53:44 | efried | aspiers: You can put the os-traits bump in whatever patch first needs it. That's fine. | |
| 14:53:58 | stephenfin | jangutter, sean-k-mooney, ralonsoh: I've royally confused myself again (every damn time) - either of you care to remind me of the difference between VIF_TYPE_foo and VNIC_TYPE_foo? | |
| 14:54:24 | efried | aspiers: I'm saying that exposing the inventory is a nice master switch, because without it, there's zero expectation of anything else working. | |
| 14:54:26 | aspiers | efried: o-r-c not o-t, but yeah sure, I can split the o-r-c patch in two | |
| 14:54:35 | stephenfin | I think one refers to the backend that handles the VIF, while the other determines how it's actually attached to the instance. Right? | |
| 14:54:44 | aspiers | efried: wfm | |
| 14:55:11 | efried | aspiers: As far as I'm concerned, we can *always* bump ot and orc to latest. (In other projects we've implemented canary tests that will fail as soon as a new release comes through upper-constraints.) | |
| 14:55:11 | jangutter | stephenfin: VNIC_TYPE can in theory be set by the user, where VIF type is behind the scenes. | |
| 14:55:25 | aspiers | efried: yup, makes sense | |
| 14:55:43 | efried | aspiers: So for my money, you could do that in its own patch on the bottom, and not even have to tag it against this blueprint. | |
| 14:55:53 | jangutter | stephenfin: VNIC is an _attribute_ of the VIF. | |
| 14:56:43 | Sundar | sean-k-mooney, mriedem, gibi, johnthetubaguy: There has been further discussion on Line 276 of https://review.opendev.org/#/c/603955/14. That seems to be the sole point to address before this spec is merged. Please take a look. Thanks. | |
| 14:57:09 | stephenfin | jangutter: So in theory any VIF type could use one of the...six VNIC types we provide? | |
| 14:57:45 | jangutter | stephenfin: there are two checks I think. Neutron has to allow it, and Nova has to translate it right from Neutron. | |
| 14:58:23 | stephenfin | Yeah, in theory but not in practice | |
| 14:58:35 | jangutter | stephenfin: there's no easy way to answer the question: "does this cloud support this VNIC type" without trying to spawn an instance. | |
| 14:58:56 | openstackgerrit | Matt Riedemann proposed openstack/os-traits master: Add COMPUTE_SAME_HOST_COLD_MIGRATE trait https://review.opendev.org/666604 | |
| 14:59:32 | jangutter | stephenfin: your first NACK might be neutron saying "this combination of driver + vnic type is not valid". But it could go right up to Nova failing to find a free PCI device conforming to the NUMA topology. | |
| 15:00:06 | stephenfin | How did I ever get dragged into this stuff? :D | |
| 15:00:09 | kashyap | aspiers: I need to get back to this, afraid. Saddled with another meeting | |
| 15:00:29 | aspiers | kashyap: np | |
| 15:00:43 | stephenfin | jangutter: That does clarify some things | |
| 15:00:47 | stephenfin | ...though | |
| 15:00:49 | jangutter | stephenfin: in them olden days, I think there was a field that you could set that could select e1000 or realtek or something. | |
| 15:01:05 | jangutter | stephenfin: confusingly that was called vif type. | |
| 15:01:14 | kashyap | aspiers: But I _did_ ask the question to libvirt folks if there's any other way than injecting the "IOMMU on" at device config time | |
| 15:01:20 | gibi | Sundar: do you mean L376 ? | |
| 15:01:20 | aspiers | kashyap: thanks for that | |
| 15:01:20 | kashyap | [If the device is 'virtio'] | |
| 15:01:23 | kashyap | I'll keep you posted | |
| 15:02:14 | aspiers | kashyap: I don't expect there to be. I was more wondering whether it should be done device by device, or near the end of instance config time, after all devices have already been constructed | |
| 15:02:15 | Sundar | gibi: Yes, sorry for the typo | |
| 15:02:26 | aspiers | kashyap: but this is just a matter of how to construct the guest XML | |
| 15:02:29 | gibi | Sundar: OK, reading the discussion | |
| 15:02:40 | Sundar | Thanks | |
| 15:02:56 | kashyap | aspiers: While I'm zoned out on the call, a quick questioN: | |
| 15:02:57 | kashyap | cfg.driver_iommu = True | |
| 15:02:57 | kashyap | self._sev_required(...): | |
| 15:02:57 | kashyap | if ('virtio' in info['bus'] and | |
| 15:02:57 | kashyap | ... | |
| 15:02:59 | kashyap | ... | |
| 15:03:14 | kashyap | aspiers: Shouldnt that be "or", instead of "and"? | |
| 15:03:23 | aspiers | no | |
| 15:04:09 | aspiers | from the spec: "The iommu attribute is on for all virtio devices. Despite the name, this does not require the guest or host to have an IOMMU device, but merely enables the virtio flag which indicates that virtualized DMA should be used. This ties into the SEV code to handle memory encryption/decryption, and prevents IO buffers being shared between host and guest." | |
| 15:04:55 | jangutter | stephenfin: Neutron's checks can look like this: https://github.com/openstack/neutron/blob/master/neutron/plugins/ml2/drivers/openvswitch/mech_driver/mech_openvswitch.py#L80 | |
| 15:06:06 | kashyap | aspiers: I see, thanks for the context. | |
| 15:07:46 | mriedem | Sundar: replied | |
| 15:10:25 | Sundar | mriedem: Thanks. "If we agree to add a "fail on binding timeout" option" --> IUC, you are not saying that the option must be added. Only that, if we were to add it, it should be named a certain way, right? | |
| 15:15:59 | melwitt | mriedem: thanks, replied and +W on the bottom change | |
| 15:17:06 | mriedem | Sundar: correct, though i tend to think we should add that option | |
| 15:17:47 | mriedem | melwitt: thanks | |
| 15:19:14 | aspiers | efried: what else needs to go in https://review.opendev.org/#/c/664397/ ? | |
| 15:20:33 | efried | aspiers: Nothing currently known. Could approve it now, or could wait until we're pretty sure the series is a go. | |
| 15:20:55 | aspiers | efried: let's get it in now - I can easily submit follow-ups if needed | |
| 15:21:16 | efried | okay, +2 (someone else still needs to push it) | |
| 15:21:23 | aspiers | thanks | |
| 15:21:31 | aspiers | stephenfin maybe? | |
| 15:22:48 | openstackgerrit | Adam Spiers proposed openstack/nova master: Add extra spec parameter and image property for memory encryption https://review.opendev.org/664420 | |
| 15:23:00 | aspiers | efried: ^^^ s/ers/ed/ | |
| 15:23:26 | efried | +2 | |
| 15:24:23 | stephenfin | aspiers: Sure, but not for another hour at least | |