* [PATCH v5 1/8] arch, arm: domain build: let dom0 access I/O memory of mapped devices
2014-04-06 23:31 [PATCH v5 0/8] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
@ 2014-04-06 23:31 ` Arianna Avanzini
2014-04-06 23:31 ` [PATCH v5 2/8] arch, arm: add consistency check to REMOVE p2m changes Arianna Avanzini
` (6 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Arianna Avanzini @ 2014-04-06 23:31 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
JBeulich, avanzini.arianna, viktor.kleinik
Currently, dom0 is allowed access to the I/O memory ranges used
to access devices exposed to it, but it doesn't have those
ranges in its iomem_caps. This commit attempts to implement the
correct bookkeeping in the generic function which actually maps
a device's I/O memory to the domain, adding the ranges to the
domain's iomem_caps.
Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Acked-by: Ian Campbell <Ian.Campbell@eu.citrix.com>
Acked-by: Julien Grall <julien.grall@citrix.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
v4:
- Hopefully improve commit description.
v3:
- Print the domain id in the error message instead of always
printing "dom0".
- Fix commit description.
---
xen/arch/arm/domain_build.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 502db84..f4902bb 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -11,6 +11,7 @@
#include <xen/device_tree.h>
#include <xen/libfdt/libfdt.h>
#include <xen/guest_access.h>
+#include <xen/iocap.h>
#include <asm/setup.h>
#include <asm/platform.h>
#include <asm/psci.h>
@@ -733,6 +734,16 @@ static int map_device(struct domain *d, const struct dt_device_node *dev)
DPRINT("addr %u = 0x%"PRIx64" - 0x%"PRIx64"\n",
i, addr, addr + size - 1);
+ res = iomem_permit_access(d, paddr_to_pfn(addr & PAGE_MASK),
+ paddr_to_pfn(PAGE_ALIGN(addr + size - 1)));
+ if ( res )
+ {
+ printk(XENLOG_ERR "Unable to permit to dom%d access to"
+ " 0x%"PRIx64" - 0x%"PRIx64"\n",
+ d->domain_id,
+ addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
+ return res;
+ }
res = map_mmio_regions(d, addr & PAGE_MASK,
PAGE_ALIGN(addr + size) - 1,
addr & PAGE_MASK);
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v5 2/8] arch, arm: add consistency check to REMOVE p2m changes
2014-04-06 23:31 [PATCH v5 0/8] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
2014-04-06 23:31 ` [PATCH v5 1/8] arch, arm: domain build: let dom0 access I/O memory of mapped devices Arianna Avanzini
@ 2014-04-06 23:31 ` Arianna Avanzini
2014-04-07 11:05 ` Julien Grall
2014-04-06 23:31 ` [PATCH v5 3/8] arch, arm: let map_mmio_regions() take pfn as parameters Arianna Avanzini
` (5 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Arianna Avanzini @ 2014-04-06 23:31 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
JBeulich, avanzini.arianna, viktor.kleinik
Currently, the REMOVE case of the switch in apply_p2m_changes()
does not perform any consistency check on the mapping to be removed.
More in detail, the code does not check if the guest address to be
unmapped is actually mapped to the machine address given as a
parameter.
This commit attempts to add the above-described consistency check
to the REMOVE path of apply_p2m_changes(). This is instrumental to
one of the following commits which implements the possibility to
trigger the removal of p2m ranges via the memory_mapping DOMCTL
for ARM.
Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Julien Grall <julien.grall@citrix.com>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
v5:
- Do not use a temporary variable to hold the machine address:
use the "maddr" function parameter itself.
- Increment the machine address also when first and second level
mappings are not valid.
- Get the actual machine frame number mapped to the guest frame
number given as parameter to the function directly in the
REMOVE case of the switch construct, as it might not be valid
in other cases and its value might be uncorrectly used in the
future.
- Remove useless and/or harmful ASSERT; check however if the
mapping is valid and skip the page if it is not.
v4:
- Remove useless and slow lookup and use already-available
data from pte instead.
- Correctly increment the local variable used to keep the
machine address whose mapping is currently being removed.
- Return with an error upon finding a mismatch between the
actual machine address mapped to the guest address and
the machine address passed as parameter, instead of just
skipping the page.
---
xen/arch/arm/p2m.c | 27 +++++++++++++++++++++++++--
1 file changed, 25 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 403fd89..395a0b2 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -319,6 +319,7 @@ static int apply_p2m_changes(struct domain *d,
if ( !populate )
{
addr = (addr + FIRST_SIZE) & FIRST_MASK;
+ maddr = (maddr + FIRST_SIZE) & FIRST_MASK;
continue;
}
@@ -345,6 +346,7 @@ static int apply_p2m_changes(struct domain *d,
if ( !populate )
{
addr = (addr + SECOND_SIZE) & SECOND_MASK;
+ maddr = (maddr + SECOND_SIZE) & SECOND_MASK;
continue;
}
@@ -406,12 +408,32 @@ static int apply_p2m_changes(struct domain *d,
{
pte = mfn_to_p2m_entry(maddr >> PAGE_SHIFT, mattr, t);
write_pte(&third[third_table_offset(addr)], pte);
- maddr += PAGE_SIZE;
}
break;
- case RELINQUISH:
case REMOVE:
{
+ unsigned long mfn;
+
+ if ( !pte.p2m.valid )
+ {
+ count++;
+ break;
+ }
+ mfn = pte.p2m.base;
+ /*
+ * Ensure that the guest address given as argument to
+ * this function is actually mapped to the specified
+ * machine address. maddr here is the machine address
+ * given to the function, while mfn is the machine
+ * frame number actually mapped to the guest address:
+ * check if the two correspond.
+ */
+ if ( maddr != pfn_to_paddr(mfn) )
+ return -EINVAL;
+ }
+ /* fall through */
+ case RELINQUISH:
+ {
if ( !pte.p2m.valid )
{
count++;
@@ -450,6 +472,7 @@ static int apply_p2m_changes(struct domain *d,
/* Got the next page */
addr += PAGE_SIZE;
+ maddr += PAGE_SIZE;
}
if ( flush )
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v5 2/8] arch, arm: add consistency check to REMOVE p2m changes
2014-04-06 23:31 ` [PATCH v5 2/8] arch, arm: add consistency check to REMOVE p2m changes Arianna Avanzini
@ 2014-04-07 11:05 ` Julien Grall
2014-04-09 13:39 ` Ian Campbell
0 siblings, 1 reply; 19+ messages in thread
From: Julien Grall @ 2014-04-07 11:05 UTC (permalink / raw)
To: Arianna Avanzini
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, xen-devel, Ian.Campbell, etrudeau,
JBeulich, viktor.kleinik
Hi Arianna,
Thank you for the patch.
On 04/07/2014 12:31 AM, Arianna Avanzini wrote:
> ---
> xen/arch/arm/p2m.c | 27 +++++++++++++++++++++++++--
> 1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 403fd89..395a0b2 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -319,6 +319,7 @@ static int apply_p2m_changes(struct domain *d,
> if ( !populate )
> {
> addr = (addr + FIRST_SIZE) & FIRST_MASK;
> + maddr = (maddr + FIRST_SIZE) & FIRST_MASK;
> continue;
> }
>
> @@ -345,6 +346,7 @@ static int apply_p2m_changes(struct domain *d,
> if ( !populate )
> {
> addr = (addr + SECOND_SIZE) & SECOND_MASK;
> + maddr = (maddr + SECOND_SIZE) & SECOND_MASK;
> continue;
> }
>
> @@ -406,12 +408,32 @@ static int apply_p2m_changes(struct domain *d,
> {
> pte = mfn_to_p2m_entry(maddr >> PAGE_SHIFT, mattr, t);
> write_pte(&third[third_table_offset(addr)], pte);
> - maddr += PAGE_SIZE;
> }
> break;
> - case RELINQUISH:
> case REMOVE:
> {
> + unsigned long mfn;
> +
> + if ( !pte.p2m.valid )
> + {
> + count++;
count is only used for RELINQUISH mapping, you don't need to update here.
> + break;
I think we should fail if we can't remove rather than continuing.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v5 2/8] arch, arm: add consistency check to REMOVE p2m changes
2014-04-07 11:05 ` Julien Grall
@ 2014-04-09 13:39 ` Ian Campbell
2014-04-22 19:27 ` Julien Grall
0 siblings, 1 reply; 19+ messages in thread
From: Ian Campbell @ 2014-04-09 13:39 UTC (permalink / raw)
To: Julien Grall
Cc: paolo.valente, keir, stefano.stabellini, tim, dario.faggioli,
Ian.Jackson, xen-devel, julien.grall, etrudeau, JBeulich,
Arianna Avanzini, viktor.kleinik
On Mon, 2014-04-07 at 12:05 +0100, Julien Grall wrote:
> count is only used for RELINQUISH mapping, you don't need to update here.
Not related to this patch, but the current code is:
if ( !pte.p2m.valid )
{
count++;
break;
}
count += 0x10;
memset(&pte, 0x00, sizeof(pte));
write_pte(&third[third_table_offset(addr)], pte);
count++;
}
It seems like that final count++ is unnecessary/wrong.
Ian.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v5 2/8] arch, arm: add consistency check to REMOVE p2m changes
2014-04-09 13:39 ` Ian Campbell
@ 2014-04-22 19:27 ` Julien Grall
0 siblings, 0 replies; 19+ messages in thread
From: Julien Grall @ 2014-04-22 19:27 UTC (permalink / raw)
To: Ian Campbell
Cc: paolo.valente, keir, stefano.stabellini, tim, dario.faggioli,
Ian.Jackson, xen-devel, julien.grall, etrudeau, JBeulich,
Arianna Avanzini, viktor.kleinik
On 09/04/14 14:39, Ian Campbell wrote:
> On Mon, 2014-04-07 at 12:05 +0100, Julien Grall wrote:
>> count is only used for RELINQUISH mapping, you don't need to update here.
>
> Not related to this patch, but the current code is:
>
> if ( !pte.p2m.valid )
> {
> count++;
> break;
> }
>
> count += 0x10;
>
> memset(&pte, 0x00, sizeof(pte));
> write_pte(&third[third_table_offset(addr)], pte);
> count++;
> }
>
> It seems like that final count++ is unnecessary/wrong.
Right. I will send a patch to fix it.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v5 3/8] arch, arm: let map_mmio_regions() take pfn as parameters
2014-04-06 23:31 [PATCH v5 0/8] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
2014-04-06 23:31 ` [PATCH v5 1/8] arch, arm: domain build: let dom0 access I/O memory of mapped devices Arianna Avanzini
2014-04-06 23:31 ` [PATCH v5 2/8] arch, arm: add consistency check to REMOVE p2m changes Arianna Avanzini
@ 2014-04-06 23:31 ` Arianna Avanzini
2014-04-07 14:56 ` Julien Grall
2014-04-09 13:54 ` Ian Campbell
2014-04-06 23:31 ` [PATCH v5 4/8] arch, x86: check if mapping exists before memory_mapping removes it Arianna Avanzini
` (4 subsequent siblings)
7 siblings, 2 replies; 19+ messages in thread
From: Arianna Avanzini @ 2014-04-06 23:31 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
JBeulich, avanzini.arianna, viktor.kleinik
Currently, the map_mmio_regions() function, defined for the ARM
architecture, has parameters with paddr_t type. This interface,
however, needs caller functions to correctly page-align addresses
given as parameters to map_mmio_regions(). This commit changes the
function's interface to accept page frame numbers as parameters.
This commit also modifies caller functions in an attempt to adapt
them to the new interface.
This commit attempts to produce the minimum indispensable needed
changes; these are also instrumental to making the interface of
map_mmio_regions() symmetric with the unmap_mmio_regions() function,
that will be introduced in one of the next commits of the series
and will feature a pfn-based interface.
NOTE: platform-specific code has not been tested.
Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Julien Grall <julien.grall@citrix.com>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
v5:
- Add a macro for the paddr_to_pfn(PAGE_ALIGN(...)) pattern.
- Hopefully improve commit description.
---
xen/arch/arm/domain_build.c | 7 ++++---
xen/arch/arm/gic.c | 21 ++++++++++++---------
xen/arch/arm/p2m.c | 13 ++++++++-----
xen/arch/arm/platforms/exynos5.c | 12 +++++++-----
xen/arch/arm/platforms/omap5.c | 21 ++++++++++++---------
xen/arch/arm/platforms/xgene-storm.c | 4 +++-
xen/include/asm-arm/mm.h | 2 ++
xen/include/asm-arm/p2m.h | 11 ++++++-----
8 files changed, 54 insertions(+), 37 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index f4902bb..3c75672 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -744,9 +744,10 @@ static int map_device(struct domain *d, const struct dt_device_node *dev)
addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
return res;
}
- res = map_mmio_regions(d, addr & PAGE_MASK,
- PAGE_ALIGN(addr + size) - 1,
- addr & PAGE_MASK);
+ res = map_mmio_regions(d,
+ paddr_to_pfn(addr & PAGE_MASK),
+ paddr_to_pfn_aligned(addr + size - 1),
+ paddr_to_pfn(addr & PAGE_MASK));
if ( res )
{
printk(XENLOG_ERR "Unable to map 0x%"PRIx64
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 0095b97..0ba31d0 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -888,20 +888,23 @@ int gicv_setup(struct domain *d)
* The second page is always mapped at +4K irrespective of the
* GIC_64K_STRIDE quirk. The DTB passed to the guest reflects this.
*/
- ret = map_mmio_regions(d, d->arch.vgic.cbase,
- d->arch.vgic.cbase + PAGE_SIZE - 1,
- gic.vbase);
+ ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase),
+ paddr_to_pfn_aligned(d->arch.vgic.cbase +
+ PAGE_SIZE - 1),
+ paddr_to_pfn(gic.vbase));
if (ret)
return ret;
if ( !platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
- ret = map_mmio_regions(d, d->arch.vgic.cbase + PAGE_SIZE,
- d->arch.vgic.cbase + (2 * PAGE_SIZE) - 1,
- gic.vbase + PAGE_SIZE);
+ ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + PAGE_SIZE),
+ paddr_to_pfn_aligned(d->arch.vgic.cbase +
+ (2 * PAGE_SIZE) - 1),
+ paddr_to_pfn(gic.vbase + PAGE_SIZE));
else
- ret = map_mmio_regions(d, d->arch.vgic.cbase + PAGE_SIZE,
- d->arch.vgic.cbase + (2 * PAGE_SIZE) - 1,
- gic.vbase + 16*PAGE_SIZE);
+ ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase + PAGE_SIZE),
+ paddr_to_pfn_aligned(d->arch.vgic.cbase +
+ (2 * PAGE_SIZE) - 1),
+ paddr_to_pfn(gic.vbase + 16*PAGE_SIZE));
return ret;
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 395a0b2..110b63a 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -518,12 +518,15 @@ int p2m_populate_ram(struct domain *d,
}
int map_mmio_regions(struct domain *d,
- paddr_t start_gaddr,
- paddr_t end_gaddr,
- paddr_t maddr)
+ unsigned long start_gfn,
+ unsigned long end_gfn,
+ unsigned long mfn)
{
- return apply_p2m_changes(d, INSERT, start_gaddr, end_gaddr,
- maddr, MATTR_DEV, p2m_mmio_direct);
+ return apply_p2m_changes(d, INSERT,
+ pfn_to_paddr(start_gfn),
+ pfn_to_paddr(end_gfn),
+ pfn_to_paddr(mfn),
+ MATTR_DEV, p2m_mmio_direct);
}
int guest_physmap_add_entry(struct domain *d,
diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
index 65e584f..acd8781 100644
--- a/xen/arch/arm/platforms/exynos5.c
+++ b/xen/arch/arm/platforms/exynos5.c
@@ -54,13 +54,15 @@ static int exynos5_init_time(void)
static int exynos5_specific_mapping(struct domain *d)
{
/* Map the chip ID */
- map_mmio_regions(d, EXYNOS5_PA_CHIPID, EXYNOS5_PA_CHIPID + PAGE_SIZE - 1,
- EXYNOS5_PA_CHIPID);
+ map_mmio_regions(d, paddr_to_pfn(EXYNOS5_PA_CHIPID),
+ paddr_to_pfn_aligned(EXYNOS5_PA_CHIPID + PAGE_SIZE - 1),
+ paddr_to_pfn(EXYNOS5_PA_CHIPID));
/* Map the PWM region */
- map_mmio_regions(d, EXYNOS5_PA_TIMER,
- EXYNOS5_PA_TIMER + (PAGE_SIZE * 2) - 1,
- EXYNOS5_PA_TIMER);
+ map_mmio_regions(d, paddr_to_pfn(EXYNOS5_PA_TIMER),
+ paddr_to_pfn_aligned(EXYNOS5_PA_TIMER +
+ (PAGE_SIZE * 2) - 1),
+ paddr_to_pfn(EXYNOS5_PA_TIMER));
return 0;
}
diff --git a/xen/arch/arm/platforms/omap5.c b/xen/arch/arm/platforms/omap5.c
index 76d4d9b..c975020 100644
--- a/xen/arch/arm/platforms/omap5.c
+++ b/xen/arch/arm/platforms/omap5.c
@@ -102,21 +102,24 @@ static int omap5_init_time(void)
static int omap5_specific_mapping(struct domain *d)
{
/* Map the PRM module */
- map_mmio_regions(d, OMAP5_PRM_BASE, OMAP5_PRM_BASE + (PAGE_SIZE * 2) - 1,
- OMAP5_PRM_BASE);
+ map_mmio_regions(d, paddr_to_pfn(OMAP5_PRM_BASE),
+ paddr_to_pfn_aligned(OMAP5_PRM_BASE + (PAGE_SIZE * 2) - 1),
+ paddr_to_pfn(OMAP5_PRM_BASE));
/* Map the PRM_MPU */
- map_mmio_regions(d, OMAP5_PRCM_MPU_BASE,
- OMAP5_PRCM_MPU_BASE + PAGE_SIZE - 1,
- OMAP5_PRCM_MPU_BASE);
+ map_mmio_regions(d, paddr_to_pfn(OMAP5_PRCM_MPU_BASE),
+ paddr_to_pfn_aligned(OMAP5_PRCM_MPU_BASE + PAGE_SIZE - 1),
+ paddr_to_pfn(OMAP5_PRCM_MPU_BASE));
/* Map the Wakeup Gen */
- map_mmio_regions(d, OMAP5_WKUPGEN_BASE, OMAP5_WKUPGEN_BASE + PAGE_SIZE - 1,
- OMAP5_WKUPGEN_BASE);
+ map_mmio_regions(d, paddr_to_pfn(OMAP5_WKUPGEN_BASE),
+ paddr_to_pfn_aligned(OMAP5_WKUPGEN_BASE + PAGE_SIZE - 1),
+ paddr_to_pfn(OMAP5_WKUPGEN_BASE));
/* Map the on-chip SRAM */
- map_mmio_regions(d, OMAP5_SRAM_PA, OMAP5_SRAM_PA + (PAGE_SIZE * 32) - 1,
- OMAP5_SRAM_PA);
+ map_mmio_regions(d, paddr_to_pfn(OMAP5_SRAM_PA),
+ paddr_to_pfn_aligned(OMAP5_SRAM_PA + (PAGE_SIZE * 32) - 1),
+ paddr_to_pfn(OMAP5_SRAM_PA));
return 0;
}
diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index af3b71c..f3662fd 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -47,7 +47,9 @@ static int map_one_mmio(struct domain *d, const char *what,
printk("Additional MMIO %"PRIpaddr"-%"PRIpaddr" (%s)\n",
start, end, what);
- ret = map_mmio_regions(d, start, end, start);
+ ret = map_mmio_regions(d, paddr_to_pfn(start),
+ paddr_to_pfn_aligned(end),
+ paddr_to_pfn(start));
if ( ret )
printk("Failed to map %s @ %"PRIpaddr" to dom%d\n",
what, start, d->domain_id);
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index b8d4e7d..97e0a7b 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -209,6 +209,8 @@ static inline void __iomem *ioremap_wc(paddr_t start, size_t len)
#define paddr_to_pfn(pa) ((unsigned long)((pa) >> PAGE_SHIFT))
#define paddr_to_pdx(pa) pfn_to_pdx(paddr_to_pfn(pa))
+/* Page-align address and convert to frame number format */
+#define paddr_to_pfn_aligned(paddr) paddr_to_pfn(PAGE_ALIGN(paddr))
static inline paddr_t __virt_to_maddr(vaddr_t va)
{
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index bd71abe..c7dd6aa 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -84,11 +84,12 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn);
/* Setup p2m RAM mapping for domain d from start-end. */
int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end);
-/* Map MMIO regions in the p2m: start_gaddr and end_gaddr is the range
- * in the guest physical address space to map, starting from the machine
- * address maddr. */
-int map_mmio_regions(struct domain *d, paddr_t start_gaddr,
- paddr_t end_gaddr, paddr_t maddr);
+/* Map MMIO regions in the p2m: start_gfn and end_gfn is the range in the guest
+ * physical address space to map, starting from the machine frame number mfn. */
+int map_mmio_regions(struct domain *d,
+ unsigned long start_gfn,
+ unsigned long end_gfn,
+ unsigned long mfn);
int guest_physmap_add_entry(struct domain *d,
unsigned long gfn,
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v5 3/8] arch, arm: let map_mmio_regions() take pfn as parameters
2014-04-06 23:31 ` [PATCH v5 3/8] arch, arm: let map_mmio_regions() take pfn as parameters Arianna Avanzini
@ 2014-04-07 14:56 ` Julien Grall
2014-04-09 13:54 ` Ian Campbell
1 sibling, 0 replies; 19+ messages in thread
From: Julien Grall @ 2014-04-07 14:56 UTC (permalink / raw)
To: Arianna Avanzini
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, xen-devel, Ian.Campbell, etrudeau,
JBeulich, viktor.kleinik
Hi Arianna,
Thank you for the patch.
On 04/07/2014 12:31 AM, Arianna Avanzini wrote:
> Currently, the map_mmio_regions() function, defined for the ARM
> architecture, has parameters with paddr_t type. This interface,
> however, needs caller functions to correctly page-align addresses
> given as parameters to map_mmio_regions(). This commit changes the
> function's interface to accept page frame numbers as parameters.
> This commit also modifies caller functions in an attempt to adapt
> them to the new interface.
> This commit attempts to produce the minimum indispensable needed
> changes; these are also instrumental to making the interface of
> map_mmio_regions() symmetric with the unmap_mmio_regions() function,
> that will be introduced in one of the next commits of the series
> and will feature a pfn-based interface.
>
> NOTE: platform-specific code has not been tested.
>
> Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
> Cc: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Paolo Valente <paolo.valente@unimore.it>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v5 3/8] arch, arm: let map_mmio_regions() take pfn as parameters
2014-04-06 23:31 ` [PATCH v5 3/8] arch, arm: let map_mmio_regions() take pfn as parameters Arianna Avanzini
2014-04-07 14:56 ` Julien Grall
@ 2014-04-09 13:54 ` Ian Campbell
2014-04-12 9:20 ` Arianna Avanzini
1 sibling, 1 reply; 19+ messages in thread
From: Ian Campbell @ 2014-04-09 13:54 UTC (permalink / raw)
To: Arianna Avanzini
Cc: paolo.valente, keir, stefano.stabellini, Ian.Jackson,
dario.faggioli, tim, xen-devel, julien.grall, etrudeau, JBeulich,
viktor.kleinik
On Mon, 2014-04-07 at 01:31 +0200, Arianna Avanzini wrote:
> - res = map_mmio_regions(d, addr & PAGE_MASK,
> - PAGE_ALIGN(addr + size) - 1,
> - addr & PAGE_MASK);
> + res = map_mmio_regions(d,
> + paddr_to_pfn(addr & PAGE_MASK),
> + paddr_to_pfn_aligned(addr + size - 1),
With
+#define paddr_to_pfn_aligned(paddr) paddr_to_pfn(PAGE_ALIGN(paddr))
There is a subtle difference here, which is that the "- 1" is now inside
the align. Does this always have the same result? I'm not sure.
If addr+size == 0x1000 (page aligned) then:
PAGE_ALIGN(0x10000)-1 = 0x10000-1 = 0xffff
But
paddr_to_pfn_aligned(0x10000 - 1) =
paddr_to_pfn(PAGE_ALIGN(0xffff)) = paddr_to_pfn(0x10000) = 0x10
The new map_mmio_regions uses pfn_to_paddr which is:
#define pfn_to_paddr(pfn) ((paddr_t)(pfn) << PAGE_SHIFT)
So with the old code the end address would be 0xffff, while with the new
code it is 0x10<<12 = 0x10000.
I suspect the implementation of apply_to_p2m_changes is such that it
doesn't actually change anything. Can you confirm that this was your
intention?
Is the end argument tio map_mmio_regions now intended to be inclusive or
exclusive? This sort of issue can be avoided by using a count instead of
an end in the interface.
Ian.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v5 3/8] arch, arm: let map_mmio_regions() take pfn as parameters
2014-04-09 13:54 ` Ian Campbell
@ 2014-04-12 9:20 ` Arianna Avanzini
0 siblings, 0 replies; 19+ messages in thread
From: Arianna Avanzini @ 2014-04-12 9:20 UTC (permalink / raw)
To: Ian Campbell
Cc: paolo.valente, keir, stefano.stabellini, Ian.Jackson,
dario.faggioli, tim, xen-devel, julien.grall, etrudeau, JBeulich,
viktor.kleinik
Hello,
thank you for your comments, and sorry for the huge delay in replying.
On 04/09/2014 03:54 PM, Ian Campbell wrote:
> On Mon, 2014-04-07 at 01:31 +0200, Arianna Avanzini wrote:
>> - res = map_mmio_regions(d, addr & PAGE_MASK,
>> - PAGE_ALIGN(addr + size) - 1,
>> - addr & PAGE_MASK);
>> + res = map_mmio_regions(d,
>> + paddr_to_pfn(addr & PAGE_MASK),
>> + paddr_to_pfn_aligned(addr + size - 1),
>
>
> With
> +#define paddr_to_pfn_aligned(paddr) paddr_to_pfn(PAGE_ALIGN(paddr))
>
> There is a subtle difference here, which is that the "- 1" is now inside
> the align. Does this always have the same result? I'm not sure.
>
> If addr+size == 0x1000 (page aligned) then:
>
> PAGE_ALIGN(0x10000)-1 = 0x10000-1 = 0xffff
>
> But
>
> paddr_to_pfn_aligned(0x10000 - 1) =
> paddr_to_pfn(PAGE_ALIGN(0xffff)) = paddr_to_pfn(0x10000) = 0x10
>
> The new map_mmio_regions uses pfn_to_paddr which is:
> #define pfn_to_paddr(pfn) ((paddr_t)(pfn) << PAGE_SHIFT)
>
> So with the old code the end address would be 0xffff, while with the new
> code it is 0x10<<12 = 0x10000.
>
> I suspect the implementation of apply_to_p2m_changes is such that it
> doesn't actually change anything. Can you confirm that this was your
> intention?
>
It wasn't my intention to change the size of the address range to be mapped,
thank you for pointing that out. As far as I can understand, the changes in this
patch would let apply_p2m_changes() use one extra address for the mapping; sorry
for that.
While preparing the patch I saw that, e.g., with addr + size = 0x10000,
end_gfn = paddr_to_pfn(PAGE_ALIGN(0x10000) - 1) =
paddr_to_pfn(PAGE_ALIGN(0x10000)) - 1 = 0xf
and
pfn_to_paddr(end_gfn) = 0xf000
which seemed to me to be wrong, as the previous end address was, as you wrote,
0xffff.
Instead, if
end_gfn = paddr_to_pfn(PAGE_ALIGN(0x10000 - 1)) = 0x10
then
pfn_to_paddr(end_gfn) = 0x10000
which I thought lets the needed address range be mapped; however I didn't see
what you pointed out, that it also lets one extra address be used for the mapping.
> Is the end argument tio map_mmio_regions now intended to be inclusive or
> exclusive?
As far as I understand, apply_p2m_changes(), which is called by the ARM version
of map_mmio_regions(), seems to take it as exclusive, as the mapping is
performed while (addr < end_gpaddr).
In the x86 version of map_mmio_regions() it is instead intended to be inclusive;
sorry for this mismatch, I'll try to make the behavior of the two versions
consistent.
> This sort of issue can be avoided by using a count instead of
> an end in the interface.
>
Thank you very much for the hint.
> Ian.
>
--
/*
* Arianna Avanzini
* avanzini.arianna@gmail.com
* 73628@studenti.unimore.it
*/
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v5 4/8] arch, x86: check if mapping exists before memory_mapping removes it
2014-04-06 23:31 [PATCH v5 0/8] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (2 preceding siblings ...)
2014-04-06 23:31 ` [PATCH v5 3/8] arch, arm: let map_mmio_regions() take pfn as parameters Arianna Avanzini
@ 2014-04-06 23:31 ` Arianna Avanzini
2014-04-06 23:31 ` [PATCH v5 5/8] xen, common: add the XEN_DOMCTL_memory_mapping hypercall Arianna Avanzini
` (3 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Arianna Avanzini @ 2014-04-06 23:31 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
JBeulich, avanzini.arianna, viktor.kleinik
Currently, when a memory mapping is removed with the memory_mapping
DOMCTL, no check is performed on the existence of such a mapping.
This commit attempts to add such a consistency check to the code
performing the unmap.
Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Julien Grall <julien.grall@citrix.com>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
xen/arch/x86/domctl.c | 4 ++--
xen/arch/x86/mm/p2m.c | 23 +++++++++++++++++------
xen/include/asm-x86/p2m.h | 2 +-
3 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 26635ff..1e51289 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -679,7 +679,7 @@ long arch_do_domctl(
"memory_map:fail: dom%d gfn=%lx mfn=%lx\n",
d->domain_id, gfn + i, mfn + i);
while ( i-- )
- clear_mmio_p2m_entry(d, gfn + i);
+ clear_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
if ( iomem_deny_access(d, mfn, mfn + nr_mfns - 1) &&
is_hardware_domain(current->domain) )
printk(XENLOG_ERR
@@ -696,7 +696,7 @@ long arch_do_domctl(
if ( paging_mode_translate(d) )
for ( i = 0; i < nr_mfns; i++ )
- add |= !clear_mmio_p2m_entry(d, gfn + i);
+ add |= !clear_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
if ( !ret && add )
ret = -EIO;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index c38f334..2c894b8 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -778,10 +778,10 @@ set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
}
int
-clear_mmio_p2m_entry(struct domain *d, unsigned long gfn)
+clear_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
{
int rc = 0;
- mfn_t mfn;
+ mfn_t actual_mfn;
p2m_access_t a;
p2m_type_t t;
struct p2m_domain *p2m = p2m_get_hostp2m(d);
@@ -790,16 +790,27 @@ clear_mmio_p2m_entry(struct domain *d, unsigned long gfn)
return 0;
gfn_lock(p2m, gfn, 0);
- mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL);
+ actual_mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL);
/* Do not use mfn_valid() here as it will usually fail for MMIO pages. */
- if ( (INVALID_MFN == mfn_x(mfn)) || (t != p2m_mmio_direct) )
+ if ( (INVALID_MFN == mfn_x(actual_mfn)) || (t != p2m_mmio_direct) )
+ {
+ gdprintk(XENLOG_ERR,
+ "clear_mmio_p2m_entry: gfn_to_mfn failed! gfn=%08lx\n",
+ gfn);
+ goto out;
+ }
+
+ if ( mfn_x(mfn) != mfn_x(actual_mfn) )
{
gdprintk(XENLOG_ERR,
- "clear_mmio_p2m_entry: gfn_to_mfn failed! gfn=%08lx\n", gfn);
+ "clear_mmio_p2m_entry: mapping between mfn %08lx and "
+ "gfn %08lx does not exist (mapped to %08lx)\n",
+ mfn_x(mfn), gfn, mfn_x(actual_mfn));
goto out;
}
- rc = set_p2m_entry(p2m, gfn, _mfn(INVALID_MFN), PAGE_ORDER_4K, p2m_invalid, p2m->default_access);
+ rc = set_p2m_entry(p2m, gfn, _mfn(INVALID_MFN), PAGE_ORDER_4K,
+ p2m_invalid, p2m->default_access);
out:
gfn_unlock(p2m, gfn, 0);
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index d644f82..c403534 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -508,7 +508,7 @@ p2m_type_t p2m_change_type(struct domain *d, unsigned long gfn,
/* Set mmio addresses in the p2m table (for pass-through) */
int set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
-int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn);
+int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
/*
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v5 5/8] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-04-06 23:31 [PATCH v5 0/8] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (3 preceding siblings ...)
2014-04-06 23:31 ` [PATCH v5 4/8] arch, x86: check if mapping exists before memory_mapping removes it Arianna Avanzini
@ 2014-04-06 23:31 ` Arianna Avanzini
2014-04-07 7:55 ` Jan Beulich
2014-04-09 14:03 ` Ian Campbell
2014-04-06 23:31 ` [PATCH v5 6/8] tools, libxl: parse optional start gfn from the iomem config option Arianna Avanzini
` (2 subsequent siblings)
7 siblings, 2 replies; 19+ messages in thread
From: Arianna Avanzini @ 2014-04-06 23:31 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
JBeulich, avanzini.arianna, viktor.kleinik
This commit introduces a first attempt of implementation of the
XEN_DOMCTL_memory_mapping hypercall for ARM. As the implementation
would have been almost identical to the one for x86, this code has
been made common to both the architectures. The only difference
between the two procedures lies in the arch-specific implementation
of the map_mmio_regions() and unmap_mmio_regions() functions.
Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Julien Grall <julien.grall@citrix.com>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
v5:
- Let the unmap_mmio_regions() function for x86 return a proper
error code upon failure.
- Restore correct handling of errors in the remove path of the
hypercall, assigning to the "ret" local variable the error
code returned by the unmap_mmio_regions() function only if
iomem_deny_access() didn't return with an error.
- Compute gfn_end - 1 and mfn_end - 1 only once in the DOMCTL.
- Use a local variable to keep the return value of the function
unmap_mmio_regions() instead of re-using the "add" variable.
- Add a comment to make hopefully clearer how error values of
functions are handled in the remove path of the DOMCTL.
- Rename new header to p2m-common.h.
v4:
- Use a define for paddr_bits instead of a new variable.
- Define prototypes for common functions map_mmio_regions() and
unmap_mmio_regions() only once in a common header.
- Fix type and signedness of local variables used as indexes in
map_mmio_regions() and unmap_mmio_regions() for x86.
- Clear p2m entries in map_mmio_regions() for x86 only if
set_mmio_p2m_entry() returned with an error.
- Make ranges inclusive of the end address in map_mmio_regions()
and unmap_mmio_regions() for x86.
- Turn hard tabs into spaces.
v3:
- Move code to xen/common/domctl.c; abstract out differences between
the x86 and ARM code:
. add map_mmio_regions() and unmap_mmio_regions() functions for x86;
. add a paddr_bits variable for ARM.
- Use pfn as parameters to the unmap_mmio_regions() function.
- Compute gfn + nr_mfns and mfn + nr_mfns only once.
v2:
- Move code to xen/arm/domctl.c.
- Use the already-defined PADDR_BITS constant in the new DOMCTL.
- Use paddr_t as arguments to the map_mmio_regions() function.
- Page-align addresses given as arguments to map_mmio_regions() and
unmap_mmio_regions().
---
xen/arch/arm/p2m.c | 12 ++++++++
xen/arch/x86/domctl.c | 70 -------------------------------------------
xen/arch/x86/mm/p2m.c | 42 ++++++++++++++++++++++++++
xen/common/domctl.c | 71 ++++++++++++++++++++++++++++++++++++++++++++
xen/include/asm-arm/p2m.h | 9 ++----
xen/include/asm-x86/p2m.h | 1 +
xen/include/xen/p2m-common.h | 16 ++++++++++
7 files changed, 145 insertions(+), 76 deletions(-)
create mode 100644 xen/include/xen/p2m-common.h
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 110b63a..cc71a5d 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -529,6 +529,18 @@ int map_mmio_regions(struct domain *d,
MATTR_DEV, p2m_mmio_direct);
}
+int unmap_mmio_regions(struct domain *d,
+ unsigned long start_gfn,
+ unsigned long end_gfn,
+ unsigned long mfn)
+{
+ return apply_p2m_changes(d, REMOVE,
+ pfn_to_paddr(start_gfn),
+ pfn_to_paddr(end_gfn),
+ pfn_to_paddr(mfn),
+ MATTR_DEV, p2m_mmio_direct);
+}
+
int guest_physmap_add_entry(struct domain *d,
unsigned long gpfn,
unsigned long mfn,
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 1e51289..69a7fbf 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -639,76 +639,6 @@ long arch_do_domctl(
}
break;
- case XEN_DOMCTL_memory_mapping:
- {
- unsigned long gfn = domctl->u.memory_mapping.first_gfn;
- unsigned long mfn = domctl->u.memory_mapping.first_mfn;
- unsigned long nr_mfns = domctl->u.memory_mapping.nr_mfns;
- int add = domctl->u.memory_mapping.add_mapping;
- unsigned long i;
-
- ret = -EINVAL;
- if ( (mfn + nr_mfns - 1) < mfn || /* wrap? */
- ((mfn | (mfn + nr_mfns - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
- (gfn + nr_mfns - 1) < gfn ) /* wrap? */
- break;
-
- ret = -EPERM;
- if ( !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 1) )
- break;
-
- ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, add);
- if ( ret )
- break;
-
- if ( add )
- {
- printk(XENLOG_G_INFO
- "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx\n",
- d->domain_id, gfn, mfn, nr_mfns);
-
- ret = iomem_permit_access(d, mfn, mfn + nr_mfns - 1);
- if ( !ret && paging_mode_translate(d) )
- {
- for ( i = 0; !ret && i < nr_mfns; i++ )
- if ( !set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i)) )
- ret = -EIO;
- if ( ret )
- {
- printk(XENLOG_G_WARNING
- "memory_map:fail: dom%d gfn=%lx mfn=%lx\n",
- d->domain_id, gfn + i, mfn + i);
- while ( i-- )
- clear_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
- if ( iomem_deny_access(d, mfn, mfn + nr_mfns - 1) &&
- is_hardware_domain(current->domain) )
- printk(XENLOG_ERR
- "memory_map: failed to deny dom%d access to [%lx,%lx]\n",
- d->domain_id, mfn, mfn + nr_mfns - 1);
- }
- }
- }
- else
- {
- printk(XENLOG_G_INFO
- "memory_map:remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
- d->domain_id, gfn, mfn, nr_mfns);
-
- if ( paging_mode_translate(d) )
- for ( i = 0; i < nr_mfns; i++ )
- add |= !clear_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
- ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
- if ( !ret && add )
- ret = -EIO;
- if ( ret && is_hardware_domain(current->domain) )
- printk(XENLOG_ERR
- "memory_map: error %ld %s dom%d access to [%lx,%lx]\n",
- ret, add ? "removing" : "denying", d->domain_id,
- mfn, mfn + nr_mfns - 1);
- }
- }
- break;
-
case XEN_DOMCTL_ioport_mapping:
{
#define MAX_IOPORTS 0x10000
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 2c894b8..2c6ef24 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1631,6 +1631,48 @@ unsigned long paging_gva_to_gfn(struct vcpu *v,
return hostmode->gva_to_gfn(v, hostp2m, va, pfec);
}
+int map_mmio_regions(struct domain *d,
+ unsigned long start_gfn,
+ unsigned long end_gfn,
+ unsigned long mfn)
+{
+ int ret = 0;
+ unsigned long i;
+ unsigned long nr_mfns = end_gfn - start_gfn + 1;
+
+ if ( !paging_mode_translate(d) )
+ return 0;
+
+ for ( i = 0; !ret && i < nr_mfns; i++ )
+ if ( !set_mmio_p2m_entry(d, start_gfn + i, _mfn(mfn + i)) )
+ ret = -EIO;
+ if ( ret )
+ while ( i-- )
+ clear_mmio_p2m_entry(d, start_gfn + i, _mfn(mfn + i));
+
+ return ret;
+}
+
+int unmap_mmio_regions(struct domain *d,
+ unsigned long start_gfn,
+ unsigned long end_gfn,
+ unsigned long mfn)
+{
+ int ret = 0;
+ unsigned long nr_mfns = end_gfn - start_gfn + 1;
+ unsigned long i;
+
+ if ( !paging_mode_translate(d) )
+ return 0;
+
+ for ( i = 0; i < nr_mfns; i++ )
+ ret |= !clear_mmio_p2m_entry(d, start_gfn + i, _mfn(mfn + i));
+
+ if ( ret )
+ return -EIO;
+ return 0;
+}
+
/*** Audit ***/
#if P2M_AUDIT
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 5342e5d..fe228fb 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -818,6 +818,77 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
}
break;
+ case XEN_DOMCTL_memory_mapping:
+ {
+ unsigned long gfn = op->u.memory_mapping.first_gfn;
+ unsigned long mfn = op->u.memory_mapping.first_mfn;
+ unsigned long nr_mfns = op->u.memory_mapping.nr_mfns;
+ unsigned long mfn_end = mfn + nr_mfns - 1;
+ unsigned long gfn_end = gfn + nr_mfns - 1;
+ int add = op->u.memory_mapping.add_mapping;
+
+ ret = -EINVAL;
+ if ( (mfn_end - 1) < mfn || /* wrap? */
+ ((mfn | (mfn_end - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
+ (gfn_end - 1) < gfn ) /* wrap? */
+ return ret;
+
+ ret = -EPERM;
+ if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
+ return ret;
+
+ ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, add);
+ if ( ret )
+ return ret;
+
+ if ( add )
+ {
+ printk(XENLOG_G_INFO
+ "memory_map: add: dom%d gfn=%lx mfn=%lx nr=%lx\n",
+ d->domain_id, gfn, mfn, nr_mfns);
+ ret = iomem_permit_access(d, mfn, mfn_end);
+ if ( !ret )
+ {
+ ret = map_mmio_regions(d, gfn, gfn_end, mfn);
+ if ( ret )
+ {
+ printk(XENLOG_G_WARNING
+ "memory_map: fail: dom%d gfn=%lx mfn=%lx\n",
+ d->domain_id, gfn, mfn);
+ if ( iomem_deny_access(d, mfn, mfn_end) &&
+ is_hardware_domain(current->domain) )
+ printk(XENLOG_ERR
+ "memory_map: failed to deny dom%d access "
+ "to [%lx,%lx]\n",
+ d->domain_id, mfn, mfn_end);
+ }
+ }
+ }
+ else
+ {
+ long unmap_ret;
+
+ printk(XENLOG_G_INFO
+ "memory_map: remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
+ d->domain_id, gfn, mfn, nr_mfns);
+
+ unmap_ret = unmap_mmio_regions(d, gfn, gfn_end, mfn);
+ ret = iomem_deny_access(d, mfn, mfn_end);
+ /*
+ * Let an error value returned by iomem_deny_access() prevail on
+ * the one possibly returned by unmap_mmio_regions().
+ */
+ if ( !ret && unmap_ret )
+ ret = unmap_ret;
+ if ( ret && is_hardware_domain(current->domain) )
+ printk(XENLOG_ERR
+ "memory_map: error %ld %s dom%d access to [%lx,%lx]\n",
+ ret, unmap_ret ? "removing" : "denying", d->domain_id,
+ mfn, mfn_end);
+ }
+ }
+ break;
+
case XEN_DOMCTL_settimeoffset:
{
domain_set_time_offset(d, op->u.settimeoffset.time_offset_seconds);
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index c7dd6aa..e9ece8d 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -2,6 +2,9 @@
#define _XEN_P2M_H
#include <xen/mm.h>
+#include <xen/p2m-common.h>
+
+#define paddr_bits PADDR_BITS
struct domain;
@@ -84,12 +87,6 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn);
/* Setup p2m RAM mapping for domain d from start-end. */
int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end);
-/* Map MMIO regions in the p2m: start_gfn and end_gfn is the range in the guest
- * physical address space to map, starting from the machine frame number mfn. */
-int map_mmio_regions(struct domain *d,
- unsigned long start_gfn,
- unsigned long end_gfn,
- unsigned long mfn);
int guest_physmap_add_entry(struct domain *d,
unsigned long gfn,
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index c403534..af53816 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -29,6 +29,7 @@
#include <xen/config.h>
#include <xen/paging.h>
+#include <xen/p2m-common.h>
#include <asm/mem_sharing.h>
#include <asm/page.h> /* for pagetable_t */
diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
new file mode 100644
index 0000000..2be9a86
--- /dev/null
+++ b/xen/include/xen/p2m-common.h
@@ -0,0 +1,16 @@
+#ifndef _XEN_P2M_COMMON_H
+#define _XEN_P2M_COMMON_H
+
+/* Map MMIO regions in the p2m: start_gfn and end_gfn is the range
+ * in the guest physical address space to map, starting from the
+ * machine frame number mfn. */
+int map_mmio_regions(struct domain *d,
+ unsigned long start_gfn,
+ unsigned long end_gfn,
+ unsigned long mfn);
+int unmap_mmio_regions(struct domain *d,
+ unsigned long start_gfn,
+ unsigned long end_gfn,
+ unsigned long mfn);
+
+#endif /* _XEN_P2M_COMMON_H */
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v5 5/8] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-04-06 23:31 ` [PATCH v5 5/8] xen, common: add the XEN_DOMCTL_memory_mapping hypercall Arianna Avanzini
@ 2014-04-07 7:55 ` Jan Beulich
2014-04-09 14:03 ` Ian Campbell
1 sibling, 0 replies; 19+ messages in thread
From: Jan Beulich @ 2014-04-07 7:55 UTC (permalink / raw)
To: Arianna Avanzini
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, xen-devel, julien.grall, etrudeau,
viktor.kleinik
>>> On 07.04.14 at 01:31, <avanzini.arianna@gmail.com> wrote:
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -818,6 +818,77 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> }
> break;
>
> + case XEN_DOMCTL_memory_mapping:
> + {
> + unsigned long gfn = op->u.memory_mapping.first_gfn;
> + unsigned long mfn = op->u.memory_mapping.first_mfn;
> + unsigned long nr_mfns = op->u.memory_mapping.nr_mfns;
> + unsigned long mfn_end = mfn + nr_mfns - 1;
> + unsigned long gfn_end = gfn + nr_mfns - 1;
> + int add = op->u.memory_mapping.add_mapping;
> +
> + ret = -EINVAL;
> + if ( (mfn_end - 1) < mfn || /* wrap? */
> + ((mfn | (mfn_end - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
> + (gfn_end - 1) < gfn ) /* wrap? */
You subtracted 1 from [mg]fn_end above already.
> + return ret;
> +
> + ret = -EPERM;
> + if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
> + return ret;
> +
> + ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, add);
> + if ( ret )
> + return ret;
> +
> + if ( add )
> + {
> + printk(XENLOG_G_INFO
> + "memory_map: add: dom%d gfn=%lx mfn=%lx nr=%lx\n",
> + d->domain_id, gfn, mfn, nr_mfns);
> + ret = iomem_permit_access(d, mfn, mfn_end);
> + if ( !ret )
> + {
> + ret = map_mmio_regions(d, gfn, gfn_end, mfn);
> + if ( ret )
> + {
> + printk(XENLOG_G_WARNING
> + "memory_map: fail: dom%d gfn=%lx mfn=%lx\n",
> + d->domain_id, gfn, mfn);
> + if ( iomem_deny_access(d, mfn, mfn_end) &&
> + is_hardware_domain(current->domain) )
> + printk(XENLOG_ERR
> + "memory_map: failed to deny dom%d access "
> + "to [%lx,%lx]\n",
> + d->domain_id, mfn, mfn_end);
> + }
> + }
> + }
> + else
> + {
> + long unmap_ret;
Hard tab.
> +
> + printk(XENLOG_G_INFO
> + "memory_map: remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
> + d->domain_id, gfn, mfn, nr_mfns);
> +
> + unmap_ret = unmap_mmio_regions(d, gfn, gfn_end, mfn);
> + ret = iomem_deny_access(d, mfn, mfn_end);
> + /*
> + * Let an error value returned by iomem_deny_access() prevail on
> + * the one possibly returned by unmap_mmio_regions().
> + */
I would omit this comment - it's not that important which of the two
errors we return, what is important is that we don't drop either of the
two possible failures.
> --- /dev/null
> +++ b/xen/include/xen/p2m-common.h
> @@ -0,0 +1,16 @@
> +#ifndef _XEN_P2M_COMMON_H
> +#define _XEN_P2M_COMMON_H
> +
> +/* Map MMIO regions in the p2m: start_gfn and end_gfn is the range
To make things entirely obvious, please add the word "inclusive" here.
> + * in the guest physical address space to map, starting from the
> + * machine frame number mfn. */
> +int map_mmio_regions(struct domain *d,
> + unsigned long start_gfn,
> + unsigned long end_gfn,
> + unsigned long mfn);
> +int unmap_mmio_regions(struct domain *d,
> + unsigned long start_gfn,
> + unsigned long end_gfn,
> + unsigned long mfn);
> +
> +#endif /* _XEN_P2M_COMMON_H */
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v5 5/8] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
2014-04-06 23:31 ` [PATCH v5 5/8] xen, common: add the XEN_DOMCTL_memory_mapping hypercall Arianna Avanzini
2014-04-07 7:55 ` Jan Beulich
@ 2014-04-09 14:03 ` Ian Campbell
1 sibling, 0 replies; 19+ messages in thread
From: Ian Campbell @ 2014-04-09 14:03 UTC (permalink / raw)
To: Arianna Avanzini
Cc: paolo.valente, keir, stefano.stabellini, Ian.Jackson,
dario.faggioli, tim, xen-devel, julien.grall, etrudeau, JBeulich,
viktor.kleinik
On Mon, 2014-04-07 at 01:31 +0200, Arianna Avanzini wrote:
> This commit introduces a first attempt of implementation of the
> XEN_DOMCTL_memory_mapping hypercall for ARM. As the implementation
> would have been almost identical to the one for x86, this code has
> been made common to both the architectures. The only difference
> between the two procedures lies in the arch-specific implementation
> of the map_mmio_regions() and unmap_mmio_regions() functions.
>
> Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
> Cc: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Paolo Valente <paolo.valente@unimore.it>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Julien Grall <julien.grall@citrix.com>
> Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Eric Trudeau <etrudeau@broadcom.com>
> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
>
> ---
>
> v5:
> - Let the unmap_mmio_regions() function for x86 return a proper
> error code upon failure.
> - Restore correct handling of errors in the remove path of the
> hypercall, assigning to the "ret" local variable the error
> code returned by the unmap_mmio_regions() function only if
> iomem_deny_access() didn't return with an error.
> - Compute gfn_end - 1 and mfn_end - 1 only once in the DOMCTL.
> - Use a local variable to keep the return value of the function
> unmap_mmio_regions() instead of re-using the "add" variable.
> - Add a comment to make hopefully clearer how error values of
> functions are handled in the remove path of the DOMCTL.
> - Rename new header to p2m-common.h.
>
> v4:
> - Use a define for paddr_bits instead of a new variable.
> - Define prototypes for common functions map_mmio_regions() and
> unmap_mmio_regions() only once in a common header.
> - Fix type and signedness of local variables used as indexes in
> map_mmio_regions() and unmap_mmio_regions() for x86.
> - Clear p2m entries in map_mmio_regions() for x86 only if
> set_mmio_p2m_entry() returned with an error.
> - Make ranges inclusive of the end address in map_mmio_regions()
> and unmap_mmio_regions() for x86.
> - Turn hard tabs into spaces.
>
> v3:
> - Move code to xen/common/domctl.c; abstract out differences between
> the x86 and ARM code:
> . add map_mmio_regions() and unmap_mmio_regions() functions for x86;
> . add a paddr_bits variable for ARM.
> - Use pfn as parameters to the unmap_mmio_regions() function.
> - Compute gfn + nr_mfns and mfn + nr_mfns only once.
>
> v2:
> - Move code to xen/arm/domctl.c.
> - Use the already-defined PADDR_BITS constant in the new DOMCTL.
> - Use paddr_t as arguments to the map_mmio_regions() function.
> - Page-align addresses given as arguments to map_mmio_regions() and
> unmap_mmio_regions().
>
> ---
> xen/arch/arm/p2m.c | 12 ++++++++
> xen/arch/x86/domctl.c | 70 -------------------------------------------
> xen/arch/x86/mm/p2m.c | 42 ++++++++++++++++++++++++++
> xen/common/domctl.c | 71 ++++++++++++++++++++++++++++++++++++++++++++
> xen/include/asm-arm/p2m.h | 9 ++----
> xen/include/asm-x86/p2m.h | 1 +
> xen/include/xen/p2m-common.h | 16 ++++++++++
> 7 files changed, 145 insertions(+), 76 deletions(-)
> create mode 100644 xen/include/xen/p2m-common.h
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 110b63a..cc71a5d 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -529,6 +529,18 @@ int map_mmio_regions(struct domain *d,
> MATTR_DEV, p2m_mmio_direct);
> }
>
> +int unmap_mmio_regions(struct domain *d,
> + unsigned long start_gfn,
> + unsigned long end_gfn,
> + unsigned long mfn)
> +{
> + return apply_p2m_changes(d, REMOVE,
> + pfn_to_paddr(start_gfn),
> + pfn_to_paddr(end_gfn),
> + pfn_to_paddr(mfn),
> + MATTR_DEV, p2m_mmio_direct);
The final argument here is not used in the REMOVE case. Please pass
p2m_invalid here, unless/until that changes.
> +}
> +
> int guest_physmap_add_entry(struct domain *d,
> unsigned long gpfn,
> unsigned long mfn,
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 1e51289..69a7fbf 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -639,76 +639,6 @@ long arch_do_domctl(
> }
> break;
>
> - case XEN_DOMCTL_memory_mapping:
> - {
> - unsigned long gfn = domctl->u.memory_mapping.first_gfn;
> - unsigned long mfn = domctl->u.memory_mapping.first_mfn;
> - unsigned long nr_mfns = domctl->u.memory_mapping.nr_mfns;
> - int add = domctl->u.memory_mapping.add_mapping;
> - unsigned long i;
> -
> - ret = -EINVAL;
> - if ( (mfn + nr_mfns - 1) < mfn || /* wrap? */
> - ((mfn | (mfn + nr_mfns - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
> - (gfn + nr_mfns - 1) < gfn ) /* wrap? */
> - break;
> -
> - ret = -EPERM;
> - if ( !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 1) )
> - break;
> -
> - ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, add);
> - if ( ret )
> - break;
> -
> - if ( add )
> - {
> - printk(XENLOG_G_INFO
> - "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx\n",
> - d->domain_id, gfn, mfn, nr_mfns);
> -
> - ret = iomem_permit_access(d, mfn, mfn + nr_mfns - 1);
> - if ( !ret && paging_mode_translate(d) )
> - {
> - for ( i = 0; !ret && i < nr_mfns; i++ )
> - if ( !set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i)) )
> - ret = -EIO;
> - if ( ret )
> - {
> - printk(XENLOG_G_WARNING
> - "memory_map:fail: dom%d gfn=%lx mfn=%lx\n",
> - d->domain_id, gfn + i, mfn + i);
> - while ( i-- )
> - clear_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
> - if ( iomem_deny_access(d, mfn, mfn + nr_mfns - 1) &&
> - is_hardware_domain(current->domain) )
> - printk(XENLOG_ERR
> - "memory_map: failed to deny dom%d access to [%lx,%lx]\n",
> - d->domain_id, mfn, mfn + nr_mfns - 1);
> - }
> - }
> - }
> - else
> - {
> - printk(XENLOG_G_INFO
> - "memory_map:remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
> - d->domain_id, gfn, mfn, nr_mfns);
> -
> - if ( paging_mode_translate(d) )
> - for ( i = 0; i < nr_mfns; i++ )
> - add |= !clear_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
> - ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
> - if ( !ret && add )
> - ret = -EIO;
> - if ( ret && is_hardware_domain(current->domain) )
> - printk(XENLOG_ERR
> - "memory_map: error %ld %s dom%d access to [%lx,%lx]\n",
> - ret, add ? "removing" : "denying", d->domain_id,
> - mfn, mfn + nr_mfns - 1);
> - }
> - }
> - break;
> -
> case XEN_DOMCTL_ioport_mapping:
> {
> #define MAX_IOPORTS 0x10000
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 2c894b8..2c6ef24 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1631,6 +1631,48 @@ unsigned long paging_gva_to_gfn(struct vcpu *v,
> return hostmode->gva_to_gfn(v, hostp2m, va, pfec);
> }
>
> +int map_mmio_regions(struct domain *d,
> + unsigned long start_gfn,
> + unsigned long end_gfn,
> + unsigned long mfn)
Personally I would have preferred to see x86 switch to this interface
separately from moving the implementation of the user from x86 to
generic code, since doing them at the same time makes it hard to see
what actually changes in XEN_DOMCTL_memory_mapping.
> + long unmap_ret;
Stray hard tab.
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index c403534..af53816 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -29,6 +29,7 @@
>
> #include <xen/config.h>
> #include <xen/paging.h>
> +#include <xen/p2m-common.h>
More normally xen/p2m.h (which you have as p2m-common.h) would include
the asm version. But I suppose that involves a lot more code changes.
Ian.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v5 6/8] tools, libxl: parse optional start gfn from the iomem config option
2014-04-06 23:31 [PATCH v5 0/8] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (4 preceding siblings ...)
2014-04-06 23:31 ` [PATCH v5 5/8] xen, common: add the XEN_DOMCTL_memory_mapping hypercall Arianna Avanzini
@ 2014-04-06 23:31 ` Arianna Avanzini
2014-04-09 14:10 ` Ian Campbell
2014-04-06 23:31 ` [PATCH v5 7/8] tools, libxl: add helpers to establish if guest is auto-translated Arianna Avanzini
2014-04-06 23:32 ` [PATCH v5 8/8] tools, libxl: handle the iomem parameter with the memory_mapping hcall Arianna Avanzini
7 siblings, 1 reply; 19+ messages in thread
From: Arianna Avanzini @ 2014-04-06 23:31 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
JBeulich, avanzini.arianna, viktor.kleinik
Currently, the "iomem" domU config option allows to specify a machine
address range to be mapped to the domU. However, there is no way to
specify the guest address range used for the mapping. This commit
extends the iomem option handling code to parse an additional, optional
parameter: this parameter, if given, specifies the start guest address
used for the mapping; if no start guest address is given, a 1:1 mapping
is performed as default.
Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Julien Grall <julien.grall@citrix.com>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
v5:
- Initialize the gfn field of the iomem structure with the
value "(uint64_t)-1".
- Defer the assignment of the value of "start" to "gfn", if
"gfn" has been initialized to the default value, in libxl.
- Use a local variable to keep the return value of sscanf()
for better code readability.
v4:
- Add definition of a LIBXL_HAVE_BUILDINFO_IOMEM_START_GFN macro
to indicate the availability of the new option.
- Simplify the code parsing the new optional parameter by using
the switch construct on the return value of the sscanf() function
instead of calling the function twice.
- Add a short paragraph to the manpage about the use of the iomem
option to passthrough devices protected by an IOMMU.
- Add comments to the fields of the "iomem" structure.
---
docs/man/xl.cfg.pod.5 | 13 +++++++++----
tools/libxl/libxl.h | 10 ++++++++++
tools/libxl/libxl_create.c | 6 ++++++
tools/libxl/libxl_types.idl | 7 +++++--
tools/libxl/xl_cmdimpl.c | 19 ++++++++++---------
5 files changed, 40 insertions(+), 15 deletions(-)
diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index a6663b9..d54f601 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -602,12 +602,17 @@ is given in hexadecimal and may either a span e.g. C<2f8-2ff>
It is recommended to use this option only for trusted VMs under
administrator control.
-=item B<iomem=[ "IOMEM_START,NUM_PAGES", "IOMEM_START,NUM_PAGES", ... ]>
+=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN]", "IOMEM_START,NUM_PAGES[@GFN]", ... ]>
Allow guest to access specific hardware I/O memory pages. B<IOMEM_START>
-is a physical page number. B<NUM_PAGES> is the number
-of pages beginning with B<START_PAGE> to allow access. Both values
-must be given in hexadecimal.
+is a physical page number. B<NUM_PAGES> is the number of pages beginning
+with B<START_PAGE> to allow access. B<GFN> specifies the guest frame number
+where the mapping will start in the domU's address space.
+All of these values must be given in hexadecimal.
+
+Note that the IOMMU won't be updated with the mappings specified with this
+option. This option therefore should not be used to passthrough any
+IOMMU-protected device.
It is recommended to use this option only for trusted VMs under
administrator control.
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index b2c3015..e4a1128 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -95,6 +95,16 @@
#define LIBXL_HAVE_BUILDINFO_EVENT_CHANNELS 1
/*
+ * LIBXL_HAVE_BUILDINFO_IOMEM_START_GFN indicated that it is possible
+ * to specify the start guest frame number used to map a range of I/O
+ * memory machine frame numbers via the 'gfn' field (of type uint64)
+ * of the 'iomem' structure. An array of iomem structures is embedded
+ * in libxl_domain_build_info and used to map the indicated memory
+ * ranges during domain build.
+ */
+#define LIBXL_HAVE_BUILDINFO_IOMEM_START_GFN 1
+
+/*
* libxl ABI compatibility
*
* The only guarantee which libxl makes regarding ABI compatibility
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index d015cf4..369cd77 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -102,6 +102,8 @@ static int sched_params_valid(libxl__gc *gc,
int libxl__domain_build_info_setdefault(libxl__gc *gc,
libxl_domain_build_info *b_info)
{
+ int i;
+
if (b_info->type != LIBXL_DOMAIN_TYPE_HVM &&
b_info->type != LIBXL_DOMAIN_TYPE_PV)
return ERROR_INVAL;
@@ -212,6 +214,10 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
libxl_defbool_setdefault(&b_info->disable_migrate, false);
+ for (i = 0 ; i < b_info->num_iomem; i++)
+ if (b_info->iomem[i].gfn == (uint64_t)-1)
+ b_info->iomem[i].gfn = b_info->iomem[i].start;
+
if (!b_info->event_channels)
b_info->event_channels = 1023;
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 612645c..69f8ea3 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -170,8 +170,11 @@ libxl_ioport_range = Struct("ioport_range", [
])
libxl_iomem_range = Struct("iomem_range", [
- ("start", uint64),
- ("number", uint64),
+ ("start", uint64), # start host frame number to be mapped
+ # to the guest
+ ("number", uint64), # number of frames to be mapped
+ ("gfn", uint64, {'init_val': -1}), # guest frame number used as a start
+ # for the mapping
])
libxl_vga_interface_info = Struct("vga_interface_info", [
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 8389468..10df96c 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1200,6 +1200,7 @@ static void parse_config_data(const char *config_source,
}
if (!xlu_cfg_get_list(config, "iomem", &iomem, &num_iomem, 0)) {
+ int ret;
b_info->num_iomem = num_iomem;
b_info->iomem = calloc(num_iomem, sizeof(*b_info->iomem));
if (b_info->iomem == NULL) {
@@ -1213,19 +1214,19 @@ static void parse_config_data(const char *config_source,
"xl: Unable to get element %d in iomem list\n", i);
exit(1);
}
- if(sscanf(buf, "%" SCNx64",%" SCNx64,
- &b_info->iomem[i].start,
- &b_info->iomem[i].number)
- != 2) {
- fprintf(stderr,
- "xl: Invalid argument parsing iomem: %s\n", buf);
- exit(1);
+ libxl_iomem_range_init(&b_info->iomem[i]);
+ ret = sscanf(buf, "%" SCNx64",%" SCNx64"@%" SCNx64,
+ &b_info->iomem[i].start,
+ &b_info->iomem[i].number,
+ &b_info->iomem[i].gfn);
+ if (ret < 2) {
+ fprintf(stderr,
+ "xl: Invalid argument parsing iomem: %s\n", buf);
+ exit(1);
}
}
}
-
-
if (!xlu_cfg_get_list (config, "disk", &vbds, 0, 0)) {
d_config->num_disks = 0;
d_config->disks = NULL;
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v5 6/8] tools, libxl: parse optional start gfn from the iomem config option
2014-04-06 23:31 ` [PATCH v5 6/8] tools, libxl: parse optional start gfn from the iomem config option Arianna Avanzini
@ 2014-04-09 14:10 ` Ian Campbell
0 siblings, 0 replies; 19+ messages in thread
From: Ian Campbell @ 2014-04-09 14:10 UTC (permalink / raw)
To: Arianna Avanzini
Cc: paolo.valente, keir, stefano.stabellini, Ian.Jackson,
dario.faggioli, tim, xen-devel, julien.grall, etrudeau, JBeulich,
viktor.kleinik
On Mon, 2014-04-07 at 01:31 +0200, Arianna Avanzini wrote:
> Currently, the "iomem" domU config option allows to specify a machine
> address range to be mapped to the domU. However, there is no way to
> specify the guest address range used for the mapping. This commit
> extends the iomem option handling code to parse an additional, optional
> parameter: this parameter, if given, specifies the start guest address
> used for the mapping; if no start guest address is given, a 1:1 mapping
> is performed as default.
>
> Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
> Cc: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Paolo Valente <paolo.valente@unimore.it>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Julien Grall <julien.grall@citrix.com>
> Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Eric Trudeau <etrudeau@broadcom.com>
> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
>
> ---
>
> v5:
> - Initialize the gfn field of the iomem structure with the
> value "(uint64_t)-1".
> - Defer the assignment of the value of "start" to "gfn", if
> "gfn" has been initialized to the default value, in libxl.
> - Use a local variable to keep the return value of sscanf()
> for better code readability.
>
> v4:
> - Add definition of a LIBXL_HAVE_BUILDINFO_IOMEM_START_GFN macro
> to indicate the availability of the new option.
> - Simplify the code parsing the new optional parameter by using
> the switch construct on the return value of the sscanf() function
> instead of calling the function twice.
> - Add a short paragraph to the manpage about the use of the iomem
> option to passthrough devices protected by an IOMMU.
> - Add comments to the fields of the "iomem" structure.
>
> ---
> docs/man/xl.cfg.pod.5 | 13 +++++++++----
> tools/libxl/libxl.h | 10 ++++++++++
> tools/libxl/libxl_create.c | 6 ++++++
> tools/libxl/libxl_types.idl | 7 +++++--
> tools/libxl/xl_cmdimpl.c | 19 ++++++++++---------
> 5 files changed, 40 insertions(+), 15 deletions(-)
>
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index a6663b9..d54f601 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -602,12 +602,17 @@ is given in hexadecimal and may either a span e.g. C<2f8-2ff>
> It is recommended to use this option only for trusted VMs under
> administrator control.
>
> -=item B<iomem=[ "IOMEM_START,NUM_PAGES", "IOMEM_START,NUM_PAGES", ... ]>
> +=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN]", "IOMEM_START,NUM_PAGES[@GFN]", ... ]>
>
> Allow guest to access specific hardware I/O memory pages. B<IOMEM_START>
> -is a physical page number. B<NUM_PAGES> is the number
> -of pages beginning with B<START_PAGE> to allow access. Both values
> -must be given in hexadecimal.
> +is a physical page number. B<NUM_PAGES> is the number of pages beginning
> +with B<START_PAGE> to allow access. B<GFN> specifies the guest frame number
> +where the mapping will start in the domU's address space.
"If B<GFN> is not given then ..."
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index b2c3015..e4a1128 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -95,6 +95,16 @@
> #define LIBXL_HAVE_BUILDINFO_EVENT_CHANNELS 1
>
> /*
> + * LIBXL_HAVE_BUILDINFO_IOMEM_START_GFN indicated that it is possible
s/indicated/indicates/
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index d015cf4..369cd77 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> + if (b_info->iomem[i].gfn == (uint64_t)-1)
Please can you #define something like LIBXL_INVALID_GFN and use it both
here and in the idl. The value should probably be "~(uint64_t)0" rather
than -1.
Other than those small nits:
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v5 7/8] tools, libxl: add helpers to establish if guest is auto-translated
2014-04-06 23:31 [PATCH v5 0/8] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (5 preceding siblings ...)
2014-04-06 23:31 ` [PATCH v5 6/8] tools, libxl: parse optional start gfn from the iomem config option Arianna Avanzini
@ 2014-04-06 23:31 ` Arianna Avanzini
2014-04-06 23:32 ` [PATCH v5 8/8] tools, libxl: handle the iomem parameter with the memory_mapping hcall Arianna Avanzini
7 siblings, 0 replies; 19+ messages in thread
From: Arianna Avanzini @ 2014-04-06 23:31 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
JBeulich, avanzini.arianna, viktor.kleinik
This commit adds, for each architecture supported by libxl, a helper
function which establishes if a domain uses an auto-translated
physmap from the libxl_domain_config structure related to it.
Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Julien Grall <julien.grall@citrix.com>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
tools/libxl/libxl_arch.h | 2 ++
tools/libxl/libxl_arm.c | 5 +++++
tools/libxl/libxl_x86.c | 6 ++++++
3 files changed, 13 insertions(+)
diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
index aee0a91..963d98b 100644
--- a/tools/libxl/libxl_arch.h
+++ b/tools/libxl/libxl_arch.h
@@ -22,4 +22,6 @@ int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
int libxl__arch_domain_configure(libxl__gc *gc,
libxl_domain_build_info *info,
struct xc_dom_image *dom);
+
+int libxl__arch_auto_translated_physmap(libxl_domain_config *d_config);
#endif
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index 0cfd0cf..637f906 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -11,6 +11,11 @@ int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
return 0;
}
+int libxl__arch_auto_translated_physmap(libxl_domain_config *d_config)
+{
+ return 1;
+}
+
static struct arch_info {
const char *guest_type;
const char *timer_compat;
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index b11d036..c6f9c68 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -317,3 +317,9 @@ int libxl__arch_domain_configure(libxl__gc *gc,
{
return 0;
}
+
+int libxl__arch_auto_translated_physmap(libxl_domain_config *d_config)
+{
+ return (d_config->b_info.type == LIBXL_DOMAIN_TYPE_HVM ||
+ libxl_defbool_val(d_config->c_info.pvh));
+}
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v5 8/8] tools, libxl: handle the iomem parameter with the memory_mapping hcall
2014-04-06 23:31 [PATCH v5 0/8] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (6 preceding siblings ...)
2014-04-06 23:31 ` [PATCH v5 7/8] tools, libxl: add helpers to establish if guest is auto-translated Arianna Avanzini
@ 2014-04-06 23:32 ` Arianna Avanzini
2014-04-09 14:25 ` Ian Campbell
7 siblings, 1 reply; 19+ messages in thread
From: Arianna Avanzini @ 2014-04-06 23:32 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
JBeulich, avanzini.arianna, viktor.kleinik
Currently, the configuration-parsing code concerning the handling of the
iomem parameter only invokes the XEN_DOMCTL_iomem_permission hypercall.
This commit lets the XEN_DOMCTL_memory_mapping hypercall be invoked
after XEN_DOMCTL_iomem_permission when the iomem parameter is parsed
from a domU configuration file, so that the address range can be mapped
to the address space of the domU. The hypercall is invoked only in case
of domains using an auto-translated physmap.
Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Julien Grall <julien.grall@citrix.com>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
---
v4:
- Let the hypercall be called only in case the guest uses an
auto-translated physmap.
v3:
- Add ifdefs to let the hypercall be called only by ARM or x86
HVM guests, with a whitelist approach.
- Remove the NOTE comment.
v2:
- Add a comment explaining outstanding issues.
---
tools/libxl/libxl_create.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 369cd77..5ca3b37 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1113,6 +1113,19 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
"failed give dom%d access to iomem range %"PRIx64"-%"PRIx64,
domid, io->start, io->start + io->number - 1);
ret = ERROR_FAIL;
+ continue;
+ }
+ if (!libxl__arch_auto_translated_physmap(d_config))
+ continue;
+ ret = xc_domain_memory_mapping(CTX->xch, domid,
+ io->gfn, io->start,
+ io->number, 1);
+ if (ret < 0) {
+ LOGE(ERROR,
+ "failed to map to dom%d iomem range %"PRIx64"-%"PRIx64
+ " to guest address %"PRIx64,
+ domid, io->start, io->start + io->number - 1, io->gfn);
+ ret = ERROR_FAIL;
}
}
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v5 8/8] tools, libxl: handle the iomem parameter with the memory_mapping hcall
2014-04-06 23:32 ` [PATCH v5 8/8] tools, libxl: handle the iomem parameter with the memory_mapping hcall Arianna Avanzini
@ 2014-04-09 14:25 ` Ian Campbell
0 siblings, 0 replies; 19+ messages in thread
From: Ian Campbell @ 2014-04-09 14:25 UTC (permalink / raw)
To: Arianna Avanzini
Cc: paolo.valente, keir, stefano.stabellini, Ian.Jackson,
dario.faggioli, tim, xen-devel, julien.grall, etrudeau, JBeulich,
Daniel De Graaf, viktor.kleinik
On Mon, 2014-04-07 at 01:32 +0200, Arianna Avanzini wrote:
> Currently, the configuration-parsing code concerning the handling of the
> iomem parameter only invokes the XEN_DOMCTL_iomem_permission hypercall.
> This commit lets the XEN_DOMCTL_memory_mapping hypercall be invoked
> after XEN_DOMCTL_iomem_permission when the iomem parameter is parsed
> from a domU configuration file, so that the address range can be mapped
> to the address space of the domU. The hypercall is invoked only in case
> of domains using an auto-translated physmap.
I suppose http://article.gmane.org/gmane.comp.emulators.xen.devel/194504
applies here, hence this patch and patch #7 haven't changed this time
round.
IIRC the main issue to be decided is the one from:
http://article.gmane.org/gmane.comp.emulators.xen.devel/193808 and
http://article.gmane.org/gmane.comp.emulators.xen.devel/194058 .
The question is whether XEN_DOMCTL_memory_mapping should also implicitly
grant permissions to the region or if it should require that a call to
XEN_DOMCTL_iomem_permission has already been made.
In the former case (implicitly granting) the calls to
xc_domain_memory_mapping replace xc_domain_iomem_permission in this
patch instead of being added.
In the latter case (require iomem_perm first) then te
iomem_permit_access should be removed from the
XEN_DOMCTL_memory_mapping implementation and be replaced with a
permissions check instead of grant. We'd also want to make sure that
e.g. libxl_pci.c was making the correct xc_domain_iomem_permission calls
to subsequently allow qemu to use xc_domain_memory_mapping (which would
mean refactoring do_pci_add to make those calls for both HVM and PV
guests, AFAICT).
Although the first change seems simpler the second way has the advantage
of separating the permission from the mapping, which might be beneficial
because it handles the case of two mappings of the same thing better. It
might also have implications for XSM and the separation of privilege
into toolstack and qemu.
Ian.
^ permalink raw reply [flat|nested] 19+ messages in thread