From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v3 3/5] xen, common: add the XEN_DOMCTL_memory_mapping hypercall Date: Sat, 15 Mar 2014 22:32:46 +0000 Message-ID: <5324D50E.2090301@linaro.org> References: <1394914286-29713-1-git-send-email-avanzini.arianna@gmail.com> <1394914286-29713-4-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: <1394914286-29713-4-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, viktor.kleinik@globallogic.com List-Id: xen-devel@lists.xenproject.org Hello Arianna, Thank for the patch. On 15/03/14 20:11, Arianna Avanzini wrote: > diff --git a/xen/arch/arm/cpu.c b/xen/arch/arm/cpu.c > index afc564f..28f2611 100644 > --- a/xen/arch/arm/cpu.c > +++ b/xen/arch/arm/cpu.c > @@ -17,6 +17,8 @@ > > #include > > +unsigned int paddr_bits __read_mostly = 40; > + I would prefer a define for paddr_bits on ARM rather than creating a new variable. Something like #define paddr_bits PADDR_BITS in asm-arm/p2m.h Furthermore, you should not hardcode 40, you must use PADDR_BITS. [..] > diff --git a/xen/common/domctl.c b/xen/common/domctl.c > index 7cf610a..b18fd46 100644 > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -818,6 +818,75 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) [..] > + if ( ret ) > + { > + printk(XENLOG_G_WARNING > + "memory_map: fail: dom%d gfn=%lx mfn=%lx\n", > + d->domain_id, gfn, mfn); > + if ( iomem_deny_access(d, mfn, mfn_end - 1) && > + is_hardware_domain(current->domain) ) > + printk(XENLOG_ERR > + "memory_map: failed to deny dom%d access " > + "to [%lx,%lx]\n", > + d->domain_id, mfn, mfn_end - 1); > + } > + } > + } > + else Hard tab here. You should use space. > + { > + printk(XENLOG_G_INFO > + "memory_map: remove: dom%d gfn=%lx mfn=%lx nr=%lx\n", > + d->domain_id, gfn, mfn, nr_mfns); > + > + add = unmap_mmio_regions(d, gfn, gfn_end - 1, mfn); > + ret = iomem_deny_access(d, mfn, mfn_end - 1); > + if ( ret && add ) > + ret = -EIO; > + if ( ret && is_hardware_domain(current->domain) ) > + printk(XENLOG_ERR > + "memory_map: error %ld %s dom%d access to [%lx,%lx]\n", > + ret, add ? "removing" : "denying", d->domain_id, > + mfn, mfn_end - 1); > + } > + } > + break; > + > case XEN_DOMCTL_settimeoffset: > { > domain_set_time_offset(d, op->u.settimeoffset.time_offset_seconds); > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h > index 3b39c45..00e3bba 100644 > --- a/xen/include/asm-arm/p2m.h > +++ b/xen/include/asm-arm/p2m.h > @@ -88,6 +88,10 @@ int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end); > * address maddr. */ > int map_mmio_regions(struct domain *d, paddr_t start_gaddr, > paddr_t end_gaddr, paddr_t maddr); > +int unmap_mmio_regions(struct domain *d, > + unsigned long start_gfn, > + unsigned long end_gfn, > + unsigned long mfn); > > int guest_physmap_add_entry(struct domain *d, > unsigned long gfn, > diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h > index 06e638f..4869a75 100644 > --- a/xen/include/asm-arm/processor.h > +++ b/xen/include/asm-arm/processor.h > @@ -119,6 +119,8 @@ > > #include > > +extern unsigned int paddr_bits; > + > struct cpuinfo_arm { > union { > uint32_t bits; > diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h > index f4e7253..91ef110 100644 > --- a/xen/include/asm-x86/p2m.h > +++ b/xen/include/asm-x86/p2m.h > @@ -657,6 +657,16 @@ void p2m_flush(struct vcpu *v, struct p2m_domain *p2m); > /* Flushes all nested p2m tables */ > void p2m_flush_nestedp2m(struct domain *d); > > +/* Map MMIO regions in the p2m: start_gaddr and end_gaddr is the range > + * in the guest physical address space to map, starting from the machine > + * address maddr. */ > +int map_mmio_regions(struct domain *d, paddr_t start_gaddr, > + paddr_t end_gaddr, paddr_t maddr); > +int unmap_mmio_regions(struct domain *d, > + unsigned long start_gfn, > + unsigned long end_gfn, > + unsigned long mfn); > + The both functions are used on common code. I would prefer to define the prototype only once in common header (e.g in include/xen). At the same time, it's not necessary to give the full addresses on argument. GFN, MFN is enough. I think we already talk about it on V1. I said it wasn't urgent, but as the function is common now ... :) Regards, -- Julien Grall