From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v8 10/14] xen/common: move the memory_mapping DOMCTL hypercall to common code Date: Sun, 25 May 2014 17:42:39 +0100 Message-ID: <53821D7F.4040701@linaro.org> References: <1401015115-7610-1-git-send-email-avanzini.arianna@gmail.com> <1401015115-7610-11-git-send-email-avanzini.arianna@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1401015115-7610-11-git-send-email-avanzini.arianna@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Arianna Avanzini , xen-devel@lists.xen.org Cc: julien.grall@citrix.com, paolo.valente@unimore.it, keir@xen.org, stefano.stabellini@eu.citrix.com, tim@xen.org, dario.faggioli@citrix.com, Ian.Jackson@eu.citrix.com, Ian.Campbell@eu.citrix.com, etrudeau@broadcom.com, JBeulich@suse.com, andrew.cooper3@citrix.com, viktor.kleinik@globallogic.com List-Id: xen-devel@lists.xenproject.org Hi Arianna, On 25/05/14 11:51, Arianna Avanzini wrote: > +#define mfn_t unsigned long You should use typedef rather than define a type with a define... > +#include > + That would make more sense to include asm/p2m.h in xen/p2m-common.h rather than the invert. This will avoid issue such as, a source file is trying to include xen/p2m-common.h directly without adding asm/p2m.h before. This will result to a compilation issue. Anyway, if the maintainers are fine with this solution, let's stick > /* > diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h > new file mode 100644 > index 0000000..8760b9d > --- /dev/null > +++ b/xen/include/xen/p2m-common.h > @@ -0,0 +1,16 @@ > +#ifndef _XEN_P2M_COMMON_H > +#define _XEN_P2M_COMMON_H > + > +/* Map MMIO regions in the p2m: start_gfn and nr_mfns describe the range > + * in the guest physical address space to map, starting from the machine > + * frame number mfn. */ > +int map_mmio_regions(struct domain *d, > + unsigned long start_gfn, > + unsigned long nr_mfns, > + mfn_t mfn); > +int unmap_mmio_regions(struct domain *d, > + unsigned long start_gfn, > + unsigned long nr_mfns, > + mfn_t mfn); I don't like the idea to expose mfn_t and _mfn to the common code. All the other interfaces (i.e guest_physmap_*) are using unsigned long here. Why shouldn't we use the same thing here? Hence, this is not consistent with the other P2M interface. Regards, -- Julien Grall