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

From: Julien Grall <julien@xen.org>
To: Jan Beulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Julien Grall <jgrall@amazon.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] [PATCH 03/17] xen/mm: Move the MM types in a separate header
Date: Wed, 25 Mar 2020 18:09:43 +0000	[thread overview]
Message-ID: <993d82aa-9f19-0b27-a562-53f4c9b2a7a4@xen.org> (raw)
In-Reply-To: <80c98b3e-efa7-66e6-bd47-61bc0560f535@suse.com>

Hi Jan,

On 25/03/2020 15:00, Jan Beulich wrote:
> On 22.03.2020 17:14, julien@xen.org wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> 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@xensource.com>
>> - *
>> - *                         +---------------------+
>> - *                          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