Re: [PATCH v3 05/14] x86/svm: move nestedsvm declarations used only by svm code to private header

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/02/2023 8:28 pm, Xenia Ragiadakou wrote:
>
> On 2/24/23 22:12, Andrew Cooper wrote:
>> On 24/02/2023 6:50 pm, Xenia Ragiadakou wrote:
>>> diff --git a/xen/arch/x86/hvm/svm/nestedhvm.h
>>> b/xen/arch/x86/hvm/svm/nestedhvm.h
>>> new file mode 100644
>>> index 0000000000..43245e13de
>>> --- /dev/null
>>> +++ b/xen/arch/x86/hvm/svm/nestedhvm.h
>>> @@ -0,0 +1,77 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + * nestedsvm.h: Nested Virtualization
>>> + *
>>> + * Copyright (c) 2011, Advanced Micro Devices, Inc
>>> + */
>>> +
>>> +#ifndef __X86_HVM_SVM_NESTEDHVM_PRIV_H__
>>> +#define __X86_HVM_SVM_NESTEDHVM_PRIV_H__
>>> +
>>> +#include <xen/mm.h>
>>> +#include <xen/types.h>
>>> +
>>> +#include <asm/hvm/vcpu.h>
>>> +#include <asm/hvm/hvm.h>
>>> +#include <asm/hvm/nestedhvm.h>
>>> +#include <asm/msr-index.h>
>>> +
>>> +/* SVM specific intblk types, cannot be an enum because gcc 4.5
>>> complains */
>>> +/* GIF cleared */
>>> +#define hvm_intblk_svm_gif      hvm_intblk_arch
>>
>> I know you're just moving code, but I simply don't believe this comment.
>>
>> This additional delta seems to compile fine:
>>
>> diff --git a/xen/arch/x86/hvm/svm/intr.c b/xen/arch/x86/hvm/svm/intr.c
>> index dbb0022190a8..111b10673cf4 100644
>> --- a/xen/arch/x86/hvm/svm/intr.c
>> +++ b/xen/arch/x86/hvm/svm/intr.c
>> @@ -154,7 +154,7 @@ void svm_intr_assist(void)
>>               return;
>>             intblk = hvm_interrupt_blocked(v, intack);
>> -        if ( intblk == hvm_intblk_svm_gif )
>> +        if ( intblk == hvm_intblk_arch ) /* GIF */
>>           {
>>               ASSERT(nestedhvm_enabled(v->domain));
>>               return;
>> diff --git a/xen/arch/x86/hvm/svm/nestedhvm.h
>> b/xen/arch/x86/hvm/svm/nestedhvm.h
>> index 43245e13deb7..c7747daae24a 100644
>> --- a/xen/arch/x86/hvm/svm/nestedhvm.h
>> +++ b/xen/arch/x86/hvm/svm/nestedhvm.h
>> @@ -16,10 +16,6 @@
>>   #include <asm/hvm/nestedhvm.h>
>>   #include <asm/msr-index.h>
>>   -/* SVM specific intblk types, cannot be an enum because gcc 4.5
>> complains */
>> -/* GIF cleared */
>> -#define hvm_intblk_svm_gif      hvm_intblk_arch
>> -
>>   #define vcpu_nestedsvm(v) (vcpu_nestedhvm(v).u.nsvm)
>>     /* True when l1 guest enabled SVM in EFER */
>> diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c
>> b/xen/arch/x86/hvm/svm/nestedsvm.c
>> index 92316c6624ce..1794eb2105be 100644
>> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
>> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
>> @@ -1247,7 +1247,7 @@ enum hvm_intblk cf_check nsvm_intr_blocked(struct
>> vcpu *v)
>>       ASSERT(nestedhvm_enabled(v->domain));
>>         if ( !nestedsvm_gif_isset(v) )
>> -        return hvm_intblk_svm_gif;
>> +        return hvm_intblk_arch;
>>         if ( nestedhvm_vcpu_in_guestmode(v) )
>>       {
>>
>>
>> but the first hunk demonstrates an error in the original logic.
>> Architecturally, GIF can become set for SKINIT, and in systems where SVM
>> isn't available.
>>
>> I'm unsure whether its better to fix this up in this patch, or defer it
>> for later.
>
> I think this change merits its own patch.

Yeah, it probably does.

Seeing as you've reviewed my two alt patches, I'll commit some more as
I've already resolved the conflicts around adjacent headers.

~Andrew