* [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
2019-02-26 23:06 [PATCH 0/6] iomem cacheability Stefano Stabellini
@ 2019-02-26 23:07 ` Stefano Stabellini
2019-02-26 23:18 ` Julien Grall
` (3 more replies)
2019-02-26 23:07 ` [PATCH 2/6] libxc: xc_domain_memory_mapping, " Stefano Stabellini
` (5 subsequent siblings)
6 siblings, 4 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-02-26 23:07 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, julien.grall, sstabellini, JBeulich, andrew.cooper3
Reuse the existing padding field to pass cacheability information about
the memory mapping, specifically, whether the memory should be mapped as
normal memory or as device memory (this is what we have today).
Add a cacheability parameter to map_mmio_regions. 0 means device
memory, which is what we have today.
On ARM, map device memory as p2m_mmio_direct_dev (as it is already done
today) and normal memory as p2m_ram_rw.
On x86, return error if the cacheability requested is not device memory.
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
CC: JBeulich@suse.com
CC: andrew.cooper3@citrix.com
---
xen/arch/arm/gic-v2.c | 3 ++-
xen/arch/arm/p2m.c | 19 +++++++++++++++++--
xen/arch/arm/platforms/exynos5.c | 4 ++--
xen/arch/arm/platforms/omap5.c | 8 ++++----
xen/arch/arm/vgic-v2.c | 2 +-
xen/arch/arm/vgic/vgic-v2.c | 2 +-
xen/arch/x86/hvm/dom0_build.c | 7 +++++--
xen/arch/x86/mm/p2m.c | 6 +++++-
xen/common/domctl.c | 8 +++++---
| 3 ++-
xen/include/public/domctl.h | 4 +++-
xen/include/xen/p2m-common.h | 3 ++-
12 files changed, 49 insertions(+), 20 deletions(-)
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index e7eb01f..1ea3da2 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -690,7 +690,8 @@ static int gicv2_map_hwdown_extra_mappings(struct domain *d)
ret = map_mmio_regions(d, gaddr_to_gfn(v2m_data->addr),
PFN_UP(v2m_data->size),
- maddr_to_mfn(v2m_data->addr));
+ maddr_to_mfn(v2m_data->addr),
+ CACHEABILITY_DEVMEM);
if ( ret )
{
printk(XENLOG_ERR "GICv2: Map v2m frame to d%d failed.\n",
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 30cfb01..5b8fcc5 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1068,9 +1068,24 @@ int unmap_regions_p2mt(struct domain *d,
int map_mmio_regions(struct domain *d,
gfn_t start_gfn,
unsigned long nr,
- mfn_t mfn)
+ mfn_t mfn,
+ uint32_t cache_policy)
{
- return p2m_insert_mapping(d, start_gfn, nr, mfn, p2m_mmio_direct_dev);
+ p2m_type_t t;
+
+ switch ( cache_policy )
+ {
+ case CACHEABILITY_MEMORY:
+ t = p2m_ram_rw;
+ break;
+ case CACHEABILITY_DEVMEM:
+ t = p2m_mmio_direct_dev;
+ break;
+ default:
+ return -ENOSYS;
+ }
+
+ return p2m_insert_mapping(d, start_gfn, nr, mfn, t);
}
int unmap_mmio_regions(struct domain *d,
diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
index 6560507..3af5fd3 100644
--- a/xen/arch/arm/platforms/exynos5.c
+++ b/xen/arch/arm/platforms/exynos5.c
@@ -83,11 +83,11 @@ static int exynos5250_specific_mapping(struct domain *d)
{
/* Map the chip ID */
map_mmio_regions(d, gaddr_to_gfn(EXYNOS5_PA_CHIPID), 1,
- maddr_to_mfn(EXYNOS5_PA_CHIPID));
+ maddr_to_mfn(EXYNOS5_PA_CHIPID), CACHEABILITY_DEVMEM);
/* Map the PWM region */
map_mmio_regions(d, gaddr_to_gfn(EXYNOS5_PA_TIMER), 2,
- maddr_to_mfn(EXYNOS5_PA_TIMER));
+ maddr_to_mfn(EXYNOS5_PA_TIMER), CACHEABILITY_DEVMEM);
return 0;
}
diff --git a/xen/arch/arm/platforms/omap5.c b/xen/arch/arm/platforms/omap5.c
index aee24e4..4899332 100644
--- a/xen/arch/arm/platforms/omap5.c
+++ b/xen/arch/arm/platforms/omap5.c
@@ -99,19 +99,19 @@ static int omap5_specific_mapping(struct domain *d)
{
/* Map the PRM module */
map_mmio_regions(d, gaddr_to_gfn(OMAP5_PRM_BASE), 2,
- maddr_to_mfn(OMAP5_PRM_BASE));
+ maddr_to_mfn(OMAP5_PRM_BASE), CACHEABILITY_DEVMEM);
/* Map the PRM_MPU */
map_mmio_regions(d, gaddr_to_gfn(OMAP5_PRCM_MPU_BASE), 1,
- maddr_to_mfn(OMAP5_PRCM_MPU_BASE));
+ maddr_to_mfn(OMAP5_PRCM_MPU_BASE), CACHEABILITY_DEVMEM);
/* Map the Wakeup Gen */
map_mmio_regions(d, gaddr_to_gfn(OMAP5_WKUPGEN_BASE), 1,
- maddr_to_mfn(OMAP5_WKUPGEN_BASE));
+ maddr_to_mfn(OMAP5_WKUPGEN_BASE), CACHEABILITY_DEVMEM);
/* Map the on-chip SRAM */
map_mmio_regions(d, gaddr_to_gfn(OMAP5_SRAM_PA), 32,
- maddr_to_mfn(OMAP5_SRAM_PA));
+ maddr_to_mfn(OMAP5_SRAM_PA), CACHEABILITY_DEVMEM);
return 0;
}
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index bf77899..2c9c15c 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -691,7 +691,7 @@ static int vgic_v2_domain_init(struct domain *d)
* region of the guest.
*/
ret = map_mmio_regions(d, gaddr_to_gfn(cbase), csize / PAGE_SIZE,
- maddr_to_mfn(vbase));
+ maddr_to_mfn(vbase), CACHEABILITY_DEVMEM);
if ( ret )
return ret;
diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c
index b5ba4ac..bb305a2 100644
--- a/xen/arch/arm/vgic/vgic-v2.c
+++ b/xen/arch/arm/vgic/vgic-v2.c
@@ -309,7 +309,7 @@ int vgic_v2_map_resources(struct domain *d)
* region of the guest.
*/
ret = map_mmio_regions(d, gaddr_to_gfn(cbase), csize / PAGE_SIZE,
- maddr_to_mfn(vbase));
+ maddr_to_mfn(vbase), CACHEABILITY_DEVMEM);
if ( ret )
{
gdprintk(XENLOG_ERR, "Unable to remap VGIC CPU to VCPU\n");
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 3e29cd3..3058560 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -67,8 +67,11 @@ static int __init modify_identity_mmio(struct domain *d, unsigned long pfn,
for ( ; ; )
{
- rc = (map ? map_mmio_regions : unmap_mmio_regions)
- (d, _gfn(pfn), nr_pages, _mfn(pfn));
+ if ( map )
+ rc = map_mmio_regions(d, _gfn(pfn), nr_pages, _mfn(pfn),
+ CACHEABILITY_DEVMEM);
+ else
+ rc = unmap_mmio_regions(d, _gfn(pfn), nr_pages, _mfn(pfn));
if ( rc == 0 )
break;
if ( rc < 0 )
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 4bdc5e3..0bbb2a4 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2110,12 +2110,16 @@ static unsigned int mmio_order(const struct domain *d,
int map_mmio_regions(struct domain *d,
gfn_t start_gfn,
unsigned long nr,
- mfn_t mfn)
+ mfn_t mfn,
+ uint32_t cache_policy)
{
int ret = 0;
unsigned long i;
unsigned int iter, order;
+ if ( cache_policy != CACHEABILITY_DEVMEM )
+ return -ENOSYS;
+
if ( !paging_mode_translate(d) )
return 0;
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index b294881..f4a7c16 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -935,6 +935,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
unsigned long nr_mfns = op->u.memory_mapping.nr_mfns;
unsigned long mfn_end = mfn + nr_mfns - 1;
int add = op->u.memory_mapping.add_mapping;
+ uint32_t cache_policy = op->u.memory_mapping.cache_policy;
ret = -EINVAL;
if ( mfn_end < mfn || /* wrap? */
@@ -961,10 +962,11 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
if ( add )
{
printk(XENLOG_G_DEBUG
- "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx\n",
- d->domain_id, gfn, mfn, nr_mfns);
+ "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx cache=%u\n",
+ d->domain_id, gfn, mfn, nr_mfns, cache_policy);
- ret = map_mmio_regions(d, _gfn(gfn), nr_mfns, _mfn(mfn));
+ ret = map_mmio_regions(d, _gfn(gfn), nr_mfns, _mfn(mfn),
+ cache_policy);
if ( ret < 0 )
printk(XENLOG_G_WARNING
"memory_map:fail: dom%d gfn=%lx mfn=%lx nr=%lx ret:%ld\n",
--git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 4573cca..f968c2d 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -52,7 +52,8 @@ static int map_range(unsigned long s, unsigned long e, void *data,
* - {un}map_mmio_regions doesn't support preemption.
*/
- rc = map->map ? map_mmio_regions(map->d, _gfn(s), size, _mfn(s))
+ rc = map->map ? map_mmio_regions(map->d, _gfn(s), size, _mfn(s),
+ CACHEABILITY_DEVMEM)
: unmap_mmio_regions(map->d, _gfn(s), size, _mfn(s));
if ( rc == 0 )
{
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 4a46c28..82704dd 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -571,12 +571,14 @@ struct xen_domctl_bind_pt_irq {
*/
#define DPCI_ADD_MAPPING 1
#define DPCI_REMOVE_MAPPING 0
+#define CACHEABILITY_DEVMEM 0 /* device memory, the default */
+#define CACHEABILITY_MEMORY 1 /* normal memory */
struct xen_domctl_memory_mapping {
uint64_aligned_t first_gfn; /* first page (hvm guest phys page) in range */
uint64_aligned_t first_mfn; /* first page (machine page) in range */
uint64_aligned_t nr_mfns; /* number of pages in range (>0) */
uint32_t add_mapping; /* add or remove mapping */
- uint32_t padding; /* padding for 64-bit aligned structure */
+ uint32_t cache_policy; /* cacheability of the memory mapping */
};
diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
index 58031a6..1c945a1 100644
--- a/xen/include/xen/p2m-common.h
+++ b/xen/include/xen/p2m-common.h
@@ -14,7 +14,8 @@ guest_physmap_remove_page(struct domain *d, gfn_t gfn, mfn_t mfn,
int map_mmio_regions(struct domain *d,
gfn_t start_gfn,
unsigned long nr,
- mfn_t mfn);
+ mfn_t mfn,
+ uint32_t cache_policy);
int unmap_mmio_regions(struct domain *d,
gfn_t start_gfn,
unsigned long nr,
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 67+ messages in thread* Re: [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
2019-02-26 23:07 ` [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability Stefano Stabellini
@ 2019-02-26 23:18 ` Julien Grall
2019-04-20 0:02 ` [Xen-devel] " Stefano Stabellini
2019-02-27 10:34 ` Jan Beulich
` (2 subsequent siblings)
3 siblings, 1 reply; 67+ messages in thread
From: Julien Grall @ 2019-02-26 23:18 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel
Cc: Stefano Stabellini, nd, JBeulich, andrew.cooper3
Hi,
On 26/02/2019 23:07, Stefano Stabellini wrote:
> Reuse the existing padding field to pass cacheability information about
> the memory mapping, specifically, whether the memory should be mapped as
> normal memory or as device memory (this is what we have today).
>
> Add a cacheability parameter to map_mmio_regions. 0 means device
> memory, which is what we have today.
>
> On ARM, map device memory as p2m_mmio_direct_dev (as it is already done
> today) and normal memory as p2m_ram_rw.
>
> On x86, return error if the cacheability requested is not device memory.
>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> CC: JBeulich@suse.com
> CC: andrew.cooper3@citrix.com
> ---
> xen/arch/arm/gic-v2.c | 3 ++-
> xen/arch/arm/p2m.c | 19 +++++++++++++++++--
> xen/arch/arm/platforms/exynos5.c | 4 ++--
> xen/arch/arm/platforms/omap5.c | 8 ++++----
> xen/arch/arm/vgic-v2.c | 2 +-
> xen/arch/arm/vgic/vgic-v2.c | 2 +-
> xen/arch/x86/hvm/dom0_build.c | 7 +++++--
> xen/arch/x86/mm/p2m.c | 6 +++++-
> xen/common/domctl.c | 8 +++++---
> xen/drivers/vpci/header.c | 3 ++-
> xen/include/public/domctl.h | 4 +++-
> xen/include/xen/p2m-common.h | 3 ++-
> 12 files changed, 49 insertions(+), 20 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index e7eb01f..1ea3da2 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -690,7 +690,8 @@ static int gicv2_map_hwdown_extra_mappings(struct domain *d)
>
> ret = map_mmio_regions(d, gaddr_to_gfn(v2m_data->addr),
> PFN_UP(v2m_data->size),
> - maddr_to_mfn(v2m_data->addr));
> + maddr_to_mfn(v2m_data->addr),
> + CACHEABILITY_DEVMEM);
> if ( ret )
> {
> printk(XENLOG_ERR "GICv2: Map v2m frame to d%d failed.\n",
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 30cfb01..5b8fcc5 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1068,9 +1068,24 @@ int unmap_regions_p2mt(struct domain *d,
> int map_mmio_regions(struct domain *d,
> gfn_t start_gfn,
> unsigned long nr,
> - mfn_t mfn)
> + mfn_t mfn,
> + uint32_t cache_policy)
> {
> - return p2m_insert_mapping(d, start_gfn, nr, mfn, p2m_mmio_direct_dev);
> + p2m_type_t t;
> +
> + switch ( cache_policy )
> + {
> + case CACHEABILITY_MEMORY:
> + t = p2m_ram_rw;
I have already said it before, p2m_ram_rw is not a solution. This is
used in various place to know whether the page is actual RAM.
You should at least use on of the p2m_mmio_direct option. But if you
allow the guest to use cacheability attributes on device, then you
probably want to think what can happen if the iomem is re-assigned to
another domain after crash.
Potentially, you want to clean the cache here.
I will comment on the rest later on.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-20 0:02 ` Stefano Stabellini
0 siblings, 0 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-04-20 0:02 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Stefano Stabellini, andrew.cooper3, JBeulich,
xen-devel, nd
On Tue, 26 Feb 2019, Julien Grall wrote:
> Hi,
>
> On 26/02/2019 23:07, Stefano Stabellini wrote:
> > Reuse the existing padding field to pass cacheability information about
> > the memory mapping, specifically, whether the memory should be mapped as
> > normal memory or as device memory (this is what we have today).
> >
> > Add a cacheability parameter to map_mmio_regions. 0 means device
> > memory, which is what we have today.
> >
> > On ARM, map device memory as p2m_mmio_direct_dev (as it is already done
> > today) and normal memory as p2m_ram_rw.
> >
> > On x86, return error if the cacheability requested is not device memory.
> >
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > CC: JBeulich@suse.com
> > CC: andrew.cooper3@citrix.com
> > ---
> > xen/arch/arm/gic-v2.c | 3 ++-
> > xen/arch/arm/p2m.c | 19 +++++++++++++++++--
> > xen/arch/arm/platforms/exynos5.c | 4 ++--
> > xen/arch/arm/platforms/omap5.c | 8 ++++----
> > xen/arch/arm/vgic-v2.c | 2 +-
> > xen/arch/arm/vgic/vgic-v2.c | 2 +-
> > xen/arch/x86/hvm/dom0_build.c | 7 +++++--
> > xen/arch/x86/mm/p2m.c | 6 +++++-
> > xen/common/domctl.c | 8 +++++---
> > xen/drivers/vpci/header.c | 3 ++-
> > xen/include/public/domctl.h | 4 +++-
> > xen/include/xen/p2m-common.h | 3 ++-
> > 12 files changed, 49 insertions(+), 20 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> > index e7eb01f..1ea3da2 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -690,7 +690,8 @@ static int gicv2_map_hwdown_extra_mappings(struct domain *d)
> >
> > ret = map_mmio_regions(d, gaddr_to_gfn(v2m_data->addr),
> > PFN_UP(v2m_data->size),
> > - maddr_to_mfn(v2m_data->addr));
> > + maddr_to_mfn(v2m_data->addr),
> > + CACHEABILITY_DEVMEM);
> > if ( ret )
> > {
> > printk(XENLOG_ERR "GICv2: Map v2m frame to d%d failed.\n",
> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > index 30cfb01..5b8fcc5 100644
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -1068,9 +1068,24 @@ int unmap_regions_p2mt(struct domain *d,
> > int map_mmio_regions(struct domain *d,
> > gfn_t start_gfn,
> > unsigned long nr,
> > - mfn_t mfn)
> > + mfn_t mfn,
> > + uint32_t cache_policy)
> > {
> > - return p2m_insert_mapping(d, start_gfn, nr, mfn, p2m_mmio_direct_dev);
> > + p2m_type_t t;
> > +
> > + switch ( cache_policy )
> > + {
> > + case CACHEABILITY_MEMORY:
> > + t = p2m_ram_rw;
>
> Potentially, you want to clean the cache here.
We have been talking about this and I have been looking through the
code. I am still not exactly sure how to proceed.
Is there a reason why cacheable reserved_memory pages should be treated
differently from normal memory, in regards to cleaning the cache? It
seems to me that they should be the same in terms of cache issues?
Is there a place where we clean the dcache for normal pages, one that is
not tied to p2m->clean_pte, which is different (it is there for iommu
reasons)?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [Xen-devel] [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-20 0:02 ` Stefano Stabellini
0 siblings, 0 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-04-20 0:02 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Stefano Stabellini, andrew.cooper3, JBeulich,
xen-devel, nd
On Tue, 26 Feb 2019, Julien Grall wrote:
> Hi,
>
> On 26/02/2019 23:07, Stefano Stabellini wrote:
> > Reuse the existing padding field to pass cacheability information about
> > the memory mapping, specifically, whether the memory should be mapped as
> > normal memory or as device memory (this is what we have today).
> >
> > Add a cacheability parameter to map_mmio_regions. 0 means device
> > memory, which is what we have today.
> >
> > On ARM, map device memory as p2m_mmio_direct_dev (as it is already done
> > today) and normal memory as p2m_ram_rw.
> >
> > On x86, return error if the cacheability requested is not device memory.
> >
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > CC: JBeulich@suse.com
> > CC: andrew.cooper3@citrix.com
> > ---
> > xen/arch/arm/gic-v2.c | 3 ++-
> > xen/arch/arm/p2m.c | 19 +++++++++++++++++--
> > xen/arch/arm/platforms/exynos5.c | 4 ++--
> > xen/arch/arm/platforms/omap5.c | 8 ++++----
> > xen/arch/arm/vgic-v2.c | 2 +-
> > xen/arch/arm/vgic/vgic-v2.c | 2 +-
> > xen/arch/x86/hvm/dom0_build.c | 7 +++++--
> > xen/arch/x86/mm/p2m.c | 6 +++++-
> > xen/common/domctl.c | 8 +++++---
> > xen/drivers/vpci/header.c | 3 ++-
> > xen/include/public/domctl.h | 4 +++-
> > xen/include/xen/p2m-common.h | 3 ++-
> > 12 files changed, 49 insertions(+), 20 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> > index e7eb01f..1ea3da2 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -690,7 +690,8 @@ static int gicv2_map_hwdown_extra_mappings(struct domain *d)
> >
> > ret = map_mmio_regions(d, gaddr_to_gfn(v2m_data->addr),
> > PFN_UP(v2m_data->size),
> > - maddr_to_mfn(v2m_data->addr));
> > + maddr_to_mfn(v2m_data->addr),
> > + CACHEABILITY_DEVMEM);
> > if ( ret )
> > {
> > printk(XENLOG_ERR "GICv2: Map v2m frame to d%d failed.\n",
> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > index 30cfb01..5b8fcc5 100644
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -1068,9 +1068,24 @@ int unmap_regions_p2mt(struct domain *d,
> > int map_mmio_regions(struct domain *d,
> > gfn_t start_gfn,
> > unsigned long nr,
> > - mfn_t mfn)
> > + mfn_t mfn,
> > + uint32_t cache_policy)
> > {
> > - return p2m_insert_mapping(d, start_gfn, nr, mfn, p2m_mmio_direct_dev);
> > + p2m_type_t t;
> > +
> > + switch ( cache_policy )
> > + {
> > + case CACHEABILITY_MEMORY:
> > + t = p2m_ram_rw;
>
> Potentially, you want to clean the cache here.
We have been talking about this and I have been looking through the
code. I am still not exactly sure how to proceed.
Is there a reason why cacheable reserved_memory pages should be treated
differently from normal memory, in regards to cleaning the cache? It
seems to me that they should be the same in terms of cache issues?
Is there a place where we clean the dcache for normal pages, one that is
not tied to p2m->clean_pte, which is different (it is there for iommu
reasons)?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-21 17:32 ` Julien Grall
0 siblings, 0 replies; 67+ messages in thread
From: Julien Grall @ 2019-04-21 17:32 UTC (permalink / raw)
To: Stefano Stabellini
Cc: xen-devel, nd, andrew.cooper3, JBeulich, Stefano Stabellini
Hi Stefano,
On 4/20/19 1:02 AM, Stefano Stabellini wrote:
> On Tue, 26 Feb 2019, Julien Grall wrote:
>> Hi,
>>
>> On 26/02/2019 23:07, Stefano Stabellini wrote:
>>> Reuse the existing padding field to pass cacheability information about
>>> the memory mapping, specifically, whether the memory should be mapped as
>>> normal memory or as device memory (this is what we have today).
>>>
>>> Add a cacheability parameter to map_mmio_regions. 0 means device
>>> memory, which is what we have today.
>>>
>>> On ARM, map device memory as p2m_mmio_direct_dev (as it is already done
>>> today) and normal memory as p2m_ram_rw.
>>>
>>> On x86, return error if the cacheability requested is not device memory.
>>>
>>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>>> CC: JBeulich@suse.com
>>> CC: andrew.cooper3@citrix.com
>>> ---
>>> xen/arch/arm/gic-v2.c | 3 ++-
>>> xen/arch/arm/p2m.c | 19 +++++++++++++++++--
>>> xen/arch/arm/platforms/exynos5.c | 4 ++--
>>> xen/arch/arm/platforms/omap5.c | 8 ++++----
>>> xen/arch/arm/vgic-v2.c | 2 +-
>>> xen/arch/arm/vgic/vgic-v2.c | 2 +-
>>> xen/arch/x86/hvm/dom0_build.c | 7 +++++--
>>> xen/arch/x86/mm/p2m.c | 6 +++++-
>>> xen/common/domctl.c | 8 +++++---
>>> xen/drivers/vpci/header.c | 3 ++-
>>> xen/include/public/domctl.h | 4 +++-
>>> xen/include/xen/p2m-common.h | 3 ++-
>>> 12 files changed, 49 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
>>> index e7eb01f..1ea3da2 100644
>>> --- a/xen/arch/arm/gic-v2.c
>>> +++ b/xen/arch/arm/gic-v2.c
>>> @@ -690,7 +690,8 @@ static int gicv2_map_hwdown_extra_mappings(struct domain *d)
>>>
>>> ret = map_mmio_regions(d, gaddr_to_gfn(v2m_data->addr),
>>> PFN_UP(v2m_data->size),
>>> - maddr_to_mfn(v2m_data->addr));
>>> + maddr_to_mfn(v2m_data->addr),
>>> + CACHEABILITY_DEVMEM);
>>> if ( ret )
>>> {
>>> printk(XENLOG_ERR "GICv2: Map v2m frame to d%d failed.\n",
>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>> index 30cfb01..5b8fcc5 100644
>>> --- a/xen/arch/arm/p2m.c
>>> +++ b/xen/arch/arm/p2m.c
>>> @@ -1068,9 +1068,24 @@ int unmap_regions_p2mt(struct domain *d,
>>> int map_mmio_regions(struct domain *d,
>>> gfn_t start_gfn,
>>> unsigned long nr,
>>> - mfn_t mfn)
>>> + mfn_t mfn,
>>> + uint32_t cache_policy)
>>> {
>>> - return p2m_insert_mapping(d, start_gfn, nr, mfn, p2m_mmio_direct_dev);
>>> + p2m_type_t t;
>>> +
>>> + switch ( cache_policy )
>>> + {
>>> + case CACHEABILITY_MEMORY:
>>> + t = p2m_ram_rw;
>>
>> Potentially, you want to clean the cache here.
>
> We have been talking about this and I have been looking through the
> code. I am still not exactly sure how to proceed.
>
> Is there a reason why cacheable reserved_memory pages should be treated
> differently from normal memory, in regards to cleaning the cache? It
> seems to me that they should be the same in terms of cache issues?
Your wording is a bit confusing. I guess what you call "normal memory"
is guest memory, am I right?
Any memory assigned to the guest is and clean & invalidate (technically
clean is enough) before getting assigned to the guest (see
flush_page_to_ram). So this patch is introducing a different behavior
that what we currently have for other normal memory.
But my concern is you may inconsistently use the memory attributes
breaking coherency. For instance, you map in Guest A with cacheable
attributes then after the guest died, you remap to guest B with a
non-cacheable attributes. guest B may have an inconsistent view of the
memory mapped.
This is one case where cleaning the cache would be necessary. One could
consider this is part of the "device reset" (this is a bit of the name
abuse), so Xen should not take care of it.
The most important bit is to have documentation that reflect the issues
with such parameters. So the user is aware of what could go wrong when
using "iomem".
>
> Is there a place where we clean the dcache for normal pages, one that is
> not tied to p2m->clean_pte, which is different (it is there for iommu
> reasons)?
p2m->clean_pte is only here to deal with non-coherence IOMMU page-table
walker. See above for flushing normal pages.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [Xen-devel] [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-21 17:32 ` Julien Grall
0 siblings, 0 replies; 67+ messages in thread
From: Julien Grall @ 2019-04-21 17:32 UTC (permalink / raw)
To: Stefano Stabellini
Cc: xen-devel, nd, andrew.cooper3, JBeulich, Stefano Stabellini
Hi Stefano,
On 4/20/19 1:02 AM, Stefano Stabellini wrote:
> On Tue, 26 Feb 2019, Julien Grall wrote:
>> Hi,
>>
>> On 26/02/2019 23:07, Stefano Stabellini wrote:
>>> Reuse the existing padding field to pass cacheability information about
>>> the memory mapping, specifically, whether the memory should be mapped as
>>> normal memory or as device memory (this is what we have today).
>>>
>>> Add a cacheability parameter to map_mmio_regions. 0 means device
>>> memory, which is what we have today.
>>>
>>> On ARM, map device memory as p2m_mmio_direct_dev (as it is already done
>>> today) and normal memory as p2m_ram_rw.
>>>
>>> On x86, return error if the cacheability requested is not device memory.
>>>
>>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>>> CC: JBeulich@suse.com
>>> CC: andrew.cooper3@citrix.com
>>> ---
>>> xen/arch/arm/gic-v2.c | 3 ++-
>>> xen/arch/arm/p2m.c | 19 +++++++++++++++++--
>>> xen/arch/arm/platforms/exynos5.c | 4 ++--
>>> xen/arch/arm/platforms/omap5.c | 8 ++++----
>>> xen/arch/arm/vgic-v2.c | 2 +-
>>> xen/arch/arm/vgic/vgic-v2.c | 2 +-
>>> xen/arch/x86/hvm/dom0_build.c | 7 +++++--
>>> xen/arch/x86/mm/p2m.c | 6 +++++-
>>> xen/common/domctl.c | 8 +++++---
>>> xen/drivers/vpci/header.c | 3 ++-
>>> xen/include/public/domctl.h | 4 +++-
>>> xen/include/xen/p2m-common.h | 3 ++-
>>> 12 files changed, 49 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
>>> index e7eb01f..1ea3da2 100644
>>> --- a/xen/arch/arm/gic-v2.c
>>> +++ b/xen/arch/arm/gic-v2.c
>>> @@ -690,7 +690,8 @@ static int gicv2_map_hwdown_extra_mappings(struct domain *d)
>>>
>>> ret = map_mmio_regions(d, gaddr_to_gfn(v2m_data->addr),
>>> PFN_UP(v2m_data->size),
>>> - maddr_to_mfn(v2m_data->addr));
>>> + maddr_to_mfn(v2m_data->addr),
>>> + CACHEABILITY_DEVMEM);
>>> if ( ret )
>>> {
>>> printk(XENLOG_ERR "GICv2: Map v2m frame to d%d failed.\n",
>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>> index 30cfb01..5b8fcc5 100644
>>> --- a/xen/arch/arm/p2m.c
>>> +++ b/xen/arch/arm/p2m.c
>>> @@ -1068,9 +1068,24 @@ int unmap_regions_p2mt(struct domain *d,
>>> int map_mmio_regions(struct domain *d,
>>> gfn_t start_gfn,
>>> unsigned long nr,
>>> - mfn_t mfn)
>>> + mfn_t mfn,
>>> + uint32_t cache_policy)
>>> {
>>> - return p2m_insert_mapping(d, start_gfn, nr, mfn, p2m_mmio_direct_dev);
>>> + p2m_type_t t;
>>> +
>>> + switch ( cache_policy )
>>> + {
>>> + case CACHEABILITY_MEMORY:
>>> + t = p2m_ram_rw;
>>
>> Potentially, you want to clean the cache here.
>
> We have been talking about this and I have been looking through the
> code. I am still not exactly sure how to proceed.
>
> Is there a reason why cacheable reserved_memory pages should be treated
> differently from normal memory, in regards to cleaning the cache? It
> seems to me that they should be the same in terms of cache issues?
Your wording is a bit confusing. I guess what you call "normal memory"
is guest memory, am I right?
Any memory assigned to the guest is and clean & invalidate (technically
clean is enough) before getting assigned to the guest (see
flush_page_to_ram). So this patch is introducing a different behavior
that what we currently have for other normal memory.
But my concern is you may inconsistently use the memory attributes
breaking coherency. For instance, you map in Guest A with cacheable
attributes then after the guest died, you remap to guest B with a
non-cacheable attributes. guest B may have an inconsistent view of the
memory mapped.
This is one case where cleaning the cache would be necessary. One could
consider this is part of the "device reset" (this is a bit of the name
abuse), so Xen should not take care of it.
The most important bit is to have documentation that reflect the issues
with such parameters. So the user is aware of what could go wrong when
using "iomem".
>
> Is there a place where we clean the dcache for normal pages, one that is
> not tied to p2m->clean_pte, which is different (it is there for iommu
> reasons)?
p2m->clean_pte is only here to deal with non-coherence IOMMU page-table
walker. See above for flushing normal pages.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-22 21:59 ` Stefano Stabellini
0 siblings, 0 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-04-22 21:59 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Stefano Stabellini, andrew.cooper3, JBeulich,
xen-devel, nd
On Sun, 21 Apr 2019, Julien Grall wrote:
> > > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > > > index 30cfb01..5b8fcc5 100644
> > > > --- a/xen/arch/arm/p2m.c
> > > > +++ b/xen/arch/arm/p2m.c
> > > > @@ -1068,9 +1068,24 @@ int unmap_regions_p2mt(struct domain *d,
> > > > int map_mmio_regions(struct domain *d,
> > > > gfn_t start_gfn,
> > > > unsigned long nr,
> > > > - mfn_t mfn)
> > > > + mfn_t mfn,
> > > > + uint32_t cache_policy)
> > > > {
> > > > - return p2m_insert_mapping(d, start_gfn, nr, mfn,
> > > > p2m_mmio_direct_dev);
> > > > + p2m_type_t t;
> > > > +
> > > > + switch ( cache_policy )
> > > > + {
> > > > + case CACHEABILITY_MEMORY:
> > > > + t = p2m_ram_rw;
> > >
> > > Potentially, you want to clean the cache here.
> >
> > We have been talking about this and I have been looking through the
> > code. I am still not exactly sure how to proceed.
> >
> > Is there a reason why cacheable reserved_memory pages should be treated
> > differently from normal memory, in regards to cleaning the cache? It
> > seems to me that they should be the same in terms of cache issues?
>
> Your wording is a bit confusing. I guess what you call "normal memory" is
> guest memory, am I right?
Yes, right. I wonder if we need to come up with clearer terms. Given the
many types of memory we have to deal with, it might become even more
confusing going forward. Guest normal memory maybe? Or guest RAM?
> Any memory assigned to the guest is and clean & invalidate (technically clean
> is enough) before getting assigned to the guest (see flush_page_to_ram). So
> this patch is introducing a different behavior that what we currently have for
> other normal memory.
This is what I was trying to understand, thanks for the pointer. I am
unsure whether we want to do this for reserved-memory regions too: on
one hand, it would make things more consistent, on the other hand I am
not sure it is the right behavior for reserved-memory. Let's think it
through.
The use case is communication with other heterogeneous CPUs. In that
case, it would matter if a domU crashes with the ring mapped and an
unflushed write (partial?) to the ring. The domU gets restarted with the
same ring mapping. In this case, it looks like we would want to clean
the cache. It wouldn't matter if it is done at VM shutdown or at VM
creation time.
So maybe it makes sense to do something like flush_page_to_ram for
reserved-memory pages. It seems simple to do it at VM creation time,
because we could invalidate the cache when map_mmio_regions is called,
either there or from the domctl handler. On the other hand, I don't know
where to do it at domain destruction time because no domctl is called to
unmap the reserved-memory region. Also, cleaning the cache at domain
destruction time would introduce a difference compared to guest normal
memory.
I know I said the opposite in our meeting, but maybe cleaning the cache
for reserved-memory regions at domain creation time is the right way
forward?
> But my concern is you may inconsistently use the memory attributes breaking
> coherency. For instance, you map in Guest A with cacheable attributes then
> after the guest died, you remap to guest B with a non-cacheable attributes.
> guest B may have an inconsistent view of the memory mapped.
I think that anything caused by the user selecting the wrong
cacheability attributes is not something we have to support or care
about (more about it below).
> This is one case where cleaning the cache would be necessary. One could
> consider this is part of the "device reset" (this is a bit of the name abuse),
> so Xen should not take care of it.
>
> The most important bit is to have documentation that reflect the issues with
> such parameters. So the user is aware of what could go wrong when using
> "iomem".
I agree. I'll be very clear about the consequences of choosing wrong or
inconsistent attributes in the docs. Also, I'll be very clear about
cache-flushing, documenting properly the implemented behavior (which at
the moment is no-cache-flushes).
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [Xen-devel] [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-22 21:59 ` Stefano Stabellini
0 siblings, 0 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-04-22 21:59 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Stefano Stabellini, andrew.cooper3, JBeulich,
xen-devel, nd
On Sun, 21 Apr 2019, Julien Grall wrote:
> > > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > > > index 30cfb01..5b8fcc5 100644
> > > > --- a/xen/arch/arm/p2m.c
> > > > +++ b/xen/arch/arm/p2m.c
> > > > @@ -1068,9 +1068,24 @@ int unmap_regions_p2mt(struct domain *d,
> > > > int map_mmio_regions(struct domain *d,
> > > > gfn_t start_gfn,
> > > > unsigned long nr,
> > > > - mfn_t mfn)
> > > > + mfn_t mfn,
> > > > + uint32_t cache_policy)
> > > > {
> > > > - return p2m_insert_mapping(d, start_gfn, nr, mfn,
> > > > p2m_mmio_direct_dev);
> > > > + p2m_type_t t;
> > > > +
> > > > + switch ( cache_policy )
> > > > + {
> > > > + case CACHEABILITY_MEMORY:
> > > > + t = p2m_ram_rw;
> > >
> > > Potentially, you want to clean the cache here.
> >
> > We have been talking about this and I have been looking through the
> > code. I am still not exactly sure how to proceed.
> >
> > Is there a reason why cacheable reserved_memory pages should be treated
> > differently from normal memory, in regards to cleaning the cache? It
> > seems to me that they should be the same in terms of cache issues?
>
> Your wording is a bit confusing. I guess what you call "normal memory" is
> guest memory, am I right?
Yes, right. I wonder if we need to come up with clearer terms. Given the
many types of memory we have to deal with, it might become even more
confusing going forward. Guest normal memory maybe? Or guest RAM?
> Any memory assigned to the guest is and clean & invalidate (technically clean
> is enough) before getting assigned to the guest (see flush_page_to_ram). So
> this patch is introducing a different behavior that what we currently have for
> other normal memory.
This is what I was trying to understand, thanks for the pointer. I am
unsure whether we want to do this for reserved-memory regions too: on
one hand, it would make things more consistent, on the other hand I am
not sure it is the right behavior for reserved-memory. Let's think it
through.
The use case is communication with other heterogeneous CPUs. In that
case, it would matter if a domU crashes with the ring mapped and an
unflushed write (partial?) to the ring. The domU gets restarted with the
same ring mapping. In this case, it looks like we would want to clean
the cache. It wouldn't matter if it is done at VM shutdown or at VM
creation time.
So maybe it makes sense to do something like flush_page_to_ram for
reserved-memory pages. It seems simple to do it at VM creation time,
because we could invalidate the cache when map_mmio_regions is called,
either there or from the domctl handler. On the other hand, I don't know
where to do it at domain destruction time because no domctl is called to
unmap the reserved-memory region. Also, cleaning the cache at domain
destruction time would introduce a difference compared to guest normal
memory.
I know I said the opposite in our meeting, but maybe cleaning the cache
for reserved-memory regions at domain creation time is the right way
forward?
> But my concern is you may inconsistently use the memory attributes breaking
> coherency. For instance, you map in Guest A with cacheable attributes then
> after the guest died, you remap to guest B with a non-cacheable attributes.
> guest B may have an inconsistent view of the memory mapped.
I think that anything caused by the user selecting the wrong
cacheability attributes is not something we have to support or care
about (more about it below).
> This is one case where cleaning the cache would be necessary. One could
> consider this is part of the "device reset" (this is a bit of the name abuse),
> so Xen should not take care of it.
>
> The most important bit is to have documentation that reflect the issues with
> such parameters. So the user is aware of what could go wrong when using
> "iomem".
I agree. I'll be very clear about the consequences of choosing wrong or
inconsistent attributes in the docs. Also, I'll be very clear about
cache-flushing, documenting properly the implemented behavior (which at
the moment is no-cache-flushes).
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-24 10:42 ` Julien Grall
0 siblings, 0 replies; 67+ messages in thread
From: Julien Grall @ 2019-04-24 10:42 UTC (permalink / raw)
To: Stefano Stabellini
Cc: xen-devel, nd, andrew.cooper3, JBeulich, Stefano Stabellini
Hi,
On 22/04/2019 22:59, Stefano Stabellini wrote:
> On Sun, 21 Apr 2019, Julien Grall wrote:
>>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>>>> index 30cfb01..5b8fcc5 100644
>>>>> --- a/xen/arch/arm/p2m.c
>>>>> +++ b/xen/arch/arm/p2m.c
>>>>> @@ -1068,9 +1068,24 @@ int unmap_regions_p2mt(struct domain *d,
>>>>> int map_mmio_regions(struct domain *d,
>>>>> gfn_t start_gfn,
>>>>> unsigned long nr,
>>>>> - mfn_t mfn)
>>>>> + mfn_t mfn,
>>>>> + uint32_t cache_policy)
>>>>> {
>>>>> - return p2m_insert_mapping(d, start_gfn, nr, mfn,
>>>>> p2m_mmio_direct_dev);
>>>>> + p2m_type_t t;
>>>>> +
>>>>> + switch ( cache_policy )
>>>>> + {
>>>>> + case CACHEABILITY_MEMORY:
>>>>> + t = p2m_ram_rw;
>>>>
>>>> Potentially, you want to clean the cache here.
>>>
>>> We have been talking about this and I have been looking through the
>>> code. I am still not exactly sure how to proceed.
>>>
>>> Is there a reason why cacheable reserved_memory pages should be treated
>>> differently from normal memory, in regards to cleaning the cache? It
>>> seems to me that they should be the same in terms of cache issues?
>>
>> Your wording is a bit confusing. I guess what you call "normal memory" is
>> guest memory, am I right?
>
> Yes, right. I wonder if we need to come up with clearer terms. Given the
> many types of memory we have to deal with, it might become even more
> confusing going forward. Guest normal memory maybe? Or guest RAM?
The term "normal memory" is really confusing because this is a memory type on
Arm. reserved-regions are also not *MMIO* as they are part of the RAM that was
reserved for special usage. So the term "guest RAM" is also not appropriate.
I understand that 'iomem' is a quick way to get reserved-memory regions mapped
in the guest. However, this feels like an abuse of the interface because
reserved-memory are technically not an MMIO. They also can be used by the OS for
storing data when not in use (providing the DT node contain the property
'reusable').
Overall, we want to rethink how 'reserved-regions' are going to be treated. The
solution suggested in this series is not going to be viable very long.
>
>
>> Any memory assigned to the guest is and clean & invalidate (technically clean
>> is enough) before getting assigned to the guest (see flush_page_to_ram). So
>> this patch is introducing a different behavior that what we currently have for
>> other normal memory.
>
> This is what I was trying to understand, thanks for the pointer. I am
> unsure whether we want to do this for reserved-memory regions too: on
> one hand, it would make things more consistent, on the other hand I am
> not sure it is the right behavior for reserved-memory. Let's think it
> through.
>
> The use case is communication with other heterogeneous CPUs. In that
> case, it would matter if a domU crashes with the ring mapped and an
> unflushed write (partial?) to the ring. The domU gets restarted with the
> same ring mapping. In this case, it looks like we would want to clean
> the cache. It wouldn't matter if it is done at VM shutdown or at VM
> creation time.
>
> So maybe it makes sense to do something like flush_page_to_ram for
> reserved-memory pages. It seems simple to do it at VM creation time,
> because we could invalidate the cache when map_mmio_regions is called,
> either there or from the domctl handler. On the other hand, I don't know
> where to do it at domain destruction time because no domctl is called to
> unmap the reserved-memory region. Also, cleaning the cache at domain
> destruction time would introduce a difference compared to guest normal
> memory.
>
> I know I said the opposite in our meeting, but maybe cleaning the cache
> for reserved-memory regions at domain creation time is the right way
> forward?
I don't have a strong opinion on it.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [Xen-devel] [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-24 10:42 ` Julien Grall
0 siblings, 0 replies; 67+ messages in thread
From: Julien Grall @ 2019-04-24 10:42 UTC (permalink / raw)
To: Stefano Stabellini
Cc: xen-devel, nd, andrew.cooper3, JBeulich, Stefano Stabellini
Hi,
On 22/04/2019 22:59, Stefano Stabellini wrote:
> On Sun, 21 Apr 2019, Julien Grall wrote:
>>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>>>> index 30cfb01..5b8fcc5 100644
>>>>> --- a/xen/arch/arm/p2m.c
>>>>> +++ b/xen/arch/arm/p2m.c
>>>>> @@ -1068,9 +1068,24 @@ int unmap_regions_p2mt(struct domain *d,
>>>>> int map_mmio_regions(struct domain *d,
>>>>> gfn_t start_gfn,
>>>>> unsigned long nr,
>>>>> - mfn_t mfn)
>>>>> + mfn_t mfn,
>>>>> + uint32_t cache_policy)
>>>>> {
>>>>> - return p2m_insert_mapping(d, start_gfn, nr, mfn,
>>>>> p2m_mmio_direct_dev);
>>>>> + p2m_type_t t;
>>>>> +
>>>>> + switch ( cache_policy )
>>>>> + {
>>>>> + case CACHEABILITY_MEMORY:
>>>>> + t = p2m_ram_rw;
>>>>
>>>> Potentially, you want to clean the cache here.
>>>
>>> We have been talking about this and I have been looking through the
>>> code. I am still not exactly sure how to proceed.
>>>
>>> Is there a reason why cacheable reserved_memory pages should be treated
>>> differently from normal memory, in regards to cleaning the cache? It
>>> seems to me that they should be the same in terms of cache issues?
>>
>> Your wording is a bit confusing. I guess what you call "normal memory" is
>> guest memory, am I right?
>
> Yes, right. I wonder if we need to come up with clearer terms. Given the
> many types of memory we have to deal with, it might become even more
> confusing going forward. Guest normal memory maybe? Or guest RAM?
The term "normal memory" is really confusing because this is a memory type on
Arm. reserved-regions are also not *MMIO* as they are part of the RAM that was
reserved for special usage. So the term "guest RAM" is also not appropriate.
I understand that 'iomem' is a quick way to get reserved-memory regions mapped
in the guest. However, this feels like an abuse of the interface because
reserved-memory are technically not an MMIO. They also can be used by the OS for
storing data when not in use (providing the DT node contain the property
'reusable').
Overall, we want to rethink how 'reserved-regions' are going to be treated. The
solution suggested in this series is not going to be viable very long.
>
>
>> Any memory assigned to the guest is and clean & invalidate (technically clean
>> is enough) before getting assigned to the guest (see flush_page_to_ram). So
>> this patch is introducing a different behavior that what we currently have for
>> other normal memory.
>
> This is what I was trying to understand, thanks for the pointer. I am
> unsure whether we want to do this for reserved-memory regions too: on
> one hand, it would make things more consistent, on the other hand I am
> not sure it is the right behavior for reserved-memory. Let's think it
> through.
>
> The use case is communication with other heterogeneous CPUs. In that
> case, it would matter if a domU crashes with the ring mapped and an
> unflushed write (partial?) to the ring. The domU gets restarted with the
> same ring mapping. In this case, it looks like we would want to clean
> the cache. It wouldn't matter if it is done at VM shutdown or at VM
> creation time.
>
> So maybe it makes sense to do something like flush_page_to_ram for
> reserved-memory pages. It seems simple to do it at VM creation time,
> because we could invalidate the cache when map_mmio_regions is called,
> either there or from the domctl handler. On the other hand, I don't know
> where to do it at domain destruction time because no domctl is called to
> unmap the reserved-memory region. Also, cleaning the cache at domain
> destruction time would introduce a difference compared to guest normal
> memory.
>
> I know I said the opposite in our meeting, but maybe cleaning the cache
> for reserved-memory regions at domain creation time is the right way
> forward?
I don't have a strong opinion on it.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
2019-02-26 23:07 ` [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability Stefano Stabellini
2019-02-26 23:18 ` Julien Grall
@ 2019-02-27 10:34 ` Jan Beulich
2019-04-17 21:12 ` [Xen-devel] " Stefano Stabellini
2019-02-27 19:28 ` Julien Grall
2019-02-27 21:02 ` Julien Grall
3 siblings, 1 reply; 67+ messages in thread
From: Jan Beulich @ 2019-02-27 10:34 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Andrew Cooper, Julien Grall, Stefano Stabellini, xen-devel
>>> On 27.02.19 at 00:07, <sstabellini@kernel.org> wrote:
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -571,12 +571,14 @@ struct xen_domctl_bind_pt_irq {
> */
> #define DPCI_ADD_MAPPING 1
> #define DPCI_REMOVE_MAPPING 0
> +#define CACHEABILITY_DEVMEM 0 /* device memory, the default */
> +#define CACHEABILITY_MEMORY 1 /* normal memory */
> struct xen_domctl_memory_mapping {
> uint64_aligned_t first_gfn; /* first page (hvm guest phys page) in range */
> uint64_aligned_t first_mfn; /* first page (machine page) in range */
> uint64_aligned_t nr_mfns; /* number of pages in range (>0) */
> uint32_t add_mapping; /* add or remove mapping */
> - uint32_t padding; /* padding for 64-bit aligned structure */
> + uint32_t cache_policy; /* cacheability of the memory mapping */
> };
I don't think DEVMEM and MEMORY are anywhere near descriptive
enough, nor - if we want such control anyway - flexible enough. I
think what you want is to actually specify cachability, allowing on
x86 to e.g. map frame buffers or alike WC. The attribute then
would (obviously and necessarily) be architecture specific.
In vPCI code the question then will become whether prefetchable
BARs shouldn't be mapped e.g. WT instead of UC (and whatever
the Arm equivalent is, assuming PCI support will eventually get
added to Arm64, as it seems it has been the intention for quite
some time).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-17 21:12 ` Stefano Stabellini
0 siblings, 0 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-04-17 21:12 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Julien Grall, Stefano Stabellini,
Stefano Stabellini, xen-devel
On Wed, 27 Feb 2019, Jan Beulich wrote:
> >>> On 27.02.19 at 00:07, <sstabellini@kernel.org> wrote:
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -571,12 +571,14 @@ struct xen_domctl_bind_pt_irq {
> > */
> > #define DPCI_ADD_MAPPING 1
> > #define DPCI_REMOVE_MAPPING 0
> > +#define CACHEABILITY_DEVMEM 0 /* device memory, the default */
> > +#define CACHEABILITY_MEMORY 1 /* normal memory */
> > struct xen_domctl_memory_mapping {
> > uint64_aligned_t first_gfn; /* first page (hvm guest phys page) in range */
> > uint64_aligned_t first_mfn; /* first page (machine page) in range */
> > uint64_aligned_t nr_mfns; /* number of pages in range (>0) */
> > uint32_t add_mapping; /* add or remove mapping */
> > - uint32_t padding; /* padding for 64-bit aligned structure */
> > + uint32_t cache_policy; /* cacheability of the memory mapping */
> > };
>
> I don't think DEVMEM and MEMORY are anywhere near descriptive
> enough, nor - if we want such control anyway - flexible enough. I
> think what you want is to actually specify cachability, allowing on
> x86 to e.g. map frame buffers or alike WC. The attribute then
> would (obviously and necessarily) be architecture specific.
Yes, I agree with what you wrote, and also with what Julien wrote. Now
the question is how do you both think this should look like in more
details:
- are you OK with using memory_policy instead of cache_policy like
Julien's suggested as name for the field?
- are you OK with using #defines for the values?
- should the #defines for both x86 and Arm be defined here or in other
headers?
- what values would you like to see for x86?
For Arm, the ones I care about are:
- p2m_mmio_direct_dev
- p2m_mmio_direct_c
So maybe:
#define ARM_P2M_DIRECT_DEV 0 /* Device-nGnRnE, default*/
#define ARM_P2M_DIRECT_C 1 /* Write-back write-allocate */
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [Xen-devel] [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-17 21:12 ` Stefano Stabellini
0 siblings, 0 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-04-17 21:12 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Julien Grall, Stefano Stabellini,
Stefano Stabellini, xen-devel
On Wed, 27 Feb 2019, Jan Beulich wrote:
> >>> On 27.02.19 at 00:07, <sstabellini@kernel.org> wrote:
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -571,12 +571,14 @@ struct xen_domctl_bind_pt_irq {
> > */
> > #define DPCI_ADD_MAPPING 1
> > #define DPCI_REMOVE_MAPPING 0
> > +#define CACHEABILITY_DEVMEM 0 /* device memory, the default */
> > +#define CACHEABILITY_MEMORY 1 /* normal memory */
> > struct xen_domctl_memory_mapping {
> > uint64_aligned_t first_gfn; /* first page (hvm guest phys page) in range */
> > uint64_aligned_t first_mfn; /* first page (machine page) in range */
> > uint64_aligned_t nr_mfns; /* number of pages in range (>0) */
> > uint32_t add_mapping; /* add or remove mapping */
> > - uint32_t padding; /* padding for 64-bit aligned structure */
> > + uint32_t cache_policy; /* cacheability of the memory mapping */
> > };
>
> I don't think DEVMEM and MEMORY are anywhere near descriptive
> enough, nor - if we want such control anyway - flexible enough. I
> think what you want is to actually specify cachability, allowing on
> x86 to e.g. map frame buffers or alike WC. The attribute then
> would (obviously and necessarily) be architecture specific.
Yes, I agree with what you wrote, and also with what Julien wrote. Now
the question is how do you both think this should look like in more
details:
- are you OK with using memory_policy instead of cache_policy like
Julien's suggested as name for the field?
- are you OK with using #defines for the values?
- should the #defines for both x86 and Arm be defined here or in other
headers?
- what values would you like to see for x86?
For Arm, the ones I care about are:
- p2m_mmio_direct_dev
- p2m_mmio_direct_c
So maybe:
#define ARM_P2M_DIRECT_DEV 0 /* Device-nGnRnE, default*/
#define ARM_P2M_DIRECT_C 1 /* Write-back write-allocate */
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-17 21:25 ` Julien Grall
0 siblings, 0 replies; 67+ messages in thread
From: Julien Grall @ 2019-04-17 21:25 UTC (permalink / raw)
To: Stefano Stabellini, Jan Beulich
Cc: Andrew Cooper, Stefano Stabellini, xen-devel
Hi,
On 4/17/19 10:12 PM, Stefano Stabellini wrote:
> On Wed, 27 Feb 2019, Jan Beulich wrote:
>>>>> On 27.02.19 at 00:07, <sstabellini@kernel.org> wrote:
>>> --- a/xen/include/public/domctl.h
>>> +++ b/xen/include/public/domctl.h
>>> @@ -571,12 +571,14 @@ struct xen_domctl_bind_pt_irq {
>>> */
>>> #define DPCI_ADD_MAPPING 1
>>> #define DPCI_REMOVE_MAPPING 0
>>> +#define CACHEABILITY_DEVMEM 0 /* device memory, the default */
>>> +#define CACHEABILITY_MEMORY 1 /* normal memory */
>>> struct xen_domctl_memory_mapping {
>>> uint64_aligned_t first_gfn; /* first page (hvm guest phys page) in range */
>>> uint64_aligned_t first_mfn; /* first page (machine page) in range */
>>> uint64_aligned_t nr_mfns; /* number of pages in range (>0) */
>>> uint32_t add_mapping; /* add or remove mapping */
>>> - uint32_t padding; /* padding for 64-bit aligned structure */
>>> + uint32_t cache_policy; /* cacheability of the memory mapping */
>>> };
>>
>> I don't think DEVMEM and MEMORY are anywhere near descriptive
>> enough, nor - if we want such control anyway - flexible enough. I
>> think what you want is to actually specify cachability, allowing on
>> x86 to e.g. map frame buffers or alike WC. The attribute then
>> would (obviously and necessarily) be architecture specific.
>
> Yes, I agree with what you wrote, and also with what Julien wrote. Now
> the question is how do you both think this should look like in more
> details:
>
> - are you OK with using memory_policy instead of cache_policy like
> Julien's suggested as name for the field?
> - are you OK with using #defines for the values?
> - should the #defines for both x86 and Arm be defined here or in other
> headers?
> - what values would you like to see for x86?
>
> For Arm, the ones I care about are:
>
> - p2m_mmio_direct_dev
> - p2m_mmio_direct_c
First step first. What are the reason to have p2m_mmio_direct_c? Is it
only for memory sharing between DomU and Dom0?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [Xen-devel] [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-17 21:25 ` Julien Grall
0 siblings, 0 replies; 67+ messages in thread
From: Julien Grall @ 2019-04-17 21:25 UTC (permalink / raw)
To: Stefano Stabellini, Jan Beulich
Cc: Andrew Cooper, Stefano Stabellini, xen-devel
Hi,
On 4/17/19 10:12 PM, Stefano Stabellini wrote:
> On Wed, 27 Feb 2019, Jan Beulich wrote:
>>>>> On 27.02.19 at 00:07, <sstabellini@kernel.org> wrote:
>>> --- a/xen/include/public/domctl.h
>>> +++ b/xen/include/public/domctl.h
>>> @@ -571,12 +571,14 @@ struct xen_domctl_bind_pt_irq {
>>> */
>>> #define DPCI_ADD_MAPPING 1
>>> #define DPCI_REMOVE_MAPPING 0
>>> +#define CACHEABILITY_DEVMEM 0 /* device memory, the default */
>>> +#define CACHEABILITY_MEMORY 1 /* normal memory */
>>> struct xen_domctl_memory_mapping {
>>> uint64_aligned_t first_gfn; /* first page (hvm guest phys page) in range */
>>> uint64_aligned_t first_mfn; /* first page (machine page) in range */
>>> uint64_aligned_t nr_mfns; /* number of pages in range (>0) */
>>> uint32_t add_mapping; /* add or remove mapping */
>>> - uint32_t padding; /* padding for 64-bit aligned structure */
>>> + uint32_t cache_policy; /* cacheability of the memory mapping */
>>> };
>>
>> I don't think DEVMEM and MEMORY are anywhere near descriptive
>> enough, nor - if we want such control anyway - flexible enough. I
>> think what you want is to actually specify cachability, allowing on
>> x86 to e.g. map frame buffers or alike WC. The attribute then
>> would (obviously and necessarily) be architecture specific.
>
> Yes, I agree with what you wrote, and also with what Julien wrote. Now
> the question is how do you both think this should look like in more
> details:
>
> - are you OK with using memory_policy instead of cache_policy like
> Julien's suggested as name for the field?
> - are you OK with using #defines for the values?
> - should the #defines for both x86 and Arm be defined here or in other
> headers?
> - what values would you like to see for x86?
>
> For Arm, the ones I care about are:
>
> - p2m_mmio_direct_dev
> - p2m_mmio_direct_c
First step first. What are the reason to have p2m_mmio_direct_c? Is it
only for memory sharing between DomU and Dom0?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-17 21:55 ` Stefano Stabellini
0 siblings, 0 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-04-17 21:55 UTC (permalink / raw)
To: Julien Grall
Cc: Andrew Cooper, Stefano Stabellini, Stefano Stabellini,
Jan Beulich, xen-devel
On Wed, 17 Apr 2019, Julien Grall wrote:
> Hi,
>
> On 4/17/19 10:12 PM, Stefano Stabellini wrote:
> > On Wed, 27 Feb 2019, Jan Beulich wrote:
> > > > > > On 27.02.19 at 00:07, <sstabellini@kernel.org> wrote:
> > > > --- a/xen/include/public/domctl.h
> > > > +++ b/xen/include/public/domctl.h
> > > > @@ -571,12 +571,14 @@ struct xen_domctl_bind_pt_irq {
> > > > */
> > > > #define DPCI_ADD_MAPPING 1
> > > > #define DPCI_REMOVE_MAPPING 0
> > > > +#define CACHEABILITY_DEVMEM 0 /* device memory, the default */
> > > > +#define CACHEABILITY_MEMORY 1 /* normal memory */
> > > > struct xen_domctl_memory_mapping {
> > > > uint64_aligned_t first_gfn; /* first page (hvm guest phys page) in
> > > > range */
> > > > uint64_aligned_t first_mfn; /* first page (machine page) in range
> > > > */
> > > > uint64_aligned_t nr_mfns; /* number of pages in range (>0) */
> > > > uint32_t add_mapping; /* add or remove mapping */
> > > > - uint32_t padding; /* padding for 64-bit aligned structure
> > > > */
> > > > + uint32_t cache_policy; /* cacheability of the memory mapping
> > > > */
> > > > };
> > >
> > > I don't think DEVMEM and MEMORY are anywhere near descriptive
> > > enough, nor - if we want such control anyway - flexible enough. I
> > > think what you want is to actually specify cachability, allowing on
> > > x86 to e.g. map frame buffers or alike WC. The attribute then
> > > would (obviously and necessarily) be architecture specific.
> >
> > Yes, I agree with what you wrote, and also with what Julien wrote. Now
> > the question is how do you both think this should look like in more
> > details:
> >
> > - are you OK with using memory_policy instead of cache_policy like
> > Julien's suggested as name for the field?
> > - are you OK with using #defines for the values?
> > - should the #defines for both x86 and Arm be defined here or in other
> > headers?
> > - what values would you like to see for x86?
> >
> > For Arm, the ones I care about are:
> >
> > - p2m_mmio_direct_dev
> > - p2m_mmio_direct_c
>
> First step first. What are the reason to have p2m_mmio_direct_c? Is it only
> for memory sharing between DomU and Dom0?
No, Dom0-DomU sharing is only a minor positive side effect. Xilinx MPSoC
has two Cortex R5 cpus in addition to four Cortex A53 cpus on the board.
It is also possible to add additional Cortex M4 cpus and Microblaze cpus
in fabric. There could be dozen independent processors. Users need to
exchange data between the heterogeneous cpus. They usually set up their
own ring structures over shared memory, or they use OpenAMP. Either
way, they need to share a cacheable memory region between them. The
MPSoC is very flexible and the memory region can come from a multitude
of sources, including a portion of normal memory, or a portion of a
special memory area on the board. There are a couple of special SRAM
banks 64K or 256K large that could be used for that. Also, PRAM can be
easily added in fabric and used for the purpose.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [Xen-devel] [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-17 21:55 ` Stefano Stabellini
0 siblings, 0 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-04-17 21:55 UTC (permalink / raw)
To: Julien Grall
Cc: Andrew Cooper, Stefano Stabellini, Stefano Stabellini,
Jan Beulich, xen-devel
On Wed, 17 Apr 2019, Julien Grall wrote:
> Hi,
>
> On 4/17/19 10:12 PM, Stefano Stabellini wrote:
> > On Wed, 27 Feb 2019, Jan Beulich wrote:
> > > > > > On 27.02.19 at 00:07, <sstabellini@kernel.org> wrote:
> > > > --- a/xen/include/public/domctl.h
> > > > +++ b/xen/include/public/domctl.h
> > > > @@ -571,12 +571,14 @@ struct xen_domctl_bind_pt_irq {
> > > > */
> > > > #define DPCI_ADD_MAPPING 1
> > > > #define DPCI_REMOVE_MAPPING 0
> > > > +#define CACHEABILITY_DEVMEM 0 /* device memory, the default */
> > > > +#define CACHEABILITY_MEMORY 1 /* normal memory */
> > > > struct xen_domctl_memory_mapping {
> > > > uint64_aligned_t first_gfn; /* first page (hvm guest phys page) in
> > > > range */
> > > > uint64_aligned_t first_mfn; /* first page (machine page) in range
> > > > */
> > > > uint64_aligned_t nr_mfns; /* number of pages in range (>0) */
> > > > uint32_t add_mapping; /* add or remove mapping */
> > > > - uint32_t padding; /* padding for 64-bit aligned structure
> > > > */
> > > > + uint32_t cache_policy; /* cacheability of the memory mapping
> > > > */
> > > > };
> > >
> > > I don't think DEVMEM and MEMORY are anywhere near descriptive
> > > enough, nor - if we want such control anyway - flexible enough. I
> > > think what you want is to actually specify cachability, allowing on
> > > x86 to e.g. map frame buffers or alike WC. The attribute then
> > > would (obviously and necessarily) be architecture specific.
> >
> > Yes, I agree with what you wrote, and also with what Julien wrote. Now
> > the question is how do you both think this should look like in more
> > details:
> >
> > - are you OK with using memory_policy instead of cache_policy like
> > Julien's suggested as name for the field?
> > - are you OK with using #defines for the values?
> > - should the #defines for both x86 and Arm be defined here or in other
> > headers?
> > - what values would you like to see for x86?
> >
> > For Arm, the ones I care about are:
> >
> > - p2m_mmio_direct_dev
> > - p2m_mmio_direct_c
>
> First step first. What are the reason to have p2m_mmio_direct_c? Is it only
> for memory sharing between DomU and Dom0?
No, Dom0-DomU sharing is only a minor positive side effect. Xilinx MPSoC
has two Cortex R5 cpus in addition to four Cortex A53 cpus on the board.
It is also possible to add additional Cortex M4 cpus and Microblaze cpus
in fabric. There could be dozen independent processors. Users need to
exchange data between the heterogeneous cpus. They usually set up their
own ring structures over shared memory, or they use OpenAMP. Either
way, they need to share a cacheable memory region between them. The
MPSoC is very flexible and the memory region can come from a multitude
of sources, including a portion of normal memory, or a portion of a
special memory area on the board. There are a couple of special SRAM
banks 64K or 256K large that could be used for that. Also, PRAM can be
easily added in fabric and used for the purpose.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-25 10:41 ` Jan Beulich
0 siblings, 0 replies; 67+ messages in thread
From: Jan Beulich @ 2019-04-25 10:41 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Andrew Cooper, Julien Grall, Stefano Stabellini, xen-devel
>>> On 17.04.19 at 23:12, <sstabellini@kernel.org> wrote:
> On Wed, 27 Feb 2019, Jan Beulich wrote:
>> >>> On 27.02.19 at 00:07, <sstabellini@kernel.org> wrote:
>> > --- a/xen/include/public/domctl.h
>> > +++ b/xen/include/public/domctl.h
>> > @@ -571,12 +571,14 @@ struct xen_domctl_bind_pt_irq {
>> > */
>> > #define DPCI_ADD_MAPPING 1
>> > #define DPCI_REMOVE_MAPPING 0
>> > +#define CACHEABILITY_DEVMEM 0 /* device memory, the default */
>> > +#define CACHEABILITY_MEMORY 1 /* normal memory */
>> > struct xen_domctl_memory_mapping {
>> > uint64_aligned_t first_gfn; /* first page (hvm guest phys page) in
> range */
>> > uint64_aligned_t first_mfn; /* first page (machine page) in range */
>> > uint64_aligned_t nr_mfns; /* number of pages in range (>0) */
>> > uint32_t add_mapping; /* add or remove mapping */
>> > - uint32_t padding; /* padding for 64-bit aligned structure */
>> > + uint32_t cache_policy; /* cacheability of the memory mapping */
>> > };
>>
>> I don't think DEVMEM and MEMORY are anywhere near descriptive
>> enough, nor - if we want such control anyway - flexible enough. I
>> think what you want is to actually specify cachability, allowing on
>> x86 to e.g. map frame buffers or alike WC. The attribute then
>> would (obviously and necessarily) be architecture specific.
>
> Yes, I agree with what you wrote, and also with what Julien wrote. Now
> the question is how do you both think this should look like in more
> details:
>
> - are you OK with using memory_policy instead of cache_policy like
> Julien's suggested as name for the field?
Yes - in fact either is fine to me.
> - are you OK with using #defines for the values?
Yes.
> - should the #defines for both x86 and Arm be defined here or in other
> headers?
I'd say here, but I wouldn't object to placement in arch-
specific public headers.
> - what values would you like to see for x86?
Unless you intend to implement the function for x86, I'd
suggest not adding any x86 #define-s at all for now.
But I agree with Julien (in case this wasn't explicit enough from
my earlier replay) that it first needs to be clarified whether such
an interface is wanted in the first place.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [Xen-devel] [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-25 10:41 ` Jan Beulich
0 siblings, 0 replies; 67+ messages in thread
From: Jan Beulich @ 2019-04-25 10:41 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Andrew Cooper, Julien Grall, Stefano Stabellini, xen-devel
>>> On 17.04.19 at 23:12, <sstabellini@kernel.org> wrote:
> On Wed, 27 Feb 2019, Jan Beulich wrote:
>> >>> On 27.02.19 at 00:07, <sstabellini@kernel.org> wrote:
>> > --- a/xen/include/public/domctl.h
>> > +++ b/xen/include/public/domctl.h
>> > @@ -571,12 +571,14 @@ struct xen_domctl_bind_pt_irq {
>> > */
>> > #define DPCI_ADD_MAPPING 1
>> > #define DPCI_REMOVE_MAPPING 0
>> > +#define CACHEABILITY_DEVMEM 0 /* device memory, the default */
>> > +#define CACHEABILITY_MEMORY 1 /* normal memory */
>> > struct xen_domctl_memory_mapping {
>> > uint64_aligned_t first_gfn; /* first page (hvm guest phys page) in
> range */
>> > uint64_aligned_t first_mfn; /* first page (machine page) in range */
>> > uint64_aligned_t nr_mfns; /* number of pages in range (>0) */
>> > uint32_t add_mapping; /* add or remove mapping */
>> > - uint32_t padding; /* padding for 64-bit aligned structure */
>> > + uint32_t cache_policy; /* cacheability of the memory mapping */
>> > };
>>
>> I don't think DEVMEM and MEMORY are anywhere near descriptive
>> enough, nor - if we want such control anyway - flexible enough. I
>> think what you want is to actually specify cachability, allowing on
>> x86 to e.g. map frame buffers or alike WC. The attribute then
>> would (obviously and necessarily) be architecture specific.
>
> Yes, I agree with what you wrote, and also with what Julien wrote. Now
> the question is how do you both think this should look like in more
> details:
>
> - are you OK with using memory_policy instead of cache_policy like
> Julien's suggested as name for the field?
Yes - in fact either is fine to me.
> - are you OK with using #defines for the values?
Yes.
> - should the #defines for both x86 and Arm be defined here or in other
> headers?
I'd say here, but I wouldn't object to placement in arch-
specific public headers.
> - what values would you like to see for x86?
Unless you intend to implement the function for x86, I'd
suggest not adding any x86 #define-s at all for now.
But I agree with Julien (in case this wasn't explicit enough from
my earlier replay) that it first needs to be clarified whether such
an interface is wanted in the first place.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-25 22:31 ` Stefano Stabellini
0 siblings, 0 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-04-25 22:31 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Julien Grall, Stefano Stabellini,
Stefano Stabellini, xen-devel
On Thu, 25 Apr 2019, Jan Beulich wrote:
> >>> On 17.04.19 at 23:12, <sstabellini@kernel.org> wrote:
> > On Wed, 27 Feb 2019, Jan Beulich wrote:
> >> >>> On 27.02.19 at 00:07, <sstabellini@kernel.org> wrote:
> >> > --- a/xen/include/public/domctl.h
> >> > +++ b/xen/include/public/domctl.h
> >> > @@ -571,12 +571,14 @@ struct xen_domctl_bind_pt_irq {
> >> > */
> >> > #define DPCI_ADD_MAPPING 1
> >> > #define DPCI_REMOVE_MAPPING 0
> >> > +#define CACHEABILITY_DEVMEM 0 /* device memory, the default */
> >> > +#define CACHEABILITY_MEMORY 1 /* normal memory */
> >> > struct xen_domctl_memory_mapping {
> >> > uint64_aligned_t first_gfn; /* first page (hvm guest phys page) in
> > range */
> >> > uint64_aligned_t first_mfn; /* first page (machine page) in range */
> >> > uint64_aligned_t nr_mfns; /* number of pages in range (>0) */
> >> > uint32_t add_mapping; /* add or remove mapping */
> >> > - uint32_t padding; /* padding for 64-bit aligned structure */
> >> > + uint32_t cache_policy; /* cacheability of the memory mapping */
> >> > };
> >>
> >> I don't think DEVMEM and MEMORY are anywhere near descriptive
> >> enough, nor - if we want such control anyway - flexible enough. I
> >> think what you want is to actually specify cachability, allowing on
> >> x86 to e.g. map frame buffers or alike WC. The attribute then
> >> would (obviously and necessarily) be architecture specific.
> >
> > Yes, I agree with what you wrote, and also with what Julien wrote. Now
> > the question is how do you both think this should look like in more
> > details:
> >
> > - are you OK with using memory_policy instead of cache_policy like
> > Julien's suggested as name for the field?
>
> Yes - in fact either is fine to me.
>
> > - are you OK with using #defines for the values?
>
> Yes.
>
> > - should the #defines for both x86 and Arm be defined here or in other
> > headers?
>
> I'd say here, but I wouldn't object to placement in arch-
> specific public headers.
>
> > - what values would you like to see for x86?
>
> Unless you intend to implement the function for x86, I'd
> suggest not adding any x86 #define-s at all for now.
>
> But I agree with Julien (in case this wasn't explicit enough from
> my earlier replay) that it first needs to be clarified whether such
> an interface is wanted in the first place.
I have written down a few more details about the use-case elsewhere,
I'll copy/paste here:
Xilinx MPSoC has two Cortex R5 cpus in addition to four Cortex A53 cpus
on the board. It is also possible to add additional Cortex M4 cpus and
Microblaze cpus in fabric. There could be dozen independent processors.
Users need to exchange data between the heterogeneous cpus. They usually
set up their own ring structures over shared memory, or they use
OpenAMP. Either way, they need to share a cacheable memory region
between them. The MPSoC is very flexible and the memory region can come
from a multitude of sources, including a portion of normal memory, or a
portion of a special memory area on the board. There are a couple of
special SRAM banks 64K or 256K large that could be used for that. Also,
PRAM can be easily added in fabric and used for the purpose.
At the very least to handle the special memory regions, we need to be
able to allow iomem to map them as cacheable memory to a DomU. So I do
think we need this interface extension.
Let me know if you still have any doubts/questions. Otherwise I'll work
toward respinning the series in the proposed direction.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [Xen-devel] [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-25 22:31 ` Stefano Stabellini
0 siblings, 0 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-04-25 22:31 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Julien Grall, Stefano Stabellini,
Stefano Stabellini, xen-devel
On Thu, 25 Apr 2019, Jan Beulich wrote:
> >>> On 17.04.19 at 23:12, <sstabellini@kernel.org> wrote:
> > On Wed, 27 Feb 2019, Jan Beulich wrote:
> >> >>> On 27.02.19 at 00:07, <sstabellini@kernel.org> wrote:
> >> > --- a/xen/include/public/domctl.h
> >> > +++ b/xen/include/public/domctl.h
> >> > @@ -571,12 +571,14 @@ struct xen_domctl_bind_pt_irq {
> >> > */
> >> > #define DPCI_ADD_MAPPING 1
> >> > #define DPCI_REMOVE_MAPPING 0
> >> > +#define CACHEABILITY_DEVMEM 0 /* device memory, the default */
> >> > +#define CACHEABILITY_MEMORY 1 /* normal memory */
> >> > struct xen_domctl_memory_mapping {
> >> > uint64_aligned_t first_gfn; /* first page (hvm guest phys page) in
> > range */
> >> > uint64_aligned_t first_mfn; /* first page (machine page) in range */
> >> > uint64_aligned_t nr_mfns; /* number of pages in range (>0) */
> >> > uint32_t add_mapping; /* add or remove mapping */
> >> > - uint32_t padding; /* padding for 64-bit aligned structure */
> >> > + uint32_t cache_policy; /* cacheability of the memory mapping */
> >> > };
> >>
> >> I don't think DEVMEM and MEMORY are anywhere near descriptive
> >> enough, nor - if we want such control anyway - flexible enough. I
> >> think what you want is to actually specify cachability, allowing on
> >> x86 to e.g. map frame buffers or alike WC. The attribute then
> >> would (obviously and necessarily) be architecture specific.
> >
> > Yes, I agree with what you wrote, and also with what Julien wrote. Now
> > the question is how do you both think this should look like in more
> > details:
> >
> > - are you OK with using memory_policy instead of cache_policy like
> > Julien's suggested as name for the field?
>
> Yes - in fact either is fine to me.
>
> > - are you OK with using #defines for the values?
>
> Yes.
>
> > - should the #defines for both x86 and Arm be defined here or in other
> > headers?
>
> I'd say here, but I wouldn't object to placement in arch-
> specific public headers.
>
> > - what values would you like to see for x86?
>
> Unless you intend to implement the function for x86, I'd
> suggest not adding any x86 #define-s at all for now.
>
> But I agree with Julien (in case this wasn't explicit enough from
> my earlier replay) that it first needs to be clarified whether such
> an interface is wanted in the first place.
I have written down a few more details about the use-case elsewhere,
I'll copy/paste here:
Xilinx MPSoC has two Cortex R5 cpus in addition to four Cortex A53 cpus
on the board. It is also possible to add additional Cortex M4 cpus and
Microblaze cpus in fabric. There could be dozen independent processors.
Users need to exchange data between the heterogeneous cpus. They usually
set up their own ring structures over shared memory, or they use
OpenAMP. Either way, they need to share a cacheable memory region
between them. The MPSoC is very flexible and the memory region can come
from a multitude of sources, including a portion of normal memory, or a
portion of a special memory area on the board. There are a couple of
special SRAM banks 64K or 256K large that could be used for that. Also,
PRAM can be easily added in fabric and used for the purpose.
At the very least to handle the special memory regions, we need to be
able to allow iomem to map them as cacheable memory to a DomU. So I do
think we need this interface extension.
Let me know if you still have any doubts/questions. Otherwise I'll work
toward respinning the series in the proposed direction.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-26 7:12 ` Jan Beulich
0 siblings, 0 replies; 67+ messages in thread
From: Jan Beulich @ 2019-04-26 7:12 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Andrew Cooper, Julien Grall, Stefano Stabellini, xen-devel
>>> On 26.04.19 at 00:31, <sstabellini@kernel.org> wrote:
> On Thu, 25 Apr 2019, Jan Beulich wrote:
>> But I agree with Julien (in case this wasn't explicit enough from
>> my earlier replay) that it first needs to be clarified whether such
>> an interface is wanted in the first place.
>
> I have written down a few more details about the use-case elsewhere,
> I'll copy/paste here:
>
> Xilinx MPSoC has two Cortex R5 cpus in addition to four Cortex A53 cpus
> on the board. It is also possible to add additional Cortex M4 cpus and
> Microblaze cpus in fabric. There could be dozen independent processors.
> Users need to exchange data between the heterogeneous cpus. They usually
> set up their own ring structures over shared memory, or they use
> OpenAMP. Either way, they need to share a cacheable memory region
> between them. The MPSoC is very flexible and the memory region can come
> from a multitude of sources, including a portion of normal memory, or a
> portion of a special memory area on the board. There are a couple of
> special SRAM banks 64K or 256K large that could be used for that. Also,
> PRAM can be easily added in fabric and used for the purpose.
>
> At the very least to handle the special memory regions, we need to be
> able to allow iomem to map them as cacheable memory to a DomU. So I do
> think we need this interface extension.
>
> Let me know if you still have any doubts/questions. Otherwise I'll work
> toward respinning the series in the proposed direction.
Well, as long as Julien and you agree that such an interface is
needed on Arm, this is fine with me.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Xen-devel] [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-26 7:12 ` Jan Beulich
0 siblings, 0 replies; 67+ messages in thread
From: Jan Beulich @ 2019-04-26 7:12 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Andrew Cooper, Julien Grall, Stefano Stabellini, xen-devel
>>> On 26.04.19 at 00:31, <sstabellini@kernel.org> wrote:
> On Thu, 25 Apr 2019, Jan Beulich wrote:
>> But I agree with Julien (in case this wasn't explicit enough from
>> my earlier replay) that it first needs to be clarified whether such
>> an interface is wanted in the first place.
>
> I have written down a few more details about the use-case elsewhere,
> I'll copy/paste here:
>
> Xilinx MPSoC has two Cortex R5 cpus in addition to four Cortex A53 cpus
> on the board. It is also possible to add additional Cortex M4 cpus and
> Microblaze cpus in fabric. There could be dozen independent processors.
> Users need to exchange data between the heterogeneous cpus. They usually
> set up their own ring structures over shared memory, or they use
> OpenAMP. Either way, they need to share a cacheable memory region
> between them. The MPSoC is very flexible and the memory region can come
> from a multitude of sources, including a portion of normal memory, or a
> portion of a special memory area on the board. There are a couple of
> special SRAM banks 64K or 256K large that could be used for that. Also,
> PRAM can be easily added in fabric and used for the purpose.
>
> At the very least to handle the special memory regions, we need to be
> able to allow iomem to map them as cacheable memory to a DomU. So I do
> think we need this interface extension.
>
> Let me know if you still have any doubts/questions. Otherwise I'll work
> toward respinning the series in the proposed direction.
Well, as long as Julien and you agree that such an interface is
needed on Arm, this is fine with me.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
2019-02-26 23:07 ` [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability Stefano Stabellini
2019-02-26 23:18 ` Julien Grall
2019-02-27 10:34 ` Jan Beulich
@ 2019-02-27 19:28 ` Julien Grall
2019-04-19 23:20 ` [Xen-devel] " Stefano Stabellini
2019-02-27 21:02 ` Julien Grall
3 siblings, 1 reply; 67+ messages in thread
From: Julien Grall @ 2019-02-27 19:28 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel
Cc: Stefano Stabellini, JBeulich, andrew.cooper3
Hi Stefano,
On 2/26/19 11:07 PM, Stefano Stabellini wrote:
> Reuse the existing padding field to pass cacheability information about
> the memory mapping, specifically, whether the memory should be mapped as
> normal memory or as device memory (this is what we have today).
>
> Add a cacheability parameter to map_mmio_regions. 0 means device
> memory, which is what we have today.
>
> On ARM, map device memory as p2m_mmio_direct_dev (as it is already done
> today) and normal memory as p2m_ram_rw.
>
> On x86, return error if the cacheability requested is not device memory.
>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> CC: JBeulich@suse.com
> CC: andrew.cooper3@citrix.com
> ---
> xen/arch/arm/gic-v2.c | 3 ++-
> xen/arch/arm/p2m.c | 19 +++++++++++++++++--
> xen/arch/arm/platforms/exynos5.c | 4 ++--
> xen/arch/arm/platforms/omap5.c | 8 ++++----
> xen/arch/arm/vgic-v2.c | 2 +-
> xen/arch/arm/vgic/vgic-v2.c | 2 +-
> xen/arch/x86/hvm/dom0_build.c | 7 +++++--
> xen/arch/x86/mm/p2m.c | 6 +++++-
> xen/common/domctl.c | 8 +++++---
> xen/drivers/vpci/header.c | 3 ++-
> xen/include/public/domctl.h | 4 +++-
> xen/include/xen/p2m-common.h | 3 ++-
> 12 files changed, 49 insertions(+), 20 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index e7eb01f..1ea3da2 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -690,7 +690,8 @@ static int gicv2_map_hwdown_extra_mappings(struct domain *d)
>
> ret = map_mmio_regions(d, gaddr_to_gfn(v2m_data->addr),
> PFN_UP(v2m_data->size),
> - maddr_to_mfn(v2m_data->addr));
> + maddr_to_mfn(v2m_data->addr),
> + CACHEABILITY_DEVMEM);
> if ( ret )
> {
> printk(XENLOG_ERR "GICv2: Map v2m frame to d%d failed.\n",
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 30cfb01..5b8fcc5 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1068,9 +1068,24 @@ int unmap_regions_p2mt(struct domain *d,
> int map_mmio_regions(struct domain *d,
> gfn_t start_gfn,
> unsigned long nr,
> - mfn_t mfn)
> + mfn_t mfn,
> + uint32_t cache_policy)
Rather than extending map_mmio_regions, I would prefer if we kill this
function (and unmap_mmio_mmio_regions) and instead use map_regions_p2mt.
This means the conversation to p2mt should be done in the DOMCTL handling.
> {
> - return p2m_insert_mapping(d, start_gfn, nr, mfn, p2m_mmio_direct_dev);
> + p2m_type_t t;
> +
> + switch ( cache_policy )
> + {
> + case CACHEABILITY_MEMORY:
> + t = p2m_ram_rw;
> + break;
> + case CACHEABILITY_DEVMEM:
> + t = p2m_mmio_direct_dev;
> + break;
> + default:
> + return -ENOSYS;
We tend to use EOPNOTSUPP in such a case.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-19 23:20 ` Stefano Stabellini
0 siblings, 0 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-04-19 23:20 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, andrew.cooper3, JBeulich,
Stefano Stabellini
On Wed, 27 Feb 2019, Julien Grall wrote:
> Hi Stefano,
>
> On 2/26/19 11:07 PM, Stefano Stabellini wrote:
> > Reuse the existing padding field to pass cacheability information about
> > the memory mapping, specifically, whether the memory should be mapped as
> > normal memory or as device memory (this is what we have today).
> >
> > Add a cacheability parameter to map_mmio_regions. 0 means device
> > memory, which is what we have today.
> >
> > On ARM, map device memory as p2m_mmio_direct_dev (as it is already done
> > today) and normal memory as p2m_ram_rw.
> >
> > On x86, return error if the cacheability requested is not device memory.
> >
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > CC: JBeulich@suse.com
> > CC: andrew.cooper3@citrix.com
> > ---
> > xen/arch/arm/gic-v2.c | 3 ++-
> > xen/arch/arm/p2m.c | 19 +++++++++++++++++--
> > xen/arch/arm/platforms/exynos5.c | 4 ++--
> > xen/arch/arm/platforms/omap5.c | 8 ++++----
> > xen/arch/arm/vgic-v2.c | 2 +-
> > xen/arch/arm/vgic/vgic-v2.c | 2 +-
> > xen/arch/x86/hvm/dom0_build.c | 7 +++++--
> > xen/arch/x86/mm/p2m.c | 6 +++++-
> > xen/common/domctl.c | 8 +++++---
> > xen/drivers/vpci/header.c | 3 ++-
> > xen/include/public/domctl.h | 4 +++-
> > xen/include/xen/p2m-common.h | 3 ++-
> > 12 files changed, 49 insertions(+), 20 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> > index e7eb01f..1ea3da2 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -690,7 +690,8 @@ static int gicv2_map_hwdown_extra_mappings(struct domain
> > *d)
> > ret = map_mmio_regions(d, gaddr_to_gfn(v2m_data->addr),
> > PFN_UP(v2m_data->size),
> > - maddr_to_mfn(v2m_data->addr));
> > + maddr_to_mfn(v2m_data->addr),
> > + CACHEABILITY_DEVMEM);
> > if ( ret )
> > {
> > printk(XENLOG_ERR "GICv2: Map v2m frame to d%d failed.\n",
> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > index 30cfb01..5b8fcc5 100644
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -1068,9 +1068,24 @@ int unmap_regions_p2mt(struct domain *d,
> > int map_mmio_regions(struct domain *d,
> > gfn_t start_gfn,
> > unsigned long nr,
> > - mfn_t mfn)
> > + mfn_t mfn,
> > + uint32_t cache_policy)
>
> Rather than extending map_mmio_regions, I would prefer if we kill this
> function (and unmap_mmio_mmio_regions) and instead use map_regions_p2mt.
>
> This means the conversation to p2mt should be done in the DOMCTL handling.
map_regions_p2mt is an arm specific function. map_mmio_regions is the
common function across x86 and arm, called from common code.
> > {
> > - return p2m_insert_mapping(d, start_gfn, nr, mfn, p2m_mmio_direct_dev);
> > + p2m_type_t t;
> > +
> > + switch ( cache_policy )
> > + {
> > + case CACHEABILITY_MEMORY:
> > + t = p2m_ram_rw;
> > + break;
> > + case CACHEABILITY_DEVMEM:
> > + t = p2m_mmio_direct_dev;
> > + break;
> > + default:
> > + return -ENOSYS;
>
> We tend to use EOPNOTSUPP in such a case.
Yes, OK
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [Xen-devel] [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-19 23:20 ` Stefano Stabellini
0 siblings, 0 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-04-19 23:20 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, andrew.cooper3, JBeulich,
Stefano Stabellini
On Wed, 27 Feb 2019, Julien Grall wrote:
> Hi Stefano,
>
> On 2/26/19 11:07 PM, Stefano Stabellini wrote:
> > Reuse the existing padding field to pass cacheability information about
> > the memory mapping, specifically, whether the memory should be mapped as
> > normal memory or as device memory (this is what we have today).
> >
> > Add a cacheability parameter to map_mmio_regions. 0 means device
> > memory, which is what we have today.
> >
> > On ARM, map device memory as p2m_mmio_direct_dev (as it is already done
> > today) and normal memory as p2m_ram_rw.
> >
> > On x86, return error if the cacheability requested is not device memory.
> >
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > CC: JBeulich@suse.com
> > CC: andrew.cooper3@citrix.com
> > ---
> > xen/arch/arm/gic-v2.c | 3 ++-
> > xen/arch/arm/p2m.c | 19 +++++++++++++++++--
> > xen/arch/arm/platforms/exynos5.c | 4 ++--
> > xen/arch/arm/platforms/omap5.c | 8 ++++----
> > xen/arch/arm/vgic-v2.c | 2 +-
> > xen/arch/arm/vgic/vgic-v2.c | 2 +-
> > xen/arch/x86/hvm/dom0_build.c | 7 +++++--
> > xen/arch/x86/mm/p2m.c | 6 +++++-
> > xen/common/domctl.c | 8 +++++---
> > xen/drivers/vpci/header.c | 3 ++-
> > xen/include/public/domctl.h | 4 +++-
> > xen/include/xen/p2m-common.h | 3 ++-
> > 12 files changed, 49 insertions(+), 20 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> > index e7eb01f..1ea3da2 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -690,7 +690,8 @@ static int gicv2_map_hwdown_extra_mappings(struct domain
> > *d)
> > ret = map_mmio_regions(d, gaddr_to_gfn(v2m_data->addr),
> > PFN_UP(v2m_data->size),
> > - maddr_to_mfn(v2m_data->addr));
> > + maddr_to_mfn(v2m_data->addr),
> > + CACHEABILITY_DEVMEM);
> > if ( ret )
> > {
> > printk(XENLOG_ERR "GICv2: Map v2m frame to d%d failed.\n",
> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > index 30cfb01..5b8fcc5 100644
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -1068,9 +1068,24 @@ int unmap_regions_p2mt(struct domain *d,
> > int map_mmio_regions(struct domain *d,
> > gfn_t start_gfn,
> > unsigned long nr,
> > - mfn_t mfn)
> > + mfn_t mfn,
> > + uint32_t cache_policy)
>
> Rather than extending map_mmio_regions, I would prefer if we kill this
> function (and unmap_mmio_mmio_regions) and instead use map_regions_p2mt.
>
> This means the conversation to p2mt should be done in the DOMCTL handling.
map_regions_p2mt is an arm specific function. map_mmio_regions is the
common function across x86 and arm, called from common code.
> > {
> > - return p2m_insert_mapping(d, start_gfn, nr, mfn, p2m_mmio_direct_dev);
> > + p2m_type_t t;
> > +
> > + switch ( cache_policy )
> > + {
> > + case CACHEABILITY_MEMORY:
> > + t = p2m_ram_rw;
> > + break;
> > + case CACHEABILITY_DEVMEM:
> > + t = p2m_mmio_direct_dev;
> > + break;
> > + default:
> > + return -ENOSYS;
>
> We tend to use EOPNOTSUPP in such a case.
Yes, OK
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-21 17:14 ` Julien Grall
0 siblings, 0 replies; 67+ messages in thread
From: Julien Grall @ 2019-04-21 17:14 UTC (permalink / raw)
To: Stefano Stabellini
Cc: xen-devel, andrew.cooper3, JBeulich, Stefano Stabellini
Hi Stefano,
On 4/20/19 12:20 AM, Stefano Stabellini wrote:
> On Wed, 27 Feb 2019, Julien Grall wrote:
>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>> index 30cfb01..5b8fcc5 100644
>>> --- a/xen/arch/arm/p2m.c
>>> +++ b/xen/arch/arm/p2m.c
>>> @@ -1068,9 +1068,24 @@ int unmap_regions_p2mt(struct domain *d,
>>> int map_mmio_regions(struct domain *d,
>>> gfn_t start_gfn,
>>> unsigned long nr,
>>> - mfn_t mfn)
>>> + mfn_t mfn,
>>> + uint32_t cache_policy)
>>
>> Rather than extending map_mmio_regions, I would prefer if we kill this
>> function (and unmap_mmio_mmio_regions) and instead use map_regions_p2mt.
>>
>> This means the conversation to p2mt should be done in the DOMCTL handling.
>
> map_regions_p2mt is an arm specific function. map_mmio_regions is the
> common function across x86 and arm, called from common code.
I really dislike the idea to have two functions doing exactly the same
but have different parameters.
If map_regions_p2mt can't be used in the common code, then I would like
that map_mmio_regions to be renamed to arch_domctl_map_mmio_regions (or
something similar). So it is pretty clear it should be not used in other
places.
All the other callers of map_mmio_regions should be replaced with
map_regions_p2mt.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Xen-devel] [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-21 17:14 ` Julien Grall
0 siblings, 0 replies; 67+ messages in thread
From: Julien Grall @ 2019-04-21 17:14 UTC (permalink / raw)
To: Stefano Stabellini
Cc: xen-devel, andrew.cooper3, JBeulich, Stefano Stabellini
Hi Stefano,
On 4/20/19 12:20 AM, Stefano Stabellini wrote:
> On Wed, 27 Feb 2019, Julien Grall wrote:
>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>> index 30cfb01..5b8fcc5 100644
>>> --- a/xen/arch/arm/p2m.c
>>> +++ b/xen/arch/arm/p2m.c
>>> @@ -1068,9 +1068,24 @@ int unmap_regions_p2mt(struct domain *d,
>>> int map_mmio_regions(struct domain *d,
>>> gfn_t start_gfn,
>>> unsigned long nr,
>>> - mfn_t mfn)
>>> + mfn_t mfn,
>>> + uint32_t cache_policy)
>>
>> Rather than extending map_mmio_regions, I would prefer if we kill this
>> function (and unmap_mmio_mmio_regions) and instead use map_regions_p2mt.
>>
>> This means the conversation to p2mt should be done in the DOMCTL handling.
>
> map_regions_p2mt is an arm specific function. map_mmio_regions is the
> common function across x86 and arm, called from common code.
I really dislike the idea to have two functions doing exactly the same
but have different parameters.
If map_regions_p2mt can't be used in the common code, then I would like
that map_mmio_regions to be renamed to arch_domctl_map_mmio_regions (or
something similar). So it is pretty clear it should be not used in other
places.
All the other callers of map_mmio_regions should be replaced with
map_regions_p2mt.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-22 17:33 ` Stefano Stabellini
0 siblings, 0 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-04-22 17:33 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, andrew.cooper3, JBeulich,
Stefano Stabellini
On Sun, 21 Apr 2019, Julien Grall wrote:
> Hi Stefano,
>
> On 4/20/19 12:20 AM, Stefano Stabellini wrote:
> > On Wed, 27 Feb 2019, Julien Grall wrote:
> > > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > > > index 30cfb01..5b8fcc5 100644
> > > > --- a/xen/arch/arm/p2m.c
> > > > +++ b/xen/arch/arm/p2m.c
> > > > @@ -1068,9 +1068,24 @@ int unmap_regions_p2mt(struct domain *d,
> > > > int map_mmio_regions(struct domain *d,
> > > > gfn_t start_gfn,
> > > > unsigned long nr,
> > > > - mfn_t mfn)
> > > > + mfn_t mfn,
> > > > + uint32_t cache_policy)
> > >
> > > Rather than extending map_mmio_regions, I would prefer if we kill this
> > > function (and unmap_mmio_mmio_regions) and instead use map_regions_p2mt.
> > >
> > > This means the conversation to p2mt should be done in the DOMCTL handling.
> >
> > map_regions_p2mt is an arm specific function. map_mmio_regions is the
> > common function across x86 and arm, called from common code.
>
> I really dislike the idea to have two functions doing exactly the same but
> have different parameters.
In all fairness, it was already the case before this patch series:
int map_mmio_regions(struct domain *d,
gfn_t start_gfn,
unsigned long nr,
mfn_t mfn);
int map_regions_p2mt(struct domain *d,
gfn_t gfn,
unsigned long nr,
mfn_t mfn,
p2m_type_t p2mt);
> If map_regions_p2mt can't be used in the common code, then I would like that
> map_mmio_regions to be renamed to arch_domctl_map_mmio_regions (or something
> similar). So it is pretty clear it should be not used in other places.
>
> All the other callers of map_mmio_regions should be replaced with
> map_regions_p2mt.
Either way works, but I would like an agreement with the x86 maintainers
before I make any changes to the common interface.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [Xen-devel] [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-22 17:33 ` Stefano Stabellini
0 siblings, 0 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-04-22 17:33 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, andrew.cooper3, JBeulich,
Stefano Stabellini
On Sun, 21 Apr 2019, Julien Grall wrote:
> Hi Stefano,
>
> On 4/20/19 12:20 AM, Stefano Stabellini wrote:
> > On Wed, 27 Feb 2019, Julien Grall wrote:
> > > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > > > index 30cfb01..5b8fcc5 100644
> > > > --- a/xen/arch/arm/p2m.c
> > > > +++ b/xen/arch/arm/p2m.c
> > > > @@ -1068,9 +1068,24 @@ int unmap_regions_p2mt(struct domain *d,
> > > > int map_mmio_regions(struct domain *d,
> > > > gfn_t start_gfn,
> > > > unsigned long nr,
> > > > - mfn_t mfn)
> > > > + mfn_t mfn,
> > > > + uint32_t cache_policy)
> > >
> > > Rather than extending map_mmio_regions, I would prefer if we kill this
> > > function (and unmap_mmio_mmio_regions) and instead use map_regions_p2mt.
> > >
> > > This means the conversation to p2mt should be done in the DOMCTL handling.
> >
> > map_regions_p2mt is an arm specific function. map_mmio_regions is the
> > common function across x86 and arm, called from common code.
>
> I really dislike the idea to have two functions doing exactly the same but
> have different parameters.
In all fairness, it was already the case before this patch series:
int map_mmio_regions(struct domain *d,
gfn_t start_gfn,
unsigned long nr,
mfn_t mfn);
int map_regions_p2mt(struct domain *d,
gfn_t gfn,
unsigned long nr,
mfn_t mfn,
p2m_type_t p2mt);
> If map_regions_p2mt can't be used in the common code, then I would like that
> map_mmio_regions to be renamed to arch_domctl_map_mmio_regions (or something
> similar). So it is pretty clear it should be not used in other places.
>
> All the other callers of map_mmio_regions should be replaced with
> map_regions_p2mt.
Either way works, but I would like an agreement with the x86 maintainers
before I make any changes to the common interface.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-22 17:42 ` Julien Grall
0 siblings, 0 replies; 67+ messages in thread
From: Julien Grall @ 2019-04-22 17:42 UTC (permalink / raw)
To: Stefano Stabellini
Cc: xen-devel, nd, andrew.cooper3, JBeulich, Stefano Stabellini
Hi,
On 22/04/2019 18:33, Stefano Stabellini wrote:
> On Sun, 21 Apr 2019, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 4/20/19 12:20 AM, Stefano Stabellini wrote:
>>> On Wed, 27 Feb 2019, Julien Grall wrote:
>>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>>>> index 30cfb01..5b8fcc5 100644
>>>>> --- a/xen/arch/arm/p2m.c
>>>>> +++ b/xen/arch/arm/p2m.c
>>>>> @@ -1068,9 +1068,24 @@ int unmap_regions_p2mt(struct domain *d,
>>>>> int map_mmio_regions(struct domain *d,
>>>>> gfn_t start_gfn,
>>>>> unsigned long nr,
>>>>> - mfn_t mfn)
>>>>> + mfn_t mfn,
>>>>> + uint32_t cache_policy)
>>>>
>>>> Rather than extending map_mmio_regions, I would prefer if we kill this
>>>> function (and unmap_mmio_mmio_regions) and instead use map_regions_p2mt.
>>>>
>>>> This means the conversation to p2mt should be done in the DOMCTL handling.
>>>
>>> map_regions_p2mt is an arm specific function. map_mmio_regions is the
>>> common function across x86 and arm, called from common code.
>>
>> I really dislike the idea to have two functions doing exactly the same but
>> have different parameters.
>
> In all fairness, it was already the case before this patch series:
Not really...
>
> int map_mmio_regions(struct domain *d,
> gfn_t start_gfn,
> unsigned long nr,
> mfn_t mfn);
... this one always map with p2m_mmio_dev ...
> int map_regions_p2mt(struct domain *d,
> gfn_t gfn,
> unsigned long nr,
> mfn_t mfn,
> p2m_type_t p2mt);
... this one allow you to configure the p2m type.
Anyway, I wasn't already happy with that. Now, you have two functions
that will allow you to configure it. How does a user know which one to use?
>
>> If map_regions_p2mt can't be used in the common code, then I would like that
>> map_mmio_regions to be renamed to arch_domctl_map_mmio_regions (or something
>> similar). So it is pretty clear it should be not used in other places.
>>
>> All the other callers of map_mmio_regions should be replaced with
>> map_regions_p2mt.
>
> Either way works, but I would like an agreement with the x86 maintainers
> before I make any changes to the common interface.
Just to be clear, the suggested interface as it stands for Arm is a
no-go. I don't want to turn a bad interface to one that is worst and
more confusing.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Xen-devel] [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
@ 2019-04-22 17:42 ` Julien Grall
0 siblings, 0 replies; 67+ messages in thread
From: Julien Grall @ 2019-04-22 17:42 UTC (permalink / raw)
To: Stefano Stabellini
Cc: xen-devel, nd, andrew.cooper3, JBeulich, Stefano Stabellini
Hi,
On 22/04/2019 18:33, Stefano Stabellini wrote:
> On Sun, 21 Apr 2019, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 4/20/19 12:20 AM, Stefano Stabellini wrote:
>>> On Wed, 27 Feb 2019, Julien Grall wrote:
>>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>>>> index 30cfb01..5b8fcc5 100644
>>>>> --- a/xen/arch/arm/p2m.c
>>>>> +++ b/xen/arch/arm/p2m.c
>>>>> @@ -1068,9 +1068,24 @@ int unmap_regions_p2mt(struct domain *d,
>>>>> int map_mmio_regions(struct domain *d,
>>>>> gfn_t start_gfn,
>>>>> unsigned long nr,
>>>>> - mfn_t mfn)
>>>>> + mfn_t mfn,
>>>>> + uint32_t cache_policy)
>>>>
>>>> Rather than extending map_mmio_regions, I would prefer if we kill this
>>>> function (and unmap_mmio_mmio_regions) and instead use map_regions_p2mt.
>>>>
>>>> This means the conversation to p2mt should be done in the DOMCTL handling.
>>>
>>> map_regions_p2mt is an arm specific function. map_mmio_regions is the
>>> common function across x86 and arm, called from common code.
>>
>> I really dislike the idea to have two functions doing exactly the same but
>> have different parameters.
>
> In all fairness, it was already the case before this patch series:
Not really...
>
> int map_mmio_regions(struct domain *d,
> gfn_t start_gfn,
> unsigned long nr,
> mfn_t mfn);
... this one always map with p2m_mmio_dev ...
> int map_regions_p2mt(struct domain *d,
> gfn_t gfn,
> unsigned long nr,
> mfn_t mfn,
> p2m_type_t p2mt);
... this one allow you to configure the p2m type.
Anyway, I wasn't already happy with that. Now, you have two functions
that will allow you to configure it. How does a user know which one to use?
>
>> If map_regions_p2mt can't be used in the common code, then I would like that
>> map_mmio_regions to be renamed to arch_domctl_map_mmio_regions (or something
>> similar). So it is pretty clear it should be not used in other places.
>>
>> All the other callers of map_mmio_regions should be replaced with
>> map_regions_p2mt.
>
> Either way works, but I would like an agreement with the x86 maintainers
> before I make any changes to the common interface.
Just to be clear, the suggested interface as it stands for Arm is a
no-go. I don't want to turn a bad interface to one that is worst and
more confusing.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability
2019-02-26 23:07 ` [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability Stefano Stabellini
` (2 preceding siblings ...)
2019-02-27 19:28 ` Julien Grall
@ 2019-02-27 21:02 ` Julien Grall
3 siblings, 0 replies; 67+ messages in thread
From: Julien Grall @ 2019-02-27 21:02 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel
Cc: Stefano Stabellini, JBeulich, andrew.cooper3
Hi Stefano,
On 2/26/19 11:07 PM, Stefano Stabellini wrote:
> struct xen_domctl_memory_mapping {
> uint64_aligned_t first_gfn; /* first page (hvm guest phys page) in range */
> uint64_aligned_t first_mfn; /* first page (machine page) in range */
> uint64_aligned_t nr_mfns; /* number of pages in range (>0) */
> uint32_t add_mapping; /* add or remove mapping */
> - uint32_t padding; /* padding for 64-bit aligned structure */
> + uint32_t cache_policy; /* cacheability of the memory mapping */
Looking at this and the way you use it, the naming "cache" is quite
confusing. On Arm, they are memory types (see B2.7 "Memory types and
attributes" in DDI 0487D.a) and then you may have attribute such
cachability attribute (write-through, write-back...) on top. The
cacheability is also not applicable for "device memory".
"device memory" have other attributes related to gathering, re-ordering...
So a better naming would probably be "memory_policy".
Furthermore, those policies are only for configuring stage-2. The
resulting memory type and attributes will be whatever is the strongest
between stage-2 and stage-1 attributes. You can see the stage-2
attributes as a way to give more or less freedom to the guest for
configure the attributes.
For instance, by using p2m_mmio_direct_dev, the resulting attributes
will always be Device-nGnRnE whatever how stage-1 has been configured.
In the case of p2m_mmio_direct_c (similar to p2m_ram_rw). The guest will
be free to chose whatever pretty much any attributes (even Device-nGnRnE).
You might wonder why we didn't give more freedom to the guest from the
start. One of the reason is it is quite unclear what are the consequence
if you give that freedom to the guest. Whether there might be issues
with the device when the attributes are not correct.
Furthermore, there are more handling required in the hypervisor as if
the memory can be cached, you will need to clear the cache in order to
prevent leakage to another domain if the mappings get reassigned.
For completeness, I should mention the feature S2FWB present in ARMv8.4
and onwards. From my understanding, this could be used to force
resulting memory type. I am not suggesting to implement it now, but we
should keep it in my mind while writing the interface exposed in libxl.
To summarize, if we go ahead, we should try to make the documentation
more clearer on what each policy means and the implications on the
guest. I think we should also mark this a not security supported because
it the unknown interactions with devices.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* [PATCH 2/6] libxc: xc_domain_memory_mapping, handle cacheability
2019-02-26 23:06 [PATCH 0/6] iomem cacheability Stefano Stabellini
2019-02-26 23:07 ` [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability Stefano Stabellini
@ 2019-02-26 23:07 ` Stefano Stabellini
2019-02-26 23:07 ` [PATCH 3/6] libxl/xl: add cacheability option to iomem Stefano Stabellini
` (4 subsequent siblings)
6 siblings, 0 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-02-26 23:07 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, julien.grall, sstabellini, ian.jackson, wei.liu2
Add an additional parameter to xc_domain_memory_mapping to pass
cacheability information. The same parameter values are the same for the
XEN_DOMCTL_memory_mapping hypercall (0 is device memory, 1 is normal
memory). Pass CACHEABILITY_DEVMEM by default -- no changes in behavior.
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
CC: ian.jackson@eu.citrix.com
CC: wei.liu2@citrix.com
---
tools/libxc/include/xenctrl.h | 3 ++-
tools/libxc/xc_domain.c | 6 ++++--
tools/libxl/libxl_create.c | 2 +-
3 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 97ae965..3b89b05 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1719,7 +1719,8 @@ int xc_domain_memory_mapping(xc_interface *xch,
unsigned long first_gfn,
unsigned long first_mfn,
unsigned long nr_mfns,
- uint32_t add_mapping);
+ uint32_t add_mapping,
+ uint32_t cache_policy);
int xc_domain_ioport_mapping(xc_interface *xch,
uint32_t domid,
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 05d771f..f22d176 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -2048,7 +2048,8 @@ int xc_domain_memory_mapping(
unsigned long first_gfn,
unsigned long first_mfn,
unsigned long nr_mfns,
- uint32_t add_mapping)
+ uint32_t add_mapping,
+ uint32_t cache_policy)
{
DECLARE_DOMCTL;
xc_dominfo_t info;
@@ -2070,6 +2071,7 @@ int xc_domain_memory_mapping(
domctl.cmd = XEN_DOMCTL_memory_mapping;
domctl.domain = domid;
domctl.u.memory_mapping.add_mapping = add_mapping;
+ domctl.u.memory_mapping.cache_policy = cache_policy;
max_batch_sz = nr_mfns;
do
{
@@ -2106,7 +2108,7 @@ int xc_domain_memory_mapping(
*/
if ( ret && add_mapping != DPCI_REMOVE_MAPPING )
xc_domain_memory_mapping(xch, domid, first_gfn, first_mfn, nr_mfns,
- DPCI_REMOVE_MAPPING);
+ DPCI_REMOVE_MAPPING, CACHEABILITY_DEVMEM);
/* We might get E2BIG so many times that we never advance. */
if ( !done && !ret )
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index fa57334..9a9b953 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1348,7 +1348,7 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
}
ret = xc_domain_memory_mapping(CTX->xch, domid,
io->gfn, io->start,
- io->number, 1);
+ io->number, 1, CACHEABILITY_DEVMEM);
if (ret < 0) {
LOGED(ERROR, domid,
"failed to map to domain iomem range %"PRIx64"-%"PRIx64
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH 3/6] libxl/xl: add cacheability option to iomem
2019-02-26 23:06 [PATCH 0/6] iomem cacheability Stefano Stabellini
2019-02-26 23:07 ` [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability Stefano Stabellini
2019-02-26 23:07 ` [PATCH 2/6] libxc: xc_domain_memory_mapping, " Stefano Stabellini
@ 2019-02-26 23:07 ` Stefano Stabellini
2019-02-27 20:02 ` Julien Grall
2019-02-26 23:07 ` [PATCH 4/6] xen/arm: keep track of reserved-memory regions Stefano Stabellini
` (3 subsequent siblings)
6 siblings, 1 reply; 67+ messages in thread
From: Stefano Stabellini @ 2019-02-26 23:07 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, julien.grall, sstabellini, ian.jackson, wei.liu2
Parse a new cacheability option for the iomem parameter, it can be
"devmem" for device memory mappings, which is the default, or "memory"
for normal memory mappings.
Store the parameter in a new field in libxl_iomem_range.
Pass the cacheability option to xc_domain_memory_mapping.
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
CC: ian.jackson@eu.citrix.com
CC: wei.liu2@citrix.com
---
docs/man/xl.cfg.pod.5.in | 4 +++-
tools/libxl/libxl_create.c | 12 +++++++++++-
tools/libxl/libxl_types.idl | 7 +++++++
tools/xl/xl_parse.c | 17 +++++++++++++++--
4 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index b1c0be1..655008a 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -1222,7 +1222,7 @@ is given in hexadecimal format and may either be a range, e.g. C<2f8-2ff>
It is recommended to only use this option for trusted VMs under
administrator's control.
-=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN]", "IOMEM_START,NUM_PAGES[@GFN]", ...]>
+=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN],CACHEABILITY", "IOMEM_START,NUM_PAGES[@GFN],CACHEABILITY", ...]>
Allow auto-translated domains to access specific hardware I/O memory pages.
@@ -1233,6 +1233,8 @@ B<GFN> is not specified, the mapping will be performed using B<IOMEM_START>
as a start in the guest's address space, therefore performing a 1:1 mapping
by default.
All of these values must be given in hexadecimal format.
+B<CACHEABILITY> can be "devmem" for device memory, the default if not
+specified, or it can be "memory" for normal memory.
Note that the IOMMU won't be updated with the mappings specified with this
option. This option therefore should not be used to pass through any
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 9a9b953..15edf1c 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -415,6 +415,14 @@ static void init_console_info(libxl__gc *gc,
Only 'channels' when mapped to consoles have a string name. */
}
+static uint32_t libxl__cacheability_to_xc(libxl_cacheability c)
+{
+ if (c == LIBXL_CACHEABILITY_MEMORY)
+ return CACHEABILITY_MEMORY;
+ /* default to devmem */
+ return CACHEABILITY_DEVMEM;
+}
+
int libxl__domain_build(libxl__gc *gc,
libxl_domain_config *d_config,
uint32_t domid,
@@ -1348,7 +1356,9 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
}
ret = xc_domain_memory_mapping(CTX->xch, domid,
io->gfn, io->start,
- io->number, 1, CACHEABILITY_DEVMEM);
+ io->number, 1,
+ libxl__cacheability_to_xc(
+ io->cache_policy));
if (ret < 0) {
LOGED(ERROR, domid,
"failed to map to domain iomem range %"PRIx64"-%"PRIx64
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 3b8f967..897c539 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -262,6 +262,11 @@ libxl_ioport_range = Struct("ioport_range", [
("number", uint32),
])
+libxl_cacheability = Enumeration("cacheability", [
+ (0, "devmem"),
+ (1, "memory"),
+ ], init_val = "LIBXL_CACHEABILITY_DEVMEM")
+
libxl_iomem_range = Struct("iomem_range", [
# start host frame number to be mapped to the guest
("start", uint64),
@@ -269,6 +274,8 @@ libxl_iomem_range = Struct("iomem_range", [
("number", uint64),
# guest frame number used as a start for the mapping
("gfn", uint64, {'init_val': "LIBXL_INVALID_GFN"}),
+ # cacheability of the memory region
+ ("cache_policy", libxl_cacheability),
])
libxl_vga_interface_info = Struct("vga_interface_info", [
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 352cd21..1da2670 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1883,6 +1883,7 @@ void parse_config_data(const char *config_source,
}
for (i = 0; i < num_iomem; i++) {
int used;
+ char cache[7];
buf = xlu_cfg_get_listitem (iomem, i);
if (!buf) {
@@ -1891,15 +1892,27 @@ void parse_config_data(const char *config_source,
exit(1);
}
libxl_iomem_range_init(&b_info->iomem[i]);
- ret = sscanf(buf, "%" SCNx64",%" SCNx64"%n@%" SCNx64"%n",
+ ret = sscanf(buf, "%" SCNx64",%" SCNx64"%n@%" SCNx64"%n,%6s%n",
&b_info->iomem[i].start,
&b_info->iomem[i].number, &used,
- &b_info->iomem[i].gfn, &used);
+ &b_info->iomem[i].gfn, &used,
+ cache, &used);
if (ret < 2 || buf[used] != '\0') {
fprintf(stderr,
"xl: Invalid argument parsing iomem: %s\n", buf);
exit(1);
}
+ if (ret == 4) {
+ if (!strcmp(cache, "memory"))
+ b_info->iomem[i].cache_policy = LIBXL_CACHEABILITY_MEMORY;
+ else if (!strcmp(cache, "devmem"))
+ b_info->iomem[i].cache_policy = LIBXL_CACHEABILITY_DEVMEM;
+ else {
+ fprintf(stderr,
+ "xl: Invalid iomem cache parameter: %s\n", cache);
+ exit(1);
+ }
+ }
}
}
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 67+ messages in thread* Re: [PATCH 3/6] libxl/xl: add cacheability option to iomem
2019-02-26 23:07 ` [PATCH 3/6] libxl/xl: add cacheability option to iomem Stefano Stabellini
@ 2019-02-27 20:02 ` Julien Grall
2019-04-19 23:13 ` [Xen-devel] " Stefano Stabellini
0 siblings, 1 reply; 67+ messages in thread
From: Julien Grall @ 2019-02-27 20:02 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel; +Cc: Stefano Stabellini, ian.jackson, wei.liu2
Hi Stefano,
On 2/26/19 11:07 PM, Stefano Stabellini wrote:
> Parse a new cacheability option for the iomem parameter, it can be
> "devmem" for device memory mappings, which is the default, or "memory"
> for normal memory mappings.
>
> Store the parameter in a new field in libxl_iomem_range.
>
> Pass the cacheability option to xc_domain_memory_mapping.
>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> CC: ian.jackson@eu.citrix.com
> CC: wei.liu2@citrix.com
> ---
> docs/man/xl.cfg.pod.5.in | 4 +++-
> tools/libxl/libxl_create.c | 12 +++++++++++-
> tools/libxl/libxl_types.idl | 7 +++++++
Extension of the libxl_types.idl should be companied with a new define
in libxl.h. So a toolstack can deal with multiple libxl version.
> tools/xl/xl_parse.c | 17 +++++++++++++++--
> 4 files changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
> index b1c0be1..655008a 100644
> --- a/docs/man/xl.cfg.pod.5.in
> +++ b/docs/man/xl.cfg.pod.5.in
> @@ -1222,7 +1222,7 @@ is given in hexadecimal format and may either be a range, e.g. C<2f8-2ff>
> It is recommended to only use this option for trusted VMs under
> administrator's control.
>
> -=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN]", "IOMEM_START,NUM_PAGES[@GFN]", ...]>
> +=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN],CACHEABILITY", "IOMEM_START,NUM_PAGES[@GFN],CACHEABILITY", ...]>
Below you suggest the cacheability is option. However, the is not
reflected here. I think you want to put ',CACHEABILITY' between [] as it
is done for '@GFN'.
>
> Allow auto-translated domains to access specific hardware I/O memory pages.
>
> @@ -1233,6 +1233,8 @@ B<GFN> is not specified, the mapping will be performed using B<IOMEM_START>
> as a start in the guest's address space, therefore performing a 1:1 mapping
> by default.
> All of these values must be given in hexadecimal format.
> +B<CACHEABILITY> can be "devmem" for device memory, the default if not
> +specified, or it can be "memory" for normal memory.
I was planning to comment about the naming and documentation. But I will
do it in patch #1 where Jan already started a discussion about it.
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 352cd21..1da2670 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1883,6 +1883,7 @@ void parse_config_data(const char *config_source,
> }
> for (i = 0; i < num_iomem; i++) {
> int used;
> + char cache[7];
>
> buf = xlu_cfg_get_listitem (iomem, i);
> if (!buf) {
> @@ -1891,15 +1892,27 @@ void parse_config_data(const char *config_source,
> exit(1);
> }
> libxl_iomem_range_init(&b_info->iomem[i]);
> - ret = sscanf(buf, "%" SCNx64",%" SCNx64"%n@%" SCNx64"%n",
> + ret = sscanf(buf, "%" SCNx64",%" SCNx64"%n@%" SCNx64"%n,%6s%n",
> &b_info->iomem[i].start,
> &b_info->iomem[i].number, &used,
> - &b_info->iomem[i].gfn, &used);
> + &b_info->iomem[i].gfn, &used,
> + cache, &used);
If I read it correctly, you will require the GFN to be specified in
order to get the "cacheability". Am I correct?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH 3/6] libxl/xl: add cacheability option to iomem
@ 2019-04-19 23:13 ` Stefano Stabellini
0 siblings, 0 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-04-19 23:13 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, ian.jackson, wei.liu2, Stefano Stabellini
On Wed, 27 Feb 2019, Julien Grall wrote:
> Hi Stefano,
>
> On 2/26/19 11:07 PM, Stefano Stabellini wrote:
> > Parse a new cacheability option for the iomem parameter, it can be
> > "devmem" for device memory mappings, which is the default, or "memory"
> > for normal memory mappings.
> >
> > Store the parameter in a new field in libxl_iomem_range.
> >
> > Pass the cacheability option to xc_domain_memory_mapping.
> >
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > CC: ian.jackson@eu.citrix.com
> > CC: wei.liu2@citrix.com
> > ---
> > docs/man/xl.cfg.pod.5.in | 4 +++-
> > tools/libxl/libxl_create.c | 12 +++++++++++-
> > tools/libxl/libxl_types.idl | 7 +++++++
>
> Extension of the libxl_types.idl should be companied with a new define in
> libxl.h. So a toolstack can deal with multiple libxl version.
I'll do
> > tools/xl/xl_parse.c | 17 +++++++++++++++--
> > 4 files changed, 36 insertions(+), 4 deletions(-)
> >
> > diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
> > index b1c0be1..655008a 100644
> > --- a/docs/man/xl.cfg.pod.5.in
> > +++ b/docs/man/xl.cfg.pod.5.in
> > @@ -1222,7 +1222,7 @@ is given in hexadecimal format and may either be a
> > range, e.g. C<2f8-2ff>
> > It is recommended to only use this option for trusted VMs under
> > administrator's control.
> > -=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN]",
> > "IOMEM_START,NUM_PAGES[@GFN]", ...]>
> > +=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN],CACHEABILITY",
> > "IOMEM_START,NUM_PAGES[@GFN],CACHEABILITY", ...]>
>
> Below you suggest the cacheability is option. However, the is not reflected
> here. I think you want to put ',CACHEABILITY' between [] as it is done for
> '@GFN'.
Good point
> > Allow auto-translated domains to access specific hardware I/O memory
> > pages.
> > @@ -1233,6 +1233,8 @@ B<GFN> is not specified, the mapping will be
> > performed using B<IOMEM_START>
> > as a start in the guest's address space, therefore performing a 1:1
> > mapping
> > by default.
> > All of these values must be given in hexadecimal format.
> > +B<CACHEABILITY> can be "devmem" for device memory, the default if not
> > +specified, or it can be "memory" for normal memory.
>
> I was planning to comment about the naming and documentation. But I will do it
> in patch #1 where Jan already started a discussion about it.
All right. I don't have good ideas about naming. I am happy to change.
> > diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> > index 352cd21..1da2670 100644
> > --- a/tools/xl/xl_parse.c
> > +++ b/tools/xl/xl_parse.c
> > @@ -1883,6 +1883,7 @@ void parse_config_data(const char *config_source,
> > }
> > for (i = 0; i < num_iomem; i++) {
> > int used;
> > + char cache[7];
> > buf = xlu_cfg_get_listitem (iomem, i);
> > if (!buf) {
> > @@ -1891,15 +1892,27 @@ void parse_config_data(const char *config_source,
> > exit(1);
> > }
> > libxl_iomem_range_init(&b_info->iomem[i]);
> > - ret = sscanf(buf, "%" SCNx64",%" SCNx64"%n@%" SCNx64"%n",
> > + ret = sscanf(buf, "%" SCNx64",%" SCNx64"%n@%" SCNx64"%n,%6s%n",
> > &b_info->iomem[i].start,
> > &b_info->iomem[i].number, &used,
> > - &b_info->iomem[i].gfn, &used);
> > + &b_info->iomem[i].gfn, &used,
> > + cache, &used);
>
> If I read it correctly, you will require the GFN to be specified in order to
> get the "cacheability". Am I correct?
Yes, I was lazy :-) I'll fix it.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [Xen-devel] [PATCH 3/6] libxl/xl: add cacheability option to iomem
@ 2019-04-19 23:13 ` Stefano Stabellini
0 siblings, 0 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-04-19 23:13 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, ian.jackson, wei.liu2, Stefano Stabellini
On Wed, 27 Feb 2019, Julien Grall wrote:
> Hi Stefano,
>
> On 2/26/19 11:07 PM, Stefano Stabellini wrote:
> > Parse a new cacheability option for the iomem parameter, it can be
> > "devmem" for device memory mappings, which is the default, or "memory"
> > for normal memory mappings.
> >
> > Store the parameter in a new field in libxl_iomem_range.
> >
> > Pass the cacheability option to xc_domain_memory_mapping.
> >
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > CC: ian.jackson@eu.citrix.com
> > CC: wei.liu2@citrix.com
> > ---
> > docs/man/xl.cfg.pod.5.in | 4 +++-
> > tools/libxl/libxl_create.c | 12 +++++++++++-
> > tools/libxl/libxl_types.idl | 7 +++++++
>
> Extension of the libxl_types.idl should be companied with a new define in
> libxl.h. So a toolstack can deal with multiple libxl version.
I'll do
> > tools/xl/xl_parse.c | 17 +++++++++++++++--
> > 4 files changed, 36 insertions(+), 4 deletions(-)
> >
> > diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
> > index b1c0be1..655008a 100644
> > --- a/docs/man/xl.cfg.pod.5.in
> > +++ b/docs/man/xl.cfg.pod.5.in
> > @@ -1222,7 +1222,7 @@ is given in hexadecimal format and may either be a
> > range, e.g. C<2f8-2ff>
> > It is recommended to only use this option for trusted VMs under
> > administrator's control.
> > -=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN]",
> > "IOMEM_START,NUM_PAGES[@GFN]", ...]>
> > +=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN],CACHEABILITY",
> > "IOMEM_START,NUM_PAGES[@GFN],CACHEABILITY", ...]>
>
> Below you suggest the cacheability is option. However, the is not reflected
> here. I think you want to put ',CACHEABILITY' between [] as it is done for
> '@GFN'.
Good point
> > Allow auto-translated domains to access specific hardware I/O memory
> > pages.
> > @@ -1233,6 +1233,8 @@ B<GFN> is not specified, the mapping will be
> > performed using B<IOMEM_START>
> > as a start in the guest's address space, therefore performing a 1:1
> > mapping
> > by default.
> > All of these values must be given in hexadecimal format.
> > +B<CACHEABILITY> can be "devmem" for device memory, the default if not
> > +specified, or it can be "memory" for normal memory.
>
> I was planning to comment about the naming and documentation. But I will do it
> in patch #1 where Jan already started a discussion about it.
All right. I don't have good ideas about naming. I am happy to change.
> > diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> > index 352cd21..1da2670 100644
> > --- a/tools/xl/xl_parse.c
> > +++ b/tools/xl/xl_parse.c
> > @@ -1883,6 +1883,7 @@ void parse_config_data(const char *config_source,
> > }
> > for (i = 0; i < num_iomem; i++) {
> > int used;
> > + char cache[7];
> > buf = xlu_cfg_get_listitem (iomem, i);
> > if (!buf) {
> > @@ -1891,15 +1892,27 @@ void parse_config_data(const char *config_source,
> > exit(1);
> > }
> > libxl_iomem_range_init(&b_info->iomem[i]);
> > - ret = sscanf(buf, "%" SCNx64",%" SCNx64"%n@%" SCNx64"%n",
> > + ret = sscanf(buf, "%" SCNx64",%" SCNx64"%n@%" SCNx64"%n,%6s%n",
> > &b_info->iomem[i].start,
> > &b_info->iomem[i].number, &used,
> > - &b_info->iomem[i].gfn, &used);
> > + &b_info->iomem[i].gfn, &used,
> > + cache, &used);
>
> If I read it correctly, you will require the GFN to be specified in order to
> get the "cacheability". Am I correct?
Yes, I was lazy :-) I'll fix it.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* [PATCH 4/6] xen/arm: keep track of reserved-memory regions
2019-02-26 23:06 [PATCH 0/6] iomem cacheability Stefano Stabellini
` (2 preceding siblings ...)
2019-02-26 23:07 ` [PATCH 3/6] libxl/xl: add cacheability option to iomem Stefano Stabellini
@ 2019-02-26 23:07 ` Stefano Stabellini
2019-02-28 14:38 ` Julien Grall
2019-02-26 23:07 ` [PATCH 5/6] xen/arm: map reserved-memory regions as normal memory in dom0 Stefano Stabellini
` (2 subsequent siblings)
6 siblings, 1 reply; 67+ messages in thread
From: Stefano Stabellini @ 2019-02-26 23:07 UTC (permalink / raw)
To: xen-devel; +Cc: Stefano Stabellini, julien.grall, sstabellini
As we parse the device tree in Xen, keep track of the reserved-memory
regions as they need special treatment (follow-up patches will make use
of the stored information.)
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
xen/arch/arm/bootfdt.c | 73 +++++++++++++++++++++++++++++++++++++++++++++
xen/include/asm-arm/setup.h | 1 +
2 files changed, 74 insertions(+)
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 44af11c..a86b1b3 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -163,6 +163,76 @@ static void __init process_memory_node(const void *fdt, int node,
}
}
+static void __init process_reserved_memory_node(const void *fdt,
+ int node,
+ int depth,
+ const char *name,
+ u32 as,
+ u32 ss)
+{
+ const struct fdt_property *prop;
+ int i;
+ int banks;
+ const __be32 *cell;
+ paddr_t start, size;
+ u32 reg_cells;
+ u32 address_cells[DEVICE_TREE_MAX_DEPTH];
+ u32 size_cells[DEVICE_TREE_MAX_DEPTH];
+
+ address_cells[depth] = as;
+ size_cells[depth] = ss;
+ node = fdt_next_node(fdt, node, &depth);
+
+ for ( ; node >= 0 && depth > 1;
+ node = fdt_next_node(fdt, node, &depth) )
+ {
+ name = fdt_get_name(fdt, node, NULL);
+
+ if ( depth >= DEVICE_TREE_MAX_DEPTH )
+ {
+ printk("Warning: device tree node `%s' is nested too deep\n",
+ name);
+ continue;
+ }
+
+ address_cells[depth] = device_tree_get_u32(fdt, node,
+ "#address-cells",
+ address_cells[depth-1]);
+ size_cells[depth] = device_tree_get_u32(fdt, node,
+ "#size-cells",
+ size_cells[depth-1]);
+ if ( address_cells[depth-1] < 1 || size_cells[depth-1] < 1 )
+ {
+ printk("fdt: node `%s': invalid #address-cells or #size-cells",
+ name);
+ continue;
+ }
+
+ prop = fdt_get_property(fdt, node, "reg", NULL);
+ if ( !prop )
+ {
+ printk("fdt: node `%s': missing `reg' property\n", name);
+ continue;
+ }
+
+ reg_cells = address_cells[depth-1] + size_cells[depth-1];
+ cell = (const __be32 *)prop->data;
+ banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
+
+ for ( i = 0; i < banks && bootinfo.reserved_mem.nr_banks < NR_MEM_BANKS; i++ )
+ {
+ device_tree_get_reg(&cell, address_cells[depth-1], size_cells[depth-1],
+ &start, &size);
+ if ( !size )
+ continue;
+
+ bootinfo.reserved_mem.bank[bootinfo.reserved_mem.nr_banks].start = start;
+ bootinfo.reserved_mem.bank[bootinfo.reserved_mem.nr_banks].size = size;
+ bootinfo.reserved_mem.nr_banks++;
+ }
+ }
+}
+
static void __init process_multiboot_node(const void *fdt, int node,
const char *name,
u32 address_cells, u32 size_cells)
@@ -286,6 +356,9 @@ static int __init early_scan_node(const void *fdt,
{
if ( device_tree_node_matches(fdt, node, "memory") )
process_memory_node(fdt, node, name, address_cells, size_cells);
+ else if ( device_tree_node_matches(fdt, node, "reserved-memory") )
+ process_reserved_memory_node(fdt, node, depth, name,
+ address_cells, size_cells);
else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ||
device_tree_node_compatible(fdt, node, "multiboot,module" ))
process_multiboot_node(fdt, node, name, address_cells, size_cells);
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index 11e1b2a..18eca89 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -45,6 +45,7 @@ struct bootmodules {
struct bootinfo {
struct meminfo mem;
+ struct meminfo reserved_mem;
struct bootmodules modules;
#ifdef CONFIG_ACPI
struct meminfo acpi;
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 67+ messages in thread* Re: [PATCH 4/6] xen/arm: keep track of reserved-memory regions
2019-02-26 23:07 ` [PATCH 4/6] xen/arm: keep track of reserved-memory regions Stefano Stabellini
@ 2019-02-28 14:38 ` Julien Grall
0 siblings, 0 replies; 67+ messages in thread
From: Julien Grall @ 2019-02-28 14:38 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel; +Cc: Stefano Stabellini
Hi,
On 2/26/19 11:07 PM, Stefano Stabellini wrote:
> As we parse the device tree in Xen, keep track of the reserved-memory
> regions as they need special treatment (follow-up patches will make use
> of the stored information.)
>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
> xen/arch/arm/bootfdt.c | 73 +++++++++++++++++++++++++++++++++++++++++++++
> xen/include/asm-arm/setup.h | 1 +
> 2 files changed, 74 insertions(+)
>
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 44af11c..a86b1b3 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -163,6 +163,76 @@ static void __init process_memory_node(const void *fdt, int node,
> }
> }
>
> +static void __init process_reserved_memory_node(const void *fdt,
> + int node,
> + int depth,
> + const char *name,
> + u32 as,
> + u32 ss)
> +{
> + const struct fdt_property *prop;
> + int i;
> + int banks;
> + const __be32 *cell;
> + paddr_t start, size;
> + u32 reg_cells;
> + u32 address_cells[DEVICE_TREE_MAX_DEPTH];
> + u32 size_cells[DEVICE_TREE_MAX_DEPTH];
> +
> + address_cells[depth] = as;
> + size_cells[depth] = ss;
> + node = fdt_next_node(fdt, node, &depth);
> +
> + for ( ; node >= 0 && depth > 1;
> + node = fdt_next_node(fdt, node, &depth) )
> + {
Please rework device_for_each_node to allow take a depth and node in
parameter.
> + name = fdt_get_name(fdt, node, NULL); > +
> + if ( depth >= DEVICE_TREE_MAX_DEPTH )
> + {
> + printk("Warning: device tree node `%s' is nested too deep\n",
> + name);
> + continue;
> + }
> +
> + address_cells[depth] = device_tree_get_u32(fdt, node,
> + "#address-cells",
> + address_cells[depth-1]);
> + size_cells[depth] = device_tree_get_u32(fdt, node,
> + "#size-cells",
> + size_cells[depth-1]);
> + if ( address_cells[depth-1] < 1 || size_cells[depth-1] < 1 )
Why this check? 0 is a valid value for "#address-cells" and "#size-cells".
> + {
> + printk("fdt: node `%s': invalid #address-cells or #size-cells",
> + name);
> + continue;
> + }
> +
> + prop = fdt_get_property(fdt, node, "reg", NULL);
> + if ( !prop )
> + {
> + printk("fdt: node `%s': missing `reg' property\n", name);
> + continue;
> + }
The property "reg" is not mandatory for a reserved-memory region. Also
looking at the code, this looks very similar to how we deal with memory
nodes. Can you please rework the code to avoid duplication?
> +
> + reg_cells = address_cells[depth-1] + size_cells[depth-1];
> + cell = (const __be32 *)prop->data;
> + banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
> +
> + for ( i = 0; i < banks && bootinfo.reserved_mem.nr_banks < NR_MEM_BANKS; i++ )
There are few problems with this code itself. For a first, you will
ignore the reserved-memory if there are no more space. This means that
Xen will be free to allocate the memory for a guest.
I don't think we want that. Note that it is fine to ignore memory banks
(see process_memory_node) as nothing bad could happen.
Furthermore, while today we would allow up to 128 reserved-memory
region. I think this is pretty fragile as Xen would break if the DT
contains more than that. But we can cross that later one.
Lastly, we start already have different way to deal with memory, acpi
tables, modules. Now you are adding the reserved-memory.
I think it is time for us to create something like e820 internally. This
would simplify greatly the setup_mm code as we would not need to go
through all the "reserved" block (modules,...) to check intersection.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* [PATCH 5/6] xen/arm: map reserved-memory regions as normal memory in dom0
2019-02-26 23:06 [PATCH 0/6] iomem cacheability Stefano Stabellini
` (3 preceding siblings ...)
2019-02-26 23:07 ` [PATCH 4/6] xen/arm: keep track of reserved-memory regions Stefano Stabellini
@ 2019-02-26 23:07 ` Stefano Stabellini
2019-02-26 23:45 ` Julien Grall
2019-02-26 23:07 ` [PATCH 6/6] xen/docs: how to map a page between dom0 and domU using iomem Stefano Stabellini
2019-03-03 17:20 ` [PATCH 0/6] iomem cacheability Amit Tomer
6 siblings, 1 reply; 67+ messages in thread
From: Stefano Stabellini @ 2019-02-26 23:07 UTC (permalink / raw)
To: xen-devel; +Cc: Stefano Stabellini, julien.grall, sstabellini
reserved-memory regions should be mapped as normal memory. At the
moment, they get remapped as device memory in dom0 because Xen doesn't
know any better. Add an explicit check for it.
However, reserved-memory regions are allowed to overlap partially or
completely with memory nodes. In these cases, the overlapping memory is
reserved-memory and should be handled accordingly.
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
xen/arch/arm/domain_build.c | 7 ++++
xen/arch/arm/setup.c | 84 +++++++++++++++++++++++++++++++++++++--------
2 files changed, 77 insertions(+), 14 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index f552154..c7df4cf 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1307,6 +1307,13 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
"WARNING: Path %s is reserved, skip the node as we may re-use the path.\n",
path);
+ /*
+ * reserved-memory ranges should be mapped as normal memory in the
+ * p2m.
+ */
+ if ( !strcmp(dt_node_name(node), "reserved-memory") )
+ p2mt = p2m_ram_rw;
+
res = handle_device(d, node, p2mt);
if ( res)
return res;
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 80f0028..74c4707 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -470,10 +470,52 @@ static void __init init_pdx(void)
}
}
+static void __init check_reserved_memory(paddr_t *bank_start, paddr_t *bank_size)
+{
+ paddr_t bank_end = *bank_start + *bank_size;
+ struct meminfo mem = bootinfo.mem;
+ int i;
+
+ for ( i = 0; i < bootinfo.reserved_mem.nr_banks; i++ )
+ {
+ struct membank rbank = bootinfo.reserved_mem.bank[i];
+
+ if ( *bank_start < rbank.start && bank_end <= rbank.start )
+ continue;
+
+ if ( *bank_start >= (rbank.start + rbank.size) )
+ continue;
+
+ /* memory bank overlaps with reserved memory region */
+ if ( rbank.start > *bank_start )
+ {
+ bank_end = rbank.start;
+ if ( *bank_start + *bank_size > rbank.start + rbank.size )
+ {
+ mem.bank[mem.nr_banks].start = rbank.start + rbank.size;
+ mem.bank[mem.nr_banks].size = *bank_start + *bank_size -
+ mem.bank[mem.nr_banks].start;
+ mem.nr_banks++;
+ }
+ }
+ else if ( rbank.start + rbank.size > *bank_start)
+ {
+ if (rbank.start + rbank.size < bank_end )
+ *bank_start = rbank.start + rbank.size;
+ else
+ *bank_start = bank_end;
+ }
+
+ *bank_size = bank_end - *bank_start;
+ }
+}
+
#ifdef CONFIG_ARM_32
static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
{
- paddr_t ram_start, ram_end, ram_size;
+ paddr_t ram_start = ~0;
+ paddr_t ram_end = 0;
+ paddr_t ram_size = 0;
paddr_t s, e;
unsigned long ram_pages;
unsigned long heap_pages, xenheap_pages, domheap_pages;
@@ -487,18 +529,19 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
init_pdx();
- ram_start = bootinfo.mem.bank[0].start;
- ram_size = bootinfo.mem.bank[0].size;
- ram_end = ram_start + ram_size;
-
- for ( i = 1; i < bootinfo.mem.nr_banks; i++ )
+ for ( i = 0; i < bootinfo.mem.nr_banks; i++ )
{
- paddr_t bank_start = bootinfo.mem.bank[i].start;
- paddr_t bank_size = bootinfo.mem.bank[i].size;
- paddr_t bank_end = bank_start + bank_size;
+ paddr_t bank_end;
- ram_size = ram_size + bank_size;
- ram_start = min(ram_start,bank_start);
+ check_reserved_memory(&bootinfo.mem.bank[i].start,
+ &bootinfo.mem.bank[i].size);
+
+ if ( !bootinfo.mem.bank[i].size )
+ continue;
+
+ bank_end = bootinfo.mem.bank[i].start + bootinfo.mem.bank[i].size;
+ ram_size = ram_size + bootinfo.mem.bank[i].size;
+ ram_start = min(ram_start, bootinfo.mem.bank[i].start);
ram_end = max(ram_end,bank_end);
}
@@ -570,6 +613,9 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
paddr_t bank_start = bootinfo.mem.bank[i].start;
paddr_t bank_end = bank_start + bootinfo.mem.bank[i].size;
+ if ( !bootinfo.mem.bank[i].size )
+ continue;
+
s = bank_start;
while ( s < bank_end )
{
@@ -627,11 +673,21 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
total_pages = 0;
for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ )
{
- paddr_t bank_start = bootinfo.mem.bank[bank].start;
- paddr_t bank_size = bootinfo.mem.bank[bank].size;
- paddr_t bank_end = bank_start + bank_size;
+ paddr_t bank_start;
+ paddr_t bank_size;
+ paddr_t bank_end;
paddr_t s, e;
+ check_reserved_memory(&bootinfo.mem.bank[bank].start,
+ &bootinfo.mem.bank[bank].size);
+
+ bank_start = bootinfo.mem.bank[bank].start;
+ bank_size = bootinfo.mem.bank[bank].size;
+ bank_end = bank_start + bank_size;
+
+ if ( !bank_size )
+ continue;
+
ram_size = ram_size + bank_size;
ram_start = min(ram_start,bank_start);
ram_end = max(ram_end,bank_end);
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 67+ messages in thread* Re: [PATCH 5/6] xen/arm: map reserved-memory regions as normal memory in dom0
2019-02-26 23:07 ` [PATCH 5/6] xen/arm: map reserved-memory regions as normal memory in dom0 Stefano Stabellini
@ 2019-02-26 23:45 ` Julien Grall
2019-04-22 22:42 ` [Xen-devel] " Stefano Stabellini
0 siblings, 1 reply; 67+ messages in thread
From: Julien Grall @ 2019-02-26 23:45 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel; +Cc: Stefano Stabellini, nd
Hi Stefano,
On 26/02/2019 23:07, Stefano Stabellini wrote:
> reserved-memory regions should be mapped as normal memory. At the
> moment, they get remapped as device memory in dom0 because Xen doesn't
> know any better. Add an explicit check for it.
You probably use an outdated change (> 2 years ago). In recent Xen, Dom0
MMIO are mapped use p2m_mmio_direct_c. This main difference with
p2m_ram_rw is the shareability attribute (inner vs outer).
This will also have the advantage to not impair with the rest of Xen.
But I don't think this would be enough. Per [1], reserved-memory region
is used to carve memory from /memory node. So those regions should be
described in /memory of the Dom0 DT as well.
>
> However, reserved-memory regions are allowed to overlap partially or
> completely with memory nodes. In these cases, the overlapping memory is
> reserved-memory and should be handled accordingly.
Do you mind providing your source? If you look at the description in
Linux bindings, it is clearly they will always overlap with /memory.
[...]
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 80f0028..74c4707 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -470,10 +470,52 @@ static void __init init_pdx(void)
> }
> }
>
> +static void __init check_reserved_memory(paddr_t *bank_start, paddr_t *bank_size)
> +{
> + paddr_t bank_end = *bank_start + *bank_size;
> + struct meminfo mem = bootinfo.mem;
> + int i;
> +
> + for ( i = 0; i < bootinfo.reserved_mem.nr_banks; i++ )
> + {
> + struct membank rbank = bootinfo.reserved_mem.bank[i];
> +
> + if ( *bank_start < rbank.start && bank_end <= rbank.start )
> + continue;
> +
> + if ( *bank_start >= (rbank.start + rbank.size) )
> + continue;
> +
> + /* memory bank overlaps with reserved memory region */
> + if ( rbank.start > *bank_start )
> + {
> + bank_end = rbank.start;
> + if ( *bank_start + *bank_size > rbank.start + rbank.size )
> + {
> + mem.bank[mem.nr_banks].start = rbank.start + rbank.size;
> + mem.bank[mem.nr_banks].size = *bank_start + *bank_size -
> + mem.bank[mem.nr_banks].start;
> + mem.nr_banks++;
> + }
> + }
> + else if ( rbank.start + rbank.size > *bank_start)
> + {
> + if (rbank.start + rbank.size < bank_end )
> + *bank_start = rbank.start + rbank.size;
> + else
> + *bank_start = bank_end;
> + }
> +
> + *bank_size = bank_end - *bank_start;
> + }
> +}
reserved-memory nodes is more nothing more than an extension of an old
DT binding for reserved memory. We handle them in a few places (see
consider_modules and dt_unreserved_region). So mostly likely you want to
extend what we already have.
This would avoid most (if not) all the changes below.
> +
> #ifdef CONFIG_ARM_32
> static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
> {
> - paddr_t ram_start, ram_end, ram_size;
> + paddr_t ram_start = ~0;
> + paddr_t ram_end = 0;
> + paddr_t ram_size = 0;
> paddr_t s, e;
> unsigned long ram_pages;
> unsigned long heap_pages, xenheap_pages, domheap_pages;
> @@ -487,18 +529,19 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
>
> init_pdx();
>
> - ram_start = bootinfo.mem.bank[0].start;
> - ram_size = bootinfo.mem.bank[0].size;
> - ram_end = ram_start + ram_size;
> -
> - for ( i = 1; i < bootinfo.mem.nr_banks; i++ )
> + for ( i = 0; i < bootinfo.mem.nr_banks; i++ )
> {
> - paddr_t bank_start = bootinfo.mem.bank[i].start;
> - paddr_t bank_size = bootinfo.mem.bank[i].size;
> - paddr_t bank_end = bank_start + bank_size;
> + paddr_t bank_end;
>
> - ram_size = ram_size + bank_size;
> - ram_start = min(ram_start,bank_start);
> + check_reserved_memory(&bootinfo.mem.bank[i].start,
> + &bootinfo.mem.bank[i].size);
> +
> + if ( !bootinfo.mem.bank[i].size )
> + continue;
> +
> + bank_end = bootinfo.mem.bank[i].start + bootinfo.mem.bank[i].size;
> + ram_size = ram_size + bootinfo.mem.bank[i].size;
> + ram_start = min(ram_start, bootinfo.mem.bank[i].start);
> ram_end = max(ram_end,bank_end);
> }
>
> @@ -570,6 +613,9 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
> paddr_t bank_start = bootinfo.mem.bank[i].start;
> paddr_t bank_end = bank_start + bootinfo.mem.bank[i].size;
>
> + if ( !bootinfo.mem.bank[i].size )
> + continue;
> +
> s = bank_start;
> while ( s < bank_end )
> {
> @@ -627,11 +673,21 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
> total_pages = 0;
> for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ )
> {
> - paddr_t bank_start = bootinfo.mem.bank[bank].start;
> - paddr_t bank_size = bootinfo.mem.bank[bank].size;
> - paddr_t bank_end = bank_start + bank_size;
> + paddr_t bank_start;
> + paddr_t bank_size;
> + paddr_t bank_end;
> paddr_t s, e;
>
> + check_reserved_memory(&bootinfo.mem.bank[bank].start,
> + &bootinfo.mem.bank[bank].size);
> +
> + bank_start = bootinfo.mem.bank[bank].start;
> + bank_size = bootinfo.mem.bank[bank].size;
> + bank_end = bank_start + bank_size;
> +
> + if ( !bank_size )
> + continue;
> +
> ram_size = ram_size + bank_size;
> ram_start = min(ram_start,bank_start);
> ram_end = max(ram_end,bank_end);
>
[1]
https://www.kernel.org/doc/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH 5/6] xen/arm: map reserved-memory regions as normal memory in dom0
@ 2019-04-22 22:42 ` Stefano Stabellini
0 siblings, 0 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-04-22 22:42 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, nd, Stefano Stabellini, Stefano Stabellini
On Tue, 26 Feb 2019, Julien Grall wrote:
> Hi Stefano,
>
> On 26/02/2019 23:07, Stefano Stabellini wrote:
> > reserved-memory regions should be mapped as normal memory. At the
> > moment, they get remapped as device memory in dom0 because Xen doesn't
> > know any better. Add an explicit check for it.
>
> You probably use an outdated change (> 2 years ago). In recent Xen, Dom0
> MMIO are mapped use p2m_mmio_direct_c. This main difference with
> p2m_ram_rw is the shareability attribute (inner vs outer).
>
> This will also have the advantage to not impair with the rest of Xen.
I have already fixed this in my tree.
> But I don't think this would be enough. Per [1], reserved-memory region
> is used to carve memory from /memory node. So those regions should be
> described in /memory of the Dom0 DT as well.
>
> >
> > However, reserved-memory regions are allowed to overlap partially or
> > completely with memory nodes. In these cases, the overlapping memory is
> > reserved-memory and should be handled accordingly.
>
> Do you mind providing your source? If you look at the description in
> Linux bindings, it is clearly they will always overlap with /memory.
You are right. Reserved-memory regions have to fully overlap with
/memory, and that assumption can simplify the implementation.
> [...]
>
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index 80f0028..74c4707 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -470,10 +470,52 @@ static void __init init_pdx(void)
> > }
> > }
> >
> > +static void __init check_reserved_memory(paddr_t *bank_start, paddr_t *bank_size)
> > +{
> > + paddr_t bank_end = *bank_start + *bank_size;
> > + struct meminfo mem = bootinfo.mem;
> > + int i;
> > +
> > + for ( i = 0; i < bootinfo.reserved_mem.nr_banks; i++ )
> > + {
> > + struct membank rbank = bootinfo.reserved_mem.bank[i];
> > +
> > + if ( *bank_start < rbank.start && bank_end <= rbank.start )
> > + continue;
> > +
> > + if ( *bank_start >= (rbank.start + rbank.size) )
> > + continue;
> > +
> > + /* memory bank overlaps with reserved memory region */
> > + if ( rbank.start > *bank_start )
> > + {
> > + bank_end = rbank.start;
> > + if ( *bank_start + *bank_size > rbank.start + rbank.size )
> > + {
> > + mem.bank[mem.nr_banks].start = rbank.start + rbank.size;
> > + mem.bank[mem.nr_banks].size = *bank_start + *bank_size -
> > + mem.bank[mem.nr_banks].start;
> > + mem.nr_banks++;
> > + }
> > + }
> > + else if ( rbank.start + rbank.size > *bank_start)
> > + {
> > + if (rbank.start + rbank.size < bank_end )
> > + *bank_start = rbank.start + rbank.size;
> > + else
> > + *bank_start = bank_end;
> > + }
> > +
> > + *bank_size = bank_end - *bank_start;
> > + }
> > +}
>
> reserved-memory nodes is more nothing more than an extension of an old
> DT binding for reserved memory. We handle them in a few places (see
> consider_modules and dt_unreserved_region). So mostly likely you want to
> extend what we already have.
>
> This would avoid most (if not) all the changes below.
I take your point that this code could be simplified because
reserved-memory has to be described under /memory too. I can do that.
I am not sure about the suggestion of re-using the libfdt concept of
"mem_rsv", which is meant to be for the old /memreserve/. Today, libfdt
(at least our version of it) is not able to parse the new
reserved-memory bindings. I don't think it is a good idea to modify
libfdt for that. Also, the way we want to handle the old memreserve
regions is quite different from the way we want to handle
reserved-memory, right? I cannot see a way to improve this code using
mem_rsv at the moment.
> > +
> > #ifdef CONFIG_ARM_32
> > static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
> > {
> > - paddr_t ram_start, ram_end, ram_size;
> > + paddr_t ram_start = ~0;
> > + paddr_t ram_end = 0;
> > + paddr_t ram_size = 0;
> > paddr_t s, e;
> > unsigned long ram_pages;
> > unsigned long heap_pages, xenheap_pages, domheap_pages;
> > @@ -487,18 +529,19 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
> >
> > init_pdx();
> >
> > - ram_start = bootinfo.mem.bank[0].start;
> > - ram_size = bootinfo.mem.bank[0].size;
> > - ram_end = ram_start + ram_size;
> > -
> > - for ( i = 1; i < bootinfo.mem.nr_banks; i++ )
> > + for ( i = 0; i < bootinfo.mem.nr_banks; i++ )
> > {
> > - paddr_t bank_start = bootinfo.mem.bank[i].start;
> > - paddr_t bank_size = bootinfo.mem.bank[i].size;
> > - paddr_t bank_end = bank_start + bank_size;
> > + paddr_t bank_end;
> >
> > - ram_size = ram_size + bank_size;
> > - ram_start = min(ram_start,bank_start);
> > + check_reserved_memory(&bootinfo.mem.bank[i].start,
> > + &bootinfo.mem.bank[i].size);
> > +
> > + if ( !bootinfo.mem.bank[i].size )
> > + continue;
> > +
> > + bank_end = bootinfo.mem.bank[i].start + bootinfo.mem.bank[i].size;
> > + ram_size = ram_size + bootinfo.mem.bank[i].size;
> > + ram_start = min(ram_start, bootinfo.mem.bank[i].start);
> > ram_end = max(ram_end,bank_end);
> > }
> >
> > @@ -570,6 +613,9 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
> > paddr_t bank_start = bootinfo.mem.bank[i].start;
> > paddr_t bank_end = bank_start + bootinfo.mem.bank[i].size;
> >
> > + if ( !bootinfo.mem.bank[i].size )
> > + continue;
> > +
> > s = bank_start;
> > while ( s < bank_end )
> > {
> > @@ -627,11 +673,21 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
> > total_pages = 0;
> > for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ )
> > {
> > - paddr_t bank_start = bootinfo.mem.bank[bank].start;
> > - paddr_t bank_size = bootinfo.mem.bank[bank].size;
> > - paddr_t bank_end = bank_start + bank_size;
> > + paddr_t bank_start;
> > + paddr_t bank_size;
> > + paddr_t bank_end;
> > paddr_t s, e;
> >
> > + check_reserved_memory(&bootinfo.mem.bank[bank].start,
> > + &bootinfo.mem.bank[bank].size);
> > +
> > + bank_start = bootinfo.mem.bank[bank].start;
> > + bank_size = bootinfo.mem.bank[bank].size;
> > + bank_end = bank_start + bank_size;
> > +
> > + if ( !bank_size )
> > + continue;
> > +
> > ram_size = ram_size + bank_size;
> > ram_start = min(ram_start,bank_start);
> > ram_end = max(ram_end,bank_end);
> >
>
> [1]
> https://www.kernel.org/doc/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>
> --
> Julien Grall
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [Xen-devel] [PATCH 5/6] xen/arm: map reserved-memory regions as normal memory in dom0
@ 2019-04-22 22:42 ` Stefano Stabellini
0 siblings, 0 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-04-22 22:42 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, nd, Stefano Stabellini, Stefano Stabellini
On Tue, 26 Feb 2019, Julien Grall wrote:
> Hi Stefano,
>
> On 26/02/2019 23:07, Stefano Stabellini wrote:
> > reserved-memory regions should be mapped as normal memory. At the
> > moment, they get remapped as device memory in dom0 because Xen doesn't
> > know any better. Add an explicit check for it.
>
> You probably use an outdated change (> 2 years ago). In recent Xen, Dom0
> MMIO are mapped use p2m_mmio_direct_c. This main difference with
> p2m_ram_rw is the shareability attribute (inner vs outer).
>
> This will also have the advantage to not impair with the rest of Xen.
I have already fixed this in my tree.
> But I don't think this would be enough. Per [1], reserved-memory region
> is used to carve memory from /memory node. So those regions should be
> described in /memory of the Dom0 DT as well.
>
> >
> > However, reserved-memory regions are allowed to overlap partially or
> > completely with memory nodes. In these cases, the overlapping memory is
> > reserved-memory and should be handled accordingly.
>
> Do you mind providing your source? If you look at the description in
> Linux bindings, it is clearly they will always overlap with /memory.
You are right. Reserved-memory regions have to fully overlap with
/memory, and that assumption can simplify the implementation.
> [...]
>
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index 80f0028..74c4707 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -470,10 +470,52 @@ static void __init init_pdx(void)
> > }
> > }
> >
> > +static void __init check_reserved_memory(paddr_t *bank_start, paddr_t *bank_size)
> > +{
> > + paddr_t bank_end = *bank_start + *bank_size;
> > + struct meminfo mem = bootinfo.mem;
> > + int i;
> > +
> > + for ( i = 0; i < bootinfo.reserved_mem.nr_banks; i++ )
> > + {
> > + struct membank rbank = bootinfo.reserved_mem.bank[i];
> > +
> > + if ( *bank_start < rbank.start && bank_end <= rbank.start )
> > + continue;
> > +
> > + if ( *bank_start >= (rbank.start + rbank.size) )
> > + continue;
> > +
> > + /* memory bank overlaps with reserved memory region */
> > + if ( rbank.start > *bank_start )
> > + {
> > + bank_end = rbank.start;
> > + if ( *bank_start + *bank_size > rbank.start + rbank.size )
> > + {
> > + mem.bank[mem.nr_banks].start = rbank.start + rbank.size;
> > + mem.bank[mem.nr_banks].size = *bank_start + *bank_size -
> > + mem.bank[mem.nr_banks].start;
> > + mem.nr_banks++;
> > + }
> > + }
> > + else if ( rbank.start + rbank.size > *bank_start)
> > + {
> > + if (rbank.start + rbank.size < bank_end )
> > + *bank_start = rbank.start + rbank.size;
> > + else
> > + *bank_start = bank_end;
> > + }
> > +
> > + *bank_size = bank_end - *bank_start;
> > + }
> > +}
>
> reserved-memory nodes is more nothing more than an extension of an old
> DT binding for reserved memory. We handle them in a few places (see
> consider_modules and dt_unreserved_region). So mostly likely you want to
> extend what we already have.
>
> This would avoid most (if not) all the changes below.
I take your point that this code could be simplified because
reserved-memory has to be described under /memory too. I can do that.
I am not sure about the suggestion of re-using the libfdt concept of
"mem_rsv", which is meant to be for the old /memreserve/. Today, libfdt
(at least our version of it) is not able to parse the new
reserved-memory bindings. I don't think it is a good idea to modify
libfdt for that. Also, the way we want to handle the old memreserve
regions is quite different from the way we want to handle
reserved-memory, right? I cannot see a way to improve this code using
mem_rsv at the moment.
> > +
> > #ifdef CONFIG_ARM_32
> > static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
> > {
> > - paddr_t ram_start, ram_end, ram_size;
> > + paddr_t ram_start = ~0;
> > + paddr_t ram_end = 0;
> > + paddr_t ram_size = 0;
> > paddr_t s, e;
> > unsigned long ram_pages;
> > unsigned long heap_pages, xenheap_pages, domheap_pages;
> > @@ -487,18 +529,19 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
> >
> > init_pdx();
> >
> > - ram_start = bootinfo.mem.bank[0].start;
> > - ram_size = bootinfo.mem.bank[0].size;
> > - ram_end = ram_start + ram_size;
> > -
> > - for ( i = 1; i < bootinfo.mem.nr_banks; i++ )
> > + for ( i = 0; i < bootinfo.mem.nr_banks; i++ )
> > {
> > - paddr_t bank_start = bootinfo.mem.bank[i].start;
> > - paddr_t bank_size = bootinfo.mem.bank[i].size;
> > - paddr_t bank_end = bank_start + bank_size;
> > + paddr_t bank_end;
> >
> > - ram_size = ram_size + bank_size;
> > - ram_start = min(ram_start,bank_start);
> > + check_reserved_memory(&bootinfo.mem.bank[i].start,
> > + &bootinfo.mem.bank[i].size);
> > +
> > + if ( !bootinfo.mem.bank[i].size )
> > + continue;
> > +
> > + bank_end = bootinfo.mem.bank[i].start + bootinfo.mem.bank[i].size;
> > + ram_size = ram_size + bootinfo.mem.bank[i].size;
> > + ram_start = min(ram_start, bootinfo.mem.bank[i].start);
> > ram_end = max(ram_end,bank_end);
> > }
> >
> > @@ -570,6 +613,9 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
> > paddr_t bank_start = bootinfo.mem.bank[i].start;
> > paddr_t bank_end = bank_start + bootinfo.mem.bank[i].size;
> >
> > + if ( !bootinfo.mem.bank[i].size )
> > + continue;
> > +
> > s = bank_start;
> > while ( s < bank_end )
> > {
> > @@ -627,11 +673,21 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
> > total_pages = 0;
> > for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ )
> > {
> > - paddr_t bank_start = bootinfo.mem.bank[bank].start;
> > - paddr_t bank_size = bootinfo.mem.bank[bank].size;
> > - paddr_t bank_end = bank_start + bank_size;
> > + paddr_t bank_start;
> > + paddr_t bank_size;
> > + paddr_t bank_end;
> > paddr_t s, e;
> >
> > + check_reserved_memory(&bootinfo.mem.bank[bank].start,
> > + &bootinfo.mem.bank[bank].size);
> > +
> > + bank_start = bootinfo.mem.bank[bank].start;
> > + bank_size = bootinfo.mem.bank[bank].size;
> > + bank_end = bank_start + bank_size;
> > +
> > + if ( !bank_size )
> > + continue;
> > +
> > ram_size = ram_size + bank_size;
> > ram_start = min(ram_start,bank_start);
> > ram_end = max(ram_end,bank_end);
> >
>
> [1]
> https://www.kernel.org/doc/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>
> --
> Julien Grall
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH 5/6] xen/arm: map reserved-memory regions as normal memory in dom0
@ 2019-04-23 8:09 ` Julien Grall
0 siblings, 0 replies; 67+ messages in thread
From: Julien Grall @ 2019-04-23 8:09 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, nd, Stefano Stabellini
Hi Stefano,
On 4/22/19 11:42 PM, Stefano Stabellini wrote:
> On Tue, 26 Feb 2019, Julien Grall wrote:
> I am not sure about the suggestion of re-using the libfdt concept of
> "mem_rsv", which is meant to be for the old /memreserve/. Today, libfdt
> (at least our version of it) is not able to parse the new
> reserved-memory bindings. I don't think it is a good idea to modify
> libfdt for that. Also, the way we want to handle the old memreserve
> regions is quite different from the way we want to handle
> reserved-memory, right? I cannot see a way to improve this code using
> mem_rsv at the moment.
I didn't mean to extend mem_rsv in libfdt but extend consider_modules
and dt_unreserved_regions to deal with /reserved-memory. Otherwise you
may miss some cases (for instance you left out discard_initial_modules).
By extending those two functions you don't have to teach everyone how to
skip /reserved-memory.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Xen-devel] [PATCH 5/6] xen/arm: map reserved-memory regions as normal memory in dom0
@ 2019-04-23 8:09 ` Julien Grall
0 siblings, 0 replies; 67+ messages in thread
From: Julien Grall @ 2019-04-23 8:09 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, nd, Stefano Stabellini
Hi Stefano,
On 4/22/19 11:42 PM, Stefano Stabellini wrote:
> On Tue, 26 Feb 2019, Julien Grall wrote:
> I am not sure about the suggestion of re-using the libfdt concept of
> "mem_rsv", which is meant to be for the old /memreserve/. Today, libfdt
> (at least our version of it) is not able to parse the new
> reserved-memory bindings. I don't think it is a good idea to modify
> libfdt for that. Also, the way we want to handle the old memreserve
> regions is quite different from the way we want to handle
> reserved-memory, right? I cannot see a way to improve this code using
> mem_rsv at the moment.
I didn't mean to extend mem_rsv in libfdt but extend consider_modules
and dt_unreserved_regions to deal with /reserved-memory. Otherwise you
may miss some cases (for instance you left out discard_initial_modules).
By extending those two functions you don't have to teach everyone how to
skip /reserved-memory.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH 5/6] xen/arm: map reserved-memory regions as normal memory in dom0
@ 2019-04-23 17:32 ` Stefano Stabellini
0 siblings, 0 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-04-23 17:32 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, nd, Stefano Stabellini, Stefano Stabellini
On Tue, 23 Apr 2019, Julien Grall wrote:
> Hi Stefano,
>
> On 4/22/19 11:42 PM, Stefano Stabellini wrote:
> > On Tue, 26 Feb 2019, Julien Grall wrote:
> > I am not sure about the suggestion of re-using the libfdt concept of
> > "mem_rsv", which is meant to be for the old /memreserve/. Today, libfdt
> > (at least our version of it) is not able to parse the new
> > reserved-memory bindings. I don't think it is a good idea to modify
> > libfdt for that. Also, the way we want to handle the old memreserve
> > regions is quite different from the way we want to handle
> > reserved-memory, right? I cannot see a way to improve this code using
> > mem_rsv at the moment.
>
> I didn't mean to extend mem_rsv in libfdt but extend consider_modules and
> dt_unreserved_regions to deal with /reserved-memory. Otherwise you
> may miss some cases (for instance you left out discard_initial_modules).
>
> By extending those two functions you don't have to teach everyone how to skip
> /reserved-memory.
I think I get your point now. Although I don't think it should be
correct for a bootloader to use a reserved-memory area to store a boot
module, I wouldn't be suprised if that happens, so it is better to be
prepared and extend dt_unreserved_regions. I'll do that.
However, we would still need something like check_reserved_memory,
because we don't want setup_xenheap_mappings to be called on the
reserved-memory area (or a memory region including the reserved memory
area) in setup_mm. I don't think we can get away without it, but I can
simplify it.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Xen-devel] [PATCH 5/6] xen/arm: map reserved-memory regions as normal memory in dom0
@ 2019-04-23 17:32 ` Stefano Stabellini
0 siblings, 0 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-04-23 17:32 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, nd, Stefano Stabellini, Stefano Stabellini
On Tue, 23 Apr 2019, Julien Grall wrote:
> Hi Stefano,
>
> On 4/22/19 11:42 PM, Stefano Stabellini wrote:
> > On Tue, 26 Feb 2019, Julien Grall wrote:
> > I am not sure about the suggestion of re-using the libfdt concept of
> > "mem_rsv", which is meant to be for the old /memreserve/. Today, libfdt
> > (at least our version of it) is not able to parse the new
> > reserved-memory bindings. I don't think it is a good idea to modify
> > libfdt for that. Also, the way we want to handle the old memreserve
> > regions is quite different from the way we want to handle
> > reserved-memory, right? I cannot see a way to improve this code using
> > mem_rsv at the moment.
>
> I didn't mean to extend mem_rsv in libfdt but extend consider_modules and
> dt_unreserved_regions to deal with /reserved-memory. Otherwise you
> may miss some cases (for instance you left out discard_initial_modules).
>
> By extending those two functions you don't have to teach everyone how to skip
> /reserved-memory.
I think I get your point now. Although I don't think it should be
correct for a bootloader to use a reserved-memory area to store a boot
module, I wouldn't be suprised if that happens, so it is better to be
prepared and extend dt_unreserved_regions. I'll do that.
However, we would still need something like check_reserved_memory,
because we don't want setup_xenheap_mappings to be called on the
reserved-memory area (or a memory region including the reserved memory
area) in setup_mm. I don't think we can get away without it, but I can
simplify it.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH 5/6] xen/arm: map reserved-memory regions as normal memory in dom0
@ 2019-04-23 18:37 ` Julien Grall
0 siblings, 0 replies; 67+ messages in thread
From: Julien Grall @ 2019-04-23 18:37 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Julien Grall, nd, Stefano Stabellini
[-- Attachment #1.1: Type: text/plain, Size: 2492 bytes --]
Hi,
Sorry for the formatting.
On Tue, 23 Apr 2019, 18:34 Stefano Stabellini, <sstabellini@kernel.org>
wrote:
> On Tue, 23 Apr 2019, Julien Grall wrote:
> > Hi Stefano,
> >
> > On 4/22/19 11:42 PM, Stefano Stabellini wrote:
> > > On Tue, 26 Feb 2019, Julien Grall wrote:
> > > I am not sure about the suggestion of re-using the libfdt concept of
> > > "mem_rsv", which is meant to be for the old /memreserve/. Today, libfdt
> > > (at least our version of it) is not able to parse the new
> > > reserved-memory bindings. I don't think it is a good idea to modify
> > > libfdt for that. Also, the way we want to handle the old memreserve
> > > regions is quite different from the way we want to handle
> > > reserved-memory, right? I cannot see a way to improve this code using
> > > mem_rsv at the moment.
> >
> > I didn't mean to extend mem_rsv in libfdt but extend consider_modules and
> > dt_unreserved_regions to deal with /reserved-memory. Otherwise you
> > may miss some cases (for instance you left out discard_initial_modules).
> >
> > By extending those two functions you don't have to teach everyone how to
> skip
> > /reserved-memory.
>
> I think I get your point now. Although I don't think it should be
> correct for a bootloader to use a reserved-memory area to store a boot
> module, I wouldn't be suprised if that happens, so it is better to be
> prepared and extend dt_unreserved_regions. I'll do that.
>
Why wouldn't this be correct? It is nothing different /mem-reserve.
> However, we would still need something like check_reserved_memory,
> because we don't want setup_xenheap_mappings to be called on the
> reserved-memory area (or a memory region including the reserved memory
> area) in setup_mm. I don't think we can get away without it, but I can
> simplify it.
>
Hmmm, setup_xenheap_mappings is only doing the mapping in page-tables
allowing direct access in Xen. Are you worried of the memory attributes to
be different in Xen?
This would makes sense however setup_xenheap_mappings may still map the
reserved-memory because it is using 1G mapping... This is pretty wrong and
I have patches that should help to fix it.
Also if you are concerned with /reserved-memory, then it should also be
fixed for /mem-reserve as they are not different. However, this may break
free_initmem as because we try to give back page to xenheap even if they
are reserved.
The memory management is quite a mess today. I hope to make it better with
my upcoming series.
Cheers,
[-- Attachment #1.2: Type: text/html, Size: 3520 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Xen-devel] [PATCH 5/6] xen/arm: map reserved-memory regions as normal memory in dom0
@ 2019-04-23 18:37 ` Julien Grall
0 siblings, 0 replies; 67+ messages in thread
From: Julien Grall @ 2019-04-23 18:37 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Julien Grall, nd, Stefano Stabellini
[-- Attachment #1.1: Type: text/plain, Size: 2492 bytes --]
Hi,
Sorry for the formatting.
On Tue, 23 Apr 2019, 18:34 Stefano Stabellini, <sstabellini@kernel.org>
wrote:
> On Tue, 23 Apr 2019, Julien Grall wrote:
> > Hi Stefano,
> >
> > On 4/22/19 11:42 PM, Stefano Stabellini wrote:
> > > On Tue, 26 Feb 2019, Julien Grall wrote:
> > > I am not sure about the suggestion of re-using the libfdt concept of
> > > "mem_rsv", which is meant to be for the old /memreserve/. Today, libfdt
> > > (at least our version of it) is not able to parse the new
> > > reserved-memory bindings. I don't think it is a good idea to modify
> > > libfdt for that. Also, the way we want to handle the old memreserve
> > > regions is quite different from the way we want to handle
> > > reserved-memory, right? I cannot see a way to improve this code using
> > > mem_rsv at the moment.
> >
> > I didn't mean to extend mem_rsv in libfdt but extend consider_modules and
> > dt_unreserved_regions to deal with /reserved-memory. Otherwise you
> > may miss some cases (for instance you left out discard_initial_modules).
> >
> > By extending those two functions you don't have to teach everyone how to
> skip
> > /reserved-memory.
>
> I think I get your point now. Although I don't think it should be
> correct for a bootloader to use a reserved-memory area to store a boot
> module, I wouldn't be suprised if that happens, so it is better to be
> prepared and extend dt_unreserved_regions. I'll do that.
>
Why wouldn't this be correct? It is nothing different /mem-reserve.
> However, we would still need something like check_reserved_memory,
> because we don't want setup_xenheap_mappings to be called on the
> reserved-memory area (or a memory region including the reserved memory
> area) in setup_mm. I don't think we can get away without it, but I can
> simplify it.
>
Hmmm, setup_xenheap_mappings is only doing the mapping in page-tables
allowing direct access in Xen. Are you worried of the memory attributes to
be different in Xen?
This would makes sense however setup_xenheap_mappings may still map the
reserved-memory because it is using 1G mapping... This is pretty wrong and
I have patches that should help to fix it.
Also if you are concerned with /reserved-memory, then it should also be
fixed for /mem-reserve as they are not different. However, this may break
free_initmem as because we try to give back page to xenheap even if they
are reserved.
The memory management is quite a mess today. I hope to make it better with
my upcoming series.
Cheers,
[-- Attachment #1.2: Type: text/html, Size: 3520 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH 5/6] xen/arm: map reserved-memory regions as normal memory in dom0
@ 2019-04-23 21:34 ` Stefano Stabellini
0 siblings, 0 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-04-23 21:34 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Julien Grall, Stefano Stabellini, nd, Stefano Stabellini
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2876 bytes --]
On Tue, 23 Apr 2019, Julien Grall wrote:
> Hi,
>
> Sorry for the formatting.
>
> On Tue, 23 Apr 2019, 18:34 Stefano Stabellini, <sstabellini@kernel.org> wrote:
> On Tue, 23 Apr 2019, Julien Grall wrote:
> > Hi Stefano,
> >
> > On 4/22/19 11:42 PM, Stefano Stabellini wrote:
> > > On Tue, 26 Feb 2019, Julien Grall wrote:
> > > I am not sure about the suggestion of re-using the libfdt concept of
> > > "mem_rsv", which is meant to be for the old /memreserve/. Today, libfdt
> > > (at least our version of it) is not able to parse the new
> > > reserved-memory bindings. I don't think it is a good idea to modify
> > > libfdt for that. Also, the way we want to handle the old memreserve
> > > regions is quite different from the way we want to handle
> > > reserved-memory, right? I cannot see a way to improve this code using
> > > mem_rsv at the moment.
> >
> > I didn't mean to extend mem_rsv in libfdt but extend consider_modules and
> > dt_unreserved_regions to deal with /reserved-memory. Otherwise you
> > may miss some cases (for instance you left out discard_initial_modules).
> >
> > By extending those two functions you don't have to teach everyone how to skip
> > /reserved-memory.
>
> I think I get your point now. Although I don't think it should be
> correct for a bootloader to use a reserved-memory area to store a boot
> module, I wouldn't be suprised if that happens, so it is better to be
> prepared and extend dt_unreserved_regions. I'll do that.
>
> However, we would still need something like check_reserved_memory,
> because we don't want setup_xenheap_mappings to be called on the
> reserved-memory area (or a memory region including the reserved memory
> area) in setup_mm. I don't think we can get away without it, but I can
> simplify it.
>
>
> Hmmm, setup_xenheap_mappings is only doing the mapping in page-tables allowing direct access in Xen. Are you worried of the
> memory attributes to be different in Xen?
Yes; also we don't need the mappings in Xen.
> This would makes sense however setup_xenheap_mappings may still map the reserved-memory because it is using 1G mapping... This is
> pretty wrong and I have patches that should help to fix it.
I didn't notice it :-/
> Also if you are concerned with /reserved-memory, then it should also be fixed for /mem-reserve as they are not different.
I understand.
> However, this may break free_initmem as because we try to give back page to xenheap even if they are reserved.
>
> The memory management is quite a mess today. I hope to make it better with my upcoming series.
I am going to follow your original suggestion of dropping most of the
changes from this patch and rely on the existing functions.
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Xen-devel] [PATCH 5/6] xen/arm: map reserved-memory regions as normal memory in dom0
@ 2019-04-23 21:34 ` Stefano Stabellini
0 siblings, 0 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-04-23 21:34 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Julien Grall, Stefano Stabellini, nd, Stefano Stabellini
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2876 bytes --]
On Tue, 23 Apr 2019, Julien Grall wrote:
> Hi,
>
> Sorry for the formatting.
>
> On Tue, 23 Apr 2019, 18:34 Stefano Stabellini, <sstabellini@kernel.org> wrote:
> On Tue, 23 Apr 2019, Julien Grall wrote:
> > Hi Stefano,
> >
> > On 4/22/19 11:42 PM, Stefano Stabellini wrote:
> > > On Tue, 26 Feb 2019, Julien Grall wrote:
> > > I am not sure about the suggestion of re-using the libfdt concept of
> > > "mem_rsv", which is meant to be for the old /memreserve/. Today, libfdt
> > > (at least our version of it) is not able to parse the new
> > > reserved-memory bindings. I don't think it is a good idea to modify
> > > libfdt for that. Also, the way we want to handle the old memreserve
> > > regions is quite different from the way we want to handle
> > > reserved-memory, right? I cannot see a way to improve this code using
> > > mem_rsv at the moment.
> >
> > I didn't mean to extend mem_rsv in libfdt but extend consider_modules and
> > dt_unreserved_regions to deal with /reserved-memory. Otherwise you
> > may miss some cases (for instance you left out discard_initial_modules).
> >
> > By extending those two functions you don't have to teach everyone how to skip
> > /reserved-memory.
>
> I think I get your point now. Although I don't think it should be
> correct for a bootloader to use a reserved-memory area to store a boot
> module, I wouldn't be suprised if that happens, so it is better to be
> prepared and extend dt_unreserved_regions. I'll do that.
>
> However, we would still need something like check_reserved_memory,
> because we don't want setup_xenheap_mappings to be called on the
> reserved-memory area (or a memory region including the reserved memory
> area) in setup_mm. I don't think we can get away without it, but I can
> simplify it.
>
>
> Hmmm, setup_xenheap_mappings is only doing the mapping in page-tables allowing direct access in Xen. Are you worried of the
> memory attributes to be different in Xen?
Yes; also we don't need the mappings in Xen.
> This would makes sense however setup_xenheap_mappings may still map the reserved-memory because it is using 1G mapping... This is
> pretty wrong and I have patches that should help to fix it.
I didn't notice it :-/
> Also if you are concerned with /reserved-memory, then it should also be fixed for /mem-reserve as they are not different.
I understand.
> However, this may break free_initmem as because we try to give back page to xenheap even if they are reserved.
>
> The memory management is quite a mess today. I hope to make it better with my upcoming series.
I am going to follow your original suggestion of dropping most of the
changes from this patch and rely on the existing functions.
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* [PATCH 6/6] xen/docs: how to map a page between dom0 and domU using iomem
2019-02-26 23:06 [PATCH 0/6] iomem cacheability Stefano Stabellini
` (4 preceding siblings ...)
2019-02-26 23:07 ` [PATCH 5/6] xen/arm: map reserved-memory regions as normal memory in dom0 Stefano Stabellini
@ 2019-02-26 23:07 ` Stefano Stabellini
2019-03-03 17:20 ` [PATCH 0/6] iomem cacheability Amit Tomer
6 siblings, 0 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-02-26 23:07 UTC (permalink / raw)
To: xen-devel; +Cc: Stefano Stabellini, julien.grall, sstabellini
Document how to use the iomem option to share a page between Dom0 and a
DomU.
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
docs/misc/arm/dom0_shared_mem.txt | 69 +++++++++++++++++++++++++++++++++++++++
1 file changed, 69 insertions(+)
create mode 100644 docs/misc/arm/dom0_shared_mem.txt
diff --git a/docs/misc/arm/dom0_shared_mem.txt b/docs/misc/arm/dom0_shared_mem.txt
new file mode 100644
index 0000000..8de513d
--- /dev/null
+++ b/docs/misc/arm/dom0_shared_mem.txt
@@ -0,0 +1,69 @@
+This document explains how to setup a cacheable shared memory region
+between dom0 and a domU.
+
+First, we have to add a reserved-memory node to the host device tree to
+advertise the special memory region to dom0, so that it won't use it to
+allocate memory as any other pages. For that, we can make use of the
+newly introduced "xen,shared-memory" compatible string. For example:
+
+ reserved-memory {
+ #address-cells = <0x2>;
+ #size-cells = <0x2>;
+ ranges;
+
+ xen-shmem@0 {
+ compatible = "xen,shared-memory";
+ reg = <0x0 0x70000000 0x0 0x1000>;
+ };
+ };
+
+This node tells dom0 that one page at 0x70000000 is to be use as
+reserved memory.
+
+Then, we need to do the same for DomU. We can do that by adding a device
+tree fragment to the DomU VM config file. The device tree fragment could
+be for example:
+
+/dts-v1/;
+
+/ {
+ /* #*cells are here to keep DTC happy */
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ passthrough {
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges;
+
+ reserved-memory {
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges;
+
+ xen-shmem@0 {
+ compatible = "xen,shared-memory";
+ reg = <0x0 0x70000000 0x0 0x1000>;
+ };
+ };
+ };
+};
+
+Similarly to the dom0 example, it tells the domU kernel that the page at
+0x70000000 is to be used as reserved memory. We add the device tree
+fragment to the DomU device tree using the device_tree option in the VM
+config file, the same way we use it for device assignment:
+
+device_tree = "/root/snippet.dtb"
+
+Finally, we only need to map the page into the DomU address space at the
+right address, which in this example is 0x70000000. We can do that with
+the iomem VM config option. It is possible to specify the cacheability
+of the mapping, "memory" means normal cacheable memory:
+
+iomem = ["0x70000,1@0x70000,memory"]
+
+In this example, we are asking to map one page at physical address
+0x70000000 into the guest pseudo-physical address space at 0x70000000.
+We are also asking to make the mapping a normal cacheable memory
+mapping.
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 67+ messages in thread* Re: [PATCH 0/6] iomem cacheability
2019-02-26 23:06 [PATCH 0/6] iomem cacheability Stefano Stabellini
` (5 preceding siblings ...)
2019-02-26 23:07 ` [PATCH 6/6] xen/docs: how to map a page between dom0 and domU using iomem Stefano Stabellini
@ 2019-03-03 17:20 ` Amit Tomer
2019-03-05 21:22 ` Stefano Stabellini
6 siblings, 1 reply; 67+ messages in thread
From: Amit Tomer @ 2019-03-03 17:20 UTC (permalink / raw)
To: Stefano Stabellini
Cc: wei.liu2, Andrew Cooper, Julien Grall, Jan Beulich, ian.jackson,
xen-devel
Hi,
> This series introduces a cacheability parameter for the iomem option, so
> that we can map an iomem region into a guest as cacheable memory.
>
> Then, this series fixes the way Xen handles reserved memory regions on
> ARM: they should be mapped as normal memory, instead today they are
> treated as device memory.
>
We tried testing this patch series on R-CAR platform but see following crash
when booting dom0 Linux.
[ 0.577777] bd20: 0000000000000000 ffff000008b27fa0
ffffffffffffffff ffff000008b27000
[ 0.585639] bd40: ffff00000804bd50 ffff000008959164
[ 0.590565] [<ffff000008959164>] cma_init_reserved_areas+0x98/0x1d0
[ 0.596876] [<ffff000008083a50>] do_one_initcall+0x38/0x120
[ 0.602493] [<ffff000008940d04>] kernel_init_freeable+0x188/0x228
[ 0.608628] [<ffff0000086a6288>] kernel_init+0x10/0x100
[ 0.613898] [<ffff000008084c68>] ret_from_fork+0x10/0x18
[ 0.619250] ---[ end trace c2041e247871a6ff ]---
[ 0.623929] Unable to handle kernel paging request at virtual
address ffff7dffe55c0000
[ 0.631880] Mem abort info:
[ 0.634715] Exception class = DABT (current EL), IL = 32 bits
[ 0.640684] SET = 0, FnV = 0
[ 0.643786] EA = 0, S1PTW = 0
[ 0.646990] Data abort info:
[ 0.649920] ISV = 0, ISS = 0x00000006
[ 0.653821] CM = 0, WnR = 0
[ 0.656834] swapper pgtable: 4k pages, 48-bit VAs, pgd = ffff000008b47000
[ 0.663670] [ffff7dffe55c0000] *pgd=0000000700aef803,
*pud=0000000700af0803, *pmd=0000000000000000
[ 0.672652] Internal error: Oops: 96000006 [#1] PREEMPT SMP
[ 0.678259] Modules linked in:
[ 0.681371] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G W
4.14.50-yocto-standard #1
[ 0.689923] Hardware name: Renesas Salvator-X board based on
r8a7795 ES2.0+ (DT)
[ 0.697355] task: ffff80001e910000 task.stack: ffff000008048000
[ 0.703317] PC is at cma_init_reserved_areas+0xbc/0x1d0
[ 0.708587] LR is at cma_init_reserved_areas+0x94/0x1d0
[ 0.713862] pc : [<ffff000008959188>] lr : [<ffff000008959160>]
pstate: 60000045
[ 0.721287] sp : ffff00000804bd50
[ 0.724657] x29: ffff00000804bd50 x28: ffff000008a88a28
[ 0.730013] x27: 0000000000057000 x26: ffff000008994040
[ 0.735370] x25: ffff000008b27fa0 x24: ffff000008b27000
[ 0.740727] x23: ffff7e0000000000 x22: ffff0000088ed000
[ 0.746084] x21: 0000000000000000 x20: 0000000000000000
[ 0.751440] x19: 0000000000000004 x18: 0000000000000000
[ 0.756797] x17: 0000000000000001 x16: 00000000deadbeef
[ 0.762154] x15: 0000000000000000 x14: 0000000000000400
[ 0.767511] x13: 0000000000000400 x12: 0000000000000000
[ 0.772872] x11: 0000000000000000 x10: 0000000000000002
[ 0.778224] x9 : 0000000000000000 x8 : ffff80001e945800
[ 0.783586] x7 : 0000000000000000 x6 : ffff000008b24868
[ 0.788938] x5 : ffff000008b24868 x4 : 0000000000000000
[ 0.794295] x3 : 0000000000000780 x2 : 0000000700000000
[ 0.799652] x1 : ffff000008a88a28 x0 : ffffffffe55c0000
[ 0.805010] Process swapper/0 (pid: 1, stack limit = 0xffff000008048000)
[ 0.811747] Call trace:
[ 0.814254] Exception stack(0xffff00000804bc10 to 0xffff00000804bd50)
[ 0.820734] bc00:
ffffffffe55c0000 ffff000008a88a28
[ 0.828598] bc20: 0000000700000000 0000000000000780
0000000000000000 ffff000008b24868
[ 0.836460] bc40: ffff000008b24868 0000000000000000
ffff80001e945800 0000000000000000
[ 0.844322] bc60: 0000000000000002 0000000000000000
0000000000000000 0000000000000400
[ 0.852184] bc80: 0000000000000400 0000000000000000
00000000deadbeef 0000000000000001
[ 0.860047] bca0: 0000000000000000 0000000000000004
0000000000000000 0000000000000000
[ 0.867910] bcc0: ffff0000088ed000 ffff7e0000000000
ffff000008b27000 ffff000008b27fa0
[ 0.875772] bce0: ffff000008994040 0000000000057000
ffff000008a88a28 ffff00000804bd50
[ 0.883639] bd00: ffff000008959160 ffff00000804bd50
ffff000008959188 0000000060000045
[ 0.891497] bd20: 0000000000000000 ffff000008b27fa0
ffffffffffffffff ffff000008b27000
[ 0.899359] bd40: ffff00000804bd50 ffff000008959188
[ 0.904285] [<ffff000008959188>] cma_init_reserved_areas+0xbc/0x1d0
[ 0.910592] [<ffff000008083a50>] do_one_initcall+0x38/0x120
[ 0.916209] [<ffff000008940d04>] kernel_init_freeable+0x188/0x228
[ 0.922343] [<ffff0000086a6288>] kernel_init+0x10/0x100
[ 0.927613] [<ffff000008084c68>] ret_from_fork+0x10/0x18
[ 0.932975] Code: f94262c0 aa0103fc cb803360 d37ae400 (f8776800)
[ 0.939104] ---[ end trace c2041e247871a700 ]---
[ 0.943800] Kernel panic - not syncing: Attempted to kill init!
exitcode=0x0000000b
[ 0.943800]
[ 0.953021] SMP: stopping secondary CPUs
[ 0.957009] ---[ end Kernel panic - not syncing: Attempted to kill
init! exitcode=0x0000000b
Below is how reserved node looks like:
reserved-memory {
#address-cells = <2>;
#size-cells = <2>;
ranges;
/* device specific region for Lossy Decompression */
lossy_decompress: linux,lossy_decompress@54000000 {
no-map;
reg = <0x00000000 0x54000000 0x0 0x03000000>;
};
/* For Audio DSP */
adsp_reserved: linux,adsp@57000000 {
compatible = "shared-dma-pool";
reusable;
reg = <0x00000000 0x57000000 0x0 0x01000000>;
};
/* global autoconfigured region for contiguous allocations */
linux,cma@58000000 {
compatible = "shared-dma-pool";
reusable;
reg = <0x00000000 0x58000000 0x0 0x18000000>;
linux,cma-default;
};
/* device specific region for contiguous allocations */
mmp_reserved: linux,multimedia@70000000 {
compatible = "shared-dma-pool";
reusable;
reg = <0x00000000 0x70000000 0x0 0x10000000>;
};
};
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH 0/6] iomem cacheability
2019-03-03 17:20 ` [PATCH 0/6] iomem cacheability Amit Tomer
@ 2019-03-05 21:22 ` Stefano Stabellini
2019-03-05 22:45 ` Julien Grall
2019-03-06 11:30 ` Amit Tomer
0 siblings, 2 replies; 67+ messages in thread
From: Stefano Stabellini @ 2019-03-05 21:22 UTC (permalink / raw)
To: Amit Tomer
Cc: Stefano Stabellini, wei.liu2, Andrew Cooper, Julien Grall,
Jan Beulich, ian.jackson, xen-devel
On Sun, 3 Mar 2019, Amit Tomer wrote:
> Hi,
>
> > This series introduces a cacheability parameter for the iomem option, so
> > that we can map an iomem region into a guest as cacheable memory.
> >
> > Then, this series fixes the way Xen handles reserved memory regions on
> > ARM: they should be mapped as normal memory, instead today they are
> > treated as device memory.
> >
>
> We tried testing this patch series on R-CAR platform but see following crash
> when booting dom0 Linux.
Thanks for testing! You might have found a real bug in the series. Could
you please also attach the full device tree?
> [ 0.577777] bd20: 0000000000000000 ffff000008b27fa0
> ffffffffffffffff ffff000008b27000
> [ 0.585639] bd40: ffff00000804bd50 ffff000008959164
> [ 0.590565] [<ffff000008959164>] cma_init_reserved_areas+0x98/0x1d0
> [ 0.596876] [<ffff000008083a50>] do_one_initcall+0x38/0x120
> [ 0.602493] [<ffff000008940d04>] kernel_init_freeable+0x188/0x228
> [ 0.608628] [<ffff0000086a6288>] kernel_init+0x10/0x100
> [ 0.613898] [<ffff000008084c68>] ret_from_fork+0x10/0x18
> [ 0.619250] ---[ end trace c2041e247871a6ff ]---
> [ 0.623929] Unable to handle kernel paging request at virtual
> address ffff7dffe55c0000
> [ 0.631880] Mem abort info:
> [ 0.634715] Exception class = DABT (current EL), IL = 32 bits
> [ 0.640684] SET = 0, FnV = 0
> [ 0.643786] EA = 0, S1PTW = 0
> [ 0.646990] Data abort info:
> [ 0.649920] ISV = 0, ISS = 0x00000006
> [ 0.653821] CM = 0, WnR = 0
> [ 0.656834] swapper pgtable: 4k pages, 48-bit VAs, pgd = ffff000008b47000
> [ 0.663670] [ffff7dffe55c0000] *pgd=0000000700aef803,
> *pud=0000000700af0803, *pmd=0000000000000000
> [ 0.672652] Internal error: Oops: 96000006 [#1] PREEMPT SMP
> [ 0.678259] Modules linked in:
> [ 0.681371] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G W
> 4.14.50-yocto-standard #1
> [ 0.689923] Hardware name: Renesas Salvator-X board based on
> r8a7795 ES2.0+ (DT)
> [ 0.697355] task: ffff80001e910000 task.stack: ffff000008048000
> [ 0.703317] PC is at cma_init_reserved_areas+0xbc/0x1d0
> [ 0.708587] LR is at cma_init_reserved_areas+0x94/0x1d0
> [ 0.713862] pc : [<ffff000008959188>] lr : [<ffff000008959160>]
> pstate: 60000045
> [ 0.721287] sp : ffff00000804bd50
> [ 0.724657] x29: ffff00000804bd50 x28: ffff000008a88a28
> [ 0.730013] x27: 0000000000057000 x26: ffff000008994040
> [ 0.735370] x25: ffff000008b27fa0 x24: ffff000008b27000
> [ 0.740727] x23: ffff7e0000000000 x22: ffff0000088ed000
> [ 0.746084] x21: 0000000000000000 x20: 0000000000000000
> [ 0.751440] x19: 0000000000000004 x18: 0000000000000000
> [ 0.756797] x17: 0000000000000001 x16: 00000000deadbeef
> [ 0.762154] x15: 0000000000000000 x14: 0000000000000400
> [ 0.767511] x13: 0000000000000400 x12: 0000000000000000
> [ 0.772872] x11: 0000000000000000 x10: 0000000000000002
> [ 0.778224] x9 : 0000000000000000 x8 : ffff80001e945800
> [ 0.783586] x7 : 0000000000000000 x6 : ffff000008b24868
> [ 0.788938] x5 : ffff000008b24868 x4 : 0000000000000000
> [ 0.794295] x3 : 0000000000000780 x2 : 0000000700000000
> [ 0.799652] x1 : ffff000008a88a28 x0 : ffffffffe55c0000
> [ 0.805010] Process swapper/0 (pid: 1, stack limit = 0xffff000008048000)
> [ 0.811747] Call trace:
> [ 0.814254] Exception stack(0xffff00000804bc10 to 0xffff00000804bd50)
> [ 0.820734] bc00:
> ffffffffe55c0000 ffff000008a88a28
> [ 0.828598] bc20: 0000000700000000 0000000000000780
> 0000000000000000 ffff000008b24868
> [ 0.836460] bc40: ffff000008b24868 0000000000000000
> ffff80001e945800 0000000000000000
> [ 0.844322] bc60: 0000000000000002 0000000000000000
> 0000000000000000 0000000000000400
> [ 0.852184] bc80: 0000000000000400 0000000000000000
> 00000000deadbeef 0000000000000001
> [ 0.860047] bca0: 0000000000000000 0000000000000004
> 0000000000000000 0000000000000000
> [ 0.867910] bcc0: ffff0000088ed000 ffff7e0000000000
> ffff000008b27000 ffff000008b27fa0
> [ 0.875772] bce0: ffff000008994040 0000000000057000
> ffff000008a88a28 ffff00000804bd50
> [ 0.883639] bd00: ffff000008959160 ffff00000804bd50
> ffff000008959188 0000000060000045
> [ 0.891497] bd20: 0000000000000000 ffff000008b27fa0
> ffffffffffffffff ffff000008b27000
> [ 0.899359] bd40: ffff00000804bd50 ffff000008959188
> [ 0.904285] [<ffff000008959188>] cma_init_reserved_areas+0xbc/0x1d0
> [ 0.910592] [<ffff000008083a50>] do_one_initcall+0x38/0x120
> [ 0.916209] [<ffff000008940d04>] kernel_init_freeable+0x188/0x228
> [ 0.922343] [<ffff0000086a6288>] kernel_init+0x10/0x100
> [ 0.927613] [<ffff000008084c68>] ret_from_fork+0x10/0x18
> [ 0.932975] Code: f94262c0 aa0103fc cb803360 d37ae400 (f8776800)
> [ 0.939104] ---[ end trace c2041e247871a700 ]---
> [ 0.943800] Kernel panic - not syncing: Attempted to kill init!
> exitcode=0x0000000b
> [ 0.943800]
> [ 0.953021] SMP: stopping secondary CPUs
> [ 0.957009] ---[ end Kernel panic - not syncing: Attempted to kill
> init! exitcode=0x0000000b
>
> Below is how reserved node looks like:
>
> reserved-memory {
> #address-cells = <2>;
> #size-cells = <2>;
> ranges;
>
> /* device specific region for Lossy Decompression */
> lossy_decompress: linux,lossy_decompress@54000000 {
> no-map;
> reg = <0x00000000 0x54000000 0x0 0x03000000>;
> };
>
> /* For Audio DSP */
> adsp_reserved: linux,adsp@57000000 {
> compatible = "shared-dma-pool";
> reusable;
> reg = <0x00000000 0x57000000 0x0 0x01000000>;
> };
>
> /* global autoconfigured region for contiguous allocations */
> linux,cma@58000000 {
> compatible = "shared-dma-pool";
> reusable;
> reg = <0x00000000 0x58000000 0x0 0x18000000>;
> linux,cma-default;
> };
>
> /* device specific region for contiguous allocations */
> mmp_reserved: linux,multimedia@70000000 {
> compatible = "shared-dma-pool";
> reusable;
> reg = <0x00000000 0x70000000 0x0 0x10000000>;
> };
> };
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH 0/6] iomem cacheability
2019-03-05 21:22 ` Stefano Stabellini
@ 2019-03-05 22:45 ` Julien Grall
2019-03-06 11:46 ` Amit Tomer
2019-03-06 11:30 ` Amit Tomer
1 sibling, 1 reply; 67+ messages in thread
From: Julien Grall @ 2019-03-05 22:45 UTC (permalink / raw)
To: Stefano Stabellini
Cc: wei.liu2, Andrew Cooper, Amit Tomer, Julien Grall, Jan Beulich,
xen-devel, ian.jackson
[-- Attachment #1.1: Type: text/plain, Size: 7253 bytes --]
Sorry for the formatting.
On Tue, 5 Mar 2019, 21:24 Stefano Stabellini, <sstabellini@kernel.org>
wrote:
> On Sun, 3 Mar 2019, Amit Tomer wrote:
> > Hi,
> >
> > > This series introduces a cacheability parameter for the iomem option,
> so
> > > that we can map an iomem region into a guest as cacheable memory.
> > >
> > > Then, this series fixes the way Xen handles reserved memory regions on
> > > ARM: they should be mapped as normal memory, instead today they are
> > > treated as device memory.
> > >
> >
> > We tried testing this patch series on R-CAR platform but see following
> crash
> > when booting dom0 Linux.
>
> Thanks for testing! You might have found a real bug in the series. Could
> you please also attach the full device tree?
>
Looking at the stack trace, this is very likely due to the issue I pointed
out earlier on. I.e reserved-memory region should be described in the
memory nodes.
Cheers,
>
> > [ 0.577777] bd20: 0000000000000000 ffff000008b27fa0
> > ffffffffffffffff ffff000008b27000
> > [ 0.585639] bd40: ffff00000804bd50 ffff000008959164
> > [ 0.590565] [<ffff000008959164>] cma_init_reserved_areas+0x98/0x1d0
> > [ 0.596876] [<ffff000008083a50>] do_one_initcall+0x38/0x120
> > [ 0.602493] [<ffff000008940d04>] kernel_init_freeable+0x188/0x228
> > [ 0.608628] [<ffff0000086a6288>] kernel_init+0x10/0x100
> > [ 0.613898] [<ffff000008084c68>] ret_from_fork+0x10/0x18
> > [ 0.619250] ---[ end trace c2041e247871a6ff ]---
> > [ 0.623929] Unable to handle kernel paging request at virtual
> > address ffff7dffe55c0000
> > [ 0.631880] Mem abort info:
> > [ 0.634715] Exception class = DABT (current EL), IL = 32 bits
> > [ 0.640684] SET = 0, FnV = 0
> > [ 0.643786] EA = 0, S1PTW = 0
> > [ 0.646990] Data abort info:
> > [ 0.649920] ISV = 0, ISS = 0x00000006
> > [ 0.653821] CM = 0, WnR = 0
> > [ 0.656834] swapper pgtable: 4k pages, 48-bit VAs, pgd =
> ffff000008b47000
> > [ 0.663670] [ffff7dffe55c0000] *pgd=0000000700aef803,
> > *pud=0000000700af0803, *pmd=0000000000000000
> > [ 0.672652] Internal error: Oops: 96000006 [#1] PREEMPT SMP
> > [ 0.678259] Modules linked in:
> > [ 0.681371] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G W
> > 4.14.50-yocto-standard #1
> > [ 0.689923] Hardware name: Renesas Salvator-X board based on
> > r8a7795 ES2.0+ (DT)
> > [ 0.697355] task: ffff80001e910000 task.stack: ffff000008048000
> > [ 0.703317] PC is at cma_init_reserved_areas+0xbc/0x1d0
> > [ 0.708587] LR is at cma_init_reserved_areas+0x94/0x1d0
> > [ 0.713862] pc : [<ffff000008959188>] lr : [<ffff000008959160>]
> > pstate: 60000045
> > [ 0.721287] sp : ffff00000804bd50
> > [ 0.724657] x29: ffff00000804bd50 x28: ffff000008a88a28
> > [ 0.730013] x27: 0000000000057000 x26: ffff000008994040
> > [ 0.735370] x25: ffff000008b27fa0 x24: ffff000008b27000
> > [ 0.740727] x23: ffff7e0000000000 x22: ffff0000088ed000
> > [ 0.746084] x21: 0000000000000000 x20: 0000000000000000
> > [ 0.751440] x19: 0000000000000004 x18: 0000000000000000
> > [ 0.756797] x17: 0000000000000001 x16: 00000000deadbeef
> > [ 0.762154] x15: 0000000000000000 x14: 0000000000000400
> > [ 0.767511] x13: 0000000000000400 x12: 0000000000000000
> > [ 0.772872] x11: 0000000000000000 x10: 0000000000000002
> > [ 0.778224] x9 : 0000000000000000 x8 : ffff80001e945800
> > [ 0.783586] x7 : 0000000000000000 x6 : ffff000008b24868
> > [ 0.788938] x5 : ffff000008b24868 x4 : 0000000000000000
> > [ 0.794295] x3 : 0000000000000780 x2 : 0000000700000000
> > [ 0.799652] x1 : ffff000008a88a28 x0 : ffffffffe55c0000
> > [ 0.805010] Process swapper/0 (pid: 1, stack limit =
> 0xffff000008048000)
> > [ 0.811747] Call trace:
> > [ 0.814254] Exception stack(0xffff00000804bc10 to 0xffff00000804bd50)
> > [ 0.820734] bc00:
> > ffffffffe55c0000 ffff000008a88a28
> > [ 0.828598] bc20: 0000000700000000 0000000000000780
> > 0000000000000000 ffff000008b24868
> > [ 0.836460] bc40: ffff000008b24868 0000000000000000
> > ffff80001e945800 0000000000000000
> > [ 0.844322] bc60: 0000000000000002 0000000000000000
> > 0000000000000000 0000000000000400
> > [ 0.852184] bc80: 0000000000000400 0000000000000000
> > 00000000deadbeef 0000000000000001
> > [ 0.860047] bca0: 0000000000000000 0000000000000004
> > 0000000000000000 0000000000000000
> > [ 0.867910] bcc0: ffff0000088ed000 ffff7e0000000000
> > ffff000008b27000 ffff000008b27fa0
> > [ 0.875772] bce0: ffff000008994040 0000000000057000
> > ffff000008a88a28 ffff00000804bd50
> > [ 0.883639] bd00: ffff000008959160 ffff00000804bd50
> > ffff000008959188 0000000060000045
> > [ 0.891497] bd20: 0000000000000000 ffff000008b27fa0
> > ffffffffffffffff ffff000008b27000
> > [ 0.899359] bd40: ffff00000804bd50 ffff000008959188
> > [ 0.904285] [<ffff000008959188>] cma_init_reserved_areas+0xbc/0x1d0
> > [ 0.910592] [<ffff000008083a50>] do_one_initcall+0x38/0x120
> > [ 0.916209] [<ffff000008940d04>] kernel_init_freeable+0x188/0x228
> > [ 0.922343] [<ffff0000086a6288>] kernel_init+0x10/0x100
> > [ 0.927613] [<ffff000008084c68>] ret_from_fork+0x10/0x18
> > [ 0.932975] Code: f94262c0 aa0103fc cb803360 d37ae400 (f8776800)
> > [ 0.939104] ---[ end trace c2041e247871a700 ]---
> > [ 0.943800] Kernel panic - not syncing: Attempted to kill init!
> > exitcode=0x0000000b
> > [ 0.943800]
> > [ 0.953021] SMP: stopping secondary CPUs
> > [ 0.957009] ---[ end Kernel panic - not syncing: Attempted to kill
> > init! exitcode=0x0000000b
> >
> > Below is how reserved node looks like:
> >
> > reserved-memory {
> > #address-cells = <2>;
> > #size-cells = <2>;
> > ranges;
> >
> > /* device specific region for Lossy Decompression */
> > lossy_decompress: linux,lossy_decompress@54000000 {
> > no-map;
> > reg = <0x00000000 0x54000000 0x0 0x03000000>;
> > };
> >
> > /* For Audio DSP */
> > adsp_reserved: linux,adsp@57000000 {
> > compatible = "shared-dma-pool";
> > reusable;
> > reg = <0x00000000 0x57000000 0x0 0x01000000>;
> > };
> >
> > /* global autoconfigured region for contiguous
> allocations */
> > linux,cma@58000000 {
> > compatible = "shared-dma-pool";
> > reusable;
> > reg = <0x00000000 0x58000000 0x0 0x18000000>;
> > linux,cma-default;
> > };
> >
> > /* device specific region for contiguous allocations */
> > mmp_reserved: linux,multimedia@70000000 {
> > compatible = "shared-dma-pool";
> > reusable;
> > reg = <0x00000000 0x70000000 0x0 0x10000000>;
> > };
> > };
> >
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
[-- Attachment #1.2: Type: text/html, Size: 9300 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH 0/6] iomem cacheability
2019-03-05 22:45 ` Julien Grall
@ 2019-03-06 11:46 ` Amit Tomer
2019-03-06 22:42 ` Stefano Stabellini
0 siblings, 1 reply; 67+ messages in thread
From: Amit Tomer @ 2019-03-06 11:46 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, wei.liu2, Andrew Cooper, Julien Grall,
Jan Beulich, xen-devel, ian.jackson
Hi,
> Looking at the stack trace, this is very likely due to the issue I pointed out earlier on. I.e reserved-memory region should be described in the memory nodes.
Do you mean, something like this(reserved-memory node is under memory node) ?
--- a/arch/arm64/boot/dts/renesas/r8a7795-h3ulcb.dts
+++ b/arch/arm64/boot/dts/renesas/r8a7795-h3ulcb.dts
@@ -17,23 +17,10 @@
memory@48000000 {
device_type = "memory";
/* first 128MB is reserved for secure area. */
- reg = <0x0 0x48000000 0x0 0x38000000>;
- };
-
- memory@500000000 {
- device_type = "memory";
- reg = <0x5 0x00000000 0x0 0x40000000>;
- };
-
- memory@600000000 {
- device_type = "memory";
- reg = <0x6 0x00000000 0x0 0x40000000>;
- };
-
- memory@700000000 {
- device_type = "memory";
- reg = <0x7 0x00000000 0x0 0x40000000>;
- };
+ reg = <0x0 0x48000000 0x0 0x38000000>,
+ <0x5 0x00000000 0x0 0x40000000>,
+ <0x6 0x00000000 0x0 0x40000000>,
+ <0x7 0x00000000 0x0 0x40000000>;
reserved-memory {
#address-cells = <2>;
@@ -61,6 +48,7 @@
reg = <0x00000000 0x70000000 0x0 0x10000000>;
};
};
+ };
Thanks
-Amit
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH 0/6] iomem cacheability
2019-03-06 11:46 ` Amit Tomer
@ 2019-03-06 22:42 ` Stefano Stabellini
2019-03-06 22:59 ` Julien Grall
0 siblings, 1 reply; 67+ messages in thread
From: Stefano Stabellini @ 2019-03-06 22:42 UTC (permalink / raw)
To: Amit Tomer
Cc: Stefano Stabellini, wei.liu2, Andrew Cooper, Julien Grall,
Julien Grall, Jan Beulich, xen-devel, ian.jackson
On Wed, 6 Mar 2019, Amit Tomer wrote:
> Hi,
> > Looking at the stack trace, this is very likely due to the issue I pointed out earlier on. I.e reserved-memory region should be described in the memory nodes.
>
> Do you mean, something like this(reserved-memory node is under memory node) ?
No, I think Julien meant that all the reserved-memory regions ranges
should be "covered" by the ranges of the memory node.
If a reserved-memory node range is 0x54000000 - 0x57000000, then we need
to make sure that the memory node includes that range. In your case, the
first memory bank is 0x48000000 - 0x80000000 so it would clearly include
the range 0x54000000 - 0x57000000. So, it looks like it is correct from
that point of view.
I am suspecting there is a bug in the patch "xen/arm: map
reserved-memory regions as normal memory in dom0", specifically in the
implementation of check_reserved_memory.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH 0/6] iomem cacheability
2019-03-06 22:42 ` Stefano Stabellini
@ 2019-03-06 22:59 ` Julien Grall
2019-03-07 8:42 ` Amit Tomer
0 siblings, 1 reply; 67+ messages in thread
From: Julien Grall @ 2019-03-06 22:59 UTC (permalink / raw)
To: Stefano Stabellini, Amit Tomer
Cc: wei.liu2, Andrew Cooper, Julien Grall, Jan Beulich, xen-devel,
ian.jackson, nd
Hi,
On 06/03/2019 22:42, Stefano Stabellini wrote:
> On Wed, 6 Mar 2019, Amit Tomer wrote:
>> Hi,
>>> Looking at the stack trace, this is very likely due to the issue I pointed out earlier on. I.e reserved-memory region should be described in the memory nodes.
>>
>> Do you mean, something like this(reserved-memory node is under memory node) ?
>
> No, I think Julien meant that all the reserved-memory regions ranges
> should be "covered" by the ranges of the memory node.
>
> If a reserved-memory node range is 0x54000000 - 0x57000000, then we need
> to make sure that the memory node includes that range. In your case, the
> first memory bank is 0x48000000 - 0x80000000 so it would clearly include
> the range 0x54000000 - 0x57000000. So, it looks like it is correct from
> that point of view.
Not really, the DT provided by Amit is for the host. The memory node
will be rewritten by Xen for Dom0 and does not include the
reserved-memory regions so far.
>
> I am suspecting there is a bug in the patch "xen/arm: map
> reserved-memory regions as normal memory in dom0", specifically in the
> implementation of check_reserved_memory.
I don't think the issue is in that code (see above).
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH 0/6] iomem cacheability
2019-03-06 22:59 ` Julien Grall
@ 2019-03-07 8:42 ` Amit Tomer
2019-03-07 10:04 ` Julien Grall
0 siblings, 1 reply; 67+ messages in thread
From: Amit Tomer @ 2019-03-07 8:42 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, wei.liu2, Andrew Cooper, Julien Grall,
Jan Beulich, xen-devel, ian.jackson, nd
Hi,
> Not really, the DT provided by Amit is for the host. The memory node
> will be rewritten by Xen for Dom0 and does not include the
> reserved-memory regions so far.
>
Thanks for pointing that out.
Is it like we need to create "separate" reserve-memory
node(make_reserved-memory_node) when
memory node is encountered?
Thanks
-Amit
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH 0/6] iomem cacheability
2019-03-07 8:42 ` Amit Tomer
@ 2019-03-07 10:04 ` Julien Grall
2019-03-07 21:24 ` Stefano Stabellini
0 siblings, 1 reply; 67+ messages in thread
From: Julien Grall @ 2019-03-07 10:04 UTC (permalink / raw)
To: Amit Tomer
Cc: Stefano Stabellini, wei.liu2, Andrew Cooper, Julien Grall,
Jan Beulich, xen-devel, ian.jackson, nd
On 07/03/2019 08:42, Amit Tomer wrote:
> Hi,
>
>> Not really, the DT provided by Amit is for the host. The memory node
>> will be rewritten by Xen for Dom0 and does not include the
>> reserved-memory regions so far.
>>
> Thanks for pointing that out.
>
> Is it like we need to create "separate" reserve-memory
> node(make_reserved-memory_node) when
> memory node is encountered?
Sorry I don't understand what you mean.
What I meant is reserved-regions is some memory that have been carved out from
the RAM. So the reserved-regions needs to be described in the memory node
(memory@<addr>). We can either append them to the memory node created by
make_memory_node. Or introduce new ones.
I haven't yet thought which one is more suitable.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH 0/6] iomem cacheability
2019-03-07 10:04 ` Julien Grall
@ 2019-03-07 21:24 ` Stefano Stabellini
2019-03-08 10:10 ` Amit Tomer
0 siblings, 1 reply; 67+ messages in thread
From: Stefano Stabellini @ 2019-03-07 21:24 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, wei.liu2, Andrew Cooper, Amit Tomer,
Julien Grall, Jan Beulich, xen-devel, ian.jackson, nd
On Thu, 7 Mar 2019, Julien Grall wrote:
> On 07/03/2019 08:42, Amit Tomer wrote:
> > Hi,
> >
> > > Not really, the DT provided by Amit is for the host. The memory node
> > > will be rewritten by Xen for Dom0 and does not include the
> > > reserved-memory regions so far.
> > >
> > Thanks for pointing that out.
> >
> > Is it like we need to create "separate" reserve-memory
> > node(make_reserved-memory_node) when
> > memory node is encountered?
>
> Sorry I don't understand what you mean.
>
> What I meant is reserved-regions is some memory that have been carved out from
> the RAM. So the reserved-regions needs to be described in the memory node
> (memory@<addr>). We can either append them to the memory node created by
> make_memory_node. Or introduce new ones.
>
> I haven't yet thought which one is more suitable.
Yes, you are right. I made a couple of quick fixes for this issue and
another issue raised by Julien during review (the usage of p2m_ram_t).
Amit, if you would like to give it a try I have a work-in-progress
branch with the fixes here:
http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git iomem_cache-wip
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH 0/6] iomem cacheability
2019-03-07 21:24 ` Stefano Stabellini
@ 2019-03-08 10:10 ` Amit Tomer
2019-03-08 16:37 ` Julien Grall
0 siblings, 1 reply; 67+ messages in thread
From: Amit Tomer @ 2019-03-08 10:10 UTC (permalink / raw)
To: Stefano Stabellini
Cc: wei.liu2, Andre Przywara, Andrew Cooper, Julien Grall,
Julien Grall, Jan Beulich, xen-devel, ian.jackson, nd
Hi,
> Yes, you are right. I made a couple of quick fixes for this issue and
> another issue raised by Julien during review (the usage of p2m_ram_t).
> Amit, if you would like to give it a try I have a work-in-progress
> branch with the fixes here:
We did try new branch with new fixes but we see some other crash now.
XEN) Loading kernel from boot module @ 000000007a000000
(XEN) Allocating 1:1 mappings totalling 2048MB for dom0:
(XEN) No bank has been allocated below 4GB.
(XEN) BANK[0] 0x00000500000000-0x00000540000000 (1024MB)
(XEN) BANK[1] 0x00000600000000-0x00000640000000 (1024MB)
(XEN) Grant table range: 0x0000073fe00000-0x0000073fe40000
(XEN) Allocating PPI 16 for event channel interrupt
(XEN) Loading zImage from 000000007a000000 to 0000000500080000-0000000501880000
(XEN) Loading dom0 DTB to 0x0000000508000000-0x00000005080117d0
(XEN) Initial low memory virq threshold set at 0x4000 pages.
(XEN) Scrubbing free RAM on in background
(XEN) Std. Loglevel: All
(XEN) Guest Loglevel: All
(XEN) *** Serial input -> DOM0 (type 'CTRL-a' three times to switch
input to Xen)
(XEN) Freed 292kB init memory.
[ 0.000000] Booting Linux on physical CPU 0x0
[ 0.000000] Boot CPU: AArch64 Processor [411fd073]
[ 0.000000] Machine model: Renesas H3ULCB board based on r8a7795 ES2.0+
[ 0.000000] earlycon: xenboot0 at I/O port 0x0 (options '')
[ 0.000000] bootconsole [xenboot0] enabled
[ 0.000000] Xen 4.12 support found
[ 0.000000] efi: Getting EFI parameters from FDT:
[ 0.000000] efi: UEFI not found.
[ 0.000000] Reserved memory: created CMA memory pool at
0x0000000057000000, size 400 MiB
[ 0.000000] OF: reserved mem: initialized node linux,cma@57000000,
compatible id shared-dma-pool
[ 0.000000] Reserved memory: created CMA memory pool at
0x0000000070000000, size 256 MiB
[ 0.000000] OF: reserved mem: initialized node
linux,multimedia@70000000, compatible id shared-dma-pool
[ 0.000000] cma: dma_contiguous_reserve(limit 100000000)
[ 0.000000] NUMA: No NUMA configuration found
[ 0.000000] NUMA: Faking a node at [mem
0x0000000000000000-0x000000063fffffff]
[ 0.000000] NUMA: NODE_DATA [mem 0x63ff90c00-0x63ff926ff]
[ 0.000000] Zone ranges:
[ 0.000000] DMA [mem 0x0000000057000000-0x00000000ffffffff]
[ 0.000000] Normal [mem 0x0000000100000000-0x000000063fffffff]
[ 0.000000] Movable zone start for each node
[ 0.000000] Early memory node ranges
[ 0.000000] node 0: [mem 0x0000000057000000-0x000000007fffffff]
[ 0.000000] node 0: [mem 0x0000000500000000-0x000000053fffffff]
[ 0.000000] node 0: [mem 0x0000000600000000-0x000000063fffffff]
[ 0.000000] Initmem setup node 0 [mem 0x0000000057000000-0x000000063fffffff]
[ 0.000000] On node 0 totalpages: 692224
[ 0.000000] DMA zone: 2624 pages used for memmap
[ 0.000000] DMA zone: 0 pages reserved
[ 0.000000] DMA zone: 167936 pages, LIFO batch:31
[ 0.000000] Normal zone: 8192 pages used for memmap
[ 0.000000] Normal zone: 524288 pages, LIFO batch:31
[ 0.000000] bootmem alloc of 64 bytes failed!
[ 0.000000] Kernel panic - not syncing: Out of memory
[ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted
4.14.75-ltsi-yocto-standard #3
[ 0.000000] Hardware name: Renesas H3ULCB board based on r8a7795 ES2.0+ (DT)
[ 0.000000] Call trace:
[ 0.000000] [<ffff000008089ae8>] dump_backtrace+0x0/0x3c0
[ 0.000000] [<ffff000008089ebc>] show_stack+0x14/0x20
[ 0.000000] [<ffff000008af20e8>] dump_stack+0x9c/0xbc
[ 0.000000] [<ffff0000080ce770>] panic+0x11c/0x28c
[ 0.000000] [<ffff0000090478fc>] free_bootmem_late+0x0/0x7c
[ 0.000000] [<ffff000009047d90>] __alloc_bootmem_low+0x2c/0x38
[ 0.000000] [<ffff0000090329fc>] setup_arch+0x258/0x4d8
[ 0.000000] [<ffff00000903083c>] start_kernel+0x64/0x3ac
[ 0.000000] ---[ end Kernel panic - not syncing: Out of memory
Thanks
-Amit
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH 0/6] iomem cacheability
2019-03-08 10:10 ` Amit Tomer
@ 2019-03-08 16:37 ` Julien Grall
2019-03-08 17:44 ` Amit Tomer
0 siblings, 1 reply; 67+ messages in thread
From: Julien Grall @ 2019-03-08 16:37 UTC (permalink / raw)
To: Amit Tomer, Stefano Stabellini
Cc: wei.liu2, Andrew Cooper, Julien Grall, Andre Przywara,
Jan Beulich, xen-devel, ian.jackson, nd
Hi,
On 3/8/19 10:10 AM, Amit Tomer wrote:
>> Yes, you are right. I made a couple of quick fixes for this issue and
>> another issue raised by Julien during review (the usage of p2m_ram_t).
>> Amit, if you would like to give it a try I have a work-in-progress
>> branch with the fixes here:
>
> We did try new branch with new fixes but we see some other crash now.
>
> XEN) Loading kernel from boot module @ 000000007a000000
> (XEN) Allocating 1:1 mappings totalling 2048MB for dom0:
> (XEN) No bank has been allocated below 4GB.
> (XEN) BANK[0] 0x00000500000000-0x00000540000000 (1024MB)
> (XEN) BANK[1] 0x00000600000000-0x00000640000000 (1024MB)
> (XEN) Grant table range: 0x0000073fe00000-0x0000073fe40000
> (XEN) Allocating PPI 16 for event channel interrupt
> (XEN) Loading zImage from 000000007a000000 to 0000000500080000-0000000501880000
> (XEN) Loading dom0 DTB to 0x0000000508000000-0x00000005080117d0
> (XEN) Initial low memory virq threshold set at 0x4000 pages.
> (XEN) Scrubbing free RAM on in background
> (XEN) Std. Loglevel: All
> (XEN) Guest Loglevel: All
> (XEN) *** Serial input -> DOM0 (type 'CTRL-a' three times to switch
> input to Xen)
> (XEN) Freed 292kB init memory.
> [ 0.000000] Booting Linux on physical CPU 0x0
> [ 0.000000] Boot CPU: AArch64 Processor [411fd073]
> [ 0.000000] Machine model: Renesas H3ULCB board based on r8a7795 ES2.0+
> [ 0.000000] earlycon: xenboot0 at I/O port 0x0 (options '')
> [ 0.000000] bootconsole [xenboot0] enabled
> [ 0.000000] Xen 4.12 support found
> [ 0.000000] efi: Getting EFI parameters from FDT:
> [ 0.000000] efi: UEFI not found.
> [ 0.000000] Reserved memory: created CMA memory pool at
> 0x0000000057000000, size 400 MiB
> [ 0.000000] OF: reserved mem: initialized node linux,cma@57000000,
> compatible id shared-dma-pool
> [ 0.000000] Reserved memory: created CMA memory pool at
> 0x0000000070000000, size 256 MiB
> [ 0.000000] OF: reserved mem: initialized node
> linux,multimedia@70000000, compatible id shared-dma-pool
> [ 0.000000] cma: dma_contiguous_reserve(limit 100000000)
> [ 0.000000] NUMA: No NUMA configuration found
> [ 0.000000] NUMA: Faking a node at [mem
> 0x0000000000000000-0x000000063fffffff]
> [ 0.000000] NUMA: NODE_DATA [mem 0x63ff90c00-0x63ff926ff]
> [ 0.000000] Zone ranges:
> [ 0.000000] DMA [mem 0x0000000057000000-0x00000000ffffffff]
> [ 0.000000] Normal [mem 0x0000000100000000-0x000000063fffffff]
> [ 0.000000] Movable zone start for each node
> [ 0.000000] Early memory node ranges
> [ 0.000000] node 0: [mem 0x0000000057000000-0x000000007fffffff]
> [ 0.000000] node 0: [mem 0x0000000500000000-0x000000053fffffff]
> [ 0.000000] node 0: [mem 0x0000000600000000-0x000000063fffffff]
> [ 0.000000] Initmem setup node 0 [mem 0x0000000057000000-0x000000063fffffff]
> [ 0.000000] On node 0 totalpages: 692224
> [ 0.000000] DMA zone: 2624 pages used for memmap
> [ 0.000000] DMA zone: 0 pages reserved
> [ 0.000000] DMA zone: 167936 pages, LIFO batch:31
> [ 0.000000] Normal zone: 8192 pages used for memmap
> [ 0.000000] Normal zone: 524288 pages, LIFO batch:31
> [ 0.000000] bootmem alloc of 64 bytes failed!
> [ 0.000000] Kernel panic - not syncing: Out of memory
> [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted
> 4.14.75-ltsi-yocto-standard #3
> [ 0.000000] Hardware name: Renesas H3ULCB board based on r8a7795 ES2.0+ (DT)
> [ 0.000000] Call trace:
> [ 0.000000] [<ffff000008089ae8>] dump_backtrace+0x0/0x3c0
> [ 0.000000] [<ffff000008089ebc>] show_stack+0x14/0x20
> [ 0.000000] [<ffff000008af20e8>] dump_stack+0x9c/0xbc
> [ 0.000000] [<ffff0000080ce770>] panic+0x11c/0x28c
> [ 0.000000] [<ffff0000090478fc>] free_bootmem_late+0x0/0x7c
> [ 0.000000] [<ffff000009047d90>] __alloc_bootmem_low+0x2c/0x38
> [ 0.000000] [<ffff0000090329fc>] setup_arch+0x258/0x4d8
> [ 0.000000] [<ffff00000903083c>] start_kernel+0x64/0x3ac
> [ 0.000000] ---[ end Kernel panic - not syncing: Out of memory
Interesting, the function __alloc_bootmem_low will try to allocate low
memory. The limit for low memory is based on the physical DMA address
limit (see ARCH_LOW_ADDRESS_LIMIT).
I guess you have CONFIG_ZONE_DMA32 (was renamed from CONFIG_ZONE_DMA is
recent Linux) set. So the DMA limit will be computed by max_zone_dma_phys().
In your setup, if I am not mistaken, the limit will be 4GB. However, you
have no available memory below 4GB because it is used for
reserved-memory. So it makes sense for __alloc_bootmem_low to fail.
In your previous setup (i.e without reserved memory), all the memory was
probably still above 4GB. In that case max_zone_dma_phys() will return
an higher "lower" limit. So __alloc_bootmem_low is going to succeed.
AFAICT, this is probably a Linux bug. Now the question is whether
__alloc_bootmem_low should be switch to alloc_bootmem or we need to fix
the implementation of the function. Note that recent Linux (5.0 and
onwards) have switch to memblock API, yet I think the issue is still here.
Fixing Linux is probably a good idea, but this means that upgrade of Xen
may break Linux boot. In one hand, reserved-memory never worked with Xen
(user have to manually disable it). So this is not a regression. On the
other hand, a user will now need to upgrade their Linux (could be
difficult if tie to a BSP kernel) or apply a fix (see above).
It might be possible to rework Dom0 memory allocation to limit the
damage or even re-order the binary in memory. Amit, could you post the
full Xen log with earlyprintk enabled?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH 0/6] iomem cacheability
2019-03-08 16:37 ` Julien Grall
@ 2019-03-08 17:44 ` Amit Tomer
0 siblings, 0 replies; 67+ messages in thread
From: Amit Tomer @ 2019-03-08 17:44 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, wei.liu2, Andrew Cooper, Julien Grall,
Andre Przywara, Jan Beulich, xen-devel, ian.jackson, nd
Hi,
> It might be possible to rework Dom0 memory allocation to limit the
> damage or even re-order the binary in memory. Amit, could you post the
> full Xen log with earlyprintk enabled?
Please find XEN logs :
[ 229.769854] Starting kernel ...
[ 229.773120]
- UART enabled -
- CPU 00000000 booting -
- Current EL 00000008 -
- Xen starting at EL2 -
- Zero BSS -
- Setting up control registers -
- Turning on paging -
- Ready -
(XEN) Checking for initrd in /chosen
(XEN) RAM: 0000000048000000 - 000000007fffffff
(XEN) RAM: 0000000500000000 - 000000053fffffff
(XEN) RAM: 0000000600000000 - 000000063fffffff
(XEN) RAM: 0000000700000000 - 000000073fffffff
(XEN)
(XEN) MODULE[0]: 000000007d70f000 - 000000007d722000 Device Tree
(XEN) MODULE[1]: 000000007a000000 - 000000007b800000 Kernel
(XEN) RESVD[0]: 000000004a000000 - 000000004a013000
(XEN) RESVD[1]: 000000007d70f000 - 000000007d722000
(XEN)
(XEN) Command line: console=dtuart dom0_mem=2048M
(XEN) Placing Xen at 0x000000073fe00000-0x0000000740000000
(XEN) Update BOOTMOD_XEN from 0000000048000000-0000000048118d81 =>
000000073fe00000-000000073ff18d81
(XEN) PFN compression on bits 19...19
(XEN) Domain heap initialised
(XEN) Booting using Device Tree
(XEN) Platform: Generic System
(XEN) Taking dtuart configuration from /chosen/stdout-path
(XEN) Looking for dtuart at "serial0", options "115200n8"
(XEN) WARNING: UART configuration is not supported
Xen 4.12-unstable
(XEN) Xen version 4.12-unstable (amit@) (aarch64-linux-gnu-gcc (Linaro
GCC 7.3-2018.05) 7.3.1 20180425 [linaro-7.3-2018.05 revision
d29120a424ecfbc167ef90065c0eeb7f91977701]) debug=y Fri Mar 8
13:09:49 IST 2019
(XEN) Latest ChangeSet: Thu Mar 7 13:22:10 2019 -0800 git:62617af
(XEN) Processor: 411fd073: "ARM Limited", variant: 0x1, part 0xd07, rev 0x3
(XEN) 64-bit Execution:
(XEN) Processor Features: 0000000000002222 0000000000000000
(XEN) Exception Levels: EL3:64+32 EL2:64+32 EL1:64+32 EL0:64+32
(XEN) Extensions: FloatingPoint AdvancedSIMD
(XEN) Debug Features: 0000000010305106 0000000000000000
(XEN) Auxiliary Features: 0000000000000000 0000000000000000
(XEN) Memory Model Features: 0000000000001124 0000000000000000
(XEN) ISA Features: 0000000000011120 0000000000000000
(XEN) 32-bit Execution:
(XEN) Processor Features: 00000131:00011011
(XEN) Instruction Sets: AArch32 A32 Thumb Thumb-2 Jazelle
(XEN) Extensions: GenericTimer Security
(XEN) Debug Features: 03010066
(XEN) Auxiliary Features: 00000000
(XEN) Memory Model Features: 10201105 40000000 01260000 02102211
(XEN) ISA Features: 02101110 13112111 21232042 01112131 00011142 00011121
(XEN) Using SMC Calling Convention v1.1
(XEN) Using PSCI v1.0
(XEN) SMP: Allowing 8 CPUs
(XEN) Generic Timer IRQ: phys=30 hyp=26 virt=27 Freq: 8333 KHz
(XEN) GICv2 initialization:
(XEN) gic_dist_addr=00000000f1010000
(XEN) gic_cpu_addr=00000000f1020000
(XEN) gic_hyp_addr=00000000f1040000
(XEN) gic_vcpu_addr=00000000f1060000
(XEN) gic_maintenance_irq=25
(XEN) GICv2: Adjusting CPU interface base to 0xf102f000
(XEN) GICv2: 512 lines, 8 cpus, secure (IID 0200043b).
(XEN) Using scheduler: SMP Credit Scheduler rev2 (credit2)
(XEN) Initializing Credit2 scheduler
(XEN) load_precision_shift: 18
(XEN) load_window_shift: 30
(XEN) underload_balance_tolerance: 0
(XEN) overload_balance_tolerance: -3
(XEN) runqueues arrangement: socket
(XEN) cap enforcement granularity: 10ms
(XEN) load tracking window length 1073741824 ns
(XEN) Adding cpu 0 to runqueue 0
(XEN) First cpu on runqueue, activating
(XEN) Allocated console ring of 64 KiB.
(XEN) Bringing up CPU1
- CPU 00000001 booting -
- Current EL 00000008 -
- Xen starting at EL2 -
- Setting up control registers -
- Turning on paging -
- Ready -
(XEN) Adding cpu 1 to runqueue 0
(XEN) CPU 1 booted.
(XEN) Bringing up CPU2
- CPU 00000002 booting -
- Current EL 00000008 -
- Xen starting at EL2 -
- Setting up control registers -
- Turning on paging -
- Ready -
(XEN) Adding cpu 2 to runqueue 0
(XEN) CPU 2 booted.
(XEN) Bringing up CPU3
- CPU 00000003 booting -
- Current EL 00000008 -
- Xen starting at EL2 -
- Setting up control registers -
- Turning on paging -
- Ready -
(XEN) Adding cpu 3 to runqueue 0
(XEN) CPU 3 booted.
(XEN) Bringing up CPU4
- CPU 00000100 booting -
- Current EL 00000008 -
- Xen starting at EL2 -
- Setting up control registers -
- Turning on paging -
- Ready -
(XEN) CPU4 MIDR (0x410fd034) does not match boot CPU MIDR (0x411fd073),
(XEN) disable cpu (see big.LITTLE.txt under docs/).
(XEN) CPU4 never came online
(XEN) Failed to bring up CPU 4 (error -5)
(XEN) Bringing up CPU5
- CPU 00000101 booting -
- Current EL 00000008 -
- Xen starting at EL2 -
- Setting up control registers -
- Turning on paging -
- Ready -
(XEN) CPU5 MIDR (0x410fd034) does not match boot CPU MIDR (0x411fd073),
(XEN) disable cpu (see big.LITTLE.txt under docs/).
(XEN) CPU5 never came online
(XEN) Failed to bring up CPU 5 (error -5)
(XEN) Bringing up CPU6
- CPU 00000102 booting -
- Current EL 00000008 -
- Xen starting at EL2 -
- Setting up control registers -
- Turning on paging -
- Ready -
(XEN) CPU6 MIDR (0x410fd034) does not match boot CPU MIDR (0x411fd073),
(XEN) disable cpu (see big.LITTLE.txt under docs/).
(XEN) CPU6 never came online
(XEN) Failed to bring up CPU 6 (error -5)
(XEN) Bringing up CPU7
- CPU 00000103 booting -
- Current EL 00000008 -
- Xen starting at EL2 -
- Setting up control registers -
- Turning on paging -
- Ready -
(XEN) CPU7 MIDR (0x410fd034) does not match boot CPU MIDR (0x411fd073),
(XEN) disable cpu (see big.LITTLE.txt under docs/).
(XEN) CPU7 never came online
(XEN) Failed to bring up CPU 7 (error -5)
(XEN) Brought up 4 CPUs
(XEN) P2M: 44-bit IPA with 44-bit PA and 8-bit VMID
(XEN) P2M: 4 levels with order-0 root, VTCR 0x80043594
(XEN) I/O virtualisation disabled
(XEN) build-id: d626f521c35090600cb1fa5875a3ca2e55c290c0
(XEN) alternatives: Patching with alt table 00000000002aba18 -> 00000000002ac018
(XEN) CPU0 will call ARM_SMCCC_ARCH_WORKAROUND_1 on exception entry
(XEN) CPU1 will call ARM_SMCCC_ARCH_WORKAROUND_1 on exception entry
(XEN) CPU3 will call ARM_SMCCC_ARCH_WORKAROUND_1 on exception entry
(XEN) CPU2 will call ARM_SMCCC_ARCH_WORKAROUND_1 on exception entry
(XEN) *** LOADING DOMAIN 0 ***
(XEN) Loading kernel from boot module @ 000000007a000000
(XEN) Allocating 1:1 mappings totalling 2048MB for dom0:
(XEN) No bank has been allocated below 4GB.
(XEN) BANK[0] 0x00000500000000-0x00000540000000 (1024MB)
(XEN) BANK[1] 0x00000600000000-0x00000640000000 (1024MB)
(XEN) Grant table range: 0x0000073fe00000-0x0000073fe40000
(XEN) Allocating PPI 16 for event channel interrupt
(XEN) Loading zImage from 000000007a000000 to 0000000500080000-0000000501880000
(XEN) Loading dom0 DTB to 0x0000000508000000-0x00000005080117d0
(XEN) Initial low memory virq threshold set at 0x4000 pages.
(XEN) Scrubbing free RAM on in background
(XEN) Std. Loglevel: All
(XEN) Guest Loglevel: All
(XEN) *** Serial input -> DOM0 (type 'CTRL-a' three times to switch
input to Xen)
(XEN) Freed 292kB init memory.
[ 0.000000] Booting Linux on physical CPU 0x0
[ 0.000000] Linux version 4.14.75-ltsi-yocto-standard (amit@) (gcc
version 7.4.1 20181213 [linaro-7.4-2019.02 revision
56ec6f6b99cc167ff0c2f8e1a2eed33b1edc85d4] (Linaro GCC 7.4-2019.02)) #3
SMP PREEMPT Sun Mar 3 22:22:22 IST9
[ 0.000000] Boot CPU: AArch64 Processor [411fd073]
[ 0.000000] Machine model: Renesas H3ULCB board based on r8a7795 ES2.0+
[ 0.000000] earlycon: xenboot0 at I/O port 0x0 (options '')
[ 0.000000] bootconsole [xenboot0] enabled
[ 0.000000] Xen 4.12 support found
[ 0.000000] efi: Getting EFI parameters from FDT:
[ 0.000000] efi: UEFI not found.
[ 0.000000] Reserved memory: created CMA memory pool at
0x0000000057000000, size 400 MiB
[ 0.000000] OF: reserved mem: initialized node linux,cma@57000000,
compatible id shared-dma-pool
[ 0.000000] Reserved memory: created CMA memory pool at
0x0000000070000000, size 256 MiB
[ 0.000000] OF: reserved mem: initialized node
linux,multimedia@70000000, compatible id shared-dma-pool
[ 0.000000] cma: dma_contiguous_reserve(limit 100000000)
[ 0.000000] NUMA: No NUMA configuration found
[ 0.000000] NUMA: Faking a node at [mem
0x0000000000000000-0x000000063fffffff]
[ 0.000000] NUMA: NODE_DATA [mem 0x63ff90c00-0x63ff926ff]
[ 0.000000] Zone ranges:
[ 0.000000] DMA [mem 0x0000000057000000-0x00000000ffffffff]
[ 0.000000] Normal [mem 0x0000000100000000-0x000000063fffffff]
[ 0.000000] Movable zone start for each node
[ 0.000000] Early memory node ranges
[ 0.000000] node 0: [mem 0x0000000057000000-0x000000007fffffff]
[ 0.000000] node 0: [mem 0x0000000500000000-0x000000053fffffff]
[ 0.000000] node 0: [mem 0x0000000600000000-0x000000063fffffff]
[ 0.000000] Initmem setup node 0 [mem 0x0000000057000000-0x000000063fffffff]
[ 0.000000] On node 0 totalpages: 692224
[ 0.000000] DMA zone: 2624 pages used for memmap
[ 0.000000] DMA zone: 0 pages reserved
[ 0.000000] DMA zone: 167936 pages, LIFO batch:31
[ 0.000000] Normal zone: 8192 pages used for memmap
[ 0.000000] Normal zone: 524288 pages, LIFO batch:31
[ 0.000000] bootmem alloc of 64 bytes failed!
[ 0.000000] Kernel panic - not syncing: Out of memory
[ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted
4.14.75-ltsi-yocto-standard #3
[ 0.000000] Hardware name: Renesas H3ULCB board based on r8a7795 ES2.0+ (DT)
[ 0.000000] Call trace:
[ 0.000000] [<ffff000008089ae8>] dump_backtrace+0x0/0x3c0
[ 0.000000] [<ffff000008089ebc>] show_stack+0x14/0x20
[ 0.000000] [<ffff000008af20e8>] dump_stack+0x9c/0xbc
[ 0.000000] [<ffff0000080ce770>] panic+0x11c/0x28c
[ 0.000000] [<ffff0000090478fc>] free_bootmem_late+0x0/0x7c
[ 0.000000] [<ffff000009047d90>] __alloc_bootmem_low+0x2c/0x38
[ 0.000000] [<ffff0000090329fc>] setup_arch+0x258/0x4d8
[ 0.000000] [<ffff00000903083c>] start_kernel+0x64/0x3ac
[ 0.000000] ---[ end Kernel panic - not syncing: Out of memory
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH 0/6] iomem cacheability
2019-03-05 21:22 ` Stefano Stabellini
2019-03-05 22:45 ` Julien Grall
@ 2019-03-06 11:30 ` Amit Tomer
1 sibling, 0 replies; 67+ messages in thread
From: Amit Tomer @ 2019-03-06 11:30 UTC (permalink / raw)
To: Stefano Stabellini
Cc: wei.liu2, Andrew Cooper, Julien Grall, Jan Beulich, ian.jackson,
xen-devel
[-- Attachment #1: Type: text/plain, Size: 200 bytes --]
Hi
> Thanks for testing! You might have found a real bug in the series. Could
> you please also attach the full device tree?
Please find the attached DTS and DTB file used for testing.
Thanks
-Amit
[-- Attachment #2: r8a7795-h3ulcb.dts --]
[-- Type: audio/vnd.dts, Size: 2624 bytes --]
[-- Attachment #3: r8a7795-h3ulcb.dtb --]
[-- Type: application/octet-stream, Size: 74933 bytes --]
[-- Attachment #4: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 67+ messages in thread