Re: [Xen-devel] [PATCH 03/17] xen/mm: Move the MM types in a separate header

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 25/03/2020 15:00, Jan Beulich wrote:
> On 22.03.2020 17:14, julien@xxxxxxx wrote:
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > It is getting incredibly difficult to use typesafe GFN/MFN/PFN in the
> > headers because of circular dependency. For instance, asm-x86/page.h
> > cannot include xen/mm.h.
> >
> > In order to convert more code to use typesafe, the types are now moved
> > in a separate header that requires only a few dependencies.
> We definitely need to do this, so thanks for investing the
> time. I think though that we want to settle up front (and
> perhaps record in a comment in the new header) what is or
> is not suitable to go into the new header. After all you're
> moving not just type definitions, but also simple helper
> functions.
I am expecting headers to use the typesafe helpers (such mfn_add) in the 
long term. So I would like the new header to contain the type 
definitions and any wrappers that would turn 'generic' operations safe.
I am not entirely sure yet how to formalize the rules in the header. Any 
ideas?
> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -1,50 +1,7 @@
> >   
> > /******************************************************************************
> >    * include/xen/mm.h
> >    *
> > - * Definitions for memory pages, frame numbers, addresses, allocations, etc.
> > - *
> >    * Copyright (c) 2002-2006, K A Fraser <keir@xxxxxxxxxxxxx>
> > - *
> > - *                         +---------------------+
> > - *                          Xen Memory Management
> > - *                         +---------------------+
> > - *
> > - * Xen has to handle many different address spaces.  It is important not to
> > - * get these spaces mixed up.  The following is a consistent terminology which
> > - * should be adhered to.
> > - *
> > - * mfn: Machine Frame Number
> > - *   The values Xen puts into its own pagetables.  This is the host physical
> > - *   memory address space with RAM, MMIO etc.
> > - *
> > - * gfn: Guest Frame Number
> > - *   The values a guest puts in its own pagetables.  For an auto-translated
> > - *   guest (hardware assisted with 2nd stage translation, or shadowed), gfn !=
> > - *   mfn.  For a non-translated guest which is aware of Xen, gfn == mfn.
> > - *
> > - * pfn: Pseudophysical Frame Number
> > - *   A linear idea of a guest physical address space. For an auto-translated
> > - *   guest, pfn == gfn while for a non-translated guest, pfn != gfn.
> > - *
> > - * dfn: Device DMA Frame Number (definitions in include/xen/iommu.h)
> > - *   The linear frame numbers of device DMA address space. All initiators for
> > - *   (i.e. all devices assigned to) a guest share a single DMA address space
> > - *   and, by default, Xen will ensure dfn == pfn.
> > - *
> > - * WARNING: Some of these terms have changed over time while others have been
> > - * used inconsistently, meaning that a lot of existing code does not match the
> > - * definitions above.  New code should use these terms as described here, and
> > - * over time older code should be corrected to be consistent.
> > - *
> > - * An incomplete list of larger work area:
> > - * - Phase out the use of 'pfn' from the x86 pagetable code.  Callers should
> > - *   know explicitly whether they are talking about mfns or gfns.
> > - * - Phase out the use of 'pfn' from the ARM mm code.  A cursory glance
> > - *   suggests that 'mfn' and 'pfn' are currently used interchangeably, where
> > - *   'mfn' is the appropriate term to use.
> > - * - Phase out the use of gpfn/gmfn where pfn/mfn are meant.  This excludes
> > - *   the x86 shadow code, which uses gmfn/smfn pairs with different,
> > - *   documented, meanings.
> >    */
> >   
> >   #ifndef __XEN_MM_H__
> > @@ -54,100 +11,11 @@
> >   #include <xen/types.h>
> >   #include <xen/list.h>
> >   #include <xen/spinlock.h>
> > -#include <xen/typesafe.h>
> >   #include <xen/kernel.h>
> > +#include <xen/mm_types.h>
> Is there anything left in the header here which requires the
> explicit inclusion of xen/kernel.h?
The header was introduced for the sole purpose of the typesafe version 
of the min/max helpers. So it should be possible to drop the include.
I will have a look and remove it if we can.

Cheers,

--
Julien Grall