Re: [PATCH v2 3/3] svm/nestedsvm: Introduce nested capabilities bit

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.03.2024 11:57, George Dunlap wrote:
> On Thu, Mar 28, 2024 at 6:44 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>> As to why to have each vendor independent code check for HAP -- there
>>> are in fact two implementations of the code; it's nice to be able to
>>> look in one place for each implementation to determine the
>>> requirements.  Additionally, it would be possible, in the future, for
>>> one of the nested virt implementations to enable shadow mode, while
>>> the other one didn't.  The current arrangement makes that easy.
>>
>> Well, first - how likely is this, when instead we've been considering
>> whether we could get rid of shadow mode? And then this is balancing
>> between ease of future changes vs avoiding redundancy where it can be
>> avoided. I'm not going to insist on centralizing the HAP check, but I
>> certainly wanted the option to be considered.
> 
> I considered it before replying to you; so consider it considered. :-)
> 
>>>>> --- a/xen/arch/x86/hvm/nestedhvm.c
>>>>> +++ b/xen/arch/x86/hvm/nestedhvm.c
>>>>> @@ -150,6 +150,16 @@ static int __init cf_check nestedhvm_setup(void)
>>>>>      __clear_bit(0x80, shadow_io_bitmap[0]);
>>>>>      __clear_bit(0xed, shadow_io_bitmap[1]);
>>>>>
>>>>> +    /*
>>>>> +     * NB this must be called after all command-line processing has been
>>>>> +     * done, so that if (for example) HAP is disabled, nested virt is
>>>>> +     * disabled as well.
>>>>> +     */
>>>>> +    if ( cpu_has_vmx )
>>>>> +        start_nested_vmx(&hvm_funcs);
>>>>> +    else if ( cpu_has_svm )
>>>>> +        start_nested_svm(&hvm_funcs);
>>>>
>>>> Instead of passing the pointer, couldn't you have the functions return
>>>> bool, and then set hvm_funcs.caps.nested_virt from that? Passing that
>>>> pointer looks somewhat odd to me.
>>>
>>> For one, it makes start_nested_XXX symmetric to start_XXX, which also
>>> has access to the full hvm_funcs structure (albeit in the former case
>>> because it's creating the structure).
>>
>> This last aspect is the crucial aspect: start{svm,vmx}() are indeed quite
>> special because of this. Everywhere else we have accessor helpers when
>> hvm_funcs needs consulting, e.g. ...
>>
>>>  For two, both of them need to read caps.hap.
>>
>> ... caps _reads_ would want using (an) accessor(s), too.
>>
>>>  For three, it's just more flexible -- there may be
>>> future things that we want to modify in the start_nested_*()
>>> functions.  If we did as you suggest, and then added (say)
>>> caps.nested_virt_needs_hap, then we'd either need to add a second
>>> function, or refactor it to look like this.
>>
>> Right, I can see this as an argument when taking, as you do, speculation
>> on future needs into account. Albeit I'm then inclined to say that even
>> such modifications may better be done through accessor helpers.
> 
> Why access it through accessor helpers?
> 
> I consider that it's the SVM and VMX "construction/setup" code
> respectively which "own" hvm_funcs (as evidenced by the fact that they
> create the structures and then return them); and I consider the
> start_nested_{vmx/svm} to be a part of the same code; so I consider it
> valid for them to have direct access to the structure.
> 
>>>> Is there a reason to use direct calls here rather than a true hook
>>>> (seeing that hvm_funcs must have been populated by the time we make it
>>>> here)? I understand we're (remotely) considering to switch away from
>>>> using hooks at some point, but right now this feels somewhat
>>>> inconsistent if not further justified.
>>>
>>> Again it mimics the calls to start_vmx/svm in hvm_enable.  But I could
>>> look at adding a function pointer to see if it works.
>>
>> Andrew - do you have any input here towards those somewhat vague plans
>> of getting rid of the hook functions? I guess they're more relevant in
>> places where we can't easily use the altcall machinery (i.e. not here).
> 
> Rather than do all that work collecting information, why don't we just
> check it in as it is?  Obviously if we're thinking about getting rid
> of hook functions at some point anyway, it can't be all that bad.
> There is an aesthetic justification for the lack of hook, in that it's
> similar to the start_vmx/svm() functions.
> 
> So far I really don't think the remaining "open" changes we're
> discussing are worth either of our efforts.  I don't plan to make any
> of these changes unless another x86 maintainer seconds your request
> (or you can convince me they're worth making).
> 
> (The other two changes -- correcting the function name in the commit
> message, and removing the extra blank line -- I've already changed
> locally, so could check in with your ack.)

After some mumbling to myself over the holidays
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan