Re: [PATCH v8 2/5] x86/tdx: Add TDX Guest event notify interrupt support

Kai Huang
2022-07-14T23:59:43Z

> > > +/*
> > > + * Handler used to report notifications about
> > > + * TDX_GUEST_EVENT_NOTIFY_VECTOR IRQ. Currently it will be
> > > + * used only by the attestation driver. So, race condition
> > > + * with read/write operation is not considered.
> > > + */
> > > +static void (*tdx_event_notify_handler)(void);
> > > +
> > > +/* Helper function to register tdx_event_notify_handler */
> > > +void tdx_setup_ev_notify_handler(void (*handler)(void))
> > > +{
> > > + tdx_event_notify_handler = handler;
> > > +}
> > > +
> > > +/* Helper function to unregister tdx_event_notify_handler */
> > > +void tdx_remove_ev_notify_handler(void)
> > > +{
> > > + tdx_event_notify_handler = NULL;
> > > +}
> > > +
> >
> > Looks it's weird that you still need it. Couldn't we pass the function to
> > handle GetQuote directly to request_irq()?
>
>
> Since event notification is not only specific to attestation, I did not want to
> directly tie it to GetQuote handler.
>
> Alternative approach is, to make this IRQ vector shared and let its users do
> request_irq() and free_irq() directly.

I think IRQF_SHARED is the logic. We don't need it now as for now we have only
one user, though.

The "multiple users" case may never happen, so doesn't justify the additional
'tdx_event_notify_handler' code at this moment.

>
> Something like below?
>
> --- a/arch/x86/coco/tdx/attest.c
> +++ b/arch/x86/coco/tdx/attest.c
> @@ -399,7 +399,7 @@ static const struct file_operations tdx_attest_fops = {
>
> static int __init tdx_attestation_init(void)
> {
> - int ret;
> + int ret, irq;
>
> /* Make sure we are in a valid TDX platform */
> if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> @@ -422,6 +422,14 @@ static int __init tdx_attestation_init(void)
> return ret;
> }
>
> + irq = tdx_get_evirq();

The function seems unnecessary. You can put 'extern int tdx_evtirq;' to
asm/tdx.h if needed and use it directly.