Re: [Xen-devel] [PATCH v3] xen: introduce VCPUOP_register_runstate_phys_memory_area hypercall

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.06.19 at 14:17, <andrii.anisov@xxxxxxxxx> wrote:
> On 12.06.19 10:27, Jan Beulich wrote:
>>> Well, my point here is to left it as is, maybe add more documentation. If
>>> one likes shooting his leg, we should only care about avoiding ricochet 
> harms
>>> hypervisor or other guests.
>>> If you disagree, please suggest your interaction model, I'll be happy to
>>> implement it.
>> 
>> Well, if "mix as you like" is fine for guests to follow, then okay. But
>> we need to be _really_ certain there's no issue with this.
> 
> I'm not aware about potential problems from the guest side. Do you have any 
> ideas about it?

I didn't spend time trying to figure something out, but ...

>> Relaxing
>> the interface is always possible, while tightening an interface is
>> almost always at least a problem, if possible at all.
> 
> True.

... you agreeing here suggests someone should, and this would
better not (only) be the reviewer(s).

>>>>> --- a/xen/include/xen/sched.h
>>>>> +++ b/xen/include/xen/sched.h
>>>>> @@ -163,17 +163,31 @@ struct vcpu
>>>>>        void            *sched_priv;    /* scheduler-specific data */
>>>>>    
>>>>>        struct vcpu_runstate_info runstate;
>>>>> +
>>>>> +    enum {
>>>>> +        RUNSTATE_NONE = 0,
>>>>> +        RUNSTATE_PADDR = 1,
>>>>> +        RUNSTATE_VADDR = 2,
>>>>> +    } runstate_guest_type;
>>>>> +
>>>>> +    unsigned long runstate_in_use;
>>>>
>>>> Why "unsigned long"? Isn't a bool all you need?
>>>
>>> Bool should be enough. But it seems we will have a lock here.
>>>
>>>> Also these would now all want to be grouped in a sub-structure named
>>>> "runstate", rather than having "runstate_" prefixes.
>>>
>>> Member `runstate` has already a type of `struct vcpu_runstate_info` which is
>>> an interface type.
>>> `runstate_guest` is a union. I'd not like moving `runstate_guest` union into
>>> another substructure. Because we would have long lines like
>>> `v->struct.runstate_guest.virt.p->state_entry_time`.
>> 
>> You didn't get my point then: What I'm after is
>> 
>>      struct {
>>          struct vcpu_runstate_info info;
>>          enum {
>>              RUNSTATE_NONE,
>>              RUNSTATE_PADDR,
>>              RUNSTATE_VADDR,
>>          } guest_type;
>>          bool in_use;
>>      } runstate;
> 
> Did you miss runstate_guest as a member of that struct?

Quite possible. I've outlined it only anyway, for you to get the idea.

Jan



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