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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jun 27, 2022 at 01:08:11PM +0200, Juergen Gross wrote:
> 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.

I see. My opinion is that when other ports need those functions they
should be pulled out of arch code into common code, until then it just
adds confusion to have them in common code.

I will take a look at the current patch, as we need to make progress
on this.

Thanks, Roger.