Re: [PATCH 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h

From: Jan Beulich <jbeulich@suse.com>
To: Julien Grall <julien@xen.org>
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Julien Grall" <jgrall@amazon.com>,
	"Ian Jackson" <ian.jackson@eu.citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	xen-devel@lists.xenproject.org,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [PATCH 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h
Date: Thu, 9 Apr 2020 10:06:52 +0200	[thread overview]
Message-ID: <f02f09ec-b643-8321-e235-ce0ee5526ab3@suse.com> (raw)
In-Reply-To: <c8e66108-7ac1-fb51-841f-21886b731f04@xen.org>

On 09.04.2020 10:01, Julien Grall wrote:
> Hi,
> 
> On 09/04/2020 07:30, Jan Beulich wrote:
>> On 09.04.2020 00:05, Julien Grall wrote:
>>> Hi Jan,
>>>
>>> On 07/04/2020 09:14, Jan Beulich wrote:
>>>> On 04.04.2020 15:10, Julien Grall wrote:
>>>>> From: Julien Grall <jgrall@amazon.com>
>>>>>
>>>>> Most of the helpers to access guest memory are implemented the same way
>>>>> on Arm and x86. The only differences are:
>>>>>       - guest_handle_{from, to}_param(): while on x86 XEN_GUEST_HANDLE()
>>>>>         and XEN_GUEST_HANDLE_PARAM() are the same, they are not on Arm. It
>>>>>         is still fine to use the Arm implementation on x86.
>>>>>       - __clear_guest_offset(): Interestingly the prototype does not match
>>>>>         between the x86 and Arm. However, the Arm one is bogus. So the x86
>>>>>         implementation can be used.
>>>>>       - guest_handle{,_subrange}_okay(): They are validly differing
>>>>>         because Arm is only supporting auto-translated guest and therefore
>>>>>         handles are always valid.
>>>>
>>>> While I'm fine in principle with such consolidation, I'm afraid I
>>>> really need to ask for some historical background to be added
>>>> here. It may very well be that there's a reason for the separation
>>>> (likely to be found in the removed ia64 or ppc ports), which may
>>>> then provide a hint at why future ports may want to have these
>>>> separated. If such reasons exist, I'd prefer to avoid the back and
>>>> forth between headers. What we could do in such a case is borrow
>>>> Linux'es asm-generic/ concept, and move the "typical"
>>>> implementation there. (And of course if there were no noticable
>>>> reasons for the split, the change as it is would be fine in
>>>> general; saying so without having looked at the details of it,
>>>> yet).
>>>
>>> Looking at the history, ia64 and ppc used to include a common
>>> header called xen/xencomm.h from asm/guest_access.h.
>>>
>>> This has now disappeared with the removal of the two ports.
>>>
>>> Regarding future arch, the fact arm and x86 gives me some confidence
>>> we are unlikely going to get a new ABI for an arch. Do you see any
>>> reason to?
>>
>> Well, there surely had be a reason for ia64 and ppc to use a
>> different approach. 
> 
> This was introduced way before my time in Xen. AFAICT, you were
> already part of the community when 'xencomm' were alive.

I was, but I was never actively involved in ia64 or ppc work.

> There are not much information about it only nor in the commits.
> So do you mind sharing a bit more what 'xencomm' meant to be
> and why it wasn't introduced on x86?

If I had details, I would have provided at least some hints already.

>> I don't see why a new port may not also want
>> to go that route instead of the x86/Arm one.
> I could accept that someone would want to reinvent a new ABI
> from scratch for just an hypothetical new arch. However it would
> be quite an effort to reinvent XEN_GUEST_HANDLE(). The chance is
> RISC-V is only going to re-use what Arm did as Arm already did
> with x86.
> 
> I would like to avoid to introduce a new directory asm-generic
> with just one header in it. Maybe you have some other headers in
> mind?

I recall having wondered a few times whether we shouldn't use this
concept elsewhere. One case iirc was bitops stuff. Looking over
the Linux ones, some atomic and barrier fallback implementations
may also sensibly live there, and there are likely more.

Jan