* [PATCH v6 01/11] arch, arm: domain build: let dom0 access I/O memory of mapped devices
2014-04-21 13:44 [PATCH v6 00/11] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
@ 2014-04-21 13:44 ` Arianna Avanzini
2014-04-29 12:37 ` Julien Grall
2014-04-21 13:44 ` [PATCH v6 02/11] arch, arm: add consistency check to REMOVE p2m changes Arianna Avanzini
` (9 subsequent siblings)
10 siblings, 1 reply; 39+ messages in thread
From: Arianna Avanzini @ 2014-04-21 13:44 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 187e071..1802b6e 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>
@@ -740,6 +741,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] 39+ messages in thread* Re: [PATCH v6 01/11] arch, arm: domain build: let dom0 access I/O memory of mapped devices
2014-04-21 13:44 ` [PATCH v6 01/11] arch, arm: domain build: let dom0 access I/O memory of mapped devices Arianna Avanzini
@ 2014-04-29 12:37 ` Julien Grall
2014-04-29 13:44 ` Julien Grall
0 siblings, 1 reply; 39+ messages in thread
From: Julien Grall @ 2014-04-29 12:37 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,
On 04/21/2014 02:44 PM, Arianna Avanzini wrote:
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 187e071..1802b6e 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>
> @@ -740,6 +741,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);
>
I though a bit more about this patch. If the device is disabled (i.e
status="disabled"), Xen doesn't call map_device.
Futhermore, in your use case (e.g with iomem=) you might want to map
memory that is not describe to the device tree.
I would either:
1) give a full access to the I/O range
2) give a full access to the I/O range except on the RAM region
IHMO, the second solution might be better but I don't know if it's easy
to implement it.
Any thoughts?
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v6 01/11] arch, arm: domain build: let dom0 access I/O memory of mapped devices
2014-04-29 12:37 ` Julien Grall
@ 2014-04-29 13:44 ` Julien Grall
2014-04-29 23:12 ` Arianna Avanzini
0 siblings, 1 reply; 39+ messages in thread
From: Julien Grall @ 2014-04-29 13:44 UTC (permalink / raw)
To: Arianna Avanzini, Ian.Campbell
Cc: paolo.valente, keir, stefano.stabellini, tim, dario.faggioli,
Ian.Jackson, xen-devel, julien.grall, etrudeau, JBeulich,
viktor.kleinik
On 04/29/2014 01:37 PM, Julien Grall wrote:
> Hi Arianna,
>
> On 04/21/2014 02:44 PM, Arianna Avanzini wrote:
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 187e071..1802b6e 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>
>> @@ -740,6 +741,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);
>>
>
> I though a bit more about this patch. If the device is disabled (i.e
> status="disabled"), Xen doesn't call map_device.
> Futhermore, in your use case (e.g with iomem=) you might want to map
> memory that is not describe to the device tree.
>
> I would either:
> 1) give a full access to the I/O range
> 2) give a full access to the I/O range except on the RAM region
>
> IHMO, the second solution might be better but I don't know if it's easy
> to implement it.
I quickly wrote a follow-up of the series (see below). We can either merge
in this patch or I can carry it with the device passthrough patch series.
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 2f6ffe9..3498549 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -717,9 +717,14 @@ static int make_timer_node(const struct domain *d, void *fdt,
return res;
}
-/* Map the device in the domain */
-static int map_device(struct domain *d, struct kernel_info *kinfo,
- struct dt_device_node *dev)
+/* For a specific device node :
+ * - Give access permission to the guest
+ * When the device is available:
+ * - Assign the device to the guest if it's protected by an IOMMU
+ * - Map the IRQs and iomem regions to DOM0
+ */
+static int handle_device(struct domain *d, struct kernel_info *kinfo,
+ struct dt_device_node *dev, bool_t map)
{
unsigned int nirq;
unsigned int naddr;
@@ -734,7 +739,7 @@ static int map_device(struct domain *d, struct kernel_info *kinfo,
DPRINT("%s nirq = %d naddr = %u\n", dt_node_full_name(dev), nirq, naddr);
- if ( dt_device_is_protected(dev) )
+ if ( dt_device_is_protected(dev) && map )
{
DPRINT("%s setup iommu\n", dt_node_full_name(dev));
res = iommu_assign_dt_device(d, dev);
@@ -778,12 +783,15 @@ static int map_device(struct domain *d, struct kernel_info *kinfo,
}
DPRINT("irq %u = %u type = 0x%x\n", i, irq.irq, irq.type);
- res = route_dt_irq_to_guest(d, &irq, dt_node_name(dev));
- if ( res )
+ if ( map )
{
- printk(XENLOG_ERR "Unable to route IRQ %u to domain %u\n",
- irq.irq, d->domain_id);
- return res;
+ res = route_dt_irq_to_guest(d, &irq, dt_node_name(dev));
+ if ( res )
+ {
+ printk(XENLOG_ERR "Unable to route IRQ %u to domain %u\n",
+ irq.irq, d->domain_id);
+ return res;
+ }
}
}
@@ -811,17 +819,21 @@ static int map_device(struct domain *d, struct kernel_info *kinfo,
addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
return res;
}
- 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 )
+
+ if ( map )
{
- printk(XENLOG_ERR "Unable to map 0x%"PRIx64
- " - 0x%"PRIx64" in domain %d\n",
- addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1,
- d->domain_id);
- return res;
+ 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
+ " - 0x%"PRIx64" in domain %d\n",
+ addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1,
+ d->domain_id);
+ return res;
+ }
}
}
@@ -902,10 +914,9 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
* property. Therefore these device doesn't need to be mapped. This
* solution can be use later for pass through.
*/
- if ( !dt_device_type_is_equal(node, "memory") &&
- dt_device_is_available(node) )
+ if ( !dt_device_type_is_equal(node, "memory") )
{
- res = map_device(d, kinfo, node);
+ res = handle_device(d, kinfo, node, dt_device_is_available(node));
if ( res )
return res;
--
Julien Grall
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v6 01/11] arch, arm: domain build: let dom0 access I/O memory of mapped devices
2014-04-29 13:44 ` Julien Grall
@ 2014-04-29 23:12 ` Arianna Avanzini
0 siblings, 0 replies; 39+ messages in thread
From: Arianna Avanzini @ 2014-04-29 23:12 UTC (permalink / raw)
To: Julien Grall, Ian.Campbell
Cc: paolo.valente, keir, stefano.stabellini, tim, dario.faggioli,
Ian.Jackson, xen-devel, julien.grall, etrudeau, JBeulich,
viktor.kleinik
On 04/29/2014 03:44 PM, Julien Grall wrote:
> On 04/29/2014 01:37 PM, Julien Grall wrote:
>> Hi Arianna,
>>
>> On 04/21/2014 02:44 PM, Arianna Avanzini wrote:
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index 187e071..1802b6e 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>
>>> @@ -740,6 +741,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);
>>>
>>
>> I though a bit more about this patch. If the device is disabled (i.e
>> status="disabled"), Xen doesn't call map_device.
>> Futhermore, in your use case (e.g with iomem=) you might want to map
>> memory that is not describe to the device tree.
>>
>> I would either:
>> 1) give a full access to the I/O range
>> 2) give a full access to the I/O range except on the RAM region
>>
>> IHMO, the second solution might be better but I don't know if it's easy
>> to implement it.
>
> I quickly wrote a follow-up of the series (see below). We can either merge
> in this patch or I can carry it with the device passthrough patch series.
>
For me it is OK that the follow-up is kept in the device passthrough patch
series, if it's fine for you.
If you agree, I'll update the commit description of the 0001 patch in the
memory_mapping patchset and try to explain its limitations with regard to the
scenario you highlighted.
Also, sorry for the delay; I'll be surely sending a v7 of the memory_mapping
series in the next few days.
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 2f6ffe9..3498549 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -717,9 +717,14 @@ static int make_timer_node(const struct domain *d, void *fdt,
> return res;
> }
>
> -/* Map the device in the domain */
> -static int map_device(struct domain *d, struct kernel_info *kinfo,
> - struct dt_device_node *dev)
> +/* For a specific device node :
> + * - Give access permission to the guest
> + * When the device is available:
> + * - Assign the device to the guest if it's protected by an IOMMU
> + * - Map the IRQs and iomem regions to DOM0
> + */
> +static int handle_device(struct domain *d, struct kernel_info *kinfo,
> + struct dt_device_node *dev, bool_t map)
> {
> unsigned int nirq;
> unsigned int naddr;
> @@ -734,7 +739,7 @@ static int map_device(struct domain *d, struct kernel_info *kinfo,
>
> DPRINT("%s nirq = %d naddr = %u\n", dt_node_full_name(dev), nirq, naddr);
>
> - if ( dt_device_is_protected(dev) )
> + if ( dt_device_is_protected(dev) && map )
> {
> DPRINT("%s setup iommu\n", dt_node_full_name(dev));
> res = iommu_assign_dt_device(d, dev);
> @@ -778,12 +783,15 @@ static int map_device(struct domain *d, struct kernel_info *kinfo,
> }
>
> DPRINT("irq %u = %u type = 0x%x\n", i, irq.irq, irq.type);
> - res = route_dt_irq_to_guest(d, &irq, dt_node_name(dev));
> - if ( res )
> + if ( map )
> {
> - printk(XENLOG_ERR "Unable to route IRQ %u to domain %u\n",
> - irq.irq, d->domain_id);
> - return res;
> + res = route_dt_irq_to_guest(d, &irq, dt_node_name(dev));
> + if ( res )
> + {
> + printk(XENLOG_ERR "Unable to route IRQ %u to domain %u\n",
> + irq.irq, d->domain_id);
> + return res;
> + }
> }
> }
>
> @@ -811,17 +819,21 @@ static int map_device(struct domain *d, struct kernel_info *kinfo,
> addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
> return res;
> }
> - 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 )
> +
> + if ( map )
> {
> - printk(XENLOG_ERR "Unable to map 0x%"PRIx64
> - " - 0x%"PRIx64" in domain %d\n",
> - addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1,
> - d->domain_id);
> - return res;
> + 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
> + " - 0x%"PRIx64" in domain %d\n",
> + addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1,
> + d->domain_id);
> + return res;
> + }
> }
> }
>
> @@ -902,10 +914,9 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
> * property. Therefore these device doesn't need to be mapped. This
> * solution can be use later for pass through.
> */
> - if ( !dt_device_type_is_equal(node, "memory") &&
> - dt_device_is_available(node) )
> + if ( !dt_device_type_is_equal(node, "memory") )
> {
> - res = map_device(d, kinfo, node);
> + res = handle_device(d, kinfo, node, dt_device_is_available(node));
>
> if ( res )
> return res;
>
>
>
--
/*
* Arianna Avanzini
* avanzini.arianna@gmail.com
* 73628@studenti.unimore.it
*/
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v6 02/11] arch, arm: add consistency check to REMOVE p2m changes
2014-04-21 13:44 [PATCH v6 00/11] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
2014-04-21 13:44 ` [PATCH v6 01/11] arch, arm: domain build: let dom0 access I/O memory of mapped devices Arianna Avanzini
@ 2014-04-21 13:44 ` Arianna Avanzini
2014-04-22 19:35 ` Julien Grall
2014-04-21 13:44 ` [PATCH v6 03/11] arch, arm: let map_mmio_regions() take pfn as parameters Arianna Avanzini
` (8 subsequent siblings)
10 siblings, 1 reply; 39+ messages in thread
From: Arianna Avanzini @ 2014-04-21 13:44 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>
---
v6:
- Don't update "count" on REMOVE as it is only used inside the
RELINQUISH case of the switch in apply_p2m_changes().
- Return with an error if removal of a page fails instead of just
skipping the page.
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 | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 403fd89..7f03aa7 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,26 @@ 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 = 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 ( !pte.p2m.valid || maddr != pfn_to_paddr(mfn) )
+ return -EINVAL;
+ }
+ /* fall through */
+ case RELINQUISH:
+ {
if ( !pte.p2m.valid )
{
count++;
@@ -425,7 +441,6 @@ static int apply_p2m_changes(struct domain *d,
count++;
}
break;
-
case CACHEFLUSH:
{
if ( !pte.p2m.valid || !p2m_is_ram(pte.p2m.type) )
@@ -450,6 +465,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] 39+ messages in thread* Re: [PATCH v6 02/11] arch, arm: add consistency check to REMOVE p2m changes
2014-04-21 13:44 ` [PATCH v6 02/11] arch, arm: add consistency check to REMOVE p2m changes Arianna Avanzini
@ 2014-04-22 19:35 ` Julien Grall
0 siblings, 0 replies; 39+ messages in thread
From: Julien Grall @ 2014-04-22 19:35 UTC (permalink / raw)
To: Arianna Avanzini, xen-devel
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, Ian.Campbell, etrudeau, JBeulich,
viktor.kleinik
Hi Arianna,
On 21/04/14 14:44, Arianna Avanzini wrote:
> ---
> xen/arch/arm/p2m.c | 22 +++++++++++++++++++---
> 1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 403fd89..7f03aa7 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;
We differ a bit from x86 on this point. We will silently ignore that the
GFN is not mapped when op == REMOVE.
I'm wondering if we should return an error in this case. I don't find
any place where it's valid to remove non-present mapping.
[..]
> @@ -406,12 +408,26 @@ 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 = 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 ( !pte.p2m.valid || maddr != pfn_to_paddr(mfn) )
> + return -EINVAL;
> + }
> + /* fall through */
> + case RELINQUISH:
> + {
> if ( !pte.p2m.valid )
> {
> count++;
> @@ -425,7 +441,6 @@ static int apply_p2m_changes(struct domain *d,
> count++;
> }
> break;
> -
Spurious change here.
Regards
--
Julien Grall
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v6 03/11] arch, arm: let map_mmio_regions() take pfn as parameters
2014-04-21 13:44 [PATCH v6 00/11] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
2014-04-21 13:44 ` [PATCH v6 01/11] arch, arm: domain build: let dom0 access I/O memory of mapped devices Arianna Avanzini
2014-04-21 13:44 ` [PATCH v6 02/11] arch, arm: add consistency check to REMOVE p2m changes Arianna Avanzini
@ 2014-04-21 13:44 ` Arianna Avanzini
2014-04-28 13:13 ` Ian Campbell
2014-04-21 13:44 ` [PATCH v6 04/11] arch, arm: make pfn range passed to map_mmio_regions() inclusive Arianna Avanzini
` (7 subsequent siblings)
10 siblings, 1 reply; 39+ messages in thread
From: Arianna Avanzini @ 2014-04-21 13:44 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>
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: 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 1802b6e..15db84b 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -751,9 +751,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 8168b7b..a5c3288 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 7f03aa7..eb9da33 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -511,12 +511,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] 39+ messages in thread* Re: [PATCH v6 03/11] arch, arm: let map_mmio_regions() take pfn as parameters
2014-04-21 13:44 ` [PATCH v6 03/11] arch, arm: let map_mmio_regions() take pfn as parameters Arianna Avanzini
@ 2014-04-28 13:13 ` Ian Campbell
0 siblings, 0 replies; 39+ messages in thread
From: Ian Campbell @ 2014-04-28 13:13 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-21 at 15:44 +0200, 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>
> 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: 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 1802b6e..15db84b 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -751,9 +751,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),
Aren't all these -1's now redundant given the defintion of
paddr_to_pfn_aligned?
I think they are harmless in practice though:
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Ian.
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v6 04/11] arch, arm: make pfn range passed to map_mmio_regions() inclusive
2014-04-21 13:44 [PATCH v6 00/11] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (2 preceding siblings ...)
2014-04-21 13:44 ` [PATCH v6 03/11] arch, arm: let map_mmio_regions() take pfn as parameters Arianna Avanzini
@ 2014-04-21 13:44 ` Arianna Avanzini
2014-04-22 19:52 ` Julien Grall
2014-04-21 13:44 ` [PATCH v6 05/11] arch, x86: check if mapping exists before memory_mapping removes it Arianna Avanzini
` (6 subsequent siblings)
10 siblings, 1 reply; 39+ messages in thread
From: Arianna Avanzini @ 2014-04-21 13:44 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 accepts a range of pfns
which is not inclusive of the end frame number. However, the
corresponding operation performed for the x86 architecture, which
will be factored out from the memory_mapping DOMCTL in one of the
following commits, accepts a pfn range which is inclusive of the
end pfn.
This commit attempts to make such an interface consistent, by
letting map_mmio_regions() consider a pfn range inclusive of the
end pfn.
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/arm/domain_build.c | 2 +-
xen/arch/arm/gic.c | 6 +++---
xen/arch/arm/p2m.c | 2 +-
xen/arch/arm/platforms/exynos5.c | 4 ++--
xen/arch/arm/platforms/omap5.c | 8 ++++----
xen/arch/arm/platforms/xgene-storm.c | 2 +-
xen/include/asm-arm/p2m.h | 5 +++--
7 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 15db84b..0d55468 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -753,7 +753,7 @@ static int map_device(struct domain *d, const struct dt_device_node *dev)
}
res = map_mmio_regions(d,
paddr_to_pfn(addr & PAGE_MASK),
- paddr_to_pfn_aligned(addr + size - 1),
+ paddr_to_pfn_aligned(addr + size) - 1,
paddr_to_pfn(addr & PAGE_MASK));
if ( res )
{
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index a5c3288..11fbd42 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -890,7 +890,7 @@ int gicv_setup(struct domain *d)
*/
ret = map_mmio_regions(d, paddr_to_pfn(d->arch.vgic.cbase),
paddr_to_pfn_aligned(d->arch.vgic.cbase +
- PAGE_SIZE - 1),
+ PAGE_SIZE) - 1,
paddr_to_pfn(gic.vbase));
if (ret)
return ret;
@@ -898,12 +898,12 @@ int gicv_setup(struct domain *d)
if ( !platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
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),
+ (2 * PAGE_SIZE)) - 1,
paddr_to_pfn(gic.vbase + PAGE_SIZE));
else
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),
+ (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 eb9da33..54a0afa 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -517,7 +517,7 @@ int map_mmio_regions(struct domain *d,
{
return apply_p2m_changes(d, INSERT,
pfn_to_paddr(start_gfn),
- pfn_to_paddr(end_gfn),
+ pfn_to_paddr(end_gfn + 1),
pfn_to_paddr(mfn),
MATTR_DEV, p2m_mmio_direct);
}
diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
index acd8781..d8c0b82 100644
--- a/xen/arch/arm/platforms/exynos5.c
+++ b/xen/arch/arm/platforms/exynos5.c
@@ -55,13 +55,13 @@ static int exynos5_specific_mapping(struct domain *d)
{
/* Map the chip ID */
map_mmio_regions(d, paddr_to_pfn(EXYNOS5_PA_CHIPID),
- paddr_to_pfn_aligned(EXYNOS5_PA_CHIPID + PAGE_SIZE - 1),
+ paddr_to_pfn_aligned(EXYNOS5_PA_CHIPID + PAGE_SIZE) - 1,
paddr_to_pfn(EXYNOS5_PA_CHIPID));
/* Map the PWM region */
map_mmio_regions(d, paddr_to_pfn(EXYNOS5_PA_TIMER),
paddr_to_pfn_aligned(EXYNOS5_PA_TIMER +
- (PAGE_SIZE * 2) - 1),
+ (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 c975020..3876456 100644
--- a/xen/arch/arm/platforms/omap5.c
+++ b/xen/arch/arm/platforms/omap5.c
@@ -103,22 +103,22 @@ static int omap5_specific_mapping(struct domain *d)
{
/* Map the PRM module */
map_mmio_regions(d, paddr_to_pfn(OMAP5_PRM_BASE),
- paddr_to_pfn_aligned(OMAP5_PRM_BASE + (PAGE_SIZE * 2) - 1),
+ 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, paddr_to_pfn(OMAP5_PRCM_MPU_BASE),
- paddr_to_pfn_aligned(OMAP5_PRCM_MPU_BASE + PAGE_SIZE - 1),
+ 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, paddr_to_pfn(OMAP5_WKUPGEN_BASE),
- paddr_to_pfn_aligned(OMAP5_WKUPGEN_BASE + PAGE_SIZE - 1),
+ 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, paddr_to_pfn(OMAP5_SRAM_PA),
- paddr_to_pfn_aligned(OMAP5_SRAM_PA + (PAGE_SIZE * 32) - 1),
+ 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 f3662fd..6187d02 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -48,7 +48,7 @@ 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, paddr_to_pfn(start),
- paddr_to_pfn_aligned(end),
+ paddr_to_pfn_aligned(end) - 1,
paddr_to_pfn(start));
if ( ret )
printk("Failed to map %s @ %"PRIpaddr" to dom%d\n",
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index c7dd6aa..f75dd44 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -84,8 +84,9 @@ 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. */
+/* Map MMIO regions in the p2m: start_gfn and end_gfn is the range, inclusive
+ * of the end_gfn frame number, 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,
--
1.9.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v6 04/11] arch, arm: make pfn range passed to map_mmio_regions() inclusive
2014-04-21 13:44 ` [PATCH v6 04/11] arch, arm: make pfn range passed to map_mmio_regions() inclusive Arianna Avanzini
@ 2014-04-22 19:52 ` Julien Grall
2014-04-28 13:15 ` Ian Campbell
0 siblings, 1 reply; 39+ messages in thread
From: Julien Grall @ 2014-04-22 19:52 UTC (permalink / raw)
To: Arianna Avanzini, xen-devel
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, Ian.Campbell, etrudeau, JBeulich,
viktor.kleinik
Hi Arianna,
On 21/04/14 14:44, Arianna Avanzini wrote:
> Currently, the map_mmio_regions() function accepts a range of pfns
> which is not inclusive of the end frame number. However, the
> corresponding operation performed for the x86 architecture, which
> will be factored out from the memory_mapping DOMCTL in one of the
> following commits, accepts a pfn range which is inclusive of the
> end pfn.
> This commit attempts to make such an interface consistent, by
> letting map_mmio_regions() consider a pfn range inclusive of the
> end pfn.
I think this patch can be merged with the previous patch (i.e #3).
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v6 04/11] arch, arm: make pfn range passed to map_mmio_regions() inclusive
2014-04-22 19:52 ` Julien Grall
@ 2014-04-28 13:15 ` Ian Campbell
0 siblings, 0 replies; 39+ messages in thread
From: Ian Campbell @ 2014-04-28 13:15 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 Tue, 2014-04-22 at 20:52 +0100, Julien Grall wrote:
> Hi Arianna,
>
> On 21/04/14 14:44, Arianna Avanzini wrote:
> > Currently, the map_mmio_regions() function accepts a range of pfns
> > which is not inclusive of the end frame number. However, the
> > corresponding operation performed for the x86 architecture, which
> > will be factored out from the memory_mapping DOMCTL in one of the
> > following commits, accepts a pfn range which is inclusive of the
> > end pfn.
> > This commit attempts to make such an interface consistent, by
> > letting map_mmio_regions() consider a pfn range inclusive of the
> > end pfn.
>
>
> I think this patch can be merged with the previous patch (i.e #3).
I think it would be better to keep the type change and the limits
handling separate. All the +1/-1/align to something is tricky enough
already done step by step.
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Ian.
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v6 05/11] arch, x86: check if mapping exists before memory_mapping removes it
2014-04-21 13:44 [PATCH v6 00/11] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (3 preceding siblings ...)
2014-04-21 13:44 ` [PATCH v6 04/11] arch, arm: make pfn range passed to map_mmio_regions() inclusive Arianna Avanzini
@ 2014-04-21 13:44 ` Arianna Avanzini
2014-04-22 8:34 ` Jan Beulich
2014-04-21 13:44 ` [PATCH v6 06/11] xen, x86: factor out map and unmap from the memory_mapping DOMCTL Arianna Avanzini
` (5 subsequent siblings)
10 siblings, 1 reply; 39+ messages in thread
From: Arianna Avanzini @ 2014-04-21 13:44 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 | 16 ++++++++++++----
xen/include/asm-x86/p2m.h | 2 +-
3 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index efe9ef2..33e31e3 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -680,7 +680,7 @@ long arch_do_domctl(
"memory_map:fail: dom%d gfn=%lx mfn=%lx ret:%ld\n",
d->domain_id, gfn + i, mfn + i, ret);
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
@@ -700,7 +700,7 @@ long arch_do_domctl(
if ( paging_mode_translate(d) )
for ( i = 0; i < nr_mfns; i++ )
{
- ret = clear_mmio_p2m_entry(d, gfn + i);
+ ret = clear_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
if ( ret )
tmp_rc = ret;
}
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 365e37a..8489482 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -807,10 +807,10 @@ int set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
}
/* Returns: 0 for success, -errno for failure */
-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)
{
int rc = -EINVAL;
- mfn_t mfn;
+ mfn_t actual_mfn;
p2m_access_t a;
p2m_type_t t;
struct p2m_domain *p2m = p2m_get_hostp2m(d);
@@ -819,15 +819,23 @@ int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn)
return -EIO;
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,
"gfn_to_mfn failed! gfn=%08lx type:%d\n", gfn, t);
goto out;
}
+ if ( mfn_x(mfn) != mfn_x(actual_mfn) )
+ {
+ gdprintk(XENLOG_ERR,
+ "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 = p2m_set_entry(p2m, gfn, _mfn(INVALID_MFN), PAGE_ORDER_4K, p2m_invalid,
p2m->default_access);
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 0308b89..4a1c129 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -514,7 +514,7 @@ void p2m_memory_type_changed(struct domain *d);
/* 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] 39+ messages in thread* Re: [PATCH v6 05/11] arch, x86: check if mapping exists before memory_mapping removes it
2014-04-21 13:44 ` [PATCH v6 05/11] arch, x86: check if mapping exists before memory_mapping removes it Arianna Avanzini
@ 2014-04-22 8:34 ` Jan Beulich
2014-04-22 8:53 ` Julien Grall
0 siblings, 1 reply; 39+ messages in thread
From: Jan Beulich @ 2014-04-22 8:34 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 21.04.14 at 15:44, <avanzini.arianna@gmail.com> wrote:
> 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.
I think this goes too far: Did you check that all existing tool stacks
actually pass a valid MFN for the unmap? It would seem quite natural
to me if some didn't, since tool stacks can be expected to know what
they're doing.
> @@ -819,15 +819,23 @@ int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn)
> return -EIO;
>
> 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) )
And if we went the route you propose, the INVALID_MFN check here
could be dropped (as being redundant with the check added below, so
long as the passed in MFN isn't INVALID_MFN).
> {
> gdprintk(XENLOG_ERR,
> "gfn_to_mfn failed! gfn=%08lx type:%d\n", gfn, t);
> goto out;
> }
> + if ( mfn_x(mfn) != mfn_x(actual_mfn) )
> + {
> + gdprintk(XENLOG_ERR,
> + "mapping between mfn %08lx and gfn %08lx does not exist "
> + "(mapped to %08lx)\n",
> + mfn_x(mfn), gfn, mfn_x(actual_mfn));
> + goto out;
> + }
But more likely you will want to make this a warning only, i.e. a
message logged without causing the function to fail. And we would
then see whether anyone complains about this having become too
verbose (in which case we might need to drop the message again).
Jan
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v6 05/11] arch, x86: check if mapping exists before memory_mapping removes it
2014-04-22 8:34 ` Jan Beulich
@ 2014-04-22 8:53 ` Julien Grall
2014-04-22 9:20 ` Jan Beulich
0 siblings, 1 reply; 39+ messages in thread
From: Julien Grall @ 2014-04-22 8:53 UTC (permalink / raw)
To: Jan Beulich, Arianna Avanzini
Cc: julien.grall, paolo.valente, keir, stefano.stabellini,
Ian.Jackson, dario.faggioli, tim, xen-devel, Ian.Campbell,
etrudeau, viktor.kleinik
Hi Jan,
On 22/04/14 09:34, Jan Beulich wrote:
>>>> On 21.04.14 at 15:44, <avanzini.arianna@gmail.com> wrote:
>> 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.
>
> I think this goes too far: Did you check that all existing tool stacks
> actually pass a valid MFN for the unmap? It would seem quite natural
> to me if some didn't, since tool stacks can be expected to know what
> they're doing.
If the toolstack doesn't give a valid MFN that would mean that the
toolstack can mess up the range permission.
On a previous mail, I suggested to skip the MFN parameter when the
toolstack is unmapping the range. Xen will take care to translate the
GFN into an MFN.
AFAIU, it's what we do on the other unmap hypercalls.
--
Julien Grall
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v6 05/11] arch, x86: check if mapping exists before memory_mapping removes it
2014-04-22 8:53 ` Julien Grall
@ 2014-04-22 9:20 ` Jan Beulich
0 siblings, 0 replies; 39+ messages in thread
From: Jan Beulich @ 2014-04-22 9:20 UTC (permalink / raw)
To: Arianna Avanzini, Julien Grall
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, xen-devel, julien.grall, etrudeau,
viktor.kleinik
>>> On 22.04.14 at 10:53, <julien.grall@linaro.org> wrote:
> On 22/04/14 09:34, Jan Beulich wrote:
>>>>> On 21.04.14 at 15:44, <avanzini.arianna@gmail.com> wrote:
>>> 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.
>>
>> I think this goes too far: Did you check that all existing tool stacks
>> actually pass a valid MFN for the unmap? It would seem quite natural
>> to me if some didn't, since tool stacks can be expected to know what
>> they're doing.
>
> If the toolstack doesn't give a valid MFN that would mean that the
> toolstack can mess up the range permission.
Oh, right. So there's at least some hope that the tool stacks get this
right (but no guarantee).
> On a previous mail, I suggested to skip the MFN parameter when the
> toolstack is unmapping the range. Xen will take care to translate the
> GFN into an MFN.
But that won't be needed anymore when access revocation and
unmapping get done through distinct hypercalls.
Jan
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v6 06/11] xen, x86: factor out map and unmap from the memory_mapping DOMCTL
2014-04-21 13:44 [PATCH v6 00/11] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (4 preceding siblings ...)
2014-04-21 13:44 ` [PATCH v6 05/11] arch, x86: check if mapping exists before memory_mapping removes it Arianna Avanzini
@ 2014-04-21 13:44 ` Arianna Avanzini
2014-04-21 16:14 ` Andrew Cooper
2014-04-22 8:48 ` Jan Beulich
2014-04-21 13:45 ` [PATCH v6 07/11] xen, common: move the memory_mapping DOMCTL hypercall to common code Arianna Avanzini
` (4 subsequent siblings)
10 siblings, 2 replies; 39+ messages in thread
From: Arianna Avanzini @ 2014-04-21 13:44 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 factors out from the XEN_DOMCTL_memory_mapping hypercall
implementation, currently available only for x86, the operations
related to memory ranges map and unmap. The code is factored out
into two {map|unmap}_mmio_regions() functions for x86 , that will match
the corresponding pair of ARM functions when the DOMCTL will be moved
to common code in the following commit.
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>
---
v6:
- Fix uncorrect usage of [mg]fn_end which, in the initial checks,
was subtracted 1 twice.
- Remove useless comment about the handling of errors in the
remove path of the hypercall.
- Replace stray hard tab with spaces.
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.
v4:
- 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:
- Add map_mmio_regions() and unmap_mmio_regions() functions for x86;
- Use pfn as parameters to the unmap_mmio_regions() function.
- Compute gfn + nr_mfns and mfn + nr_mfns only once.
v2:
- 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/x86/domctl.c | 51 ++++++++++++++++++++---------------------------
xen/arch/x86/mm/p2m.c | 42 ++++++++++++++++++++++++++++++++++++++
xen/include/asm-x86/p2m.h | 12 +++++++++++
3 files changed, 76 insertions(+), 29 deletions(-)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 33e31e3..04c6007 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -646,46 +646,45 @@ long arch_do_domctl(
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;
+ unsigned long mfn_end = mfn + nr_mfns - 1;
+ unsigned long gfn_end = gfn + nr_mfns - 1;
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;
+ if ( mfn_end < mfn || /* wrap? */
+ ((mfn | mfn_end) >> (paddr_bits - PAGE_SHIFT)) ||
+ gfn_end < gfn ) /* wrap? */
+ return ret;
ret = -EPERM;
- if ( !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 1) )
+ if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
break;
- ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, add);
+ ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, add);
if ( ret )
break;
if ( add )
{
printk(XENLOG_G_INFO
- "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx\n",
+ "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) )
+ ret = iomem_permit_access(d, mfn, mfn_end);
+ if ( !ret )
{
- for ( i = 0; !ret && i < nr_mfns; i++ )
- ret = set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
+ ret = map_mmio_regions(d, gfn, gfn_end, mfn);
if ( ret )
{
printk(XENLOG_G_WARNING
- "memory_map:fail: dom%d gfn=%lx mfn=%lx ret:%ld\n",
- d->domain_id, gfn + i, mfn + i, ret);
- while ( i-- )
- clear_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
- if ( iomem_deny_access(d, mfn, mfn + nr_mfns - 1) &&
+ "memory_map: fail: dom%d gfn=%lx mfn=%lx ret:%ld\n",
+ d->domain_id, gfn, mfn, ret);
+ 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 + nr_mfns - 1);
+ "memory_map: failed to deny dom%d access "
+ "to [%lx,%lx]\n",
+ d->domain_id, mfn, mfn_end);
}
}
}
@@ -694,24 +693,18 @@ long arch_do_domctl(
int tmp_rc = 0;
printk(XENLOG_G_INFO
- "memory_map:remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
+ "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++ )
- {
- ret = clear_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
- if ( ret )
- tmp_rc = ret;
- }
- ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
+ tmp_rc = unmap_mmio_regions(d, gfn, gfn_end, mfn);
+ ret = iomem_deny_access(d, mfn, mfn_end);
if ( !ret )
ret = tmp_rc;
if ( ret && is_hardware_domain(current->domain) )
printk(XENLOG_ERR
"memory_map: error %ld %s dom%d access to [%lx,%lx]\n",
ret, tmp_rc ? "removing" : "denying", d->domain_id,
- mfn, mfn + nr_mfns - 1);
+ mfn, mfn_end);
}
/* Do this unconditionally to cover errors on above failure paths. */
memory_type_changed(d);
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 8489482..a112fe4 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1657,6 +1657,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/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 4a1c129..1098600 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -32,6 +32,18 @@
#include <asm/mem_sharing.h>
#include <asm/page.h> /* for pagetable_t */
+/* Map MMIO regions in the p2m: start_gfn and end_gfn is the range, inclusive
+ * of the end_gfn frame number, 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);
+
extern bool_t opt_hap_1gb, opt_hap_2mb;
/*
--
1.9.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v6 06/11] xen, x86: factor out map and unmap from the memory_mapping DOMCTL
2014-04-21 13:44 ` [PATCH v6 06/11] xen, x86: factor out map and unmap from the memory_mapping DOMCTL Arianna Avanzini
@ 2014-04-21 16:14 ` Andrew Cooper
2014-04-22 8:50 ` Jan Beulich
2014-04-22 8:48 ` Jan Beulich
1 sibling, 1 reply; 39+ messages in thread
From: Andrew Cooper @ 2014-04-21 16:14 UTC (permalink / raw)
To: Arianna Avanzini, xen-devel
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, Ian.Campbell, etrudeau, JBeulich,
viktor.kleinik
On 21/04/2014 14:44, Arianna Avanzini wrote:
>
> #if P2M_AUDIT
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index 4a1c129..1098600 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -32,6 +32,18 @@
> #include <asm/mem_sharing.h>
> #include <asm/page.h> /* for pagetable_t */
>
> +/* Map MMIO regions in the p2m: start_gfn and end_gfn is the range, inclusive
> + * of the end_gfn frame number, 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);
> +
> extern bool_t opt_hap_1gb, opt_hap_2mb;
>
> /*
Given that you are changing all of this, please please take the time to
change these functions (and others in the series, where appropriate) to
take parameters more like (struct domain *d, unsigned long gfn, unsigned
long mfn, unsigned long count)
With a count parameter as opposed to a start/end pair, there can be no
confusion regarding inclusive/exclusive ranges, or at which point to
subtract 1. It will also resemble the information in the hypercall, and
avoids needing to recalculate nr_mfns everywhere.
~Andrew
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v6 06/11] xen, x86: factor out map and unmap from the memory_mapping DOMCTL
2014-04-21 16:14 ` Andrew Cooper
@ 2014-04-22 8:50 ` Jan Beulich
0 siblings, 0 replies; 39+ messages in thread
From: Jan Beulich @ 2014-04-22 8:50 UTC (permalink / raw)
To: Andrew Cooper, Arianna Avanzini, xen-devel
Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, julien.grall, etrudeau,
viktor.kleinik
>>> On 21.04.14 at 18:14, <andrew.cooper3@citrix.com> wrote:
> On 21/04/2014 14:44, Arianna Avanzini wrote:
>> --- a/xen/include/asm-x86/p2m.h
>> +++ b/xen/include/asm-x86/p2m.h
>> @@ -32,6 +32,18 @@
>> #include <asm/mem_sharing.h>
>> #include <asm/page.h> /* for pagetable_t */
>>
>> +/* Map MMIO regions in the p2m: start_gfn and end_gfn is the range, inclusive
>> + * of the end_gfn frame number, 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);
>> +
>> extern bool_t opt_hap_1gb, opt_hap_2mb;
>>
>> /*
>
> Given that you are changing all of this, please please take the time to
> change these functions (and others in the series, where appropriate) to
> take parameters more like (struct domain *d, unsigned long gfn, unsigned
> long mfn, unsigned long count)
>
> With a count parameter as opposed to a start/end pair, there can be no
> confusion regarding inclusive/exclusive ranges, or at which point to
> subtract 1. It will also resemble the information in the hypercall, and
> avoids needing to recalculate nr_mfns everywhere.
Ian had asked for this too. But I'm not sure - neither a [start,end)
pair nor start plus count can express the full address range; only
a [start,end] pair can. That said, I realize that we're not going to
ever see a (reasonable) use of these functions covering the entire
address space...
Jan
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v6 06/11] xen, x86: factor out map and unmap from the memory_mapping DOMCTL
2014-04-21 13:44 ` [PATCH v6 06/11] xen, x86: factor out map and unmap from the memory_mapping DOMCTL Arianna Avanzini
2014-04-21 16:14 ` Andrew Cooper
@ 2014-04-22 8:48 ` Jan Beulich
1 sibling, 0 replies; 39+ messages in thread
From: Jan Beulich @ 2014-04-22 8:48 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 21.04.14 at 15:44, <avanzini.arianna@gmail.com> wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -646,46 +646,45 @@ long arch_do_domctl(
> 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;
> + unsigned long mfn_end = mfn + nr_mfns - 1;
> + unsigned long gfn_end = gfn + nr_mfns - 1;
> 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;
> + if ( mfn_end < mfn || /* wrap? */
> + ((mfn | mfn_end) >> (paddr_bits - PAGE_SHIFT)) ||
> + gfn_end < gfn ) /* wrap? */
> + return ret;
>
> ret = -EPERM;
> - if ( !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 1) )
> + if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
> break;
>
> - ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, add);
> + ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, add);
> if ( ret )
> break;
>
> if ( add )
> {
> printk(XENLOG_G_INFO
> - "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx\n",
> + "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) )
> + ret = iomem_permit_access(d, mfn, mfn_end);
> + if ( !ret )
> {
> - for ( i = 0; !ret && i < nr_mfns; i++ )
> - ret = set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
> + ret = map_mmio_regions(d, gfn, gfn_end, mfn);
> if ( ret )
> {
> printk(XENLOG_G_WARNING
> - "memory_map:fail: dom%d gfn=%lx mfn=%lx ret:%ld\n",
> - d->domain_id, gfn + i, mfn + i, ret);
> - while ( i-- )
> - clear_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
> - if ( iomem_deny_access(d, mfn, mfn + nr_mfns - 1) &&
> + "memory_map: fail: dom%d gfn=%lx mfn=%lx ret:%ld\n",
> + d->domain_id, gfn, mfn, ret);
> + 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 + nr_mfns - 1);
> + "memory_map: failed to deny dom%d access "
> + "to [%lx,%lx]\n",
Please don't split format strings like this, unless they become _really_
long (such that when grep-ing for any part of the string it will be
found).
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1657,6 +1657,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)
The last parameter her should be mfn_t.
> +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));
Elsewhere you updated to Mukesh's recent changes, yet this one
certainly is wrong now - clear_mmio_p2m_entry() now returns 0 on
success. When you re-base patches, you really should be rather
careful when you have to adjust code being removed in one place and
added back in another: In almost all cases you'll have to adjust both,
even if the patch applies fine without changes on the adding-back side.
Also, now that you have this as a separate function, perhaps it
would be better to use this for the error recovery path in
map_mmio_regions()?
Jan
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v6 07/11] xen, common: move the memory_mapping DOMCTL hypercall to common code
2014-04-21 13:44 [PATCH v6 00/11] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (5 preceding siblings ...)
2014-04-21 13:44 ` [PATCH v6 06/11] xen, x86: factor out map and unmap from the memory_mapping DOMCTL Arianna Avanzini
@ 2014-04-21 13:45 ` Arianna Avanzini
2014-04-22 8:55 ` Jan Beulich
2014-04-21 13:45 ` [PATCH v6 08/11] tools, libxl: parse optional start gfn from the iomem config option Arianna Avanzini
` (3 subsequent siblings)
10 siblings, 1 reply; 39+ messages in thread
From: Arianna Avanzini @ 2014-04-21 13:45 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 moves to common code the implementation of the memory_mapping
DOMCTL, currently available only for the x86 architecture. This commit
also adds to ARM-related code an unmap_mmio_regions() function to match
the corresponding one available for x86.
NOTE: this commit also adds an empty definition for an ARM-specific
memory_type_changed() function. Such a function's implementation
is not added in this patchset.
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>
---
v6:
- Pass p2m_invalid as last parameter to unmap_mmio_regions() for ARM
as it is not (and currently must not be) used.
- Add an empty definition of the memory_type_changed() function for ARM.
v5:
- 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.
v3:
- Add a paddr_bits variable for ARM.
v2:
- Move code to xen/arm/domctl.c.
---
xen/arch/arm/p2m.c | 16 ++++++++++
xen/arch/x86/domctl.c | 70 --------------------------------------------
xen/common/domctl.c | 70 ++++++++++++++++++++++++++++++++++++++++++++
xen/include/asm-arm/p2m.h | 12 ++++----
xen/include/asm-x86/p2m.h | 13 +-------
xen/include/xen/p2m-common.h | 16 ++++++++++
6 files changed, 108 insertions(+), 89 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 54a0afa..a215959 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -14,6 +14,10 @@
#define P2M_FIRST_ORDER 1
#define P2M_FIRST_ENTRIES (LPAE_ENTRIES<<P2M_FIRST_ORDER)
+void memory_type_changed(struct domain *d)
+{
+}
+
void dump_p2m_lookup(struct domain *d, paddr_t addr)
{
struct p2m_domain *p2m = &d->arch.p2m;
@@ -522,6 +526,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 + 1),
+ pfn_to_paddr(mfn),
+ MATTR_DEV, p2m_invalid);
+}
+
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 04c6007..1ffcaa8 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -641,76 +641,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;
- unsigned long mfn_end = mfn + nr_mfns - 1;
- unsigned long gfn_end = gfn + nr_mfns - 1;
- int add = domctl->u.memory_mapping.add_mapping;
-
- ret = -EINVAL;
- if ( mfn_end < mfn || /* wrap? */
- ((mfn | mfn_end) >> (paddr_bits - PAGE_SHIFT)) ||
- gfn_end < gfn ) /* wrap? */
- return ret;
-
- ret = -EPERM;
- if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
- break;
-
- ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, 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_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 ret:%ld\n",
- d->domain_id, gfn, mfn, ret);
- 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
- {
- int tmp_rc = 0;
-
- printk(XENLOG_G_INFO
- "memory_map: remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
- d->domain_id, gfn, mfn, nr_mfns);
-
- tmp_rc = unmap_mmio_regions(d, gfn, gfn_end, mfn);
- ret = iomem_deny_access(d, mfn, mfn_end);
- if ( !ret )
- ret = tmp_rc;
- if ( ret && is_hardware_domain(current->domain) )
- printk(XENLOG_ERR
- "memory_map: error %ld %s dom%d access to [%lx,%lx]\n",
- ret, tmp_rc ? "removing" : "denying", d->domain_id,
- mfn, mfn_end);
- }
- /* Do this unconditionally to cover errors on above failure paths. */
- memory_type_changed(d);
- }
- break;
-
case XEN_DOMCTL_ioport_mapping:
{
#define MAX_IOPORTS 0x10000
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index af3614b..c550c9f 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -822,6 +822,76 @@ 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 < mfn || /* wrap? */
+ ((mfn | mfn_end) >> (paddr_bits - PAGE_SHIFT)) ||
+ gfn_end < gfn ) /* wrap? */
+ return ret;
+
+ ret = -EPERM;
+ if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
+ break;
+
+ ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, 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_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 ret:%ld\n",
+ d->domain_id, gfn, mfn, ret);
+ 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
+ {
+ int tmp_rc = 0;
+
+ printk(XENLOG_G_INFO
+ "memory_map: remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
+ d->domain_id, gfn, mfn, nr_mfns);
+
+ tmp_rc = unmap_mmio_regions(d, gfn, gfn_end, mfn);
+ ret = iomem_deny_access(d, mfn, mfn_end);
+ if ( !ret )
+ ret = tmp_rc;
+ if ( ret && is_hardware_domain(current->domain) )
+ printk(XENLOG_ERR
+ "memory_map: error %ld %s dom%d access to [%lx,%lx]\n",
+ ret, tmp_rc ? "removing" : "denying", d->domain_id,
+ mfn, mfn_end);
+ }
+ /* Do this unconditionally to cover errors on above failure paths. */
+ memory_type_changed(d);
+ }
+ 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 f75dd44..3bc72be 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;
@@ -51,6 +54,8 @@ typedef enum {
#define p2m_is_foreign(_t) ((_t) == p2m_map_foreign)
#define p2m_is_ram(_t) ((_t) == p2m_ram_rw || (_t) == p2m_ram_ro)
+void memory_type_changed(struct domain *d);
+
/* Initialise vmid allocator */
void p2m_vmid_allocator_init(void);
@@ -84,13 +89,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, inclusive
- * of the end_gfn frame number, 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 1098600..0fee7f6 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -29,21 +29,10 @@
#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 */
-/* Map MMIO regions in the p2m: start_gfn and end_gfn is the range, inclusive
- * of the end_gfn frame number, 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);
-
extern bool_t opt_hap_1gb, opt_hap_2mb;
/*
diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
new file mode 100644
index 0000000..c1210fc
--- /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, inclusive
+ * of the end_gfn frame number, 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] 39+ messages in thread* Re: [PATCH v6 07/11] xen, common: move the memory_mapping DOMCTL hypercall to common code
2014-04-21 13:45 ` [PATCH v6 07/11] xen, common: move the memory_mapping DOMCTL hypercall to common code Arianna Avanzini
@ 2014-04-22 8:55 ` Jan Beulich
0 siblings, 0 replies; 39+ messages in thread
From: Jan Beulich @ 2014-04-22 8: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 21.04.14 at 15:45, <avanzini.arianna@gmail.com> wrote:
> + else
> + {
> + int tmp_rc = 0;
Pointless initializer. Also no apparent reason for the tmp_ prefix.
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -51,6 +54,8 @@ typedef enum {
> #define p2m_is_foreign(_t) ((_t) == p2m_map_foreign)
> #define p2m_is_ram(_t) ((_t) == p2m_ram_rw || (_t) == p2m_ram_ro)
>
> +void memory_type_changed(struct domain *d);
The function becoming common should result in its declaration to move
to a common header rather than being replicated in each arch. The
more that you move the {,un}map_mmio_regions() declarations this
way already.
Jan
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v6 08/11] tools, libxl: parse optional start gfn from the iomem config option
2014-04-21 13:44 [PATCH v6 00/11] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (6 preceding siblings ...)
2014-04-21 13:45 ` [PATCH v6 07/11] xen, common: move the memory_mapping DOMCTL hypercall to common code Arianna Avanzini
@ 2014-04-21 13:45 ` Arianna Avanzini
2014-04-22 19:57 ` Julien Grall
2014-04-28 13:20 ` Ian Campbell
2014-04-21 13:45 ` [PATCH v6 09/11] tools, libxl: add helpers to establish if guest is auto-translated Arianna Avanzini
` (2 subsequent siblings)
10 siblings, 2 replies; 39+ messages in thread
From: Arianna Avanzini @ 2014-04-21 13:45 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>
Acked-by: Ian Campbell <Ian.Campbell@eu.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: Julien Grall <julien.grall@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>
---
v6:
- Document what happens if the new "gfn" parameter is omitted
while specifying the "iomem" option in the domain configuration.
- Add LIBXL_INVALID_GFN macro with value "~(uint64_t)0".
- Fix typo in the comment to the LIBXL_HAVE_BUILDINFO_IOMEM_START_GFN
macro.
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 | 15 +++++++++++----
tools/libxl/libxl.h | 10 ++++++++++
tools/libxl/libxl_create.c | 6 ++++++
tools/libxl/libxl_internal.h | 1 +
tools/libxl/libxl_types.idl | 7 +++++--
tools/libxl/xl_cmdimpl.c | 19 ++++++++++---------
6 files changed, 43 insertions(+), 15 deletions(-)
diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index a6663b9..da6e0bc 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -602,12 +602,19 @@ 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, the mapping will be performed using B<IOMEM_START> as a 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..a78944d 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 indicates 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..6aa630e 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 == LIBXL_INVALID_GFN)
+ 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_internal.h b/tools/libxl/libxl_internal.h
index c2b73c4..2dba844 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -91,6 +91,7 @@
#define LIBXL_PV_EXTRA_MEMORY 1024
#define LIBXL_HVM_EXTRA_MEMORY 2048
#define LIBXL_MIN_DOM0_MEM (128*1024)
+#define LIBXL_INVALID_GFN (~(uint64_t)0)
/* use 0 as the domid of the toolstack domain for now */
#define LIBXL_TOOLSTACK_DOMID 0
#define QEMU_SIGNATURE "DeviceModelRecord0002"
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 0f7bbf8..f3aaa46 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 0b38b32..9a0ad4e 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] 39+ messages in thread* Re: [PATCH v6 08/11] tools, libxl: parse optional start gfn from the iomem config option
2014-04-21 13:45 ` [PATCH v6 08/11] tools, libxl: parse optional start gfn from the iomem config option Arianna Avanzini
@ 2014-04-22 19:57 ` Julien Grall
2014-04-28 13:20 ` Ian Campbell
1 sibling, 0 replies; 39+ messages in thread
From: Julien Grall @ 2014-04-22 19:57 UTC (permalink / raw)
To: Arianna Avanzini, xen-devel
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, Ian.Campbell, etrudeau, JBeulich,
viktor.kleinik
Hi Arianna,
On 21/04/14 14:45, Arianna Avanzini wrote:
> + 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);
> }
> }
> }
>
> -
> -
Spurious change here.
> if (!xlu_cfg_get_list (config, "disk", &vbds, 0, 0)) {
> d_config->num_disks = 0;
> d_config->disks = NULL;
>
Other than the minor change, FWIW:
Acked-by: Julien Grall <julien.grall@linaro.org>
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v6 08/11] tools, libxl: parse optional start gfn from the iomem config option
2014-04-21 13:45 ` [PATCH v6 08/11] tools, libxl: parse optional start gfn from the iomem config option Arianna Avanzini
2014-04-22 19:57 ` Julien Grall
@ 2014-04-28 13:20 ` Ian Campbell
2014-05-01 17:29 ` Julien Grall
1 sibling, 1 reply; 39+ messages in thread
From: Ian Campbell @ 2014-04-28 13:20 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-21 at 15:45 +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>
> Acked-by: Ian Campbell <Ian.Campbell@eu.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: Julien Grall <julien.grall@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>
>
> ---
>
> v6:
> - Document what happens if the new "gfn" parameter is omitted
> while specifying the "iomem" option in the domain configuration.
> - Add LIBXL_INVALID_GFN macro with value "~(uint64_t)0".
> - Fix typo in the comment to the LIBXL_HAVE_BUILDINFO_IOMEM_START_GFN
> macro.
>
> 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 | 15 +++++++++++----
> tools/libxl/libxl.h | 10 ++++++++++
> tools/libxl/libxl_create.c | 6 ++++++
> tools/libxl/libxl_internal.h | 1 +
> tools/libxl/libxl_types.idl | 7 +++++--
> tools/libxl/xl_cmdimpl.c | 19 ++++++++++---------
> 6 files changed, 43 insertions(+), 15 deletions(-)
>
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index a6663b9..da6e0bc 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -602,12 +602,19 @@ 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, the mapping will be performed using B<IOMEM_START> as a start in the
> +domU's address space.
Please can you make some allusion to this being a 1:1 mapping in the
final sentence.
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index c2b73c4..2dba844 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -91,6 +91,7 @@
> #define LIBXL_PV_EXTRA_MEMORY 1024
> #define LIBXL_HVM_EXTRA_MEMORY 2048
> #define LIBXL_MIN_DOM0_MEM (128*1024)
> +#define LIBXL_INVALID_GFN (~(uint64_t)0)
> /* use 0 as the domid of the toolstack domain for now */
> #define LIBXL_TOOLSTACK_DOMID 0
> #define QEMU_SIGNATURE "DeviceModelRecord0002"
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 0f7bbf8..f3aaa46 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
ITYM to use LIBXL_INVALID_GFN here.
Ian.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v6 08/11] tools, libxl: parse optional start gfn from the iomem config option
2014-04-28 13:20 ` Ian Campbell
@ 2014-05-01 17:29 ` Julien Grall
0 siblings, 0 replies; 39+ messages in thread
From: Julien Grall @ 2014-05-01 17:29 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,
On 04/28/2014 02:20 PM, Ian Campbell wrote:
>> -=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, the mapping will be performed using B<IOMEM_START> as a start in the
>> +domU's address space.
>
> Please can you make some allusion to this being a 1:1 mapping in the
> final sentence.
I would also add smth to say this is for auto-translated domain. It's
not clear in the paragraph.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v6 09/11] tools, libxl: add helpers to establish if guest is auto-translated
2014-04-21 13:44 [PATCH v6 00/11] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (7 preceding siblings ...)
2014-04-21 13:45 ` [PATCH v6 08/11] tools, libxl: parse optional start gfn from the iomem config option Arianna Avanzini
@ 2014-04-21 13:45 ` Arianna Avanzini
2014-04-28 13:22 ` Ian Campbell
2014-04-21 13:45 ` [PATCH v6 10/11] tools, libxl: handle the iomem parameter with the memory_mapping hcall Arianna Avanzini
2014-04-21 13:45 ` [PATCH v6 11/11] xen, common: do not implicitly permit access to mapped I/O memory Arianna Avanzini
10 siblings, 1 reply; 39+ messages in thread
From: Arianna Avanzini @ 2014-04-21 13:45 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 | 4 ++++
tools/libxl/libxl_arm.c | 5 +++++
tools/libxl/libxl_x86.c | 6 ++++++
3 files changed, 15 insertions(+)
diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
index d3bc136..d30112c 100644
--- a/tools/libxl/libxl_arch.h
+++ b/tools/libxl/libxl_arch.h
@@ -27,4 +27,8 @@ int libxl__arch_domain_init_hw_description(libxl__gc *gc,
int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
libxl_domain_build_info *info,
struct xc_dom_image *dom);
+
+/* establish whether domain has an auto-translated physmap */
+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 4f0f0e2..c787589 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -12,6 +12,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 7589060..b4162b8 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -324,3 +324,9 @@ int libxl__arch_domain_finalise_hw_description(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] 39+ messages in thread* Re: [PATCH v6 09/11] tools, libxl: add helpers to establish if guest is auto-translated
2014-04-21 13:45 ` [PATCH v6 09/11] tools, libxl: add helpers to establish if guest is auto-translated Arianna Avanzini
@ 2014-04-28 13:22 ` Ian Campbell
0 siblings, 0 replies; 39+ messages in thread
From: Ian Campbell @ 2014-04-28 13:22 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-21 at 15:45 +0200, Arianna Avanzini wrote:
> 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.
Didn't we end up with a plan which didn't involve exposing this to
libxl?
Ian.
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v6 10/11] tools, libxl: handle the iomem parameter with the memory_mapping hcall
2014-04-21 13:44 [PATCH v6 00/11] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (8 preceding siblings ...)
2014-04-21 13:45 ` [PATCH v6 09/11] tools, libxl: add helpers to establish if guest is auto-translated Arianna Avanzini
@ 2014-04-21 13:45 ` Arianna Avanzini
2014-04-22 20:03 ` Julien Grall
2014-04-28 13:24 ` Ian Campbell
2014-04-21 13:45 ` [PATCH v6 11/11] xen, common: do not implicitly permit access to mapped I/O memory Arianna Avanzini
10 siblings, 2 replies; 39+ messages in thread
From: Arianna Avanzini @ 2014-04-21 13:45 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 6aa630e..cdf03cd 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] 39+ messages in thread* Re: [PATCH v6 10/11] tools, libxl: handle the iomem parameter with the memory_mapping hcall
2014-04-21 13:45 ` [PATCH v6 10/11] tools, libxl: handle the iomem parameter with the memory_mapping hcall Arianna Avanzini
@ 2014-04-22 20:03 ` Julien Grall
2014-04-28 13:25 ` Ian Campbell
2014-04-28 13:24 ` Ian Campbell
1 sibling, 1 reply; 39+ messages in thread
From: Julien Grall @ 2014-04-22 20:03 UTC (permalink / raw)
To: Arianna Avanzini, xen-devel
Cc: julien.grall, paolo.valente, keir, stefano.stabellini, tim,
dario.faggioli, Ian.Jackson, Ian.Campbell, etrudeau, JBeulich,
viktor.kleinik
Hi Arianna,
On 21/04/14 14:45, Arianna Avanzini wrote:
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 6aa630e..cdf03cd 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;
I know I've asked you to create the small helper
"libxl__arch_auto_translated_physmap" in libxl. I'm wondering if we can
replace xc_domain_iomem_permission by xc_domain_memory_mapping.
The latter function won't do anything for PV. It will also avoid to call
2 hypercalls.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v6 10/11] tools, libxl: handle the iomem parameter with the memory_mapping hcall
2014-04-22 20:03 ` Julien Grall
@ 2014-04-28 13:25 ` Ian Campbell
2014-04-28 13:32 ` Julien Grall
0 siblings, 1 reply; 39+ messages in thread
From: Ian Campbell @ 2014-04-28 13:25 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 Tue, 2014-04-22 at 21:03 +0100, Julien Grall wrote:
> Hi Arianna,
>
> On 21/04/14 14:45, Arianna Avanzini wrote:
> > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> > index 6aa630e..cdf03cd 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;
>
> I know I've asked you to create the small helper
> "libxl__arch_auto_translated_physmap" in libxl. I'm wondering if we can
> replace xc_domain_iomem_permission by xc_domain_memory_mapping.
The path we've decided to go down is to separate the granting/revoking
of permissions from the establishment of mappings, isn't it?
Doing as you suggest would be contrary to that plan I think.
Ian.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v6 10/11] tools, libxl: handle the iomem parameter with the memory_mapping hcall
2014-04-28 13:25 ` Ian Campbell
@ 2014-04-28 13:32 ` Julien Grall
0 siblings, 0 replies; 39+ messages in thread
From: Julien Grall @ 2014-04-28 13:32 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 04/28/2014 02:25 PM, Ian Campbell wrote:
> On Tue, 2014-04-22 at 21:03 +0100, Julien Grall wrote:
>> Hi Arianna,
>>
>> On 21/04/14 14:45, Arianna Avanzini wrote:
>>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>>> index 6aa630e..cdf03cd 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;
>>
>> I know I've asked you to create the small helper
>> "libxl__arch_auto_translated_physmap" in libxl. I'm wondering if we can
>> replace xc_domain_iomem_permission by xc_domain_memory_mapping.
>
> The path we've decided to go down is to separate the granting/revoking
> of permissions from the establishment of mappings, isn't it?
>
> Doing as you suggest would be contrary to that plan I think.
Hmmm right. Sorry, I start to get lost with all the mail.
In another case, as the {,un}map code do nothing for translated domain
(return 0). Shall we let the toolstack call this function unconditionally?
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v6 10/11] tools, libxl: handle the iomem parameter with the memory_mapping hcall
2014-04-21 13:45 ` [PATCH v6 10/11] tools, libxl: handle the iomem parameter with the memory_mapping hcall Arianna Avanzini
2014-04-22 20:03 ` Julien Grall
@ 2014-04-28 13:24 ` Ian Campbell
1 sibling, 0 replies; 39+ messages in thread
From: Ian Campbell @ 2014-04-28 13:24 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-21 at 15:45 +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.
>
> 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 6aa630e..cdf03cd 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;
Maybe this could be part of xc_domain_memory_mapping and avoid the need
for libxl to know about these things?
> + 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;
> }
> }
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v6 11/11] xen, common: do not implicitly permit access to mapped I/O memory
2014-04-21 13:44 [PATCH v6 00/11] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM Arianna Avanzini
` (9 preceding siblings ...)
2014-04-21 13:45 ` [PATCH v6 10/11] tools, libxl: handle the iomem parameter with the memory_mapping hcall Arianna Avanzini
@ 2014-04-21 13:45 ` Arianna Avanzini
2014-04-22 9:12 ` Jan Beulich
2014-04-28 13:28 ` Ian Campbell
10 siblings, 2 replies; 39+ messages in thread
From: Arianna Avanzini @ 2014-04-21 13:45 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 XEN_DOMCTL_memory_mapping hypercall implicitly grants
to a domain access permission to the I/O memory areas mapped in its
guest address space. This conflicts with the presence of a specific
hypercall (XEN_DOMCTL_iomem_permission) used to grant such a permission
to a domain.
This commit attempts to separate the functions of the two hypercalls by
having only the latter be able to permit I/O memory access to a domain,
and the former just performing the mapping after a permissions check.
This commit also attempts to change existing code to be sure to grant
access permission to PCI-related I/O memory ranges (for passthrough of
PCI devices specified in the domain's configuration) and to VGA-related
memory ranges (for VGA passthrough, if gfx_passthru = 1 in the domain
configuration).
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_create.c | 17 +++++++++++++++++
tools/libxl/libxl_pci.c | 24 ++++++++++++++++++------
xen/common/domctl.c | 37 ++++++++++---------------------------
3 files changed, 45 insertions(+), 33 deletions(-)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index cdf03cd..924c2e9 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1151,6 +1151,23 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
libxl__spawn_stub_dm(egc, &dcs->dmss);
else
libxl__spawn_local_dm(egc, &dcs->dmss.dm);
+
+ /*
+ * If VGA passthru is enabled by domain config, be sure that the
+ * domain can access VGA-related iomem regions.
+ */
+ if (d_config->b_info.u.hvm.gfx_passthru.val) {
+ uint64_t vga_iomem_start = 0xa0000 >> XC_PAGE_SHIFT;
+ ret = xc_domain_iomem_permission(CTX->xch, domid,
+ vga_iomem_start, 0x20, 1);
+ if (ret < 0) {
+ LOGE(ERROR,
+ "failed to give dom%d access to iomem range "
+ "%"PRIx64"-%"PRIx64" for VGA passthru",
+ domid, vga_iomem_start, (vga_iomem_start + 0x20 - 1));
+ goto error_out;
+ }
+ }
return;
}
case LIBXL_DOMAIN_TYPE_PV:
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 44d0453..33dc3ce 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -867,7 +867,10 @@ static int do_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, i
}
if ( rc )
return ERROR_FAIL;
- break;
+ /*
+ * fall through and let I/O memory permissions for PCI devices be given
+ * also to HVM domains
+ */
case LIBXL_DOMAIN_TYPE_PV:
{
char *sysfs_path = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/resource", pcidev->domain,
@@ -886,14 +889,14 @@ static int do_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, i
continue;
size = end - start + 1;
if (start) {
- if (flags & PCI_BAR_IO) {
+ if ((flags & PCI_BAR_IO) && !hvm) {
rc = xc_domain_ioport_permission(ctx->xch, domid, start, size, 1);
if (rc < 0) {
LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Error: xc_domain_ioport_permission error 0x%llx/0x%llx", start, size);
fclose(f);
return ERROR_FAIL;
}
- } else {
+ } else if (!(flags & PCI_BAR_IO)) {
rc = xc_domain_iomem_permission(ctx->xch, domid, start>>XC_PAGE_SHIFT,
(size+(XC_PAGE_SIZE-1))>>XC_PAGE_SHIFT, 1);
if (rc < 0) {
@@ -905,6 +908,9 @@ static int do_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, i
}
}
fclose(f);
+ if (hvm)
+ break;
+
sysfs_path = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/irq", pcidev->domain,
pcidev->bus, pcidev->dev, pcidev->func);
f = fopen(sysfs_path, "r");
@@ -1203,7 +1209,10 @@ static int do_pci_remove(libxl__gc *gc, uint32_t domid,
rc = ERROR_FAIL;
goto out_fail;
}
- break;
+ /*
+ * fall through and let I/O memory permissions for PCI devices be given
+ * also to HVM domains
+ */
case LIBXL_DOMAIN_TYPE_PV:
{
char *sysfs_path = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/resource", pcidev->domain,
@@ -1222,11 +1231,11 @@ static int do_pci_remove(libxl__gc *gc, uint32_t domid,
continue;
size = end - start + 1;
if (start) {
- if (flags & PCI_BAR_IO) {
+ if ((flags & PCI_BAR_IO) && !hvm) {
rc = xc_domain_ioport_permission(ctx->xch, domid, start, size, 0);
if (rc < 0)
LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "xc_domain_ioport_permission error 0x%x/0x%x", start, size);
- } else {
+ } else if (!(flags & PCI_BAR_IO)) {
rc = xc_domain_iomem_permission(ctx->xch, domid, start>>XC_PAGE_SHIFT,
(size+(XC_PAGE_SIZE-1))>>XC_PAGE_SHIFT, 0);
if (rc < 0)
@@ -1236,6 +1245,9 @@ static int do_pci_remove(libxl__gc *gc, uint32_t domid,
}
fclose(f);
skip1:
+ if (hvm)
+ break;
+
sysfs_path = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/irq", pcidev->domain,
pcidev->bus, pcidev->dev, pcidev->func);
f = fopen(sysfs_path, "r");
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index c550c9f..18b975d 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -838,7 +838,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
return ret;
ret = -EPERM;
- if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
+ if ( !iomem_access_permitted(current->domain, mfn, mfn_end) ||
+ !iomem_access_permitted(d, mfn, mfn_end) )
break;
ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, add);
@@ -851,41 +852,23 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
"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 ret:%ld\n",
- d->domain_id, gfn, mfn, ret);
- 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);
- }
- }
+ ret = map_mmio_regions(d, gfn, gfn_end, mfn);
+ if ( ret )
+ printk(XENLOG_G_WARNING
+ "memory_map: fail: dom%d gfn=%lx mfn=%lx ret:%ld\n",
+ d->domain_id, gfn, mfn, ret);
}
else
{
- int tmp_rc = 0;
-
printk(XENLOG_G_INFO
"memory_map: remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
d->domain_id, gfn, mfn, nr_mfns);
- tmp_rc = unmap_mmio_regions(d, gfn, gfn_end, mfn);
- ret = iomem_deny_access(d, mfn, mfn_end);
- if ( !ret )
- ret = tmp_rc;
+ ret = unmap_mmio_regions(d, gfn, gfn_end, mfn);
if ( ret && is_hardware_domain(current->domain) )
printk(XENLOG_ERR
- "memory_map: error %ld %s dom%d access to [%lx,%lx]\n",
- ret, tmp_rc ? "removing" : "denying", d->domain_id,
- mfn, mfn_end);
+ "memory_map: error %ld removing dom%d mapping "
+ "to [%lx,%lx]\n", ret, d->domain_id, mfn, mfn_end);
}
/* Do this unconditionally to cover errors on above failure paths. */
memory_type_changed(d);
--
1.9.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v6 11/11] xen, common: do not implicitly permit access to mapped I/O memory
2014-04-21 13:45 ` [PATCH v6 11/11] xen, common: do not implicitly permit access to mapped I/O memory Arianna Avanzini
@ 2014-04-22 9:12 ` Jan Beulich
2014-04-28 13:33 ` Ian Campbell
2014-04-28 13:28 ` Ian Campbell
1 sibling, 1 reply; 39+ messages in thread
From: Jan Beulich @ 2014-04-22 9:12 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 21.04.14 at 15:45, <avanzini.arianna@gmail.com> wrote:
> tools/libxl/libxl_create.c | 17 +++++++++++++++++
> tools/libxl/libxl_pci.c | 24 ++++++++++++++++++------
I doubt that doing it at this layer is correct: There are quite a few uses
of xc_domain_memory_mapping() in qemu, so I think as a first step
you should retain libxc functionality unchanged by issuing both domctls
there. If that ever needs further separation, that would then be a
second patch.
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -1151,6 +1151,23 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
> libxl__spawn_stub_dm(egc, &dcs->dmss);
> else
> libxl__spawn_local_dm(egc, &dcs->dmss.dm);
> +
> + /*
> + * If VGA passthru is enabled by domain config, be sure that the
> + * domain can access VGA-related iomem regions.
> + */
> + if (d_config->b_info.u.hvm.gfx_passthru.val) {
> + uint64_t vga_iomem_start = 0xa0000 >> XC_PAGE_SHIFT;
> + ret = xc_domain_iomem_permission(CTX->xch, domid,
> + vga_iomem_start, 0x20, 1);
> + if (ret < 0) {
> + LOGE(ERROR,
> + "failed to give dom%d access to iomem range "
> + "%"PRIx64"-%"PRIx64" for VGA passthru",
> + domid, vga_iomem_start, (vga_iomem_start + 0x20 - 1));
> + goto error_out;
> + }
> + }
Considering that this code isn't being moved here - what's the
rationale for the sudden need to special case VGA pass-through
here?
> @@ -886,14 +889,14 @@ static int do_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, i
> continue;
> size = end - start + 1;
> if (start) {
> - if (flags & PCI_BAR_IO) {
> + if ((flags & PCI_BAR_IO) && !hvm) {
> rc = xc_domain_ioport_permission(ctx->xch, domid, start, size, 1);
If this whole thing gets unified, treating I/O ports and MMIO differently
isn't an optimal choice.
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -838,7 +838,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> return ret;
>
> ret = -EPERM;
> - if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
> + if ( !iomem_access_permitted(current->domain, mfn, mfn_end) ||
> + !iomem_access_permitted(d, mfn, mfn_end) )
The former check should now be pointless here, and ought to be done
in the XEN_DOMCTL_iomem_permission handler instead (in fact it
should always have got done there, and that would be a security issue
if there wasn't the waiver put in place by XSA-77).
Jan
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v6 11/11] xen, common: do not implicitly permit access to mapped I/O memory
2014-04-22 9:12 ` Jan Beulich
@ 2014-04-28 13:33 ` Ian Campbell
2014-04-28 13:54 ` Jan Beulich
0 siblings, 1 reply; 39+ messages in thread
From: Ian Campbell @ 2014-04-28 13:33 UTC (permalink / raw)
To: Jan Beulich
Cc: paolo.valente, keir, stefano.stabellini, tim, dario.faggioli,
Ian.Jackson, xen-devel, julien.grall, etrudeau, Arianna Avanzini,
viktor.kleinik
On Tue, 2014-04-22 at 10:12 +0100, Jan Beulich wrote:
> >>> On 21.04.14 at 15:45, <avanzini.arianna@gmail.com> wrote:
> > tools/libxl/libxl_create.c | 17 +++++++++++++++++
> > tools/libxl/libxl_pci.c | 24 ++++++++++++++++++------
>
> I doubt that doing it at this layer is correct: There are quite a few uses
> of xc_domain_memory_mapping() in qemu,
Here the toolstack grants permission to access the I/O mem etc
associated with a PCI device. Then later qemu uses it via the mapping.
> so I think as a first step you should retain libxc functionality
> unchanged by issuing both domctls there.
This doesn't fit very well with the use of non-PCI passthrough of
devices without qemu on ARM -- i.e. the direct iomem=[list] stuff which
Arianna is trying to make work with this series. Or does it?
> If that ever needs further separation, that would then be a
> second patch.
>
> > --- a/tools/libxl/libxl_create.c
> > +++ b/tools/libxl/libxl_create.c
> > @@ -1151,6 +1151,23 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
> > libxl__spawn_stub_dm(egc, &dcs->dmss);
> > else
> > libxl__spawn_local_dm(egc, &dcs->dmss.dm);
> > +
> > + /*
> > + * If VGA passthru is enabled by domain config, be sure that the
> > + * domain can access VGA-related iomem regions.
> > + */
> > + if (d_config->b_info.u.hvm.gfx_passthru.val) {
> > + uint64_t vga_iomem_start = 0xa0000 >> XC_PAGE_SHIFT;
> > + ret = xc_domain_iomem_permission(CTX->xch, domid,
> > + vga_iomem_start, 0x20, 1);
> > + if (ret < 0) {
> > + LOGE(ERROR,
> > + "failed to give dom%d access to iomem range "
> > + "%"PRIx64"-%"PRIx64" for VGA passthru",
> > + domid, vga_iomem_start, (vga_iomem_start + 0x20 - 1));
> > + goto error_out;
> > + }
> > + }
>
> Considering that this code isn't being moved here - what's the
> rationale for the sudden need to special case VGA pass-through
> here?
I suppose it is because VGA has these "magic" regions, which used to be
implicitly granted permissions via xc_domain_memory_mapping and now
aren't, so the toolstack has to grant them explicitly (the goal of the
patch generally).
> > @@ -886,14 +889,14 @@ static int do_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, i
> > continue;
> > size = end - start + 1;
> > if (start) {
> > - if (flags & PCI_BAR_IO) {
> > + if ((flags & PCI_BAR_IO) && !hvm) {
> > rc = xc_domain_ioport_permission(ctx->xch, domid, start, size, 1);
>
> If this whole thing gets unified, treating I/O ports and MMIO differently
> isn't an optimal choice.
Where are these granted for x86 HVM guests today -- I don't see qemu
calling this function. I must be missing something.
Ian.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v6 11/11] xen, common: do not implicitly permit access to mapped I/O memory
2014-04-28 13:33 ` Ian Campbell
@ 2014-04-28 13:54 ` Jan Beulich
2014-04-28 14:20 ` Ian Campbell
0 siblings, 1 reply; 39+ messages in thread
From: Jan Beulich @ 2014-04-28 13:54 UTC (permalink / raw)
To: Ian Campbell
Cc: paolo.valente, keir, stefano.stabellini, tim, dario.faggioli,
Ian.Jackson, xen-devel, julien.grall, etrudeau, Arianna Avanzini,
viktor.kleinik
>>> On 28.04.14 at 15:33, <Ian.Campbell@eu.citrix.com> wrote:
> On Tue, 2014-04-22 at 10:12 +0100, Jan Beulich wrote:
>> >>> On 21.04.14 at 15:45, <avanzini.arianna@gmail.com> wrote:
>> > tools/libxl/libxl_create.c | 17 +++++++++++++++++
>> > tools/libxl/libxl_pci.c | 24 ++++++++++++++++++------
>>
>> I doubt that doing it at this layer is correct: There are quite a few uses
>> of xc_domain_memory_mapping() in qemu,
>
> Here the toolstack grants permission to access the I/O mem etc
> associated with a PCI device. Then later qemu uses it via the mapping.
Am I reading this right to mean that qemu doesn't really care about
the current side effect of granting permissions? In that case the
change would seem to be okay.
>> so I think as a first step you should retain libxc functionality
>> unchanged by issuing both domctls there.
>
> This doesn't fit very well with the use of non-PCI passthrough of
> devices without qemu on ARM -- i.e. the direct iomem=[list] stuff which
> Arianna is trying to make work with this series. Or does it?
Why would it not? How the resources got specified (iomem= or
pci=) shouldn't matter here.
>> > --- a/tools/libxl/libxl_create.c
>> > +++ b/tools/libxl/libxl_create.c
>> > @@ -1151,6 +1151,23 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
>> > libxl__spawn_stub_dm(egc, &dcs->dmss);
>> > else
>> > libxl__spawn_local_dm(egc, &dcs->dmss.dm);
>> > +
>> > + /*
>> > + * If VGA passthru is enabled by domain config, be sure that the
>> > + * domain can access VGA-related iomem regions.
>> > + */
>> > + if (d_config->b_info.u.hvm.gfx_passthru.val) {
>> > + uint64_t vga_iomem_start = 0xa0000 >> XC_PAGE_SHIFT;
>> > + ret = xc_domain_iomem_permission(CTX->xch, domid,
>> > + vga_iomem_start, 0x20, 1);
>> > + if (ret < 0) {
>> > + LOGE(ERROR,
>> > + "failed to give dom%d access to iomem range "
>> > + "%"PRIx64"-%"PRIx64" for VGA passthru",
>> > + domid, vga_iomem_start, (vga_iomem_start + 0x20 - 1));
>> > + goto error_out;
>> > + }
>> > + }
>>
>> Considering that this code isn't being moved here - what's the
>> rationale for the sudden need to special case VGA pass-through
>> here?
>
> I suppose it is because VGA has these "magic" regions, which used to be
> implicitly granted permissions via xc_domain_memory_mapping and now
> aren't, so the toolstack has to grant them explicitly (the goal of the
> patch generally).
If there was anything implicit previously, pointing out where this
happens (perhaps in the commit message, so others can understand
it in the future too) would basically address that question.
>> > @@ -886,14 +889,14 @@ static int do_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, i
>> > continue;
>> > size = end - start + 1;
>> > if (start) {
>> > - if (flags & PCI_BAR_IO) {
>> > + if ((flags & PCI_BAR_IO) && !hvm) {
>> > rc = xc_domain_ioport_permission(ctx->xch, domid, start, size, 1);
>>
>> If this whole thing gets unified, treating I/O ports and MMIO differently
>> isn't an optimal choice.
>
> Where are these granted for x86 HVM guests today -- I don't see qemu
> calling this function. I must be missing something.
Right here I thought - what the patch does is _exclude_ HVM from
being given the permission here (in favor of doing it elsewhere), and
all I was trying to understand was whether we really need to go
down the same route as in xend again where for everything there
are two places where things get done.
Jan
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v6 11/11] xen, common: do not implicitly permit access to mapped I/O memory
2014-04-28 13:54 ` Jan Beulich
@ 2014-04-28 14:20 ` Ian Campbell
0 siblings, 0 replies; 39+ messages in thread
From: Ian Campbell @ 2014-04-28 14:20 UTC (permalink / raw)
To: Jan Beulich
Cc: paolo.valente, keir, stefano.stabellini, tim, dario.faggioli,
Ian.Jackson, xen-devel, julien.grall, etrudeau, Arianna Avanzini,
viktor.kleinik
On Mon, 2014-04-28 at 14:54 +0100, Jan Beulich wrote:
> >>> On 28.04.14 at 15:33, <Ian.Campbell@eu.citrix.com> wrote:
> > On Tue, 2014-04-22 at 10:12 +0100, Jan Beulich wrote:
> >> >>> On 21.04.14 at 15:45, <avanzini.arianna@gmail.com> wrote:
> >> > tools/libxl/libxl_create.c | 17 +++++++++++++++++
> >> > tools/libxl/libxl_pci.c | 24 ++++++++++++++++++------
> >>
> >> I doubt that doing it at this layer is correct: There are quite a few uses
> >> of xc_domain_memory_mapping() in qemu,
> >
> > Here the toolstack grants permission to access the I/O mem etc
> > associated with a PCI device. Then later qemu uses it via the mapping.
>
> Am I reading this right to mean that qemu doesn't really care about
> the current side effect of granting permissions?
I don't think so, it just cares that it happened at some point.
> In that case the
> change would seem to be okay.
>
> >> so I think as a first step you should retain libxc functionality
> >> unchanged by issuing both domctls there.
> >
> > This doesn't fit very well with the use of non-PCI passthrough of
> > devices without qemu on ARM -- i.e. the direct iomem=[list] stuff which
> > Arianna is trying to make work with this series. Or does it?
>
> Why would it not? How the resources got specified (iomem= or
> pci=) shouldn't matter here.
One path goes via qemu calling xc_domain_memory_map and the other via
libx[cl] having to do it, which is inconsistent and a bit confusing.
Having qemu continue to rely on just xc_domain_memory_map (which now
does both things) for pci= means we can't easily use
xc_domain_memory_map for the iomem= case (which wants only one bit of
functionality, having already called granted the perms explicitly)
> >> > @@ -886,14 +889,14 @@ static int do_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, i
> >> > continue;
> >> > size = end - start + 1;
> >> > if (start) {
> >> > - if (flags & PCI_BAR_IO) {
> >> > + if ((flags & PCI_BAR_IO) && !hvm) {
> >> > rc = xc_domain_ioport_permission(ctx->xch, domid, start, size, 1);
> >>
> >> If this whole thing gets unified, treating I/O ports and MMIO differently
> >> isn't an optimal choice.
> >
> > Where are these granted for x86 HVM guests today -- I don't see qemu
> > calling this function. I must be missing something.
>
> Right here I thought - what the patch does is _exclude_ HVM from
> being given the permission here (in favor of doing it elsewhere), and
> all I was trying to understand was whether we really need to go
> down the same route as in xend again where for everything there
> are two places where things get done.
Yes. First thing would be to understand where this does happen if not
here or in qemu.
Ian.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v6 11/11] xen, common: do not implicitly permit access to mapped I/O memory
2014-04-21 13:45 ` [PATCH v6 11/11] xen, common: do not implicitly permit access to mapped I/O memory Arianna Avanzini
2014-04-22 9:12 ` Jan Beulich
@ 2014-04-28 13:28 ` Ian Campbell
1 sibling, 0 replies; 39+ messages in thread
From: Ian Campbell @ 2014-04-28 13:28 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-21 at 15:45 +0200, Arianna Avanzini wrote:
> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index 44d0453..33dc3ce 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -867,7 +867,10 @@ static int do_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, i
> }
> if ( rc )
> return ERROR_FAIL;
> - break;
> + /*
> + * fall through and let I/O memory permissions for PCI devices be given
> + * also to HVM domains
> + */
With this in place I'm not sure the use of a switch is really the right
structure any more. How about:
if (it's an HVM guest)
{
device model related stuff
}
the remaining common setup (what used to be PV specific)
? Same on remove.
Ian.
^ permalink raw reply [flat|nested] 39+ messages in thread