| Message ID | 20211208155606.20029-3-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: drop hypercall function tables | expand |
Hi, On 08/12/2021 15:55, Juergen Gross wrote: > Today most hypercall handlers have a return type of long, while the > compat ones return an int. There are a few exceptions from that rule, > however. So on Arm64, I don't think you can make use of the full 64-bit because a 32-bit domain would not be able to see the top 32-bit. In fact, this could potentially cause us some trouble (see [1]) in Xen. So it feels like the hypercalls should always return a 32-bit signed value on Arm. The other advantage is it would be clear that the top 32-bit are not usuable. Stefano, what do you think? > > Get rid of the exceptions by letting compat handlers always return int > and others always return long. > > For the compat hvm case use eax instead of rax for the stored result as > it should have been from the beginning. > > Additionally move some prototypes to include/asm-x86/hypercall.h > as they are x86 specific. Move the do_physdev_op() prototype from both > architecture dependant headers to the common one. Move the > compat_platform_op() prototype to the common header. > > Switch some non style compliant types (u32, s32, s64) to style compliant > ones. TBH, I think this should have been split because the modifications are done on lines that are untouched. The extra patch would have made the review easier (even if this patch is still quite small). > > Rename paging_domctl_continuation() to do_paging_domctl_cont() and add > a matching define for the associated hypercall. > > Make do_callback_op() and compat_callback_op() more similar by adding > the const attribute to compat_callback_op()'s 2nd parameter. > > Change the type of the cmd parameter for [do|compat]_kexec_op() to > unsigned int, as this is more appropriate for the compat case. > > Signed-off-by: Juergen Gross <jgross@suse.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> Cheers, [1] [1] https://lore.kernel.org/xen-devel/20211206142032.27536-1-michal.orzel@arm.com/
On 14.12.21 18:36, Julien Grall wrote: > Hi, > > On 08/12/2021 15:55, Juergen Gross wrote: >> Today most hypercall handlers have a return type of long, while the >> compat ones return an int. There are a few exceptions from that rule, >> however. > > So on Arm64, I don't think you can make use of the full 64-bit because a > 32-bit domain would not be able to see the top 32-bit. > > In fact, this could potentially cause us some trouble (see [1]) in Xen. > So it feels like the hypercalls should always return a 32-bit signed > value on Arm. This would break hypercalls like XENMEM_maximum_ram_page which are able to return larger values, right? > The other advantage is it would be clear that the top 32-bit are not > usuable. Stefano, what do you think? Wouldn't it make more sense to check the return value to be a sign extended 32-bit value for 32-bit guests in do_trap_hypercall() instead? The question is what to return if this is not the case. -EDOM? > >> >> Get rid of the exceptions by letting compat handlers always return int >> and others always return long. >> >> For the compat hvm case use eax instead of rax for the stored result as >> it should have been from the beginning. >> >> Additionally move some prototypes to include/asm-x86/hypercall.h >> as they are x86 specific. Move the do_physdev_op() prototype from both >> architecture dependant headers to the common one. Move the >> compat_platform_op() prototype to the common header. >> >> Switch some non style compliant types (u32, s32, s64) to style compliant >> ones. > > TBH, I think this should have been split because the modifications are > done on lines that are untouched. > > The extra patch would have made the review easier (even if this patch is > still quite small). I can split the patch if you want. Juergen
On Wed, 15 Dec 2021, Juergen Gross wrote: > On 14.12.21 18:36, Julien Grall wrote: > > Hi, > > > > On 08/12/2021 15:55, Juergen Gross wrote: > > > Today most hypercall handlers have a return type of long, while the > > > compat ones return an int. There are a few exceptions from that rule, > > > however. > > > > So on Arm64, I don't think you can make use of the full 64-bit because a > > 32-bit domain would not be able to see the top 32-bit. > > > > In fact, this could potentially cause us some trouble (see [1]) in Xen. > > So it feels like the hypercalls should always return a 32-bit signed value > > on Arm. > > This would break hypercalls like XENMEM_maximum_ram_page which are able > to return larger values, right? > > > The other advantage is it would be clear that the top 32-bit are not > > usuable. Stefano, what do you think? > > Wouldn't it make more sense to check the return value to be a sign > extended 32-bit value for 32-bit guests in do_trap_hypercall() instead? > > The question is what to return if this is not the case. -EDOM? I can see where Julien is coming from: we have been trying to keep the arm32 and arm64 ABIs identical since the beginning of the project. So, like Julien, my preference would be to always return 32-bit on ARM, both aarch32 and aarch64. It would make things simple. The case of XENMEM_maximum_ram_page is interesting but it is not a problem in reality because the max physical address size is only 40-bit for aarch32 guests, so 32-bit are always enough to return the highest page in memory for 32-bit guests. So XENMEM_maximum_ram_page could be the exception and return "long" which is 4 bytes on aarch32 and 8 bytes on aarch64, and it is exactly what we need.
On 16.12.21 03:10, Stefano Stabellini wrote: > On Wed, 15 Dec 2021, Juergen Gross wrote: >> On 14.12.21 18:36, Julien Grall wrote: >>> Hi, >>> >>> On 08/12/2021 15:55, Juergen Gross wrote: >>>> Today most hypercall handlers have a return type of long, while the >>>> compat ones return an int. There are a few exceptions from that rule, >>>> however. >>> >>> So on Arm64, I don't think you can make use of the full 64-bit because a >>> 32-bit domain would not be able to see the top 32-bit. >>> >>> In fact, this could potentially cause us some trouble (see [1]) in Xen. >>> So it feels like the hypercalls should always return a 32-bit signed value >>> on Arm. >> >> This would break hypercalls like XENMEM_maximum_ram_page which are able >> to return larger values, right? >> >>> The other advantage is it would be clear that the top 32-bit are not >>> usuable. Stefano, what do you think? >> >> Wouldn't it make more sense to check the return value to be a sign >> extended 32-bit value for 32-bit guests in do_trap_hypercall() instead? >> >> The question is what to return if this is not the case. -EDOM? > > > I can see where Julien is coming from: we have been trying to keep the > arm32 and arm64 ABIs identical since the beginning of the project. So, > like Julien, my preference would be to always return 32-bit on ARM, both > aarch32 and aarch64. It would make things simple. > > The case of XENMEM_maximum_ram_page is interesting but it is not a > problem in reality because the max physical address size is only 40-bit > for aarch32 guests, so 32-bit are always enough to return the highest > page in memory for 32-bit guests. You are aware that this isn't the guest's max page, but the host's? And all of this is about a 32-bit guest on a 64-bit host. Juergen
On Thu, 16 Dec 2021, Juergen Gross wrote: > On 16.12.21 03:10, Stefano Stabellini wrote: > > On Wed, 15 Dec 2021, Juergen Gross wrote: > > > On 14.12.21 18:36, Julien Grall wrote: > > > > Hi, > > > > > > > > On 08/12/2021 15:55, Juergen Gross wrote: > > > > > Today most hypercall handlers have a return type of long, while the > > > > > compat ones return an int. There are a few exceptions from that rule, > > > > > however. > > > > > > > > So on Arm64, I don't think you can make use of the full 64-bit because a > > > > 32-bit domain would not be able to see the top 32-bit. > > > > > > > > In fact, this could potentially cause us some trouble (see [1]) in Xen. > > > > So it feels like the hypercalls should always return a 32-bit signed > > > > value > > > > on Arm. > > > > > > This would break hypercalls like XENMEM_maximum_ram_page which are able > > > to return larger values, right? > > > > > > > The other advantage is it would be clear that the top 32-bit are not > > > > usuable. Stefano, what do you think? > > > > > > Wouldn't it make more sense to check the return value to be a sign > > > extended 32-bit value for 32-bit guests in do_trap_hypercall() instead? > > > > > > The question is what to return if this is not the case. -EDOM? > > > > > > I can see where Julien is coming from: we have been trying to keep the > > arm32 and arm64 ABIs identical since the beginning of the project. So, > > like Julien, my preference would be to always return 32-bit on ARM, both > > aarch32 and aarch64. It would make things simple. > > > > The case of XENMEM_maximum_ram_page is interesting but it is not a > > problem in reality because the max physical address size is only 40-bit > > for aarch32 guests, so 32-bit are always enough to return the highest > > page in memory for 32-bit guests. > > You are aware that this isn't the guest's max page, but the host's? > > And all of this is about a 32-bit guest on a 64-bit host. Yes, I am following, and it is always difficult to get the right information out of the ARM Reference Manual, but from my search ARMv8/aarch32 (which is 32-bit mode on 64-bit capable hardware) supports max 40 bits. From G5.1.3 (ARM DDI 0487G.a): --- For execution in AArch32 state, the Armv8 architecture supports: - A VA space of up to 32 bits. The actual width is IMPLEMENTATION DEFINED . - An IPA space of up to 40 bits. The translation tables and associated System registers define the width of the implemented address space. --- Julien, Bertrand, am I reading this right?
On Thu, 16 Dec 2021, Stefano Stabellini wrote: > On Thu, 16 Dec 2021, Juergen Gross wrote: > > On 16.12.21 03:10, Stefano Stabellini wrote: > > > On Wed, 15 Dec 2021, Juergen Gross wrote: > > > > On 14.12.21 18:36, Julien Grall wrote: > > > > > Hi, > > > > > > > > > > On 08/12/2021 15:55, Juergen Gross wrote: > > > > > > Today most hypercall handlers have a return type of long, while the > > > > > > compat ones return an int. There are a few exceptions from that rule, > > > > > > however. > > > > > > > > > > So on Arm64, I don't think you can make use of the full 64-bit because a > > > > > 32-bit domain would not be able to see the top 32-bit. > > > > > > > > > > In fact, this could potentially cause us some trouble (see [1]) in Xen. > > > > > So it feels like the hypercalls should always return a 32-bit signed > > > > > value > > > > > on Arm. > > > > > > > > This would break hypercalls like XENMEM_maximum_ram_page which are able > > > > to return larger values, right? > > > > > > > > > The other advantage is it would be clear that the top 32-bit are not > > > > > usuable. Stefano, what do you think? > > > > > > > > Wouldn't it make more sense to check the return value to be a sign > > > > extended 32-bit value for 32-bit guests in do_trap_hypercall() instead? > > > > > > > > The question is what to return if this is not the case. -EDOM? > > > > > > > > > I can see where Julien is coming from: we have been trying to keep the > > > arm32 and arm64 ABIs identical since the beginning of the project. So, > > > like Julien, my preference would be to always return 32-bit on ARM, both > > > aarch32 and aarch64. It would make things simple. > > > > > > The case of XENMEM_maximum_ram_page is interesting but it is not a > > > problem in reality because the max physical address size is only 40-bit > > > for aarch32 guests, so 32-bit are always enough to return the highest > > > page in memory for 32-bit guests. > > > > You are aware that this isn't the guest's max page, but the host's? I can see now that you meant to say that, no matter what is the max pseudo-physical address supported by the VM, XENMEM_maximum_ram_page is supposed to return the max memory page, which could go above the addressibility limit of the VM. So XENMEM_maximum_ram_page should potentially be able to return (1<<44) even when called by an aarch32 VM, with max IPA 40-bit. I would imagine it could be useful if dom0 is 32-bit but domUs are 64-bit on a 64-bit hypervisor (which I think it would be a very rare configuration on ARM.) Then it looks like XENMEM_maximum_ram_page needs to be able to return a value > 32-bit when called by a 32-bit guest. The hypercall ABI follows the ARM C calling convention, so a 64-bit value should be returned using r0 and r1. But looking at xen/arch/arm/traps.c:do_trap_hypercall, it doesn't seem it ever sets r1 today. Only r0 is set, so effectively we only support 32-bit return values on aarch32 and for aarch32 guests. In other words, today all hypercalls on ARM return 64-bit to 64-bit guests and 32-bit to 32-bit guests. Which in the case of memory_op is "technically" the correct thing to do because it matches the C declaration in xen/include/xen/hypercall.h: extern long do_memory_op( unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg); So... I guess the conclusion is that on ARM do_memory_op should return "long" although it is not actually enough for a correct implementation of XENMEM_maximum_ram_page for aarch32 guests ?
On 16.12.21 22:15, Stefano Stabellini wrote: > On Thu, 16 Dec 2021, Stefano Stabellini wrote: >> On Thu, 16 Dec 2021, Juergen Gross wrote: >>> On 16.12.21 03:10, Stefano Stabellini wrote: >>>> On Wed, 15 Dec 2021, Juergen Gross wrote: >>>>> On 14.12.21 18:36, Julien Grall wrote: >>>>>> Hi, >>>>>> >>>>>> On 08/12/2021 15:55, Juergen Gross wrote: >>>>>>> Today most hypercall handlers have a return type of long, while the >>>>>>> compat ones return an int. There are a few exceptions from that rule, >>>>>>> however. >>>>>> >>>>>> So on Arm64, I don't think you can make use of the full 64-bit because a >>>>>> 32-bit domain would not be able to see the top 32-bit. >>>>>> >>>>>> In fact, this could potentially cause us some trouble (see [1]) in Xen. >>>>>> So it feels like the hypercalls should always return a 32-bit signed >>>>>> value >>>>>> on Arm. >>>>> >>>>> This would break hypercalls like XENMEM_maximum_ram_page which are able >>>>> to return larger values, right? >>>>> >>>>>> The other advantage is it would be clear that the top 32-bit are not >>>>>> usuable. Stefano, what do you think? >>>>> >>>>> Wouldn't it make more sense to check the return value to be a sign >>>>> extended 32-bit value for 32-bit guests in do_trap_hypercall() instead? >>>>> >>>>> The question is what to return if this is not the case. -EDOM? >>>> >>>> >>>> I can see where Julien is coming from: we have been trying to keep the >>>> arm32 and arm64 ABIs identical since the beginning of the project. So, >>>> like Julien, my preference would be to always return 32-bit on ARM, both >>>> aarch32 and aarch64. It would make things simple. >>>> >>>> The case of XENMEM_maximum_ram_page is interesting but it is not a >>>> problem in reality because the max physical address size is only 40-bit >>>> for aarch32 guests, so 32-bit are always enough to return the highest >>>> page in memory for 32-bit guests. >>> >>> You are aware that this isn't the guest's max page, but the host's? > > I can see now that you meant to say that, no matter what is the max > pseudo-physical address supported by the VM, XENMEM_maximum_ram_page is > supposed to return the max memory page, which could go above the > addressibility limit of the VM. > > So XENMEM_maximum_ram_page should potentially be able to return (1<<44) > even when called by an aarch32 VM, with max IPA 40-bit. > > I would imagine it could be useful if dom0 is 32-bit but domUs are > 64-bit on a 64-bit hypervisor (which I think it would be a very rare > configuration on ARM.) > > Then it looks like XENMEM_maximum_ram_page needs to be able to return a > value > 32-bit when called by a 32-bit guest. > > The hypercall ABI follows the ARM C calling convention, so a 64-bit > value should be returned using r0 and r1. But looking at > xen/arch/arm/traps.c:do_trap_hypercall, it doesn't seem it ever sets r1 > today. Only r0 is set, so effectively we only support 32-bit return > values on aarch32 and for aarch32 guests. > > In other words, today all hypercalls on ARM return 64-bit to 64-bit > guests and 32-bit to 32-bit guests. Which in the case of memory_op is > "technically" the correct thing to do because it matches the C > declaration in xen/include/xen/hypercall.h: > > extern long > do_memory_op( > unsigned long cmd, > XEN_GUEST_HANDLE_PARAM(void) arg); > > So... I guess the conclusion is that on ARM do_memory_op should return > "long" although it is not actually enough for a correct implementation > of XENMEM_maximum_ram_page for aarch32 guests ? > Hence my suggestion to check the return value of _all_ hypercalls to be proper sign extended int values for 32-bit guests. This would fix all potential issues without silently returning truncated values. Juergen
On 16.12.2021 22:15, Stefano Stabellini wrote: > On Thu, 16 Dec 2021, Stefano Stabellini wrote: >> On Thu, 16 Dec 2021, Juergen Gross wrote: >>> On 16.12.21 03:10, Stefano Stabellini wrote: >>>> On Wed, 15 Dec 2021, Juergen Gross wrote: >>>>> On 14.12.21 18:36, Julien Grall wrote: >>>>>> Hi, >>>>>> >>>>>> On 08/12/2021 15:55, Juergen Gross wrote: >>>>>>> Today most hypercall handlers have a return type of long, while the >>>>>>> compat ones return an int. There are a few exceptions from that rule, >>>>>>> however. >>>>>> >>>>>> So on Arm64, I don't think you can make use of the full 64-bit because a >>>>>> 32-bit domain would not be able to see the top 32-bit. >>>>>> >>>>>> In fact, this could potentially cause us some trouble (see [1]) in Xen. >>>>>> So it feels like the hypercalls should always return a 32-bit signed >>>>>> value >>>>>> on Arm. >>>>> >>>>> This would break hypercalls like XENMEM_maximum_ram_page which are able >>>>> to return larger values, right? >>>>> >>>>>> The other advantage is it would be clear that the top 32-bit are not >>>>>> usuable. Stefano, what do you think? >>>>> >>>>> Wouldn't it make more sense to check the return value to be a sign >>>>> extended 32-bit value for 32-bit guests in do_trap_hypercall() instead? >>>>> >>>>> The question is what to return if this is not the case. -EDOM? >>>> >>>> >>>> I can see where Julien is coming from: we have been trying to keep the >>>> arm32 and arm64 ABIs identical since the beginning of the project. So, >>>> like Julien, my preference would be to always return 32-bit on ARM, both >>>> aarch32 and aarch64. It would make things simple. >>>> >>>> The case of XENMEM_maximum_ram_page is interesting but it is not a >>>> problem in reality because the max physical address size is only 40-bit >>>> for aarch32 guests, so 32-bit are always enough to return the highest >>>> page in memory for 32-bit guests. >>> >>> You are aware that this isn't the guest's max page, but the host's? > > I can see now that you meant to say that, no matter what is the max > pseudo-physical address supported by the VM, XENMEM_maximum_ram_page is > supposed to return the max memory page, which could go above the > addressibility limit of the VM. > > So XENMEM_maximum_ram_page should potentially be able to return (1<<44) > even when called by an aarch32 VM, with max IPA 40-bit. > > I would imagine it could be useful if dom0 is 32-bit but domUs are > 64-bit on a 64-bit hypervisor (which I think it would be a very rare > configuration on ARM.) > > Then it looks like XENMEM_maximum_ram_page needs to be able to return a > value > 32-bit when called by a 32-bit guest. > > The hypercall ABI follows the ARM C calling convention, so a 64-bit > value should be returned using r0 and r1. But looking at > xen/arch/arm/traps.c:do_trap_hypercall, it doesn't seem it ever sets r1 > today. Only r0 is set, so effectively we only support 32-bit return > values on aarch32 and for aarch32 guests. > > In other words, today all hypercalls on ARM return 64-bit to 64-bit > guests and 32-bit to 32-bit guests. Which in the case of memory_op is > "technically" the correct thing to do because it matches the C > declaration in xen/include/xen/hypercall.h: > > extern long > do_memory_op( > unsigned long cmd, > XEN_GUEST_HANDLE_PARAM(void) arg); > > So... I guess the conclusion is that on ARM do_memory_op should return > "long" although it is not actually enough for a correct implementation > of XENMEM_maximum_ram_page for aarch32 guests ? For 32-bit guests the value needs to be saturated and allocations be restricted to the "visible" part of physical address space. Just like we do on x86. Except of course for Arm the question is in how far knowledge of the largest physical address in the system is actually relevant to guests (i.e. like for HVM on x86): This isn't transparent only for PV, or at least it better would be restricted to PV. Jan
On 17.12.2021 06:34, Juergen Gross wrote: > On 16.12.21 22:15, Stefano Stabellini wrote: >> On Thu, 16 Dec 2021, Stefano Stabellini wrote: >>> On Thu, 16 Dec 2021, Juergen Gross wrote: >>>> On 16.12.21 03:10, Stefano Stabellini wrote: >>>>> The case of XENMEM_maximum_ram_page is interesting but it is not a >>>>> problem in reality because the max physical address size is only 40-bit >>>>> for aarch32 guests, so 32-bit are always enough to return the highest >>>>> page in memory for 32-bit guests. >>>> >>>> You are aware that this isn't the guest's max page, but the host's? >> >> I can see now that you meant to say that, no matter what is the max >> pseudo-physical address supported by the VM, XENMEM_maximum_ram_page is >> supposed to return the max memory page, which could go above the >> addressibility limit of the VM. >> >> So XENMEM_maximum_ram_page should potentially be able to return (1<<44) >> even when called by an aarch32 VM, with max IPA 40-bit. >> >> I would imagine it could be useful if dom0 is 32-bit but domUs are >> 64-bit on a 64-bit hypervisor (which I think it would be a very rare >> configuration on ARM.) >> >> Then it looks like XENMEM_maximum_ram_page needs to be able to return a >> value > 32-bit when called by a 32-bit guest. >> >> The hypercall ABI follows the ARM C calling convention, so a 64-bit >> value should be returned using r0 and r1. But looking at >> xen/arch/arm/traps.c:do_trap_hypercall, it doesn't seem it ever sets r1 >> today. Only r0 is set, so effectively we only support 32-bit return >> values on aarch32 and for aarch32 guests. >> >> In other words, today all hypercalls on ARM return 64-bit to 64-bit >> guests and 32-bit to 32-bit guests. Which in the case of memory_op is >> "technically" the correct thing to do because it matches the C >> declaration in xen/include/xen/hypercall.h: >> >> extern long >> do_memory_op( >> unsigned long cmd, >> XEN_GUEST_HANDLE_PARAM(void) arg); >> >> So... I guess the conclusion is that on ARM do_memory_op should return >> "long" although it is not actually enough for a correct implementation >> of XENMEM_maximum_ram_page for aarch32 guests ? >> > > Hence my suggestion to check the return value of _all_ hypercalls to be > proper sign extended int values for 32-bit guests. This would fix all > potential issues without silently returning truncated values. Are we absolutely certain we have no other paths left where a possibly large unsigned values might be returned? In fact while compat_memory_op() does the necessary saturation, I've never been fully convinced of this being the best way of dealing with things. The range of error indicators is much smaller than [-INT_MIN,-1], so almost double the range of effectively unsigned values could be passed back fine. (Obviously we can't change existing interfaces, so this mem-op will need to remain as is.) Jan
On 17.12.21 08:45, Jan Beulich wrote: > On 17.12.2021 06:34, Juergen Gross wrote: >> On 16.12.21 22:15, Stefano Stabellini wrote: >>> On Thu, 16 Dec 2021, Stefano Stabellini wrote: >>>> On Thu, 16 Dec 2021, Juergen Gross wrote: >>>>> On 16.12.21 03:10, Stefano Stabellini wrote: >>>>>> The case of XENMEM_maximum_ram_page is interesting but it is not a >>>>>> problem in reality because the max physical address size is only 40-bit >>>>>> for aarch32 guests, so 32-bit are always enough to return the highest >>>>>> page in memory for 32-bit guests. >>>>> >>>>> You are aware that this isn't the guest's max page, but the host's? >>> >>> I can see now that you meant to say that, no matter what is the max >>> pseudo-physical address supported by the VM, XENMEM_maximum_ram_page is >>> supposed to return the max memory page, which could go above the >>> addressibility limit of the VM. >>> >>> So XENMEM_maximum_ram_page should potentially be able to return (1<<44) >>> even when called by an aarch32 VM, with max IPA 40-bit. >>> >>> I would imagine it could be useful if dom0 is 32-bit but domUs are >>> 64-bit on a 64-bit hypervisor (which I think it would be a very rare >>> configuration on ARM.) >>> >>> Then it looks like XENMEM_maximum_ram_page needs to be able to return a >>> value > 32-bit when called by a 32-bit guest. >>> >>> The hypercall ABI follows the ARM C calling convention, so a 64-bit >>> value should be returned using r0 and r1. But looking at >>> xen/arch/arm/traps.c:do_trap_hypercall, it doesn't seem it ever sets r1 >>> today. Only r0 is set, so effectively we only support 32-bit return >>> values on aarch32 and for aarch32 guests. >>> >>> In other words, today all hypercalls on ARM return 64-bit to 64-bit >>> guests and 32-bit to 32-bit guests. Which in the case of memory_op is >>> "technically" the correct thing to do because it matches the C >>> declaration in xen/include/xen/hypercall.h: >>> >>> extern long >>> do_memory_op( >>> unsigned long cmd, >>> XEN_GUEST_HANDLE_PARAM(void) arg); >>> >>> So... I guess the conclusion is that on ARM do_memory_op should return >>> "long" although it is not actually enough for a correct implementation >>> of XENMEM_maximum_ram_page for aarch32 guests ? >>> >> >> Hence my suggestion to check the return value of _all_ hypercalls to be >> proper sign extended int values for 32-bit guests. This would fix all >> potential issues without silently returning truncated values. > > Are we absolutely certain we have no other paths left where a possibly > large unsigned values might be returned? In fact while > compat_memory_op() does the necessary saturation, I've never been fully > convinced of this being the best way of dealing with things. The range > of error indicators is much smaller than [-INT_MIN,-1], so almost > double the range of effectively unsigned values could be passed back > fine. (Obviously we can't change existing interfaces, so this mem-op > will need to remain as is.) In fact libxenctrl tries do deal with this fact by wrapping a memory_op for a 32-bit environment into a multicall. This will work fine for a 32-bit Arm guest, as xen_ulong_t is a uint64 there. So do_memory_op should return long on Arm, yes. OTOH doing so will continue to be a problem in case a 32-bit guest doesn't use the multicall technique for handling possible 64-bit return values. So I continue to argue that on Arm the return value of a hypercall should be tested to fit into 32 bits. The only really clean alternative would be to have separate hypercall function classes for Arm 32- and 64-bit guests (which still could share most of the functions by letting those return "int"). This would allow to use the 64-bit variant even for 32-bit guests in multicall (fine as the return field is 64-bit wide), and a probably saturating compat version for the 32-bit guest direct hypercall. The needed adaptions in my series would be rather limited (an additional column in the hypercall table, a split which macro to use in do_trap_hypercall() on Arm depending on the bitness of the guest, the addition of the Arm compat variant of do_memory_op()). Thoughts? Juergen
Hi Juergen, On 15/12/2021 07:03, Juergen Gross wrote: > On 14.12.21 18:36, Julien Grall wrote: >> Hi, >> >> On 08/12/2021 15:55, Juergen Gross wrote: >>> Today most hypercall handlers have a return type of long, while the >>> compat ones return an int. There are a few exceptions from that rule, >>> however. >> >> So on Arm64, I don't think you can make use of the full 64-bit because >> a 32-bit domain would not be able to see the top 32-bit. >> >> In fact, this could potentially cause us some trouble (see [1]) in Xen. >> So it feels like the hypercalls should always return a 32-bit signed >> value on Arm. > > This would break hypercalls like XENMEM_maximum_ram_page which are able > to return larger values, right? > >> The other advantage is it would be clear that the top 32-bit are not >> usuable. Stefano, what do you think? > > Wouldn't it make more sense to check the return value to be a sign > extended 32-bit value for 32-bit guests in do_trap_hypercall() instead? > > The question is what to return if this is not the case. -EDOM? It looks like there was a lot of discussion afterwards. I will read everything and answer on the last part of the thread :). > >> >>> >>> Get rid of the exceptions by letting compat handlers always return int >>> and others always return long. >>> >>> For the compat hvm case use eax instead of rax for the stored result as >>> it should have been from the beginning. >>> >>> Additionally move some prototypes to include/asm-x86/hypercall.h >>> as they are x86 specific. Move the do_physdev_op() prototype from both >>> architecture dependant headers to the common one. Move the >>> compat_platform_op() prototype to the common header. >>> >>> Switch some non style compliant types (u32, s32, s64) to style compliant >>> ones. >> >> TBH, I think this should have been split because the modifications are >> done on lines that are untouched. >> >> The extra patch would have made the review easier (even if this patch >> is still quite small). > > I can split the patch if you want. I have already reviewed this patch. So it is not going to help me :). However, I would appreciate if in the future the coding style changes are separated at least when they are not meant to be untouched. Cheers,
Hi Stefano, On 16/12/2021 21:15, Stefano Stabellini wrote: > On Thu, 16 Dec 2021, Stefano Stabellini wrote: >> On Thu, 16 Dec 2021, Juergen Gross wrote: >>> On 16.12.21 03:10, Stefano Stabellini wrote: >>>> On Wed, 15 Dec 2021, Juergen Gross wrote: >>>>> On 14.12.21 18:36, Julien Grall wrote: >>>>>> Hi, >>>>>> >>>>>> On 08/12/2021 15:55, Juergen Gross wrote: >>>>>>> Today most hypercall handlers have a return type of long, while the >>>>>>> compat ones return an int. There are a few exceptions from that rule, >>>>>>> however. >>>>>> >>>>>> So on Arm64, I don't think you can make use of the full 64-bit because a >>>>>> 32-bit domain would not be able to see the top 32-bit. >>>>>> >>>>>> In fact, this could potentially cause us some trouble (see [1]) in Xen. >>>>>> So it feels like the hypercalls should always return a 32-bit signed >>>>>> value >>>>>> on Arm. >>>>> >>>>> This would break hypercalls like XENMEM_maximum_ram_page which are able >>>>> to return larger values, right? >>>>> >>>>>> The other advantage is it would be clear that the top 32-bit are not >>>>>> usuable. Stefano, what do you think? >>>>> >>>>> Wouldn't it make more sense to check the return value to be a sign >>>>> extended 32-bit value for 32-bit guests in do_trap_hypercall() instead? >>>>> >>>>> The question is what to return if this is not the case. -EDOM? >>>> >>>> >>>> I can see where Julien is coming from: we have been trying to keep the >>>> arm32 and arm64 ABIs identical since the beginning of the project. So, >>>> like Julien, my preference would be to always return 32-bit on ARM, both >>>> aarch32 and aarch64. It would make things simple. >>>> >>>> The case of XENMEM_maximum_ram_page is interesting but it is not a >>>> problem in reality because the max physical address size is only 40-bit >>>> for aarch32 guests, so 32-bit are always enough to return the highest >>>> page in memory for 32-bit guests. >>> >>> You are aware that this isn't the guest's max page, but the host's? > > I can see now that you meant to say that, no matter what is the max > pseudo-physical address supported by the VM, XENMEM_maximum_ram_page is > supposed to return the max memory page, which could go above the > addressibility limit of the VM. > > So XENMEM_maximum_ram_page should potentially be able to return (1<<44) > even when called by an aarch32 VM, with max IPA 40-bit. I am a bit confused with what you wrote. Yes, 32-bit VM can only address 40-bit, but this is only limiting its own (guest) physical address space. Such VM would still be able to map any host physical address (assuming GFN != MFN). > > I would imagine it could be useful if dom0 is 32-bit but domUs are > 64-bit on a 64-bit hypervisor (which I think it would be a very rare > configuration on ARM.) Looking at the implementation, the hypercall is accessible by any domain. IOW a domU 32-bit could read a wrong value. That said, it is not clear to me why an Arm or HVM x86 guest would want to read the value. Cheers,
Hi Juergen, On 17/12/2021 08:50, Juergen Gross wrote: > On 17.12.21 08:45, Jan Beulich wrote: >> On 17.12.2021 06:34, Juergen Gross wrote: >>> On 16.12.21 22:15, Stefano Stabellini wrote: >>>> On Thu, 16 Dec 2021, Stefano Stabellini wrote: >>>>> On Thu, 16 Dec 2021, Juergen Gross wrote: >>>>>> On 16.12.21 03:10, Stefano Stabellini wrote: >>>>>>> The case of XENMEM_maximum_ram_page is interesting but it is not a >>>>>>> problem in reality because the max physical address size is only >>>>>>> 40-bit >>>>>>> for aarch32 guests, so 32-bit are always enough to return the >>>>>>> highest >>>>>>> page in memory for 32-bit guests. >>>>>> >>>>>> You are aware that this isn't the guest's max page, but the host's? >>>> >>>> I can see now that you meant to say that, no matter what is the max >>>> pseudo-physical address supported by the VM, XENMEM_maximum_ram_page is >>>> supposed to return the max memory page, which could go above the >>>> addressibility limit of the VM. >>>> >>>> So XENMEM_maximum_ram_page should potentially be able to return (1<<44) >>>> even when called by an aarch32 VM, with max IPA 40-bit. >>>> >>>> I would imagine it could be useful if dom0 is 32-bit but domUs are >>>> 64-bit on a 64-bit hypervisor (which I think it would be a very rare >>>> configuration on ARM.) >>>> >>>> Then it looks like XENMEM_maximum_ram_page needs to be able to return a >>>> value > 32-bit when called by a 32-bit guest. >>>> >>>> The hypercall ABI follows the ARM C calling convention, so a 64-bit >>>> value should be returned using r0 and r1. But looking at >>>> xen/arch/arm/traps.c:do_trap_hypercall, it doesn't seem it ever sets r1 >>>> today. Only r0 is set, so effectively we only support 32-bit return >>>> values on aarch32 and for aarch32 guests. >>>> >>>> In other words, today all hypercalls on ARM return 64-bit to 64-bit >>>> guests and 32-bit to 32-bit guests. Which in the case of memory_op is >>>> "technically" the correct thing to do because it matches the C >>>> declaration in xen/include/xen/hypercall.h: >>>> >>>> extern long >>>> do_memory_op( >>>> unsigned long cmd, >>>> XEN_GUEST_HANDLE_PARAM(void) arg); >>>> >>>> So... I guess the conclusion is that on ARM do_memory_op should return >>>> "long" although it is not actually enough for a correct implementation >>>> of XENMEM_maximum_ram_page for aarch32 guests ? >>>> >>> >>> Hence my suggestion to check the return value of _all_ hypercalls to be >>> proper sign extended int values for 32-bit guests. This would fix all >>> potential issues without silently returning truncated values. >> >> Are we absolutely certain we have no other paths left where a possibly >> large unsigned values might be returned? In fact while >> compat_memory_op() does the necessary saturation, I've never been fully >> convinced of this being the best way of dealing with things. The range >> of error indicators is much smaller than [-INT_MIN,-1], so almost >> double the range of effectively unsigned values could be passed back >> fine. (Obviously we can't change existing interfaces, so this mem-op >> will need to remain as is.) > > In fact libxenctrl tries do deal with this fact by wrapping a memory_op > for a 32-bit environment into a multicall. This will work fine for a > 32-bit Arm guest, as xen_ulong_t is a uint64 there. > > So do_memory_op should return long on Arm, yes. OTOH doing so will > continue to be a problem in case a 32-bit guest doesn't use the > multicall technique for handling possible 64-bit return values. > > So I continue to argue that on Arm the return value of a hypercall > should be tested to fit into 32 bits. It would make sense. But what would you return if the value doesn't fit? > The only really clean alternative > would be to have separate hypercall function classes for Arm 32- and > 64-bit guests (which still could share most of the functions by letting > those return "int"). This would allow to use the 64-bit variant even for > 32-bit guests in multicall (fine as the return field is 64-bit wide), > and a probably saturating compat version for the 32-bit guest direct > hypercall. I am not entirely sure to understand this proposal. Can you clarify it? > > The needed adaptions in my series would be rather limited (an additional > column in the hypercall table, a split which macro to use in > do_trap_hypercall() on Arm depending on the bitness of the guest, the > addition of the Arm compat variant of do_memory_op()). Cheers,
On 17.12.21 11:41, Julien Grall wrote: > Hi Juergen, > > On 17/12/2021 08:50, Juergen Gross wrote: >> On 17.12.21 08:45, Jan Beulich wrote: >>> On 17.12.2021 06:34, Juergen Gross wrote: >>>> On 16.12.21 22:15, Stefano Stabellini wrote: >>>>> On Thu, 16 Dec 2021, Stefano Stabellini wrote: >>>>>> On Thu, 16 Dec 2021, Juergen Gross wrote: >>>>>>> On 16.12.21 03:10, Stefano Stabellini wrote: >>>>>>>> The case of XENMEM_maximum_ram_page is interesting but it is not a >>>>>>>> problem in reality because the max physical address size is only >>>>>>>> 40-bit >>>>>>>> for aarch32 guests, so 32-bit are always enough to return the >>>>>>>> highest >>>>>>>> page in memory for 32-bit guests. >>>>>>> >>>>>>> You are aware that this isn't the guest's max page, but the host's? >>>>> >>>>> I can see now that you meant to say that, no matter what is the max >>>>> pseudo-physical address supported by the VM, >>>>> XENMEM_maximum_ram_page is >>>>> supposed to return the max memory page, which could go above the >>>>> addressibility limit of the VM. >>>>> >>>>> So XENMEM_maximum_ram_page should potentially be able to return >>>>> (1<<44) >>>>> even when called by an aarch32 VM, with max IPA 40-bit. >>>>> >>>>> I would imagine it could be useful if dom0 is 32-bit but domUs are >>>>> 64-bit on a 64-bit hypervisor (which I think it would be a very rare >>>>> configuration on ARM.) >>>>> >>>>> Then it looks like XENMEM_maximum_ram_page needs to be able to >>>>> return a >>>>> value > 32-bit when called by a 32-bit guest. >>>>> >>>>> The hypercall ABI follows the ARM C calling convention, so a 64-bit >>>>> value should be returned using r0 and r1. But looking at >>>>> xen/arch/arm/traps.c:do_trap_hypercall, it doesn't seem it ever >>>>> sets r1 >>>>> today. Only r0 is set, so effectively we only support 32-bit return >>>>> values on aarch32 and for aarch32 guests. >>>>> >>>>> In other words, today all hypercalls on ARM return 64-bit to 64-bit >>>>> guests and 32-bit to 32-bit guests. Which in the case of memory_op is >>>>> "technically" the correct thing to do because it matches the C >>>>> declaration in xen/include/xen/hypercall.h: >>>>> >>>>> extern long >>>>> do_memory_op( >>>>> unsigned long cmd, >>>>> XEN_GUEST_HANDLE_PARAM(void) arg); >>>>> >>>>> So... I guess the conclusion is that on ARM do_memory_op should >>>>> return >>>>> "long" although it is not actually enough for a correct implementation >>>>> of XENMEM_maximum_ram_page for aarch32 guests ? >>>>> >>>> >>>> Hence my suggestion to check the return value of _all_ hypercalls to be >>>> proper sign extended int values for 32-bit guests. This would fix all >>>> potential issues without silently returning truncated values. >>> >>> Are we absolutely certain we have no other paths left where a possibly >>> large unsigned values might be returned? In fact while >>> compat_memory_op() does the necessary saturation, I've never been fully >>> convinced of this being the best way of dealing with things. The range >>> of error indicators is much smaller than [-INT_MIN,-1], so almost >>> double the range of effectively unsigned values could be passed back >>> fine. (Obviously we can't change existing interfaces, so this mem-op >>> will need to remain as is.) >> >> In fact libxenctrl tries do deal with this fact by wrapping a memory_op >> for a 32-bit environment into a multicall. This will work fine for a >> 32-bit Arm guest, as xen_ulong_t is a uint64 there. >> >> So do_memory_op should return long on Arm, yes. OTOH doing so will >> continue to be a problem in case a 32-bit guest doesn't use the >> multicall technique for handling possible 64-bit return values. >> >> So I continue to argue that on Arm the return value of a hypercall >> should be tested to fit into 32 bits. > > It would make sense. But what would you return if the value doesn't fit? I guess some errno value would be appropriate, like -EDOM, -ERANGE or -E2BIG. > >> The only really clean alternative >> would be to have separate hypercall function classes for Arm 32- and >> 64-bit guests (which still could share most of the functions by letting >> those return "int"). This would allow to use the 64-bit variant even for >> 32-bit guests in multicall (fine as the return field is 64-bit wide), >> and a probably saturating compat version for the 32-bit guest direct >> hypercall. > > I am not entirely sure to understand this proposal. Can you clarify it? 1. In patch 5 modify the hypercall table by adding another column, so instead of: +table: pv32 pv64 hvm32 hvm64 arm use: +table: pv32 pv64 hvm32 hvm64 arm32 arm64 2. Let most of the hypercalls just return int instead of long: +rettype: do int 3. Have an explicit 64-bit variant of memory_op (the 32-bit one is the compat variant existing already): +rettype: do64 long +prefix: do64 PREFIX_hvm +memory_op(unsigned long cmd, void *arg) 4. Use the appropriate calls in each column: +memory_op compat do64 hvm hvm compat do64 5. In the Arm hypercall trap handler do: if ( is_32bit_domain(current->domain) ) call_handlers_arm32(...); else call_handlers_arm64(...); 6. In the multicall handler always do: call_handlers_arm64(...); Juergen
On Fri, 17 Dec 2021, Juergen Gross wrote: > On 17.12.21 11:41, Julien Grall wrote: > > Hi Juergen, > > > > On 17/12/2021 08:50, Juergen Gross wrote: > > > On 17.12.21 08:45, Jan Beulich wrote: > > > > On 17.12.2021 06:34, Juergen Gross wrote: > > > > > On 16.12.21 22:15, Stefano Stabellini wrote: > > > > > > On Thu, 16 Dec 2021, Stefano Stabellini wrote: > > > > > > > On Thu, 16 Dec 2021, Juergen Gross wrote: > > > > > > > > On 16.12.21 03:10, Stefano Stabellini wrote: > > > > > > > > > The case of XENMEM_maximum_ram_page is interesting but it is > > > > > > > > > not a > > > > > > > > > problem in reality because the max physical address size is > > > > > > > > > only 40-bit > > > > > > > > > for aarch32 guests, so 32-bit are always enough to return the > > > > > > > > > highest > > > > > > > > > page in memory for 32-bit guests. > > > > > > > > > > > > > > > > You are aware that this isn't the guest's max page, but the > > > > > > > > host's? > > > > > > > > > > > > I can see now that you meant to say that, no matter what is the max > > > > > > pseudo-physical address supported by the VM, XENMEM_maximum_ram_page > > > > > > is > > > > > > supposed to return the max memory page, which could go above the > > > > > > addressibility limit of the VM. > > > > > > > > > > > > So XENMEM_maximum_ram_page should potentially be able to return > > > > > > (1<<44) > > > > > > even when called by an aarch32 VM, with max IPA 40-bit. > > > > > > > > > > > > I would imagine it could be useful if dom0 is 32-bit but domUs are > > > > > > 64-bit on a 64-bit hypervisor (which I think it would be a very rare > > > > > > configuration on ARM.) > > > > > > > > > > > > Then it looks like XENMEM_maximum_ram_page needs to be able to > > > > > > return a > > > > > > value > 32-bit when called by a 32-bit guest. > > > > > > > > > > > > The hypercall ABI follows the ARM C calling convention, so a 64-bit > > > > > > value should be returned using r0 and r1. But looking at > > > > > > xen/arch/arm/traps.c:do_trap_hypercall, it doesn't seem it ever sets > > > > > > r1 > > > > > > today. Only r0 is set, so effectively we only support 32-bit return > > > > > > values on aarch32 and for aarch32 guests. > > > > > > > > > > > > In other words, today all hypercalls on ARM return 64-bit to 64-bit > > > > > > guests and 32-bit to 32-bit guests. Which in the case of memory_op > > > > > > is > > > > > > "technically" the correct thing to do because it matches the C > > > > > > declaration in xen/include/xen/hypercall.h: > > > > > > > > > > > > extern long > > > > > > do_memory_op( > > > > > > unsigned long cmd, > > > > > > XEN_GUEST_HANDLE_PARAM(void) arg); > > > > > > > > > > > > So... I guess the conclusion is that on ARM do_memory_op should > > > > > > return > > > > > > "long" although it is not actually enough for a correct > > > > > > implementation > > > > > > of XENMEM_maximum_ram_page for aarch32 guests ? > > > > > > > > > > > > > > > > Hence my suggestion to check the return value of _all_ hypercalls to > > > > > be > > > > > proper sign extended int values for 32-bit guests. This would fix all > > > > > potential issues without silently returning truncated values. > > > > > > > > Are we absolutely certain we have no other paths left where a possibly > > > > large unsigned values might be returned? In fact while > > > > compat_memory_op() does the necessary saturation, I've never been fully > > > > convinced of this being the best way of dealing with things. The range > > > > of error indicators is much smaller than [-INT_MIN,-1], so almost > > > > double the range of effectively unsigned values could be passed back > > > > fine. (Obviously we can't change existing interfaces, so this mem-op > > > > will need to remain as is.) > > > > > > In fact libxenctrl tries do deal with this fact by wrapping a memory_op > > > for a 32-bit environment into a multicall. This will work fine for a > > > 32-bit Arm guest, as xen_ulong_t is a uint64 there. > > > > > > So do_memory_op should return long on Arm, yes. OTOH doing so will > > > continue to be a problem in case a 32-bit guest doesn't use the > > > multicall technique for handling possible 64-bit return values. > > > > > > So I continue to argue that on Arm the return value of a hypercall > > > should be tested to fit into 32 bits. > > > > It would make sense. But what would you return if the value doesn't fit? > > I guess some errno value would be appropriate, like -EDOM, -ERANGE or > -E2BIG. This seems to be better than the alternative below as it is a lot simpler. > > > The only really clean alternative > > > would be to have separate hypercall function classes for Arm 32- and > > > 64-bit guests (which still could share most of the functions by letting > > > those return "int"). This would allow to use the 64-bit variant even for > > > 32-bit guests in multicall (fine as the return field is 64-bit wide), > > > and a probably saturating compat version for the 32-bit guest direct > > > hypercall. > > > > I am not entirely sure to understand this proposal. Can you clarify it? > > 1. In patch 5 modify the hypercall table by adding another column, so > instead of: > +table: pv32 pv64 hvm32 hvm64 arm > use: > +table: pv32 pv64 hvm32 hvm64 arm32 arm64 > > 2. Let most of the hypercalls just return int instead of long: > +rettype: do int > > 3. Have an explicit 64-bit variant of memory_op (the 32-bit one is the > compat variant existing already): > +rettype: do64 long > +prefix: do64 PREFIX_hvm > +memory_op(unsigned long cmd, void *arg) > > 4. Use the appropriate calls in each column: > +memory_op compat do64 hvm hvm compat do64 > > 5. In the Arm hypercall trap handler do: > if ( is_32bit_domain(current->domain) ) > call_handlers_arm32(...); > else > call_handlers_arm64(...); > > 6. In the multicall handler always do: > call_handlers_arm64(...);
On Fri, 17 Dec 2021, Julien Grall wrote: > Hi Stefano, > > On 16/12/2021 21:15, Stefano Stabellini wrote: > > On Thu, 16 Dec 2021, Stefano Stabellini wrote: > > > On Thu, 16 Dec 2021, Juergen Gross wrote: > > > > On 16.12.21 03:10, Stefano Stabellini wrote: > > > > > On Wed, 15 Dec 2021, Juergen Gross wrote: > > > > > > On 14.12.21 18:36, Julien Grall wrote: > > > > > > > Hi, > > > > > > > > > > > > > > On 08/12/2021 15:55, Juergen Gross wrote: > > > > > > > > Today most hypercall handlers have a return type of long, while > > > > > > > > the > > > > > > > > compat ones return an int. There are a few exceptions from that > > > > > > > > rule, > > > > > > > > however. > > > > > > > > > > > > > > So on Arm64, I don't think you can make use of the full 64-bit > > > > > > > because a > > > > > > > 32-bit domain would not be able to see the top 32-bit. > > > > > > > > > > > > > > In fact, this could potentially cause us some trouble (see [1]) in > > > > > > > Xen. > > > > > > > So it feels like the hypercalls should always return a 32-bit > > > > > > > signed > > > > > > > value > > > > > > > on Arm. > > > > > > > > > > > > This would break hypercalls like XENMEM_maximum_ram_page which are > > > > > > able > > > > > > to return larger values, right? > > > > > > > > > > > > > The other advantage is it would be clear that the top 32-bit are > > > > > > > not > > > > > > > usuable. Stefano, what do you think? > > > > > > > > > > > > Wouldn't it make more sense to check the return value to be a sign > > > > > > extended 32-bit value for 32-bit guests in do_trap_hypercall() > > > > > > instead? > > > > > > > > > > > > The question is what to return if this is not the case. -EDOM? > > > > > > > > > > > > > > > I can see where Julien is coming from: we have been trying to keep the > > > > > arm32 and arm64 ABIs identical since the beginning of the project. So, > > > > > like Julien, my preference would be to always return 32-bit on ARM, > > > > > both > > > > > aarch32 and aarch64. It would make things simple. > > > > > > > > > > The case of XENMEM_maximum_ram_page is interesting but it is not a > > > > > problem in reality because the max physical address size is only > > > > > 40-bit > > > > > for aarch32 guests, so 32-bit are always enough to return the highest > > > > > page in memory for 32-bit guests. > > > > > > > > You are aware that this isn't the guest's max page, but the host's? > > > > I can see now that you meant to say that, no matter what is the max > > pseudo-physical address supported by the VM, XENMEM_maximum_ram_page is > > supposed to return the max memory page, which could go above the > > addressibility limit of the VM. > > > > So XENMEM_maximum_ram_page should potentially be able to return (1<<44) > > even when called by an aarch32 VM, with max IPA 40-bit. > > I am a bit confused with what you wrote. Yes, 32-bit VM can only address > 40-bit, but this is only limiting its own (guest) physical address space. Such > VM would still be able to map any host physical address (assuming GFN != MFN). I meant to say the same thing that you wrote, sorry it wasn't clear > > I would imagine it could be useful if dom0 is 32-bit but domUs are > > 64-bit on a 64-bit hypervisor (which I think it would be a very rare > > configuration on ARM.) > > Looking at the implementation, the hypercall is accessible by any domain. IOW > a domU 32-bit could read a wrong value. > > That said, it is not clear to me why an Arm or HVM x86 guest would want to > read the value. Right, indeed. AFAICT it is currently unused on ARM. Going through the code, the only caller that could potentially use it on ARM is libxl_domain_core_dump and xc_maximum_ram_page is called on the if ( !auto_translated_physmap ) code path.
Hi, On 18/12/2021 00:00, Stefano Stabellini wrote: > On Fri, 17 Dec 2021, Juergen Gross wrote: >> On 17.12.21 11:41, Julien Grall wrote: >>> Hi Juergen, >>> >>> On 17/12/2021 08:50, Juergen Gross wrote: >>>> On 17.12.21 08:45, Jan Beulich wrote: >>>>> On 17.12.2021 06:34, Juergen Gross wrote: >>>>>> On 16.12.21 22:15, Stefano Stabellini wrote: >>>>>>> On Thu, 16 Dec 2021, Stefano Stabellini wrote: >>>>>>>> On Thu, 16 Dec 2021, Juergen Gross wrote: >>>>>>>>> On 16.12.21 03:10, Stefano Stabellini wrote: >>>>>>>>>> The case of XENMEM_maximum_ram_page is interesting but it is >>>>>>>>>> not a >>>>>>>>>> problem in reality because the max physical address size is >>>>>>>>>> only 40-bit >>>>>>>>>> for aarch32 guests, so 32-bit are always enough to return the >>>>>>>>>> highest >>>>>>>>>> page in memory for 32-bit guests. >>>>>>>>> >>>>>>>>> You are aware that this isn't the guest's max page, but the >>>>>>>>> host's? >>>>>>> >>>>>>> I can see now that you meant to say that, no matter what is the max >>>>>>> pseudo-physical address supported by the VM, XENMEM_maximum_ram_page >>>>>>> is >>>>>>> supposed to return the max memory page, which could go above the >>>>>>> addressibility limit of the VM. >>>>>>> >>>>>>> So XENMEM_maximum_ram_page should potentially be able to return >>>>>>> (1<<44) >>>>>>> even when called by an aarch32 VM, with max IPA 40-bit. >>>>>>> >>>>>>> I would imagine it could be useful if dom0 is 32-bit but domUs are >>>>>>> 64-bit on a 64-bit hypervisor (which I think it would be a very rare >>>>>>> configuration on ARM.) >>>>>>> >>>>>>> Then it looks like XENMEM_maximum_ram_page needs to be able to >>>>>>> return a >>>>>>> value > 32-bit when called by a 32-bit guest. >>>>>>> >>>>>>> The hypercall ABI follows the ARM C calling convention, so a 64-bit >>>>>>> value should be returned using r0 and r1. But looking at >>>>>>> xen/arch/arm/traps.c:do_trap_hypercall, it doesn't seem it ever sets >>>>>>> r1 >>>>>>> today. Only r0 is set, so effectively we only support 32-bit return >>>>>>> values on aarch32 and for aarch32 guests. >>>>>>> >>>>>>> In other words, today all hypercalls on ARM return 64-bit to 64-bit >>>>>>> guests and 32-bit to 32-bit guests. Which in the case of memory_op >>>>>>> is >>>>>>> "technically" the correct thing to do because it matches the C >>>>>>> declaration in xen/include/xen/hypercall.h: >>>>>>> >>>>>>> extern long >>>>>>> do_memory_op( >>>>>>> unsigned long cmd, >>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg); >>>>>>> >>>>>>> So... I guess the conclusion is that on ARM do_memory_op should >>>>>>> return >>>>>>> "long" although it is not actually enough for a correct >>>>>>> implementation >>>>>>> of XENMEM_maximum_ram_page for aarch32 guests ? >>>>>>> >>>>>> >>>>>> Hence my suggestion to check the return value of _all_ hypercalls to >>>>>> be >>>>>> proper sign extended int values for 32-bit guests. This would fix all >>>>>> potential issues without silently returning truncated values. >>>>> >>>>> Are we absolutely certain we have no other paths left where a possibly >>>>> large unsigned values might be returned? In fact while >>>>> compat_memory_op() does the necessary saturation, I've never been fully >>>>> convinced of this being the best way of dealing with things. The range >>>>> of error indicators is much smaller than [-INT_MIN,-1], so almost >>>>> double the range of effectively unsigned values could be passed back >>>>> fine. (Obviously we can't change existing interfaces, so this mem-op >>>>> will need to remain as is.) >>>> >>>> In fact libxenctrl tries do deal with this fact by wrapping a memory_op >>>> for a 32-bit environment into a multicall. This will work fine for a >>>> 32-bit Arm guest, as xen_ulong_t is a uint64 there. >>>> >>>> So do_memory_op should return long on Arm, yes. OTOH doing so will >>>> continue to be a problem in case a 32-bit guest doesn't use the >>>> multicall technique for handling possible 64-bit return values. >>>> >>>> So I continue to argue that on Arm the return value of a hypercall >>>> should be tested to fit into 32 bits. >>> >>> It would make sense. But what would you return if the value doesn't fit? >> >> I guess some errno value would be appropriate, like -EDOM, -ERANGE or >> -E2BIG. > > This seems to be better than the alternative below as it is a lot > simpler. We would still need to special case XENMEM_maximum_reservation (or rework the implementation of the sub-op) because the value returned is an unsigned long. So technically, the unsigned value for -EDOM & co could be interpreted as the maximum host frame number. I also would like to see the hypercall returning 'int' when they are only meant to return 32-bit value. This will make easier to spot someone that decide to return a 64-bit value. > > >>>> The only really clean alternative >>>> would be to have separate hypercall function classes for Arm 32- and >>>> 64-bit guests (which still could share most of the functions by letting >>>> those return "int"). This would allow to use the 64-bit variant even for >>>> 32-bit guests in multicall (fine as the return field is 64-bit wide), >>>> and a probably saturating compat version for the 32-bit guest direct >>>> hypercall. >>> >>> I am not entirely sure to understand this proposal. Can you clarify it? >> >> 1. In patch 5 modify the hypercall table by adding another column, so >> instead of: >> +table: pv32 pv64 hvm32 hvm64 arm >> use: >> +table: pv32 pv64 hvm32 hvm64 arm32 arm64 >> >> 2. Let most of the hypercalls just return int instead of long: >> +rettype: do int >> >> 3. Have an explicit 64-bit variant of memory_op (the 32-bit one is the >> compat variant existing already): >> +rettype: do64 long >> +prefix: do64 PREFIX_hvm >> +memory_op(unsigned long cmd, void *arg) >> >> 4. Use the appropriate calls in each column: >> +memory_op compat do64 hvm hvm compat do64 >> >> 5. In the Arm hypercall trap handler do: >> if ( is_32bit_domain(current->domain) ) >> call_handlers_arm32(...); >> else >> call_handlers_arm64(...); >> >> 6. In the multicall handler always do: >> call_handlers_arm64(...); I am probably missing something. But why do we need to have separate call handlers for arm32/arm64? Cheers,
On Mon, 20 Dec 2021, Julien Grall wrote: > On 18/12/2021 00:00, Stefano Stabellini wrote: > > On Fri, 17 Dec 2021, Juergen Gross wrote: > > > On 17.12.21 11:41, Julien Grall wrote: > > > > Hi Juergen, > > > > > > > > On 17/12/2021 08:50, Juergen Gross wrote: > > > > > On 17.12.21 08:45, Jan Beulich wrote: > > > > > > On 17.12.2021 06:34, Juergen Gross wrote: > > > > > > > On 16.12.21 22:15, Stefano Stabellini wrote: > > > > > > > > On Thu, 16 Dec 2021, Stefano Stabellini wrote: > > > > > > > > > On Thu, 16 Dec 2021, Juergen Gross wrote: > > > > > > > > > > On 16.12.21 03:10, Stefano Stabellini wrote: > > > > > > > > > > > The case of XENMEM_maximum_ram_page is interesting but it > > > > > > > > > > > is > > > > > > > > > > > not a > > > > > > > > > > > problem in reality because the max physical address size > > > > > > > > > > > is > > > > > > > > > > > only 40-bit > > > > > > > > > > > for aarch32 guests, so 32-bit are always enough to return > > > > > > > > > > > the > > > > > > > > > > > highest > > > > > > > > > > > page in memory for 32-bit guests. > > > > > > > > > > > > > > > > > > > > You are aware that this isn't the guest's max page, but the > > > > > > > > > > host's? > > > > > > > > > > > > > > > > I can see now that you meant to say that, no matter what is the > > > > > > > > max > > > > > > > > pseudo-physical address supported by the VM, > > > > > > > > XENMEM_maximum_ram_page > > > > > > > > is > > > > > > > > supposed to return the max memory page, which could go above the > > > > > > > > addressibility limit of the VM. > > > > > > > > > > > > > > > > So XENMEM_maximum_ram_page should potentially be able to return > > > > > > > > (1<<44) > > > > > > > > even when called by an aarch32 VM, with max IPA 40-bit. > > > > > > > > > > > > > > > > I would imagine it could be useful if dom0 is 32-bit but domUs > > > > > > > > are > > > > > > > > 64-bit on a 64-bit hypervisor (which I think it would be a very > > > > > > > > rare > > > > > > > > configuration on ARM.) > > > > > > > > > > > > > > > > Then it looks like XENMEM_maximum_ram_page needs to be able to > > > > > > > > return a > > > > > > > > value > 32-bit when called by a 32-bit guest. > > > > > > > > > > > > > > > > The hypercall ABI follows the ARM C calling convention, so a > > > > > > > > 64-bit > > > > > > > > value should be returned using r0 and r1. But looking at > > > > > > > > xen/arch/arm/traps.c:do_trap_hypercall, it doesn't seem it ever > > > > > > > > sets > > > > > > > > r1 > > > > > > > > today. Only r0 is set, so effectively we only support 32-bit > > > > > > > > return > > > > > > > > values on aarch32 and for aarch32 guests. > > > > > > > > > > > > > > > > In other words, today all hypercalls on ARM return 64-bit to > > > > > > > > 64-bit > > > > > > > > guests and 32-bit to 32-bit guests. Which in the case of > > > > > > > > memory_op > > > > > > > > is > > > > > > > > "technically" the correct thing to do because it matches the C > > > > > > > > declaration in xen/include/xen/hypercall.h: > > > > > > > > > > > > > > > > extern long > > > > > > > > do_memory_op( > > > > > > > > unsigned long cmd, > > > > > > > > XEN_GUEST_HANDLE_PARAM(void) arg); > > > > > > > > > > > > > > > > So... I guess the conclusion is that on ARM do_memory_op should > > > > > > > > return > > > > > > > > "long" although it is not actually enough for a correct > > > > > > > > implementation > > > > > > > > of XENMEM_maximum_ram_page for aarch32 guests ? > > > > > > > > > > > > > > > > > > > > > > Hence my suggestion to check the return value of _all_ hypercalls > > > > > > > to > > > > > > > be > > > > > > > proper sign extended int values for 32-bit guests. This would fix > > > > > > > all > > > > > > > potential issues without silently returning truncated values. > > > > > > > > > > > > Are we absolutely certain we have no other paths left where a > > > > > > possibly > > > > > > large unsigned values might be returned? In fact while > > > > > > compat_memory_op() does the necessary saturation, I've never been > > > > > > fully > > > > > > convinced of this being the best way of dealing with things. The > > > > > > range > > > > > > of error indicators is much smaller than [-INT_MIN,-1], so almost > > > > > > double the range of effectively unsigned values could be passed back > > > > > > fine. (Obviously we can't change existing interfaces, so this mem-op > > > > > > will need to remain as is.) > > > > > > > > > > In fact libxenctrl tries do deal with this fact by wrapping a > > > > > memory_op > > > > > for a 32-bit environment into a multicall. This will work fine for a > > > > > 32-bit Arm guest, as xen_ulong_t is a uint64 there. > > > > > > > > > > So do_memory_op should return long on Arm, yes. OTOH doing so will > > > > > continue to be a problem in case a 32-bit guest doesn't use the > > > > > multicall technique for handling possible 64-bit return values. > > > > > > > > > > So I continue to argue that on Arm the return value of a hypercall > > > > > should be tested to fit into 32 bits. > > > > > > > > It would make sense. But what would you return if the value doesn't fit? > > > > > > I guess some errno value would be appropriate, like -EDOM, -ERANGE or > > > -E2BIG. > > > > This seems to be better than the alternative below as it is a lot > > simpler. > > We would still need to special case XENMEM_maximum_reservation (or rework the > implementation of the sub-op) because the value returned is an unsigned long. > So technically, the unsigned value for -EDOM & co could be interpreted as the > maximum host frame number. > > I also would like to see the hypercall returning 'int' when they are only > meant to return 32-bit value. This will make easier to spot someone that > decide to return a 64-bit value. I am completely aligned with you on both points. XENMEM_maximum_reservation is a bit of a distraction given that is unused (even unsupported?) on ARM. In general, to switch to "int" as return type we would have to (manually) check that all the sub-ops of a given hypercall return 32-bit values, right? Otherwise, how can we be sure that we don't start to silently truncate the top 32-bit on a sub-op on arm64? In theory we could use -Wconversion to automatically spot any truncations but unfortunately -Wconversion breaks the build at the moment.
On 20.12.21 18:14, Julien Grall wrote: > Hi, > > On 18/12/2021 00:00, Stefano Stabellini wrote: >> On Fri, 17 Dec 2021, Juergen Gross wrote: >>> On 17.12.21 11:41, Julien Grall wrote: >>>> Hi Juergen, >>>> >>>> On 17/12/2021 08:50, Juergen Gross wrote: >>>>> On 17.12.21 08:45, Jan Beulich wrote: >>>>>> On 17.12.2021 06:34, Juergen Gross wrote: >>>>>>> On 16.12.21 22:15, Stefano Stabellini wrote: >>>>>>>> On Thu, 16 Dec 2021, Stefano Stabellini wrote: >>>>>>>>> On Thu, 16 Dec 2021, Juergen Gross wrote: >>>>>>>>>> On 16.12.21 03:10, Stefano Stabellini wrote: >>>>>>>>>>> The case of XENMEM_maximum_ram_page is interesting but it is >>>>>>>>>>> not a >>>>>>>>>>> problem in reality because the max physical address size is >>>>>>>>>>> only 40-bit >>>>>>>>>>> for aarch32 guests, so 32-bit are always enough to return the >>>>>>>>>>> highest >>>>>>>>>>> page in memory for 32-bit guests. >>>>>>>>>> >>>>>>>>>> You are aware that this isn't the guest's max page, but the >>>>>>>>>> host's? >>>>>>>> >>>>>>>> I can see now that you meant to say that, no matter what is the max >>>>>>>> pseudo-physical address supported by the VM, >>>>>>>> XENMEM_maximum_ram_page >>>>>>>> is >>>>>>>> supposed to return the max memory page, which could go above the >>>>>>>> addressibility limit of the VM. >>>>>>>> >>>>>>>> So XENMEM_maximum_ram_page should potentially be able to return >>>>>>>> (1<<44) >>>>>>>> even when called by an aarch32 VM, with max IPA 40-bit. >>>>>>>> >>>>>>>> I would imagine it could be useful if dom0 is 32-bit but domUs are >>>>>>>> 64-bit on a 64-bit hypervisor (which I think it would be a very >>>>>>>> rare >>>>>>>> configuration on ARM.) >>>>>>>> >>>>>>>> Then it looks like XENMEM_maximum_ram_page needs to be able to >>>>>>>> return a >>>>>>>> value > 32-bit when called by a 32-bit guest. >>>>>>>> >>>>>>>> The hypercall ABI follows the ARM C calling convention, so a 64-bit >>>>>>>> value should be returned using r0 and r1. But looking at >>>>>>>> xen/arch/arm/traps.c:do_trap_hypercall, it doesn't seem it ever >>>>>>>> sets >>>>>>>> r1 >>>>>>>> today. Only r0 is set, so effectively we only support 32-bit return >>>>>>>> values on aarch32 and for aarch32 guests. >>>>>>>> >>>>>>>> In other words, today all hypercalls on ARM return 64-bit to 64-bit >>>>>>>> guests and 32-bit to 32-bit guests. Which in the case of memory_op >>>>>>>> is >>>>>>>> "technically" the correct thing to do because it matches the C >>>>>>>> declaration in xen/include/xen/hypercall.h: >>>>>>>> >>>>>>>> extern long >>>>>>>> do_memory_op( >>>>>>>> unsigned long cmd, >>>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg); >>>>>>>> >>>>>>>> So... I guess the conclusion is that on ARM do_memory_op should >>>>>>>> return >>>>>>>> "long" although it is not actually enough for a correct >>>>>>>> implementation >>>>>>>> of XENMEM_maximum_ram_page for aarch32 guests ? >>>>>>>> >>>>>>> >>>>>>> Hence my suggestion to check the return value of _all_ hypercalls to >>>>>>> be >>>>>>> proper sign extended int values for 32-bit guests. This would fix >>>>>>> all >>>>>>> potential issues without silently returning truncated values. >>>>>> >>>>>> Are we absolutely certain we have no other paths left where a >>>>>> possibly >>>>>> large unsigned values might be returned? In fact while >>>>>> compat_memory_op() does the necessary saturation, I've never been >>>>>> fully >>>>>> convinced of this being the best way of dealing with things. The >>>>>> range >>>>>> of error indicators is much smaller than [-INT_MIN,-1], so almost >>>>>> double the range of effectively unsigned values could be passed back >>>>>> fine. (Obviously we can't change existing interfaces, so this mem-op >>>>>> will need to remain as is.) >>>>> >>>>> In fact libxenctrl tries do deal with this fact by wrapping a >>>>> memory_op >>>>> for a 32-bit environment into a multicall. This will work fine for a >>>>> 32-bit Arm guest, as xen_ulong_t is a uint64 there. >>>>> >>>>> So do_memory_op should return long on Arm, yes. OTOH doing so will >>>>> continue to be a problem in case a 32-bit guest doesn't use the >>>>> multicall technique for handling possible 64-bit return values. >>>>> >>>>> So I continue to argue that on Arm the return value of a hypercall >>>>> should be tested to fit into 32 bits. >>>> >>>> It would make sense. But what would you return if the value doesn't >>>> fit? >>> >>> I guess some errno value would be appropriate, like -EDOM, -ERANGE or >>> -E2BIG. >> >> This seems to be better than the alternative below as it is a lot >> simpler. > > We would still need to special case XENMEM_maximum_reservation (or > rework the implementation of the sub-op) because the value returned is > an unsigned long. So technically, the unsigned value for -EDOM & co > could be interpreted as the maximum host frame number. I guess you meant XENMEM_maximum_ram_page. What about setting -EDOM only if the high 32 bits are not all the same? This would mean to clamp the highest RAM page to -EDOM in case the caller is interpreting it as an unsigned value. This would still be better than silently dropping the high bits, which could lead to a rather low page number instead. > I also would like to see the hypercall returning 'int' when they are > only meant to return 32-bit value. This will make easier to spot someone > that decide to return a 64-bit value. I guess this would need to include all hypercalls handled in common code? >>>>> The only really clean alternative >>>>> would be to have separate hypercall function classes for Arm 32- and >>>>> 64-bit guests (which still could share most of the functions by >>>>> letting >>>>> those return "int"). This would allow to use the 64-bit variant >>>>> even for >>>>> 32-bit guests in multicall (fine as the return field is 64-bit wide), >>>>> and a probably saturating compat version for the 32-bit guest direct >>>>> hypercall. >>>> >>>> I am not entirely sure to understand this proposal. Can you clarify it? >>> >>> 1. In patch 5 modify the hypercall table by adding another column, so >>> instead of: >>> +table: pv32 pv64 hvm32 hvm64 arm >>> use: >>> +table: pv32 pv64 hvm32 hvm64 arm32 arm64 >>> >>> 2. Let most of the hypercalls just return int instead of long: >>> +rettype: do int >>> >>> 3. Have an explicit 64-bit variant of memory_op (the 32-bit one is the >>> compat variant existing already): >>> +rettype: do64 long >>> +prefix: do64 PREFIX_hvm >>> +memory_op(unsigned long cmd, void *arg) >>> >>> 4. Use the appropriate calls in each column: >>> +memory_op compat do64 hvm hvm compat do64 >>> >>> 5. In the Arm hypercall trap handler do: >>> if ( is_32bit_domain(current->domain) ) >>> call_handlers_arm32(...); >>> else >>> call_handlers_arm64(...); >>> >>> 6. In the multicall handler always do: >>> call_handlers_arm64(...); > I am probably missing something. But why do we need to have separate > call handlers for arm32/arm64? How else could you have different functions called for 32- and 64-bit guests (other than doing the distinction in the called functions)? Juergen
Hi Juergen, On 21/12/2021 08:45, Juergen Gross wrote: > On 20.12.21 18:14, Julien Grall wrote: >> Hi, >> >> On 18/12/2021 00:00, Stefano Stabellini wrote: >>> On Fri, 17 Dec 2021, Juergen Gross wrote: >>>> On 17.12.21 11:41, Julien Grall wrote: >>>>> Hi Juergen, >>>>> >>>>> On 17/12/2021 08:50, Juergen Gross wrote: >>>>>> On 17.12.21 08:45, Jan Beulich wrote: >>>>>>> On 17.12.2021 06:34, Juergen Gross wrote: >>>>>>>> On 16.12.21 22:15, Stefano Stabellini wrote: >>>>>>>>> On Thu, 16 Dec 2021, Stefano Stabellini wrote: >>>>>>>>>> On Thu, 16 Dec 2021, Juergen Gross wrote: >>>>>>>>>>> On 16.12.21 03:10, Stefano Stabellini wrote: >>>>>>>>>>>> The case of XENMEM_maximum_ram_page is interesting but it is >>>>>>>>>>>> not a >>>>>>>>>>>> problem in reality because the max physical address size is >>>>>>>>>>>> only 40-bit >>>>>>>>>>>> for aarch32 guests, so 32-bit are always enough to return the >>>>>>>>>>>> highest >>>>>>>>>>>> page in memory for 32-bit guests. >>>>>>>>>>> >>>>>>>>>>> You are aware that this isn't the guest's max page, but the >>>>>>>>>>> host's? >>>>>>>>> >>>>>>>>> I can see now that you meant to say that, no matter what is the >>>>>>>>> max >>>>>>>>> pseudo-physical address supported by the VM, >>>>>>>>> XENMEM_maximum_ram_page >>>>>>>>> is >>>>>>>>> supposed to return the max memory page, which could go above the >>>>>>>>> addressibility limit of the VM. >>>>>>>>> >>>>>>>>> So XENMEM_maximum_ram_page should potentially be able to return >>>>>>>>> (1<<44) >>>>>>>>> even when called by an aarch32 VM, with max IPA 40-bit. >>>>>>>>> >>>>>>>>> I would imagine it could be useful if dom0 is 32-bit but domUs are >>>>>>>>> 64-bit on a 64-bit hypervisor (which I think it would be a very >>>>>>>>> rare >>>>>>>>> configuration on ARM.) >>>>>>>>> >>>>>>>>> Then it looks like XENMEM_maximum_ram_page needs to be able to >>>>>>>>> return a >>>>>>>>> value > 32-bit when called by a 32-bit guest. >>>>>>>>> >>>>>>>>> The hypercall ABI follows the ARM C calling convention, so a >>>>>>>>> 64-bit >>>>>>>>> value should be returned using r0 and r1. But looking at >>>>>>>>> xen/arch/arm/traps.c:do_trap_hypercall, it doesn't seem it ever >>>>>>>>> sets >>>>>>>>> r1 >>>>>>>>> today. Only r0 is set, so effectively we only support 32-bit >>>>>>>>> return >>>>>>>>> values on aarch32 and for aarch32 guests. >>>>>>>>> >>>>>>>>> In other words, today all hypercalls on ARM return 64-bit to >>>>>>>>> 64-bit >>>>>>>>> guests and 32-bit to 32-bit guests. Which in the case of memory_op >>>>>>>>> is >>>>>>>>> "technically" the correct thing to do because it matches the C >>>>>>>>> declaration in xen/include/xen/hypercall.h: >>>>>>>>> >>>>>>>>> extern long >>>>>>>>> do_memory_op( >>>>>>>>> unsigned long cmd, >>>>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg); >>>>>>>>> >>>>>>>>> So... I guess the conclusion is that on ARM do_memory_op should >>>>>>>>> return >>>>>>>>> "long" although it is not actually enough for a correct >>>>>>>>> implementation >>>>>>>>> of XENMEM_maximum_ram_page for aarch32 guests ? >>>>>>>>> >>>>>>>> >>>>>>>> Hence my suggestion to check the return value of _all_ >>>>>>>> hypercalls to >>>>>>>> be >>>>>>>> proper sign extended int values for 32-bit guests. This would >>>>>>>> fix all >>>>>>>> potential issues without silently returning truncated values. >>>>>>> >>>>>>> Are we absolutely certain we have no other paths left where a >>>>>>> possibly >>>>>>> large unsigned values might be returned? In fact while >>>>>>> compat_memory_op() does the necessary saturation, I've never been >>>>>>> fully >>>>>>> convinced of this being the best way of dealing with things. The >>>>>>> range >>>>>>> of error indicators is much smaller than [-INT_MIN,-1], so almost >>>>>>> double the range of effectively unsigned values could be passed back >>>>>>> fine. (Obviously we can't change existing interfaces, so this mem-op >>>>>>> will need to remain as is.) >>>>>> >>>>>> In fact libxenctrl tries do deal with this fact by wrapping a >>>>>> memory_op >>>>>> for a 32-bit environment into a multicall. This will work fine for a >>>>>> 32-bit Arm guest, as xen_ulong_t is a uint64 there. >>>>>> >>>>>> So do_memory_op should return long on Arm, yes. OTOH doing so will >>>>>> continue to be a problem in case a 32-bit guest doesn't use the >>>>>> multicall technique for handling possible 64-bit return values. >>>>>> >>>>>> So I continue to argue that on Arm the return value of a hypercall >>>>>> should be tested to fit into 32 bits. >>>>> >>>>> It would make sense. But what would you return if the value doesn't >>>>> fit? >>>> >>>> I guess some errno value would be appropriate, like -EDOM, -ERANGE or >>>> -E2BIG. >>> >>> This seems to be better than the alternative below as it is a lot >>> simpler. >> >> We would still need to special case XENMEM_maximum_reservation (or >> rework the implementation of the sub-op) because the value returned is >> an unsigned long. So technically, the unsigned value for -EDOM & co >> could be interpreted as the maximum host frame number. > > I guess you meant XENMEM_maximum_ram_page. Hmmmm yes. > > What about setting -EDOM only if the high 32 bits are not all the same? > This would mean to clamp the highest RAM page to -EDOM in case the > caller is interpreting it as an unsigned value. This would still be > better than silently dropping the high bits, which could lead to a > rather low page number instead. This feels like quite a hack. So I would prefer the low page number. I am interested to hear about the others. > >> I also would like to see the hypercall returning 'int' when they are >> only meant to return 32-bit value. This will make easier to spot >> someone that decide to return a 64-bit value. > > I guess this would need to include all hypercalls handled in common > code? Ideally yes. > >>>>>> The only really clean alternative >>>>>> would be to have separate hypercall function classes for Arm 32- and >>>>>> 64-bit guests (which still could share most of the functions by >>>>>> letting >>>>>> those return "int"). This would allow to use the 64-bit variant >>>>>> even for >>>>>> 32-bit guests in multicall (fine as the return field is 64-bit wide), >>>>>> and a probably saturating compat version for the 32-bit guest direct >>>>>> hypercall. >>>>> >>>>> I am not entirely sure to understand this proposal. Can you clarify >>>>> it? >>>> >>>> 1. In patch 5 modify the hypercall table by adding another column, so >>>> instead of: >>>> +table: pv32 pv64 hvm32 hvm64 arm >>>> use: >>>> +table: pv32 pv64 hvm32 hvm64 arm32 >>>> arm64 >>>> >>>> 2. Let most of the hypercalls just return int instead of long: >>>> +rettype: do int >>>> >>>> 3. Have an explicit 64-bit variant of memory_op (the 32-bit one is the >>>> compat variant existing already): >>>> +rettype: do64 long >>>> +prefix: do64 PREFIX_hvm >>>> +memory_op(unsigned long cmd, void *arg) >>>> >>>> 4. Use the appropriate calls in each column: >>>> +memory_op compat do64 hvm hvm compat do64 >>>> >>>> 5. In the Arm hypercall trap handler do: >>>> if ( is_32bit_domain(current->domain) ) >>>> call_handlers_arm32(...); >>>> else >>>> call_handlers_arm64(...); >>>> >>>> 6. In the multicall handler always do: >>>> call_handlers_arm64(...); >> I am probably missing something. But why do we need to have separate >> call handlers for arm32/arm64? > > How else could you have different functions called for 32- and 64-bit > guests (other than doing the distinction in the called functions)? At least for near future, I expect do_memory_op() to be the only one requiring special distinction. So my preference would be to handle the difference there rather than adding extra logic/indirection. Cheers,
diff --git a/xen/arch/arm/physdev.c b/xen/arch/arm/physdev.c index f9aa274dda..5a7593fa8f 100644 --- a/xen/arch/arm/physdev.c +++ b/xen/arch/arm/physdev.c @@ -11,7 +11,7 @@ #include <xen/hypercall.h> -int do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) +long do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { #ifdef CONFIG_HAS_PCI return pci_physdev_op(cmd, arg); diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index 7d102e0647..b01ea81373 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -221,8 +221,8 @@ long arch_do_domctl( case XEN_DOMCTL_shadow_op: ret = paging_domctl(d, &domctl->u.shadow_op, u_domctl, 0); if ( ret == -ERESTART ) - return hypercall_create_continuation(__HYPERVISOR_arch_1, - "h", u_domctl); + return hypercall_create_continuation( + __HYPERVISOR_paging_domctl_cont, "h", u_domctl); copyback = true; break; diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c index 1f04ffb272..9d3b193bad 100644 --- a/xen/arch/x86/hvm/hypercall.c +++ b/xen/arch/x86/hvm/hypercall.c @@ -120,8 +120,6 @@ static long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) [ __HYPERVISOR_ ## x ] = { (hypercall_fn_t *) do_ ## x, \ (hypercall_fn_t *) compat_ ## x } -#define do_arch_1 paging_domctl_continuation - static const struct { hypercall_fn_t *native, *compat; } hvm_hypercall_table[] = { @@ -154,11 +152,9 @@ static const struct { #ifdef CONFIG_HYPFS HYPERCALL(hypfs_op), #endif - HYPERCALL(arch_1) + HYPERCALL(paging_domctl_cont) }; -#undef do_arch_1 - #undef HYPERCALL #undef HVM_CALL #undef COMPAT_CALL @@ -296,7 +292,7 @@ int hvm_hypercall(struct cpu_user_regs *regs) #endif curr->hcall_compat = true; - regs->rax = hvm_hypercall_table[eax].compat(ebx, ecx, edx, esi, edi); + regs->eax = hvm_hypercall_table[eax].compat(ebx, ecx, edx, esi, edi); curr->hcall_compat = false; #ifndef NDEBUG diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c index 2370d31d3f..07e1a45ef5 100644 --- a/xen/arch/x86/hypercall.c +++ b/xen/arch/x86/hypercall.c @@ -75,7 +75,7 @@ const hypercall_args_t hypercall_args_table[NR_hypercalls] = ARGS(dm_op, 3), ARGS(hypfs_op, 5), ARGS(mca, 1), - ARGS(arch_1, 1), + ARGS(paging_domctl_cont, 1), }; #undef COMP diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c index dd6b2bdf6f..6cc2636bf4 100644 --- a/xen/arch/x86/mm/paging.c +++ b/xen/arch/x86/mm/paging.c @@ -21,6 +21,7 @@ #include <xen/init.h> #include <xen/guest_access.h> +#include <xen/hypercall.h> #include <asm/paging.h> #include <asm/shadow.h> #include <asm/p2m.h> @@ -756,7 +757,7 @@ int paging_domctl(struct domain *d, struct xen_domctl_shadow_op *sc, return shadow_domctl(d, sc, u_domctl); } -long paging_domctl_continuation(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) +long do_paging_domctl_cont(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) { struct xen_domctl op; struct domain *d; diff --git a/xen/arch/x86/pv/callback.c b/xen/arch/x86/pv/callback.c index 42a6aa0831..6d60263dbc 100644 --- a/xen/arch/x86/pv/callback.c +++ b/xen/arch/x86/pv/callback.c @@ -207,9 +207,9 @@ long do_set_callbacks(unsigned long event_address, #include <compat/callback.h> #include <compat/nmi.h> -static long compat_register_guest_callback(struct compat_callback_register *reg) +static int compat_register_guest_callback(struct compat_callback_register *reg) { - long ret = 0; + int ret = 0; struct vcpu *curr = current; fixup_guest_code_selector(curr->domain, reg->address.cs); @@ -256,10 +256,10 @@ static long compat_register_guest_callback(struct compat_callback_register *reg) return ret; } -static long compat_unregister_guest_callback( +static int compat_unregister_guest_callback( struct compat_callback_unregister *unreg) { - long ret; + int ret; switch ( unreg->type ) { @@ -283,9 +283,9 @@ static long compat_unregister_guest_callback( return ret; } -long compat_callback_op(int cmd, XEN_GUEST_HANDLE(void) arg) +int compat_callback_op(int cmd, XEN_GUEST_HANDLE(const_void) arg) { - long ret; + int ret; switch ( cmd ) { @@ -321,10 +321,10 @@ long compat_callback_op(int cmd, XEN_GUEST_HANDLE(void) arg) return ret; } -long compat_set_callbacks(unsigned long event_selector, - unsigned long event_address, - unsigned long failsafe_selector, - unsigned long failsafe_address) +int compat_set_callbacks(unsigned long event_selector, + unsigned long event_address, + unsigned long failsafe_selector, + unsigned long failsafe_address) { struct compat_callback_register event = { .type = CALLBACKTYPE_event, diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c index 8ba65178e9..e0d74365f2 100644 --- a/xen/arch/x86/pv/emul-priv-op.c +++ b/xen/arch/x86/pv/emul-priv-op.c @@ -22,12 +22,12 @@ #include <xen/domain_page.h> #include <xen/event.h> #include <xen/guest_access.h> +#include <xen/hypercall.h> #include <xen/iocap.h> #include <asm/amd.h> #include <asm/debugreg.h> #include <asm/hpet.h> -#include <asm/hypercall.h> #include <asm/mc146818rtc.h> #include <asm/pv/domain.h> #include <asm/pv/trace.h> diff --git a/xen/arch/x86/pv/hypercall.c b/xen/arch/x86/pv/hypercall.c index 16a77e3a35..7e99dbda34 100644 --- a/xen/arch/x86/pv/hypercall.c +++ b/xen/arch/x86/pv/hypercall.c @@ -40,8 +40,6 @@ #define COMPAT_CALL(x) HYPERCALL(x) #endif -#define do_arch_1 paging_domctl_continuation - const pv_hypercall_table_t pv_hypercall_table[] = { COMPAT_CALL(set_trap_table), HYPERCALL(mmu_update), @@ -102,11 +100,10 @@ const pv_hypercall_table_t pv_hypercall_table[] = { #endif HYPERCALL(mca), #ifndef CONFIG_PV_SHIM_EXCLUSIVE - HYPERCALL(arch_1), + HYPERCALL(paging_domctl_cont), #endif }; -#undef do_arch_1 #undef COMPAT_CALL #undef HYPERCALL diff --git a/xen/arch/x86/pv/iret.c b/xen/arch/x86/pv/iret.c index 29a2f7cc45..90946c4629 100644 --- a/xen/arch/x86/pv/iret.c +++ b/xen/arch/x86/pv/iret.c @@ -48,7 +48,7 @@ static void async_exception_cleanup(struct vcpu *curr) curr->arch.async_exception_state(trap).old_mask; } -unsigned long do_iret(void) +long do_iret(void) { struct cpu_user_regs *regs = guest_cpu_user_regs(); struct iret_context iret_saved; @@ -105,7 +105,7 @@ unsigned long do_iret(void) } #ifdef CONFIG_PV32 -unsigned int compat_iret(void) +int compat_iret(void) { struct cpu_user_regs *regs = guest_cpu_user_regs(); struct vcpu *v = current; diff --git a/xen/arch/x86/pv/misc-hypercalls.c b/xen/arch/x86/pv/misc-hypercalls.c index 5dade24726..aaaf70eb63 100644 --- a/xen/arch/x86/pv/misc-hypercalls.c +++ b/xen/arch/x86/pv/misc-hypercalls.c @@ -28,12 +28,16 @@ long do_set_debugreg(int reg, unsigned long value) return set_debugreg(current, reg, value); } -unsigned long do_get_debugreg(int reg) +long do_get_debugreg(int reg) { - unsigned long val; - int res = x86emul_read_dr(reg, &val, NULL); - - return res == X86EMUL_OKAY ? val : -ENODEV; + /* Avoid implementation defined behavior casting unsigned long to long. */ + union { + unsigned long val; + long ret; + } u; + int res = x86emul_read_dr(reg, &u.val, NULL); + + return res == X86EMUL_OKAY ? u.ret : -ENODEV; } long do_fpu_taskswitch(int set) diff --git a/xen/arch/x86/x86_64/platform_hypercall.c b/xen/arch/x86/x86_64/platform_hypercall.c index fbba893a47..f84252bac6 100644 --- a/xen/arch/x86/x86_64/platform_hypercall.c +++ b/xen/arch/x86/x86_64/platform_hypercall.c @@ -4,10 +4,10 @@ EMIT_FILE; +#include <xen/hypercall.h> #include <xen/lib.h> #include <compat/platform.h> -DEFINE_XEN_GUEST_HANDLE(compat_platform_op_t); #define xen_platform_op compat_platform_op #define xen_platform_op_t compat_platform_op_t #define do_platform_op(x) compat_platform_op(_##x) diff --git a/xen/common/argo.c b/xen/common/argo.c index eaea7ba888..bf6aac7655 100644 --- a/xen/common/argo.c +++ b/xen/common/argo.c @@ -2207,13 +2207,13 @@ do_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1, } #ifdef CONFIG_COMPAT -long -compat_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1, - XEN_GUEST_HANDLE_PARAM(void) arg2, unsigned long arg3, - unsigned long arg4) +int compat_argo_op(unsigned int cmd, + XEN_GUEST_HANDLE_PARAM(void) arg1, + XEN_GUEST_HANDLE_PARAM(void) arg2, + unsigned long arg3, unsigned long arg4) { struct domain *currd = current->domain; - long rc; + int rc; xen_argo_send_addr_t send_addr; xen_argo_iov_t iovs[XEN_ARGO_MAXIOV]; compat_argo_iov_t compat_iovs[XEN_ARGO_MAXIOV]; @@ -2267,7 +2267,7 @@ compat_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1, rc = sendv(currd, &send_addr.src, &send_addr.dst, iovs, niov, arg4); out: - argo_dprintk("<-compat_argo_op(%u)=%ld\n", cmd, rc); + argo_dprintk("<-compat_argo_op(%u)=%d\n", cmd, rc); return rc; } diff --git a/xen/common/kexec.c b/xen/common/kexec.c index c63db618a7..d7373233e1 100644 --- a/xen/common/kexec.c +++ b/xen/common/kexec.c @@ -1213,7 +1213,7 @@ static int kexec_status(XEN_GUEST_HANDLE_PARAM(void) uarg) return !!test_bit(bit, &kexec_flags); } -static int do_kexec_op_internal(unsigned long op, +static int do_kexec_op_internal(unsigned int op, XEN_GUEST_HANDLE_PARAM(void) uarg, bool_t compat) { @@ -1265,13 +1265,13 @@ static int do_kexec_op_internal(unsigned long op, return ret; } -long do_kexec_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) uarg) +long do_kexec_op(unsigned int op, XEN_GUEST_HANDLE_PARAM(void) uarg) { return do_kexec_op_internal(op, uarg, 0); } #ifdef CONFIG_COMPAT -int compat_kexec_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) uarg) +int compat_kexec_op(unsigned int op, XEN_GUEST_HANDLE_PARAM(void) uarg) { return do_kexec_op_internal(op, uarg, 1); } diff --git a/xen/include/asm-arm/hypercall.h b/xen/include/asm-arm/hypercall.h index 9fd13c6b2c..cadafd76c7 100644 --- a/xen/include/asm-arm/hypercall.h +++ b/xen/include/asm-arm/hypercall.h @@ -2,7 +2,6 @@ #define __ASM_ARM_HYPERCALL_H__ #include <public/domctl.h> /* for arch_do_domctl */ -int do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg); long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d, XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl); diff --git a/xen/include/asm-x86/hypercall.h b/xen/include/asm-x86/hypercall.h index e614f7c78c..9c0981defd 100644 --- a/xen/include/asm-x86/hypercall.h +++ b/xen/include/asm-x86/hypercall.h @@ -11,6 +11,8 @@ #include <public/arch-x86/xen-mca.h> /* for do_mca */ #include <asm/paging.h> +#define __HYPERVISOR_paging_domctl_cont __HYPERVISOR_arch_1 + typedef unsigned long hypercall_fn_t( unsigned long, unsigned long, unsigned long, unsigned long, unsigned long); @@ -88,7 +90,7 @@ do_set_debugreg( int reg, unsigned long value); -extern unsigned long +extern long do_get_debugreg( int reg); @@ -102,17 +104,13 @@ do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc); extern long do_update_va_mapping( unsigned long va, - u64 val64, + uint64_t val64, unsigned long flags); -extern long -do_physdev_op( - int cmd, XEN_GUEST_HANDLE_PARAM(void) arg); - extern long do_update_va_mapping_otherdomain( unsigned long va, - u64 val64, + uint64_t val64, unsigned long flags, domid_t domid); @@ -126,7 +124,7 @@ do_mmuext_op( extern long do_callback_op( int cmd, XEN_GUEST_HANDLE_PARAM(const_void) arg); -extern unsigned long +extern long do_iret( void); @@ -141,16 +139,18 @@ do_set_segment_base( unsigned int which, unsigned long base); +long do_nmi_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg); + +long do_xenpmu_op(unsigned int op, + XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg); + +long do_paging_domctl_cont(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl); + #ifdef CONFIG_COMPAT #include <compat/arch-x86/xen.h> #include <compat/physdev.h> -extern int -compat_physdev_op( - int cmd, - XEN_GUEST_HANDLE_PARAM(void) arg); - extern int compat_common_vcpu_op( int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg); @@ -161,17 +161,14 @@ extern int compat_mmuext_op( XEN_GUEST_HANDLE_PARAM(uint) pdone, unsigned int foreigndom); -extern int compat_platform_op( - XEN_GUEST_HANDLE_PARAM(void) u_xenpf_op); - -extern long compat_callback_op( - int cmd, XEN_GUEST_HANDLE(void) arg); +extern int compat_callback_op( + int cmd, XEN_GUEST_HANDLE(const_void) arg); extern int compat_update_va_mapping( - unsigned int va, u32 lo, u32 hi, unsigned int flags); + unsigned int va, uint32_t lo, uint32_t hi, unsigned int flags); extern int compat_update_va_mapping_otherdomain( - unsigned int va, u32 lo, u32 hi, unsigned int flags, domid_t domid); + unsigned int va, uint32_t lo, uint32_t hi, unsigned int flags, domid_t domid); DEFINE_XEN_GUEST_HANDLE(trap_info_compat_t); extern int compat_set_trap_table(XEN_GUEST_HANDLE(trap_info_compat_t) traps); @@ -180,13 +177,13 @@ extern int compat_set_gdt( XEN_GUEST_HANDLE_PARAM(uint) frame_list, unsigned int entries); extern int compat_update_descriptor( - u32 pa_lo, u32 pa_hi, u32 desc_lo, u32 desc_hi); + uint32_t pa_lo, uint32_t pa_hi, uint32_t desc_lo, uint32_t desc_hi); -extern unsigned int compat_iret(void); +extern int compat_iret(void); extern int compat_nmi_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg); -extern long compat_set_callbacks( +extern int compat_set_callbacks( unsigned long event_selector, unsigned long event_address, unsigned long failsafe_selector, unsigned long failsafe_address); diff --git a/xen/include/asm-x86/paging.h b/xen/include/asm-x86/paging.h index 308f1115dd..9cc5d383a4 100644 --- a/xen/include/asm-x86/paging.h +++ b/xen/include/asm-x86/paging.h @@ -234,9 +234,6 @@ int paging_domctl(struct domain *d, struct xen_domctl_shadow_op *sc, XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl, bool_t resuming); -/* Helper hypercall for dealing with continuations. */ -long paging_domctl_continuation(XEN_GUEST_HANDLE_PARAM(xen_domctl_t)); - /* Call when destroying a vcpu/domain */ void paging_vcpu_teardown(struct vcpu *v); int paging_teardown(struct domain *d); diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h index 30558d3c61..9266bc86e9 100644 --- a/xen/include/xen/hypercall.h +++ b/xen/include/xen/hypercall.h @@ -114,11 +114,6 @@ common_vcpu_op(int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg); -extern long -do_nmi_op( - unsigned int cmd, - XEN_GUEST_HANDLE_PARAM(void) arg); - extern long do_hvm_op( unsigned long op, @@ -126,13 +121,15 @@ do_hvm_op( extern long do_kexec_op( - unsigned long op, + unsigned int op, XEN_GUEST_HANDLE_PARAM(void) uarg); extern long do_xsm_op( XEN_GUEST_HANDLE_PARAM(void) u_xsm_op); +long do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg); + #ifdef CONFIG_ARGO extern long do_argo_op( unsigned int cmd, @@ -145,9 +142,6 @@ extern long do_argo_op( extern long do_xenoprof_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg); -extern long -do_xenpmu_op(unsigned int op, XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg); - extern long do_dm_op( domid_t domid, @@ -198,21 +192,27 @@ compat_sched_op( extern int compat_set_timer_op( - u32 lo, - s32 hi); + uint32_t lo, + int32_t hi); extern int compat_xsm_op( XEN_GUEST_HANDLE_PARAM(void) op); -extern int compat_kexec_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) uarg); +extern int compat_kexec_op(unsigned int op, XEN_GUEST_HANDLE_PARAM(void) uarg); DEFINE_XEN_GUEST_HANDLE(multicall_entry_compat_t); extern int compat_multicall( XEN_GUEST_HANDLE_PARAM(multicall_entry_compat_t) call_list, uint32_t nr_calls); +int compat_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg); + +typedef struct compat_platform_op compat_platform_op_t; +DEFINE_XEN_GUEST_HANDLE(compat_platform_op_t); +int compat_platform_op(XEN_GUEST_HANDLE_PARAM(compat_platform_op_t) u_xenpf_op); + #ifdef CONFIG_ARGO -extern long compat_argo_op( +extern int compat_argo_op( unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1, XEN_GUEST_HANDLE_PARAM(void) arg2,