Re: [PATCH v15 2/3] virt: Add TDX guest driver

[Greg Kroah-Hartman]

On Fri, Oct 21, 2022 at 04:51:34PM -0700, Sathyanarayanan Kuppuswamy wrote:
> Hi Greg,
> 
> On 10/20/22 9:39 PM, Greg Kroah-Hartman wrote:
> >>>> +#ifdef MODULE
> >>>> +static const struct x86_cpu_id tdx_guest_ids[] = {
> >>>> +	X86_MATCH_FEATURE(X86_FEATURE_TDX_GUEST, NULL),
> >>>> +	{}
> >>>> +};
> >>>> +MODULE_DEVICE_TABLE(x86cpu, tdx_guest_ids);
> >>>> +#endif
> >>> Why the #ifdef?  Should not be needed, right?
> >> I have added it to fix the following warning reported by 0-day.
> >>
> >> https://lore.kernel.org/lkml/202209211607.tCtTWKbV-lkp@xxxxxxxxx/
> >>
> >> It is related to nullifying the MODULE_DEVICE_TABLE in #ifndef MODULE
> >> case in linux/module.h.
> > Then fix it properly, by correctly using that structure no matter what.
> > You don't do that here...
> 
> I think we can use __maybe_unused attribute to fix this warning like
> mentioned below. Are you fine with it?
> 
> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
> @@ -118,13 +118,11 @@ static void __exit tdx_guest_exit(void)
>  }
>  module_exit(tdx_guest_exit);
>  
> -#ifdef MODULE
> -static const struct x86_cpu_id tdx_guest_ids[] = {
> +static const struct x86_cpu_id __maybe_unused tdx_guest_ids[] = {
>         X86_MATCH_FEATURE(X86_FEATURE_TDX_GUEST, NULL),
>         {}
>  };
>  MODULE_DEVICE_TABLE(x86cpu, tdx_guest_ids);
> -#endif
> 
> Solution 2:
> -----------
> 
> We can also modify the code to use this structure in all cases like
> below. But it requires me to use slower x86_match_cpu() in place of 
> cpu_feature_enabled() which I think is unnecessary.
> 
> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
> @@ -103,9 +103,15 @@ static struct miscdevice tdx_misc_dev = {
>         .fops = &tdx_guest_fops,
>  };
>  
> +static const struct x86_cpu_id tdx_guest_ids[] = {
> +       X86_MATCH_FEATURE(X86_FEATURE_TDX_GUEST, NULL),
> +       {}
> +};
> +MODULE_DEVICE_TABLE(x86cpu, tdx_guest_ids);
> +
>  static int __init tdx_guest_init(void)
>  {
> -       if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> +       if (!x86_match_cpu(tdx_guest_ids))

Please use this as it's what all other users of the x86cpu module device
table code uses, right?

And what is the "speed" difference here?  Is is measurable and where
does it matter?

thanks,

greg k-h