| Message ID | 99979695-e53e-7764-85e1-64dd4cf9447b@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | allow xc_domain_maximum_gpfn() to observe full GFN value | expand |
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
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
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
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
--- 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 ) {
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>