[svsm-devel] [PATCH v3 02/14] x86/sev: Make the VMPL0 checking function more generic
Tom Lendacky
thomas.lendacky at amd.com
Wed Apr 17 22:35:53 CEST 2024
On 4/17/24 06:46, Borislav Petkov wrote:
> On Mon, Mar 25, 2024 at 05:26:21PM -0500, Tom Lendacky wrote:
>> -static void enforce_vmpl0(void)
>> +static bool running_at_vmpl0(void *va)
>
> Not too crazy about it: you're turning it into a function which runs in
> boolean context but takes a void *?!
>
> And the boolean result is only a side-effect or what it does to the
> argument - modify its permissions. Which is weird and not really
> obvious.
Well, it doesn't really modify any permissions that matter. It tries to
change the permission of a lesser privileged VMPL level. Since the
kernel only runs at a single VMPL it would never be effected. The
operation performed here is to update VMPL1 permission levels (which can
only be done successfully at VMPL0) and return the result of the
operation. A success implies running at VMPL0 and failure implies not
running at VMPL0.
>
> I'd prefer it if you made it into
>
> static void vmpl0_modify_permissions(void *va)
I guess this confuses me, since it sounds like you're trying to modify
the VMPL0 permissions, which you can't do. Maybe calling it
modify_vmpl1_permissions() would be better. And a void return doesn't
tell me whether it was successful and, therefore, whether the kernel is
running at VMPL0.
Thanks,
Tom
>
> which basically says, modify the permissions of @va in vmpl0, which is
> a lot closer to what the function does.
>
> And then do
>
> #define running_at_vmpl0(va) vmpl0_modify_permissions((va))
>
> because then through the indirection is at least clear how that "am
> I running at VMPL0?" check is being done.
>
> And later, if we need other VMPLs, we can extend
> vmpl0_modify_permissions() and even do a more generic
>
> vmpl_modify_permissions(unsigned int vmpl_level, void *va)
>
> and so on and kill the silly macro.
>
> Thx.
>
More information about the Svsm-devel
mailing list