[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 1/4] public: Add page related definitions for accessing guests memory
Hi Jan, 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? Cheers, -- Julien Grall
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |