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

From: Juergen Gross <jgross@suse.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH 04/12] xen/x86: modify hvm_memory_op() prototype
Date: Mon, 18 Oct 2021 17:34:13 +0200	[thread overview]
Message-ID: <a2031437-d5c9-158e-d569-e34d79880ad4@suse.com> (raw)
In-Reply-To: <0a1aee06-41da-7045-4bdd-33993ce3741e@suse.com>

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

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@suse.com>
>>>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> 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 #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

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