Re: [Xen-devel] [PATCH v2 2/2] xen: implement VCPUOP_register_runstate_phys_memory_area

From: "Jan Beulich" <JBeulich@suse.com>
To: "Julien Grall" <julien.grall@arm.com>,<andrii.anisov@gmail.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>,
	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 v2 2/2] xen: implement VCPUOP_register_runstate_phys_memory_area
Date: Thu, 09 May 2019 03:27:02 -0600	[thread overview]
Message-ID: <5CD3F266020000780022D0F3@prv1-mh.provo.novell.com> (raw)
Message-ID: <20190509092702.yO4lDm7_9RZEXiBtWyvCUWKNF3ztEHI0iNNwxM5dQcs@z> (raw)
In-Reply-To: <e248dae9-c91c-c735-ea16-9bcb70c65e9d@arm.com>

>>> On 08.05.19 at 17:40, <julien.grall@arm.com> wrote:
> On 23/04/2019 09:10, Andrii Anisov wrote:
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -163,15 +163,23 @@ struct vcpu
>>       void            *sched_priv;    /* scheduler-specific data */
>>   
>>       struct vcpu_runstate_info runstate;
>> +
>> +    spinlock_t      mapped_runstate_lock;
>> +
>>   #ifndef CONFIG_COMPAT
>>   # define runstate_guest(v) ((v)->runstate_guest)
>>       XEN_GUEST_HANDLE(vcpu_runstate_info_t) runstate_guest; /* guest address */
>> +    vcpu_runstate_info_t *mapped_runstate;
>>   #else
>>   # define runstate_guest(v) ((v)->runstate_guest.native)
>>       union {
>>           XEN_GUEST_HANDLE(vcpu_runstate_info_t) native;
>>           XEN_GUEST_HANDLE(vcpu_runstate_info_compat_t) compat;
>>       } runstate_guest; /* guest address */
>> +    union {
>> +        vcpu_runstate_info_t* native;
>> +        vcpu_runstate_info_compat_t* compat;
>> +    } mapped_runstate; /* guest address */
> 
> The combination of mapped_runstate and runstate_guest is a bit confusing. I 
> think you want to rework the interface to show that only one is possible at the 
> time and make clear which one is used by who. Maybe:
> 
> union
> {
>     /* Legacy interface to be used when the guest provides a virtual address */
>     union {
>        XEN_GUEST_HANDLE(vcpu_runstate_info_t) native;
>        ...
>     } virt;
> 
>     /* Interface used when the guest provides a physical address */
>     union {
>     } phys;
> } runstate_guest.
> 
> runstate_guest_type /* could be a bool or enum */
> 
> Jan what do you think?

I fully agree - no mixing of approaches here, if possible. However,
care needs to be taken that after a domain reset the new kernel
can chose the opposite model. Question is whether there are even
other cases where independent components (say boot loader and
OS) may need to be permitted to chose models independently of
one another.

As a side note, Andrii - would you please avoid sending double mail
to xen-devel (addresses differing just by domain used)?

Jan

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