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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 11/12/2019 07:41, Jan Beulich wrote:
> 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.

I'm not fixing any bug.

I am making a fundamental change in behaviour, so tasklet context can
use -ERESTART.

Tasklet context doesn't even know what the primary hypercall index needs
to be to correctly create a continuation.

~Andrew

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