On 24.08.2021 15:03, Costin Lupu wrote: > On 8/23/21 8:16 PM, Julien Grall wrote: >> On 20/08/2021 10:26, Jan Beulich wrote: >>> On 20.08.2021 11:08, Julien Grall wrote: >>>> On 20/08/2021 08:44, Costin Lupu wrote: >>>>> On 8/20/21 9:52 AM, Jan Beulich wrote: >>>>>>> --- /dev/null >>>>>>> +++ b/xen/include/public/page.h >>>>>>> @@ -0,0 +1,36 @@ >>>>>>> +/****************************************************************************** >>>>>>> >>>>>>> + * page.h >>>>>>> + * >>>>>>> + * Page definitions for accessing guests memory >>>>>>> + * >>>>>>> + * Permission is hereby granted, free of charge, to any person >>>>>>> obtaining a copy >>>>>>> + * of this software and associated documentation files (the >>>>>>> "Software"), to >>>>>>> + * deal in the Software without restriction, including without >>>>>>> limitation the >>>>>>> + * rights to use, copy, modify, merge, publish, distribute, >>>>>>> sublicense, and/or >>>>>>> + * sell copies of the Software, and to permit persons to whom the >>>>>>> Software is >>>>>>> + * furnished to do so, subject to the following conditions: >>>>>>> + * >>>>>>> + * The above copyright notice and this permission notice shall be >>>>>>> included in >>>>>>> + * all copies or substantial portions of the Software. >>>>>>> + * >>>>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY >>>>>>> KIND, EXPRESS OR >>>>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF >>>>>>> MERCHANTABILITY, >>>>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO >>>>>>> EVENT SHALL THE >>>>>>> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES >>>>>>> OR OTHER >>>>>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR >>>>>>> OTHERWISE, ARISING >>>>>>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR >>>>>>> OTHER >>>>>>> + * DEALINGS IN THE SOFTWARE. >>>>>>> + * >>>>>>> + * Copyright (c) 2021, Costin Lupu >>>>>>> + */ >>>>>>> + >>>>>>> +#ifndef __XEN_PUBLIC_PAGE_H__ >>>>>>> +#define __XEN_PUBLIC_PAGE_H__ >>>>>>> + >>>>>>> +#include "xen.h" >>>>>>> + >>>>>>> +#define XEN_PAGE_SHIFT 12 >>>>>>> +#define XEN_PAGE_SIZE (xen_mk_long(1) << XEN_PAGE_SHIFT) >>>> >>>> This will use UL whereas on Arm a page frame should always be 64-bit >>>> regardless the bitness. Shouldn't this be converted to use xen_ulong_t >>>> instead? >>> >>> As pointed out on v1, XEN_PAGE_SIZE would better not end up as a >>> value of signed type, for ... >> >> Did you mean "not end up as a value of **unsigned** type"... >> >>> >>>>>>> +#define XEN_PAGE_MASK (~(XEN_PAGE_SIZE - 1)) >>> >>> ... this to suitably sign-extend to wider types is necessary. >> >> ... because, if I am not mistaken, the sign-extension wouldn't happen >> with unsigned type. But then on v1 you wrote: >> >> "Imo the smallest type this should evaluate to is xen_ulong_t" >> >> Which I interpreted as this value should be 64-bit on Arm32. If this not >> what you meant then I am lost. >> >>> >>> Also unless you expect someone to use typeof(XEN_PAGE_SIZE) I'm >>> afraid I don't see where the constant being long vs xen_long_t >>> (if such existed) might matter. >>> Otoh perhaps xen_mk_ulong() would >>> better have produced a xen_ulong_t typed values in the first >>> place, but I'm afraid we can't alter the existing macro. >> >> We can create a new one. >> >>>> Our stable ABI has not been designed with multiple page granularity in >>>> mind. We could introduce a hypercall to query the page size used by the >>>> ABI. But then, I don't think we have the full picture of how this is >>>> going to pan out (I haven't try to use another page size on Xen yet). >>>> >>>> I think we have three choices here: >>>> 1) Stick with the existing definition in the tools >>>> 2) Move the definition in the public headers and only expose them to >>>> the tools. >>>> 3) Query the page size via a new hypervisor >>>> >>>> As I wrote above, 3) is going to take some time to get it right. So the >>>> question here is whether 2) is temporarily better than 1). >>> >>> Because I understand 3) is some way out, and because I think 2) is >>> better than 1), I wrote "might be an option" for what you call 2). >>> But I could see people (Andrew for example) to take a different >>> position and object to such a temporary measure. >> >> I think we need to make a decision so Costin doesn't keep sending >> version on something that can't be merged. What does the others thinks? > > From what I understood, in his last reply to 'stubdom: foreignmemory: > Fix build after 0dbb4be739c5' thread, Andrew was OK with solution 2).
I agree it can be read this way. Jan
