Re: [Xen-devel] [PATCH 5/6] xen/tasklet: Return -ERESTART from continue_hypercall_on_cpu()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.12.2019 18:55, Andrew Cooper wrote:
> On 10/12/2019 08:55, Jan Beulich wrote:
>> On 09.12.2019 18:49, Andrew Cooper wrote:
>>> On 09/12/2019 16:52, Jan Beulich wrote:
>>>> On 05.12.2019 23:30, Andrew Cooper wrote:
>>>>> Some hypercalls tasklets want to create a continuation, rather than fail 
>>>>> the
>>>>> hypercall with a hard error.  By the time the tasklet is executing, it is 
>>>>> too
>>>>> late to create the continuation, and even continue_hypercall_on_cpu() 
>>>>> doesn't
>>>>> have enough state to do it correctly.
>>>> I think it would be quite nice if you made clear what piece of state
>>>> it is actually missing. To be honest, I don't recall anymore.
>>> How to correctly mutate the registers and/or memory (which is specific
>>> to the hypercall subop in some cases).
>> Well, in-memory arguments can be accessed as long as the mapping is
>> the right one (which it typically wouldn't be inside a tasklet). Do
>> existing continue_hypercall_on_cpu() users need this? Looking over
>> patch 4 again, I didn't think so. (Which isn't to say that removing
>> the latent issue is not a good thing.)
>>
>> In-register values can be changed as long as the respective exit
>> path will suitably pick up the value, which I thought was always
>> the case.
>>
>> Hence I'm afraid your single reply sentence didn't really clarify
>> matters. I'm sorry if this is just because of me being dense.
> 
> How, physically, would you arrange for continue_hypercall_on_cpu() to
> make the requisite state adjustments?

You can't (at least not without having sufficient further context),
I agree. Yet ...

> Yes - registers and memory can be accessed, but only the hypercall
> (sub?)op handler knows how to mutate them appropriately.
> 
> You'd have to copy the mutation logic into continue_hypercall_on_cpu(),
> and pass in op/subops and a union of all pointers, *and* whatever
> intermediate state the subop handler needs.
> 
> Or you can return -ERESTART and let the caller DTRT with the state it
> has in context, as it would in other cases requiring a continuation.

... it continues to be unclear to me whether you're fixing an actual
bug here, or just a latent one. The existing uses of
continue_hypercall_on_cpu() don't look to require state updates
beyond the hypercall return value (or else, aiui, they wouldn't have
worked in the first place), and that one had a way to get modified.

>>>>> The current behaviour with this patch is to not cancel the continuation, 
>>>>> which
>>>>> I think is less bad, but still not great.  Thoughts?
>>>> Well, that's a guest live lock then aiui.
>>> It simply continues again.  It will livelock only if the hypercall picks
>>> a bad cpu all the time.
>> Oh, I see I was mislead by continue_hypercall_tasklet_handler() not
>> updating info->cpu, not paying attention to it actually freeing info.
>> Plus a crucial aspect looks to be that there are no "chained" uses of
>> continue_hypercall_on_cpu() anymore (the microcode loading one being
>> gone now) - afaict any such wouldn't guarantee forward progress with
>> this new model (without recording somewhere which CPUs had been dealt
>> with already).
> 
> I'd forgotten that we had that, but I can't say I'm sad to see the back
> of it.  I recall at the time saying that it wasn't a clever move.
> 
> For now, I suggest that we ignore that case.  If an when a real usecase
> appears, we can consider making adjustments.

Oh, of course - I didn't mean to even remotely suggest anything else.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel