| Message ID | 1467130643-23868-11-git-send-email-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
(CC Tamas) On 06/07/16 11:43, Stefano Stabellini wrote: > On Tue, 28 Jun 2016, Julien Grall wrote: >> The parameter 'access' is used by memaccess to restrict temporarily the >> permission. This parameter should not be used for other purpose (such >> as restricting permanently the permission). >> >> The type p2m_mmio_direct will map the region Read-Write and >> non-executable. Note that this is already the current behavior with the >> combination of the type and the access. So there is no functional >> change. >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> > > I could be mistaken, but isn't default_access actually p2m_access_rwx? By default, the access is p2m_access_rwx. However this can be changed by memaccess and the new default value is stored in arch->p2m.default_access. I have CCed Tamas to confirm that. Note that this is how the other calls are done. Regards,
On 06/07/16 12:17, Stefano Stabellini wrote: > On Wed, 6 Jul 2016, Julien Grall wrote: >> (CC Tamas) >> >> On 06/07/16 11:43, Stefano Stabellini wrote: >>> On Tue, 28 Jun 2016, Julien Grall wrote: >>>> The parameter 'access' is used by memaccess to restrict temporarily the >>>> permission. This parameter should not be used for other purpose (such >>>> as restricting permanently the permission). >>>> >>>> The type p2m_mmio_direct will map the region Read-Write and >>>> non-executable. Note that this is already the current behavior with the >>>> combination of the type and the access. So there is no functional >>>> change. >>>> >>>> Signed-off-by: Julien Grall <julien.grall@arm.com> >>> >>> I could be mistaken, but isn't default_access actually p2m_access_rwx? >> >> By default, the access is p2m_access_rwx. However this can be changed by >> memaccess and the new default value is stored in arch->p2m.default_access. I >> have CCed Tamas to confirm that. >> >> Note that this is how the other calls are done. > > This patch replaces p2m_access_rw with default_access, which is > p2m_access_rwx by default. Is it actually indended? Yes, I explained why in the commit message. "The type p2m_mmio_direct will map the region Read-Write and non-executable. Note that this is already the current behavior with the combination of the type and the access. So there is no functional change." Regards,
On 06/07/16 12:22, Julien Grall wrote: > > > On 06/07/16 12:17, Stefano Stabellini wrote: >> On Wed, 6 Jul 2016, Julien Grall wrote: >>> (CC Tamas) >>> >>> On 06/07/16 11:43, Stefano Stabellini wrote: >>>> On Tue, 28 Jun 2016, Julien Grall wrote: >>>>> The parameter 'access' is used by memaccess to restrict temporarily >>>>> the >>>>> permission. This parameter should not be used for other purpose (such >>>>> as restricting permanently the permission). >>>>> >>>>> The type p2m_mmio_direct will map the region Read-Write and >>>>> non-executable. Note that this is already the current behavior with >>>>> the >>>>> combination of the type and the access. So there is no functional >>>>> change. >>>>> >>>>> Signed-off-by: Julien Grall <julien.grall@arm.com> >>>> >>>> I could be mistaken, but isn't default_access actually p2m_access_rwx? >>> >>> By default, the access is p2m_access_rwx. However this can be changed by >>> memaccess and the new default value is stored in >>> arch->p2m.default_access. I >>> have CCed Tamas to confirm that. >>> >>> Note that this is how the other calls are done. >> >> This patch replaces p2m_access_rw with default_access, which is >> p2m_access_rwx by default. Is it actually indended? > > Yes, I explained why in the commit message. > > "The type p2m_mmio_direct will map the region Read-Write and > non-executable. Note that this is already the current behavior with the > combination of the type and the access. So there is no functional > change." Thinking a bit more, I will reword the commit message with: "The parameter 'access' used by memaccess to restrict temporarily the permission. This parameter should not be for other purpose (such as restricting permanently the permission). Instead, we should use the default_access provided by memaccess. When it is not enabled, the access will be p2m_access_rwx (i.e no restriction applied). The type p2m_mmio_direct will map the region read-write and non-executable before any further restriction by memaccess. note that this is already the current behavior with the combination of the type and the access. So there is no functional change". Regards,
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 1cfb62b..fcc4513 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1231,7 +1231,7 @@ int map_regions_rw_cache(struct domain *d, 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, @@ -1244,7 +1244,7 @@ int unmap_regions_rw_cache(struct domain *d, 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,
The parameter 'access' is used by memaccess to restrict temporarily the permission. This parameter should not be used for other purpose (such as restricting permanently the permission). The type p2m_mmio_direct will map the region Read-Write and non-executable. Note that this is already the current behavior with the combination of the type and the access. So there is no functional change. Signed-off-by: Julien Grall <julien.grall@arm.com> --- Cc: Shannon Zhao <shannon.zhao@linaro.org> This patch is a candidate for Xen 4.7. Currently this function is only used to map ACPI regions. I am wondering if we should introduce a new p2m type for it. And map this region RO (I am not sure why a guest would want to modify this region). Changes in v4: - Patch added --- xen/arch/arm/p2m.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)