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

[Juergen Gross <jgross@xxxxxxxx>]

On 27.06.22 13:35, Roger Pau Monné wrote:
> 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 for that.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature