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

From: Andrii Anisov <andrii.anisov@gmail.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	Julien Grall <julien.grall@arm.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	"andrii_anisov@epam.com" <andrii_anisov@epam.com>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v3] xen: introduce VCPUOP_register_runstate_phys_memory_area hypercall
Date: Thu, 13 Jun 2019 15:17:12 +0300	[thread overview]
Message-ID: <4f83cfb1-a317-d486-d1e0-2ed15fe1be59@gmail.com> (raw)
In-Reply-To: <5D00A97B02000078002374E5@prv1-mh.provo.novell.com>

Jan,

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?

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

True.

>>> In a prior version you did the mechanical part of adjusting the VA-based
>>> code in a prereq patch, aiding review. Is there a particular reason you
>>> folded everything into one patch now?
>>
>> I silently followed suggestion from George [1]. Any objections?
> 
> Hmm, I can't read this into George's suggestion. Aiui he did suggest
> not to split the definition of the new interface from its implementation.
> But that doesn't necessarily mean to squash _everything_ in one
> patch.

OK.
It looks that what you said firstly is closer to V1 of this stuff. Will keep this in mind for the next version.

> Well, I'm afraid I don't understand what you're after. Of course
> compat mode guests need to continue to be supported, and the
> new interface would also better be available to them. And it is
> a fact that their runstate area layout differs from that of 64-bit
> guests.

OK.

>>>> --- 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?

-- 
Sincerely,
Andrii Anisov.

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