Messages in this thread | | | Date | Fri, 24 Jun 2022 15:23:18 -0700 | Subject | Re: [PATCH v8 2/5] x86/tdx: Add TDX Guest event notify interrupt support | From | Sathyanarayanan Kuppuswamy <> |
| |
+ Jiewen
Jiewen, Can you please comment on the specification related queries?
On 6/20/22 8:44 AM, Sathyanarayanan Kuppuswamy wrote: > Hi, > > + Jun > > On 6/20/22 5:33 AM, Kai Huang wrote: >> On Wed, 2022-06-08 at 19:52 -0700, Kuppuswamy Sathyanarayanan wrote: >>> Host-guest event notification via configured interrupt vector is useful >>> in cases where a guest makes an asynchronous request and needs a >>> callback from the host to indicate the completion or to let the host >>> notify the guest about events like device removal. One usage example is, >>> callback requirement of GetQuote asynchronous hypercall. >> >> Although this paragraph is from GHCI spec, IMHO it is not very helpful. In >> fact, I think this paragraph is not that right and should be removed from GHCI. >> The reason is such event notification from VMM in cases like "device removal" is >> too vague. There's no _specification_ in GHCI around which "device removal" >> should VMM inject such event. For instance, I _think_ the Qemu enumerated ACPI- >> based hotplug should continue to work in TD. > > Yes. It just says that it *can* be used to signal a device removal. It is just > an example for where it can be used. But I agree that such a use case is vague. > If it makes it better, I am fine with removing it. > > Copied from sec 3.5 TDG.VP.VMCALL<SetupEventNotifyInterrupt>: > > "Example of an operation that can use the event notify is the host > VMM signaling a device removal to the TD, in response to which a TD may > unload a device driver." > >> >> That being said, if a TD has multiple devices, it cannot know whether the VMM >> will inject the removal event via the vector set by SetupEventNotifyInterrupt. >> And for the same device in the same TD, different VMMs may use different way to >> notify its removal. > > As per current design, If it is used for device removal, I think all registered > device drivers will get the notification and the individual device driver has > to check whether it is applicable for them. > > If the SetupEventNotifyInterrupt TDVMCALL specification is extended to specify > the exact device or use case detail, then it can optimize the implementation. > >> >> It seems GetQuote is the only user of SetupEventNotifyInterrupt. Maybe we >> should just declare it is for GetQuote. > > Ok. > >> >> Isaku, what do you think? Does this make sense? >> >>> >>> In TDX guest, SetupEventNotifyInterrupt hypercall can be used by the >>> guest to specify which interrupt vector to use as an event-notify >>> vector to the VMM. Details about the SetupEventNotifyInterrupt >>> hypercall can be found in TDX Guest-Host Communication Interface >>> (GHCI) Specification, sec 3.5 "VP.VMCALL<SetupEventNotifyInterrupt>". >>> Add a tdx_hcall_set_notify_intr() helper function to implement the >>> SetupEventNotifyInterrupt hypercall. >> >> As you also used "can" above, the GHCI only says the VMM _CAN_ inject the vector >> set by SetupEventNotifyInterrupt, but not must (3.3 TDG.VP.VMCALL<GetQuote>). >> This means theoretically TD should implement pooling mode in case VMM doesn't >> support injecting event via vector done by SetupEventNotifyInterrupt? > > Yes. But GetQuote specification does not talk about the pooling mode > use case as well. So I think it is just a wording confusion. > >> >> Perhaps we should update the GHCI spec to use must.. > > Ok. > >> >>> >>> Reserve 0xec IRQ vector address for TDX guest to receive the event >>> completion notification from VMM. Also add related IDT handler to >>> process the notification event. >> >> Here lacks why we need to choose to reserve a system vector. For instance, why >> we cannot choose to use device IRQ way which only requires one vector on one > > As you have explained below, as per current spec, it just expects a system > vector. > >> cpu. As you can see reserving a system vector isn't ideal especially for >> attestation as it is not a frequent operation. It is wasteful of using IRQ > > I agree that event notification is currently only used for attestation. But I > think in future there could be other use cases for it. If the intention is just > to use it for attestation, then we can just modify the GetQuote TDVMCALL to pass > the vector address, and there is no need for new TDVMCALL. I think the intention > here is to have generic method for VMM to notify TD about some events. I am not > clear about the possible future use cases, so I cannot comment on frequency of > its use. > > Jun, any comments? > > > >> resource especially on server systems with a lot of CPUs. > > FWIW, this reservation is protected with CONFIG_INTEL_TDX_GUEST. So it will be > reserved only for TDX use case. > > >> >> The reason is SetupEventNotifyInterrupt TDVMCALL only has one argument, which is >> vector, but cannot specify which CPU that the VMM should inject the event to. >> The GHCI spec doesn't say which CPU the VMM should inject to (i.e. must inject >> to the CPU on which SetupEventNotifyInterrupt is called), so we can only assume >> VMM can inject to any CPU. >> >> Btw, x86 maintainers, >> >> I'd like to check with you to see whether we should improve the existing >> SetupEventNotifyInterrupt so we can choose to use request_irq() style for >> attestation. Using request_irq() means we don't need to reserve a system >> vector, but can allocate a vector dynamically when needed. >> >> Assuming we update SetupEventNotifyInterrupt to also allow TD to specify which >> CPU (i.e. via APICID) to inject (along with the vector), my understanding is we >> can use below way (idea only) to dynamically allocate a vector on one CPU when >> attestation is needed: >> >> >> int cpu, vector; >> int irq; >> >> // request an IRQ, and prevent it from being migrated >> irq = __irq_domain_alloc_irqs(x86_vector_domain, 0, 1, ...); >> request_irq(irq, ...); >> >> // get vector, cpu from irq >> >> TDVMCALL<SetupEventNotifyInterrupt>(vector, >> apic->cpu_present_to_apidid(cpu)); >> >> Is this reasonable? If yes, is it worth to do? >> >
-- Sathyanarayanan Kuppuswamy Linux Kernel Developer
| |