Re: [PATCH 04/12] xen/x86: modify hvm_memory_op() prototype

[Juergen Gross <jgross@xxxxxxxx>]

On 18.10.21 16:28, Jan Beulich wrote:
> On 18.10.2021 15:27, Juergen Gross wrote:
> > On 18.10.21 14:31, Jan Beulich wrote:
> > > On 15.10.2021 14:51, Juergen Gross wrote:
> > > > hvm_memory_op() should take an unsigned long as cmd, like
> > > > do_memory_op().
> > > >
> > > > As hvm_memory_op() is basically just calling do_memory_op() (or
> > > > compat_memory_op()) passing through the parameters the cmd parameter
> > > > should have no smaller size than that of the called functions.
> > > >
> > > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > > Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> > >
> > > Nevertheless ...
> > >
> > > > --- a/xen/arch/x86/hvm/hypercall.c
> > > > +++ b/xen/arch/x86/hvm/hypercall.c
> > > > @@ -31,7 +31,7 @@
> > > >    #include <public/hvm/hvm_op.h>
> > > >    #include <public/hvm/params.h>
> > > >    
> > > > -static long hvm_memory_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> > > > +static long hvm_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> > > >    {
> > > >        long rc;
> > > ... I think this would even better be dealt with by splitting the
> > > function into a native one (using unsigned long) and a compat one
> > > (using unsigned int).
> > Why? In 32-bit case the value is naturally limited to 32 bits width
> > zero-extending perfectly fine to unsigned long.
> It all ends up working fine, yes. Else I wouldn't have given R-b.
> But the .compat slot of the hypercall table really should use a
> prototype without unsigned long, and then the calls wouldn't
> zero-extend the arguments anymore. And then the declaration would
> be wrong, as then it would need to be the callee to zero-extend if
> it wants to use 64-bit values.
>
> > Otherwise I couldn't use the same definition later.
> Right. And this will be less of a problem once the function pointer
> tables are gone, as then the compiler sees the real parameter types
> for the individual functions.
Okay, I understand that.

I'd prefer to do that as a followup patch (series) then.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature