| Message ID | 1465344927-3557-1-git-send-email-edgar.iglesias@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 08, 2016 at 02:15:27AM +0200, Edgar E. Iglesias wrote: > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > Rename map_regions_rw_cache to map_regions_cache and make it use > p2m.default_access. > > Suggested-by: Julien Grall <julien.grall@arm.com> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> I don't think this is absolutely necessary for 4.7. On the other hand, it is just straight renaming, which should be quite safe. If I can get an ack or review from maintainers and confirmation that it doesn't break ARM build within today, we can shovel this in; otherwise it needs to wait for next version of Xen. We will commit our last batch of patches for 4.7 today. Wei.
Hi Wei, On 08/06/2016 09:17, Wei Liu wrote: > On Wed, Jun 08, 2016 at 02:15:27AM +0200, Edgar E. Iglesias wrote: >> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> >> >> Rename map_regions_rw_cache to map_regions_cache and make it use >> p2m.default_access. >> >> Suggested-by: Julien Grall <julien.grall@arm.com> >> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > > I don't think this is absolutely necessary for 4.7. > > On the other hand, it is just straight renaming, which should be quite > safe. This patch does not only contain a renaming, it also contain a change to fix the default memaccess attribute. However, I don't see why we should rename the function to map_regions_cache given this will always map the region Read-Write (p2m_mmio_direct prevents the execution of the memory). > If I can get an ack or review from maintainers and confirmation that it > doesn't break ARM build within today, we can shovel this in; otherwise > it needs to wait for next version of Xen. I would wait for a backport here. Stefano, any opinions? Regards,
Hi Edgar, On 08/06/2016 01:15, Edgar E. Iglesias wrote: > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > Rename map_regions_rw_cache to map_regions_cache and make it use > p2m.default_access. Why have you renamed the function? The current name corresponds to the behavior of the code, i.e map the region cache and only read-write (p2m_mmio_direct prevents the execution). Regards,
On Wed, Jun 08, 2016 at 09:22:56AM +0100, Julien Grall wrote: > Hi Wei, > > On 08/06/2016 09:17, Wei Liu wrote: > >On Wed, Jun 08, 2016 at 02:15:27AM +0200, Edgar E. Iglesias wrote: > >>From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > >> > >>Rename map_regions_rw_cache to map_regions_cache and make it use > >>p2m.default_access. > >> > >>Suggested-by: Julien Grall <julien.grall@arm.com> > >>Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > > > >I don't think this is absolutely necessary for 4.7. > > > >On the other hand, it is just straight renaming, which should be quite > >safe. > > This patch does not only contain a renaming, it also contain a change to fix > the default memaccess attribute. > Oops, I missed that. In that case this is a good reason to wait post-4.7. > However, I don't see why we should rename the function to map_regions_cache > given this will always map the region Read-Write (p2m_mmio_direct prevents > the execution of the memory). > > >If I can get an ack or review from maintainers and confirmation that it > >doesn't break ARM build within today, we can shovel this in; otherwise > >it needs to wait for next version of Xen. > > I would wait for a backport here. Stefano, any opinions? > > Regards, > > -- > Julien Grall
On Wed, 8 Jun 2016, Julien Grall wrote: > Hi Wei, > > On 08/06/2016 09:17, Wei Liu wrote: > > On Wed, Jun 08, 2016 at 02:15:27AM +0200, Edgar E. Iglesias wrote: > > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > > > > > Rename map_regions_rw_cache to map_regions_cache and make it use > > > p2m.default_access. > > > > > > Suggested-by: Julien Grall <julien.grall@arm.com> > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > > > > I don't think this is absolutely necessary for 4.7. > > > > On the other hand, it is just straight renaming, which should be quite > > safe. > > This patch does not only contain a renaming, it also contain a change to fix > the default memaccess attribute. > > However, I don't see why we should rename the function to map_regions_cache > given this will always map the region Read-Write (p2m_mmio_direct prevents the > execution of the memory). > > > If I can get an ack or review from maintainers and confirmation that it > > doesn't break ARM build within today, we can shovel this in; otherwise > > it needs to wait for next version of Xen. > > I would wait for a backport here. Stefano, any opinions? Indeed, I would also wait for a backport
On Wed, Jun 08, 2016 at 10:36:19AM +0100, Stefano Stabellini wrote: > On Wed, 8 Jun 2016, Julien Grall wrote: > > Hi Wei, > > > > On 08/06/2016 09:17, Wei Liu wrote: > > > On Wed, Jun 08, 2016 at 02:15:27AM +0200, Edgar E. Iglesias wrote: > > > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > > > > > > > Rename map_regions_rw_cache to map_regions_cache and make it use > > > > p2m.default_access. > > > > > > > > Suggested-by: Julien Grall <julien.grall@arm.com> > > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > > > > > > I don't think this is absolutely necessary for 4.7. > > > > > > On the other hand, it is just straight renaming, which should be quite > > > safe. > > > > This patch does not only contain a renaming, it also contain a change to fix > > the default memaccess attribute. > > > > However, I don't see why we should rename the function to map_regions_cache > > given this will always map the region Read-Write (p2m_mmio_direct prevents the > > execution of the memory). > > > > > If I can get an ack or review from maintainers and confirmation that it > > > doesn't break ARM build within today, we can shovel this in; otherwise > > > it needs to wait for next version of Xen. > > > > I would wait for a backport here. Stefano, any opinions? > > Indeed, I would also wait for a backport OK, Sounds good. I'm travelling at the moment so it might take a couple of weeks for me to get back to this. Cheers, Edgar
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 00dc07a..b9ffaca 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1570,10 +1570,10 @@ static void acpi_map_other_tables(struct domain *d) { addr = acpi_gbl_root_table_list.tables[i].address; size = acpi_gbl_root_table_list.tables[i].length; - res = map_regions_rw_cache(d, - paddr_to_pfn(addr & PAGE_MASK), - DIV_ROUND_UP(size, PAGE_SIZE), - paddr_to_pfn(addr & PAGE_MASK)); + res = map_regions_cache(d, + paddr_to_pfn(addr & PAGE_MASK), + DIV_ROUND_UP(size, PAGE_SIZE), + paddr_to_pfn(addr & PAGE_MASK)); if ( res ) { panic(XENLOG_ERR "Unable to map ACPI region 0x%"PRIx64 @@ -1926,10 +1926,10 @@ static int prepare_acpi(struct domain *d, struct kernel_info *kinfo) acpi_create_efi_mmap_table(d, &kinfo->mem, tbl_add); /* Map the EFI and ACPI tables to Dom0 */ - rc = map_regions_rw_cache(d, - paddr_to_pfn(d->arch.efi_acpi_gpa), - PFN_UP(d->arch.efi_acpi_len), - paddr_to_pfn(virt_to_maddr(d->arch.efi_acpi_table))); + rc = map_regions_cache(d, + paddr_to_pfn(d->arch.efi_acpi_gpa), + PFN_UP(d->arch.efi_acpi_len), + paddr_to_pfn(virt_to_maddr(d->arch.efi_acpi_table))); if ( rc != 0 ) { printk(XENLOG_ERR "Unable to map EFI/ACPI table 0x%"PRIx64 diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 838d004..3d3fd16 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1216,30 +1216,30 @@ int p2m_populate_ram(struct domain *d, d->arch.p2m.default_access); } -int map_regions_rw_cache(struct domain *d, - unsigned long start_gfn, - unsigned long nr, - unsigned long mfn) +int map_regions_cache(struct domain *d, + unsigned long start_gfn, + unsigned long nr, + unsigned long mfn) { return apply_p2m_changes(d, INSERT, pfn_to_paddr(start_gfn), pfn_to_paddr(start_gfn + nr), pfn_to_paddr(mfn), MATTR_MEM, 0, p2m_mmio_direct, - p2m_access_rw); + d->arch.p2m.default_access); } -int unmap_regions_rw_cache(struct domain *d, - unsigned long start_gfn, - unsigned long nr, - unsigned long mfn) +int unmap_regions_cache(struct domain *d, + unsigned long start_gfn, + unsigned long nr, + unsigned long mfn) { return apply_p2m_changes(d, REMOVE, pfn_to_paddr(start_gfn), pfn_to_paddr(start_gfn + nr), pfn_to_paddr(mfn), MATTR_MEM, 0, p2m_invalid, - p2m_access_rw); + d->arch.p2m.default_access); } int map_mmio_regions(struct domain *d, diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index d240d1e..a2d1bbe 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -144,15 +144,15 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn); /* Setup p2m RAM mapping for domain d from start-end. */ int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end); -int map_regions_rw_cache(struct domain *d, - unsigned long start_gfn, - unsigned long nr_mfns, - unsigned long mfn); - -int unmap_regions_rw_cache(struct domain *d, - unsigned long start_gfn, - unsigned long nr_mfns, - unsigned long mfn); +int map_regions_cache(struct domain *d, + unsigned long start_gfn, + unsigned long nr_mfns, + unsigned long mfn); + +int unmap_regions_cache(struct domain *d, + unsigned long start_gfn, + unsigned long nr_mfns, + unsigned long mfn); int map_dev_mmio_region(struct domain *d, unsigned long start_gfn,