[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 3/5] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
>>> On 15.03.14 at 21:11, Arianna Avanzini <avanzini.arianna@xxxxxxxxx> wrote: > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -1591,6 +1591,45 @@ unsigned long paging_gva_to_gfn(struct vcpu *v, > return hostmode->gva_to_gfn(v, hostp2m, va, pfec); > } > > +int map_mmio_regions(struct domain *d, > + paddr_t start_gaddr, > + paddr_t end_gaddr, > + paddr_t maddr) > +{ > + int i, ret = 0; "i" was unsigned long in the original code, and I don't see any proof of either the signedness or the width change being correct. > + unsigned long gfn = paddr_to_pfn(start_gaddr); > + unsigned long nr_mfns = paddr_to_pfn(end_gaddr) - > paddr_to_pfn(start_gaddr); So this implies the range is exclusive of end_gaddr. > + unsigned long mfn = paddr_to_pfn(maddr); > + > + if ( !paging_mode_translate(d) ) > + return 0; > + > + for ( i = 0; !ret && i < nr_mfns; i++ ) > + if ( !set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i)) ) > + ret = -EIO; > + while ( i-- ) > + clear_mmio_p2m_entry(d, gfn + i); So you're clearing all entries unconditionally again right away? This surely wants to be inside an "if ( ret )". > +int unmap_mmio_regions(struct domain *d, > + unsigned long start_gfn, > + unsigned long end_gfn, > + unsigned long mfn) > +{ > + unsigned long nr_mfns = end_gfn - start_gfn; > + int i, ret = 0; See above. > + > + if ( !paging_mode_translate(d) ) > + return 0; > + > + for ( i = 0; i < nr_mfns; i++ ) > + ret |= !clear_mmio_p2m_entry(d, start_gfn + i); With this you pretty clearly want the function to have bool_t as its return type. > + case XEN_DOMCTL_memory_mapping: > + { > + unsigned long gfn = op->u.memory_mapping.first_gfn; > + unsigned long mfn = op->u.memory_mapping.first_mfn; > + unsigned long nr_mfns = op->u.memory_mapping.nr_mfns; > + unsigned long mfn_end = mfn + nr_mfns; > + unsigned long gfn_end = gfn + nr_mfns; > + int add = op->u.memory_mapping.add_mapping; > + long int ret; > + > + ret = -EINVAL; > + if ( (mfn_end - 1) < mfn || /* wrap? */ > + ((mfn | (mfn_end - 1)) >> (paddr_bits - PAGE_SHIFT)) || > + (gfn_end - 1) < gfn ) /* wrap? */ > + return ret; > + > + ret = -EPERM; > + if ( !iomem_access_permitted(current->domain, mfn, mfn_end - 1) ) > + return ret; > + > + ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end - 1, add); > + if ( ret ) > + return ret; > + > + if ( add ) > + { > + printk(XENLOG_G_INFO > + "memory_map: add: dom%d gfn=%lx mfn=%lx nr=%lx\n", > + d->domain_id, gfn, mfn, nr_mfns); > + ret = iomem_permit_access(d, mfn, mfn_end - 1); > + if ( !ret ) > + { > + ret = map_mmio_regions(d, PAGE_ALIGN(pfn_to_paddr(gfn)), > + PAGE_ALIGN( > + pfn_to_paddr(gfn_end)) - 1, Contrary to the above, this implies the range to inclusive of the ending address. Furthermore, PAGE_ALIGN(pfn_to_paddr()) is rather pointless a construct - the outer PAGE_ALIGN() can't possibly have any effect. > +/* 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); Please have the two interfaces be consistent: Either both take frame numbers only (preferred), or both take addresses only. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |