Re: [PATCH for-4.20] hvmloader: Rework hypercall infrastructure

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.07.2024 19:24, Andrew Cooper wrote:
> On 17/07/2024 1:37 pm, Jan Beulich wrote:
>> On 17.07.2024 13:12, Andrew Cooper wrote:
>>>  static inline int
>>>  hypercall_sched_op(
>>>      int cmd, void *arg)
>>>  {
>>> -    return _hypercall2(int, sched_op, cmd, arg);
>>> +    return _hypercall2(int, __HYPERVISOR_sched_op, cmd, arg);
>>>  }
>> I know you don't really like token concatenation in cases like these ones,
>> but these adjustments all don't look as if they were necessary right here.
>> The new macros use the macro parameter only in ways where token pasting
>> would continue to work, afaics. And in the new assembly file you use very
>> similar token pasting anyway.
> 
> That's because my judgement is not about simply tokenisation (or not). 
> It's about not using ## when it is likely to interfere with
> grep/cscope/tags/etc.
> 
> The assembly file both isn't really interesting to find this way, and
> needs ## in order to work the way it does (differing prefixes in the
> hypercall names).

The "not interesting" part is very subjective. If one really wanted to find
everything, this still would get in the way. And there being differing
prefixes in the other case would then simply require passing two arguments
to the macro instead of just one (which imo is undesirable, but that is
along with considering the change above undesirable as well).

I'm not bothered enough to request to undo this altogether, or to split
this into a separate change, but I wonder whether in the opposite case you
wouldn't demand either or even both.

>>> +        .align 4096
>> PAGE_SIZE? And then again ...
>>
>>> +        .globl hypercall_page
>>> +hypercall_page:
>>> +         /* Poisoned with `ret` for safety before hypercalls are set up. */
>>> +        .fill 4096, 1, 0xc3
>>> +        .type hypercall_page, STT_OBJECT
>>> +        .size hypercall_page, 4096
>> ... here?
> 
> HVMLoader doesn't have a suitable constant, and doesn't have _AC().
> 
> Although we probably can just get away with (1 << PAGE_SHIFT) and drop
> the ul.

Oh, right, we still mean to be compatible with gas not ignoring the ul
suffix on numbers.

>> As to the "poisoning" - how does RET provide any safety? If a call happened
>> early, the uncertainty of what %eax is set to would make it unpredictable
>> how such a caller would further behave. Imo better to crash / hang in such
>> a case, perhaps by using INT3 instead.
>>
>> I notice that matches earlier behavior, but there the comment at least
>> validly said "rendering them no-ops" (yet still without considering
>> possible problems resulting from doing so).
> 
> That's a complicated one.  I can't remember why I chose that exact
> phraseology, but it's not really about accidentally-too-early case, it's
> about error handling.
> 
> HVMLoader doesn't have an IDT, so any exception is fatal.  But that's
> also something that ideally wants fixing (if it weren't for the fact
> that it's more likely that I'll complete plans to remove hvmloader
> completely before having time to do an IDT).
> 
> But for code which does have a panic() function, there's console_io
> (logging) and sched_op (SHUTDOWN_crash) which you want to use just in
> case they do work, before moving onto other methods of terminating.

All fine, but that still leaves this RET filling yielding random behavior
at possible early call sites. At the very least the pre-fill should result
in errors to be observed by too-early callers. For this to at least vaguely
resemble something valid to call "poisoned", that is.

Jan