| Message ID | 1423058539-26403-26-git-send-email-parth.dixit@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi parth, Title: this is not acpi specific. On 04/02/2015 14:02, parth.dixit@linaro.org wrote: > From: Parth Dixit <parth.dixit@linaro.org> > > 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 <parth.dixit@linaro.org> > --- > 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. Regards,
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 <parth.dixit@linaro.org> >>> >>> 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 <parth.dixit@linaro.org> >>> --- >>> 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. Regards,
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 <parth.dixit@linaro.org> >>>>> >>>>> 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 <parth.dixit@linaro.org> >>>>> --- >>>>> 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,
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) +{ + return apply_p2m_changes(d, INSERT, + pfn_to_paddr(start_gfn), + pfn_to_paddr(start_gfn + nr), + pfn_to_paddr(mfn), + MATTR_MEM, p2m_mmio_direct); +} + +int unmap_ram_regions(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, p2m_invalid); +} + int map_mmio_regions(struct domain *d, unsigned long start_gfn, unsigned long nr, diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index da36504..be76ecd 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -132,6 +132,16 @@ int guest_physmap_add_entry(struct domain *d, unsigned long page_order, p2m_type_t t); +int map_ram_regions(struct domain *d, + unsigned long start_gfn, + unsigned long nr_mfns, + unsigned long mfn); + +int unmap_ram_regions(struct domain *d, + unsigned long start_gfn, + unsigned long nr_mfns, + unsigned long mfn); + /* Untyped version for RAM only, for compatibility */ static inline int guest_physmap_add_page(struct domain *d, unsigned long gfn,