Re: [PATCH v6 1/9] xen: move do_vcpu_op() to arch specific code

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 2168 bytes --]

On 27.06.22 13:02, Roger Pau Monné wrote:
> On Mon, Jun 27, 2022 at 12:40:41PM +0200, Juergen Gross wrote:
>> On 27.06.22 12:28, Roger Pau Monné wrote:
>>> On Thu, Mar 24, 2022 at 03:01:31PM +0100, Juergen Gross wrote:
>>>> The entry point used for the vcpu_op hypercall on Arm is different
>>>> from the one on x86 today, as some of the common sub-ops are not
>>>> supported on Arm. The Arm specific handler filters out the not
>>>> supported sub-ops and then calls the common handler. This leads to the
>>>> weird call hierarchy:
>>>>
>>>>     do_arm_vcpu_op()
>>>>       do_vcpu_op()
>>>>         arch_do_vcpu_op()
>>>>
>>>> Clean this up by renaming do_vcpu_op() to common_vcpu_op() and
>>>> arch_do_vcpu_op() in each architecture to do_vcpu_op(). This way one
>>>> of above calls can be avoided without restricting any potential
>>>> future use of common sub-ops for Arm.
>>>
>>> Wouldn't it be more natural to have do_vcpu_op() contain the common
>>> code (AFAICT handlers for
>>> VCPUOP_register_{vcpu_info,runstate_memory_area}) and then everything
>>> else handled by the x86 arch_do_vcpu_op() handler?
>>>
>>> I find the common prefix misleading, as not all the VCPUOP hypercalls
>>> are available to all the architectures.
>>
>> This would end up in Arm suddenly supporting the sub-ops it doesn't
>> (want to) support today. Otherwise it would make no sense that Arm has
>> a dedicated entry for this hypercall.
> 
> My preference would be for a common do_vcpu_op() that just contains
> handlers for VCPUOP_register_{vcpu_info,runstate_memory_area} and then
> an empty arch_ handler for Arm, and everything else moved to the x86
> arch_ handler.  That obviously implies some code churn, but results in
> a cleaner implementation IMO.

I'd be fine with that.

I did it in V2 of the (then secret) series, and Jan replied:

   I'm afraid I don't agree with this movement. I could see things getting
   moved that are purely PV (on the assumption that no new PV ports will
   appear), but anything that's also usable by PVH / HVM ought to be usable
   in principle also by Arm or other PV-free ports.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]