On 3/9/23 05:59, Borislav Petkov wrote:
> First of all,
>
> thanks for proactively pointing that out instead of simply using what's
> there and we get to find out later, only by chance.
>
> Much appreciated. :-)
>
> On Thu, Mar 09, 2023 at 11:12:10AM +0000, David Woodhouse wrote:
>>> Right, I think we're ok with the following basic rules:
>>>
>>> - pure arch/x86/ code should use the x86_platform function pointers to
>>> query hypervisor capabilities/peculiarities
>>>
>>> - cc_platform_has() should be used in generic/driver code as it
>>> abstracts away the underlying platform better. IOW, querying
>>> x86_platform.... in generic, platform-agnostic driver code looks weird to
>>> say the least
>>>
>>> The hope is that those two should be enough to support most guest types
>>> and not let the zoo get too much out of hand...
>>>
>>> Thx.
>>
>> In
>> https://lore.kernel.org/all/20230308171328.1562857-13-usama.arif@bytedance.com/
>> I added an sev_es_active() helper for x86 code.
>>
>> Is that consistent with the vision here, or should I do something different?
>
> So looking at sev_es_init_vc_handling() where we set that key, I'm
> *thinking* that key can be removed now and the code should check
>
> cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT)
>
> instead.
>
> Because if some of the checks in that function below fail, the guest
> will terminate anyway.
>
> Jörg, Tom?
I believe Joerg added that key for performance reasons, since it is used
on the exception path and can avoid all the calls to cc_platform_has(). I
think that key should stay.
Maybe David can introduce an CC_ATTR_GUEST_SEV_ES attribute that returns
true if the guest is an ES or SNP guest. Or do we introduce a
CC_ATTR_PARALLEL_BOOT attribute that returns true for any SEV guest.
Then the "if cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT) && !has_sev_es"
check in arch/x86/kernel/smpboot.c can be removed and the following check
can become if (x2apic_mode || cc_platform_has(CC_ATTR_PARALLEL_BOOT))
Not sure how that affects a TDX guest, though.
Thanks,
Tom
>
> First of all,
>
> thanks for proactively pointing that out instead of simply using what's
> there and we get to find out later, only by chance.
>
> Much appreciated. :-)
>
> On Thu, Mar 09, 2023 at 11:12:10AM +0000, David Woodhouse wrote:
>>> Right, I think we're ok with the following basic rules:
>>>
>>> - pure arch/x86/ code should use the x86_platform function pointers to
>>> query hypervisor capabilities/peculiarities
>>>
>>> - cc_platform_has() should be used in generic/driver code as it
>>> abstracts away the underlying platform better. IOW, querying
>>> x86_platform.... in generic, platform-agnostic driver code looks weird to
>>> say the least
>>>
>>> The hope is that those two should be enough to support most guest types
>>> and not let the zoo get too much out of hand...
>>>
>>> Thx.
>>
>> In
>> https://lore.kernel.org/all/20230308171328.1562857-13-usama.arif@bytedance.com/
>> I added an sev_es_active() helper for x86 code.
>>
>> Is that consistent with the vision here, or should I do something different?
>
> So looking at sev_es_init_vc_handling() where we set that key, I'm
> *thinking* that key can be removed now and the code should check
>
> cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT)
>
> instead.
>
> Because if some of the checks in that function below fail, the guest
> will terminate anyway.
>
> Jörg, Tom?
I believe Joerg added that key for performance reasons, since it is used
on the exception path and can avoid all the calls to cc_platform_has(). I
think that key should stay.
Maybe David can introduce an CC_ATTR_GUEST_SEV_ES attribute that returns
true if the guest is an ES or SNP guest. Or do we introduce a
CC_ATTR_PARALLEL_BOOT attribute that returns true for any SEV guest.
Then the "if cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT) && !has_sev_es"
check in arch/x86/kernel/smpboot.c can be removed and the following check
can become if (x2apic_mode || cc_platform_has(CC_ATTR_PARALLEL_BOOT))
Not sure how that affects a TDX guest, though.
Thanks,
Tom
>