From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH RFC 25/35] arm: acpi add helper functions to map memory regions Date: Wed, 11 Feb 2015 14:49:44 +0800 Message-ID: <54DAFB88.5090600@linaro.org> References: <1423058539-26403-1-git-send-email-parth.dixit@linaro.org> <1423058539-26403-26-git-send-email-parth.dixit@linaro.org> <54D2EBAE.3080909@linaro.org> <54D40C60.9090901@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: ian.campbell@citrix.com, tim@xen.org, xen-devel@lists.xen.org, stefano.stabellini@citrix.com, jbeulich@suse.com, parth.dixit@linaro.org, christoffer.dall@linaro.org List-Id: xen-devel@lists.xenproject.org Hi Stefano, On 06/02/2015 22:12, Stefano Stabellini wrote: > On Fri, 6 Feb 2015, Julien Grall wrote: >> On 06/02/2015 00:21, Stefano Stabellini wrote: >>> On Thu, 5 Feb 2015, Julien Grall wrote: >>>> Hi parth, >>>> >>>> Title: this is not acpi specific. >>>> >>>> On 04/02/2015 14:02, parth.dixit@linaro.org wrote: >>>>> From: Parth Dixit >>>>> >>>>> For passing ACPI tables to dom0, UEFI memory needs to be mapped >>>>> by xen in dom0 address space. This patch adds helper functions for >>>>> mapping. >>>> >>>> I believe that this is not ACPI/RAM specific. Any cached MMIO regions will >>>> have same issue. >>>> >>>> This because Device memory is too strong and disallow unaligned access. >>>> >>>>> Signed-off-by: Parth Dixit >>>>> --- >>>>> xen/arch/arm/p2m.c | 24 ++++++++++++++++++++++++ >>>>> xen/include/asm-arm/p2m.h | 10 ++++++++++ >>>>> 2 files changed, 34 insertions(+) >>>>> >>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >>>>> index 8809f5a..5593a91 100644 >>>>> --- a/xen/arch/arm/p2m.c >>>>> +++ b/xen/arch/arm/p2m.c >>>>> @@ -943,6 +943,30 @@ int p2m_populate_ram(struct domain *d, >>>>> 0, MATTR_MEM, p2m_ram_rw); >>>>> } >>>>> >>>>> +int map_ram_regions(struct domain *d, >>>>> + unsigned long start_gfn, >>>>> + unsigned long nr, >>>>> + unsigned long mfn) >>>> >>>> I don't like the name of the function. It gives the impression that we map >>>> any >>>> RAM region to the guest via this function. >>>> >>>> Which is obviously wrong and should never be done. >>> >>> map_mem_regions? >> >> It's still unclear, what mem mean? is an MMIO region? Is it cached, uncached? >> Is it read-only/read-write. >> >> We already have a function map_mmio_regions. Maybe it would make sense to >> extend it to give more control about the mapping (cached/uncached, read-only, >> read-write,....). But this function is common with x86. So I'm not sure about >> what to do. > > Usually mmio refers to device memory and not just from the caching > attributes point of view. I didn't find anything which state ACPI is always living in the RAM. It could also be in the ROM. > I would rather not use map_mmio_regions for > something that doesn't belong to a device. The current MMIO code assume that the memory will always be non-cached (MATTR_DEVICE). This will lead to various issue in the guest when it's trying to access cached BARs. It would be good to have a common helper handling this case because we will have to handle it for PCI sooner or later. > That said, it is difficult to come up with a good name. Maybe we could introduce a function map_region which the following prototype map_region(strut domain *d, unsigned long start_gfn, unsigned long nr, unsigned long mfn, int mattr, p2m_type_t type) We could also drop add new types (either real or virtual) to differentiate the use cases. This is because we can infer the mattr (MATTR_DEVICE/MATTR_MEM). At the same time, it would clean up a bit the current code by dropping one parameter. Regards, -- Julien Grall