Re: [PATCH v2 1/4] public: Add page related definitions for accessing guests memory

[Julien Grall <julien@xxxxxxx>]

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