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

From: Julien Grall <julien.grall@arm.com>
To: Jan Beulich <JBeulich@suse.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: Tue, 14 May 2019 10:35:40 +0100	[thread overview]
Message-ID: <1c8b7f5f-09d7-6e38-1e78-9528c1f0a756@arm.com> (raw)
Message-ID: <20190514093540.Ac_Ex_BJaZuGn1Luqf2NbGw_ZYTx18acc3nyYNiEZaw@z> (raw)
In-Reply-To: <5CD3F266020000780022D0F3@prv1-mh.provo.novell.com>

Hi Jan,

On 09/05/2019 10:27, Jan Beulich wrote:
>>>> 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.
Good point. On a similar topic, how does Kexec works on Xen? Do we reset the 
domain as well?

Cheers,

-- 
Julien Grall

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