Mailing List Archive

[PATCH 5/5] libxc: make xc_domain_maximum_gpfn() endianness-agnostic
libxc generally uses uint32_t to represent domain IDs. This is fine as
long as addresses of such variables aren't taken, to then pass into
hypercalls: To the hypervisor, a domain ID is a 16-bit value. Use an
intermediate variable to deal with the issue. (On architectures with
arguments passed in registers, such an intermediate variable would have
been created by the compiler already anyway, just one of the wrong
type.)

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/tools/libs/ctrl/xc_domain.c
+++ b/tools/libs/ctrl/xc_domain.c
@@ -856,7 +856,9 @@ int xc_domain_get_tsc_info(xc_interface

int xc_domain_maximum_gpfn(xc_interface *xch, uint32_t domid, xen_pfn_t *gpfns)
{
- long rc = do_memory_op(xch, XENMEM_maximum_gpfn, &domid, sizeof(domid));
+ domid_t xen_domid = domid;
+ long rc = do_memory_op(xch, XENMEM_maximum_gpfn, &xen_domid,
+ sizeof(xen_domid));

if ( rc >= 0 )
{
Re: [PATCH 5/5] libxc: make xc_domain_maximum_gpfn() endianness-agnostic [ In reply to ]
On 18/06/2021 11:25, Jan Beulich wrote:
> libxc generally uses uint32_t to represent domain IDs. This is fine as
> long as addresses of such variables aren't taken, to then pass into
> hypercalls: To the hypervisor, a domain ID is a 16-bit value. Use an
> intermediate variable to deal with the issue. (On architectures with
> arguments passed in registers, such an intermediate variable would have
> been created by the compiler already anyway, just one of the wrong
> type.)
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/tools/libs/ctrl/xc_domain.c
> +++ b/tools/libs/ctrl/xc_domain.c
> @@ -856,7 +856,9 @@ int xc_domain_get_tsc_info(xc_interface
>
> int xc_domain_maximum_gpfn(xc_interface *xch, uint32_t domid, xen_pfn_t *gpfns)
> {
> - long rc = do_memory_op(xch, XENMEM_maximum_gpfn, &domid, sizeof(domid));
> + domid_t xen_domid = domid;
> + long rc = do_memory_op(xch, XENMEM_maximum_gpfn, &xen_domid,
> + sizeof(xen_domid));

Why on earth do we pass the domid in by pointer and not value?

~Andrew
Re: [PATCH 5/5] libxc: make xc_domain_maximum_gpfn() endianness-agnostic [ In reply to ]
On 18/06/2021 16:06, Andrew Cooper wrote:
> On 18/06/2021 11:25, Jan Beulich wrote:
>> libxc generally uses uint32_t to represent domain IDs. This is fine as
>> long as addresses of such variables aren't taken, to then pass into
>> hypercalls: To the hypervisor, a domain ID is a 16-bit value. Use an
>> intermediate variable to deal with the issue. (On architectures with
>> arguments passed in registers, such an intermediate variable would have
>> been created by the compiler already anyway, just one of the wrong
>> type.)
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/tools/libs/ctrl/xc_domain.c
>> +++ b/tools/libs/ctrl/xc_domain.c
>> @@ -856,7 +856,9 @@ int xc_domain_get_tsc_info(xc_interface
>>
>> int xc_domain_maximum_gpfn(xc_interface *xch, uint32_t domid, xen_pfn_t *gpfns)
>> {
>> - long rc = do_memory_op(xch, XENMEM_maximum_gpfn, &domid, sizeof(domid));
>> + domid_t xen_domid = domid;
>> + long rc = do_memory_op(xch, XENMEM_maximum_gpfn, &xen_domid,
>> + sizeof(xen_domid));
> Why on earth do we pass the domid in by pointer and not value?

This is horrible.

What we're logically doing is passing a  pointer to struct
xen_memory_$FOO { domid_t domid; }, except its all done by void
pointers, and even the public header files don't provide a suitable
structure.

I think we really do want to retrofit a suitable structure in the public
interface and use that, rather than to continue games like this.

~Andrew
Re: [PATCH 5/5] libxc: make xc_domain_maximum_gpfn() endianness-agnostic [ In reply to ]
On 18/06/2021 16:22, Andrew Cooper wrote:
> On 18/06/2021 16:06, Andrew Cooper wrote:
>> On 18/06/2021 11:25, Jan Beulich wrote:
>>> libxc generally uses uint32_t to represent domain IDs. This is fine as
>>> long as addresses of such variables aren't taken, to then pass into
>>> hypercalls: To the hypervisor, a domain ID is a 16-bit value. Use an
>>> intermediate variable to deal with the issue. (On architectures with
>>> arguments passed in registers, such an intermediate variable would have
>>> been created by the compiler already anyway, just one of the wrong
>>> type.)
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/tools/libs/ctrl/xc_domain.c
>>> +++ b/tools/libs/ctrl/xc_domain.c
>>> @@ -856,7 +856,9 @@ int xc_domain_get_tsc_info(xc_interface
>>>
>>> int xc_domain_maximum_gpfn(xc_interface *xch, uint32_t domid, xen_pfn_t *gpfns)
>>> {
>>> - long rc = do_memory_op(xch, XENMEM_maximum_gpfn, &domid, sizeof(domid));
>>> + domid_t xen_domid = domid;
>>> + long rc = do_memory_op(xch, XENMEM_maximum_gpfn, &xen_domid,
>>> + sizeof(xen_domid));
>> Why on earth do we pass the domid in by pointer and not value?
> This is horrible.
>
> What we're logically doing is passing a  pointer to struct
> xen_memory_$FOO { domid_t domid; }, except its all done by void
> pointers, and even the public header files don't provide a suitable
> structure.
>
> I think we really do want to retrofit a suitable structure in the public
> interface and use that, rather than to continue games like this.

Alternatively, declare this interface broken and reimplement it as a
domctl, which is where the functionality ought logically to live.

~Andrew
Re: [PATCH 5/5] libxc: make xc_domain_maximum_gpfn() endianness-agnostic [ In reply to ]
On 18.06.2021 17:24, Andrew Cooper wrote:
> On 18/06/2021 16:22, Andrew Cooper wrote:
>> On 18/06/2021 16:06, Andrew Cooper wrote:
>>> On 18/06/2021 11:25, Jan Beulich wrote:
>>>> libxc generally uses uint32_t to represent domain IDs. This is fine as
>>>> long as addresses of such variables aren't taken, to then pass into
>>>> hypercalls: To the hypervisor, a domain ID is a 16-bit value. Use an
>>>> intermediate variable to deal with the issue. (On architectures with
>>>> arguments passed in registers, such an intermediate variable would have
>>>> been created by the compiler already anyway, just one of the wrong
>>>> type.)
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> --- a/tools/libs/ctrl/xc_domain.c
>>>> +++ b/tools/libs/ctrl/xc_domain.c
>>>> @@ -856,7 +856,9 @@ int xc_domain_get_tsc_info(xc_interface
>>>>
>>>> int xc_domain_maximum_gpfn(xc_interface *xch, uint32_t domid, xen_pfn_t *gpfns)
>>>> {
>>>> - long rc = do_memory_op(xch, XENMEM_maximum_gpfn, &domid, sizeof(domid));
>>>> + domid_t xen_domid = domid;
>>>> + long rc = do_memory_op(xch, XENMEM_maximum_gpfn, &xen_domid,
>>>> + sizeof(xen_domid));
>>> Why on earth do we pass the domid in by pointer and not value?
>> This is horrible.
>>
>> What we're logically doing is passing a  pointer to struct
>> xen_memory_$FOO { domid_t domid; }, except its all done by void
>> pointers, and even the public header files don't provide a suitable
>> structure.
>>
>> I think we really do want to retrofit a suitable structure in the public
>> interface and use that, rather than to continue games like this.
>
> Alternatively, declare this interface broken and reimplement it as a
> domctl, which is where the functionality ought logically to live.

Not really, no - this is something a domain can inquire on itself.

Jan