[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 02/13] xen: harmonize return types of hypercall handlers
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.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |