| Message ID | 20170427143546.14662-4-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 27.04.17 at 16:35, <roger.pau@citrix.com> wrote: > And also allow it to do non-identity mappings by adding a new parameter. This > function will be needed in other parts apart from PVH Dom0 build. While there > fix the function to use gfn_t and mfn_t instead of unsigned long for memory > addresses. I'm afraid both title and description don't (or no longer) properly reflect what the patch does. I'm also afraid the reason the new parameter as well as the placement in common/memory.c aren't sufficiently explained. For example, what use is the function going to be without CONFIG_HAS_PCI? > --- a/xen/arch/x86/hvm/dom0_build.c > +++ b/xen/arch/x86/hvm/dom0_build.c > @@ -64,27 +64,7 @@ static struct acpi_madt_nmi_source __initdata *nmisrc; > static int __init modify_identity_mmio(struct domain *d, unsigned long pfn, > unsigned long nr_pages, const bool map) > { > - int rc; > - > - for ( ; ; ) > - { > - rc = (map ? map_mmio_regions : unmap_mmio_regions) > - (d, _gfn(pfn), nr_pages, _mfn(pfn)); > - if ( rc == 0 ) > - break; > - if ( rc < 0 ) > - { > - printk(XENLOG_WARNING > - "Failed to identity %smap [%#lx,%#lx) for d%d: %d\n", > - map ? "" : "un", pfn, pfn + nr_pages, d->domain_id, rc); > - break; > - } > - nr_pages -= rc; > - pfn += rc; > - process_pending_softirqs(); > - } > - > - return rc; > + return modify_mmio(d, _gfn(pfn), _mfn(pfn), nr_pages, map); > } I don't see the value of retaining this wrapper. > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -1438,6 +1438,42 @@ int prepare_ring_for_helper( > return 0; > } > > +int modify_mmio(struct domain *d, gfn_t gfn, mfn_t mfn, unsigned long nr_pages, > + const bool map) > +{ > + int rc; > + > + /* > + * Make sure this function is only used by the hardware domain, because it > + * can take an arbitrary long time, and could DoS the whole system. > + */ > + ASSERT(is_hardware_domain(d)); If that can happen arbitrarily at run time (rather than just at boot, as suggested by the removal of __init), it definitely can't remain as is and will instead need to make use of continuations. I'm therefore unconvinced you really want to move this code instead of simply calling {,un}map_mmio_regions() while taking care of preemption needs. Jan
On Fri, May 19, 2017 at 07:35:39AM -0600, Jan Beulich wrote: > >>> On 27.04.17 at 16:35, <roger.pau@citrix.com> wrote: > > And also allow it to do non-identity mappings by adding a new parameter. This > > function will be needed in other parts apart from PVH Dom0 build. While there > > fix the function to use gfn_t and mfn_t instead of unsigned long for memory > > addresses. > > I'm afraid both title and description don't (or no longer) properly reflect > what the patch does. I'm also afraid the reason the new parameter as > well as the placement in common/memory.c aren't sufficiently explained. > For example, what use is the function going to be without > CONFIG_HAS_PCI? It will still be needed in order to map the low 1MB for a PVH Dom0, but anyway, see below. > > --- a/xen/arch/x86/hvm/dom0_build.c > > +++ b/xen/arch/x86/hvm/dom0_build.c > > @@ -64,27 +64,7 @@ static struct acpi_madt_nmi_source __initdata *nmisrc; > > static int __init modify_identity_mmio(struct domain *d, unsigned long pfn, > > unsigned long nr_pages, const bool map) > > { > > - int rc; > > - > > - for ( ; ; ) > > - { > > - rc = (map ? map_mmio_regions : unmap_mmio_regions) > > - (d, _gfn(pfn), nr_pages, _mfn(pfn)); > > - if ( rc == 0 ) > > - break; > > - if ( rc < 0 ) > > - { > > - printk(XENLOG_WARNING > > - "Failed to identity %smap [%#lx,%#lx) for d%d: %d\n", > > - map ? "" : "un", pfn, pfn + nr_pages, d->domain_id, rc); > > - break; > > - } > > - nr_pages -= rc; > > - pfn += rc; > > - process_pending_softirqs(); > > - } > > - > > - return rc; > > + return modify_mmio(d, _gfn(pfn), _mfn(pfn), nr_pages, map); > > } > > I don't see the value of retaining this wrapper. > > > --- a/xen/common/memory.c > > +++ b/xen/common/memory.c > > @@ -1438,6 +1438,42 @@ int prepare_ring_for_helper( > > return 0; > > } > > > > +int modify_mmio(struct domain *d, gfn_t gfn, mfn_t mfn, unsigned long nr_pages, > > + const bool map) > > +{ > > + int rc; > > + > > + /* > > + * Make sure this function is only used by the hardware domain, because it > > + * can take an arbitrary long time, and could DoS the whole system. > > + */ > > + ASSERT(is_hardware_domain(d)); > > If that can happen arbitrarily at run time (rather than just at boot, > as suggested by the removal of __init), it definitely can't remain as > is and will instead need to make use of continuations. I'm therefore > unconvinced you really want to move this code instead of simply > calling {,un}map_mmio_regions() while taking care of preemption > needs. I'm not sure I know how to use continuations with non-hypercall vmexits. Do you have any recommendations about how to do this? pause the domain and run the mmio changes inside of a tasklet? Roger.
>>> On 21.06.17 at 13:11, <roger.pau@citrix.com> wrote: > On Fri, May 19, 2017 at 07:35:39AM -0600, Jan Beulich wrote: >> >>> On 27.04.17 at 16:35, <roger.pau@citrix.com> wrote: >> > And also allow it to do non-identity mappings by adding a new parameter. This >> > function will be needed in other parts apart from PVH Dom0 build. While there >> > fix the function to use gfn_t and mfn_t instead of unsigned long for memory >> > addresses. >> >> I'm afraid both title and description don't (or no longer) properly reflect >> what the patch does. I'm also afraid the reason the new parameter as >> well as the placement in common/memory.c aren't sufficiently explained. >> For example, what use is the function going to be without >> CONFIG_HAS_PCI? > > It will still be needed in order to map the low 1MB for a PVH Dom0, > but anyway, see below. That's still implying CONFIG_HAS_PCI, as that's still x86. The question was with ARM in mind. >> > --- a/xen/common/memory.c >> > +++ b/xen/common/memory.c >> > @@ -1438,6 +1438,42 @@ int prepare_ring_for_helper( >> > return 0; >> > } >> > >> > +int modify_mmio(struct domain *d, gfn_t gfn, mfn_t mfn, unsigned long nr_pages, >> > + const bool map) >> > +{ >> > + int rc; >> > + >> > + /* >> > + * Make sure this function is only used by the hardware domain, because it >> > + * can take an arbitrary long time, and could DoS the whole system. >> > + */ >> > + ASSERT(is_hardware_domain(d)); >> >> If that can happen arbitrarily at run time (rather than just at boot, >> as suggested by the removal of __init), it definitely can't remain as >> is and will instead need to make use of continuations. I'm therefore >> unconvinced you really want to move this code instead of simply >> calling {,un}map_mmio_regions() while taking care of preemption >> needs. > > I'm not sure I know how to use continuations with non-hypercall > vmexits. Do you have any recommendations about how to do this? pause > the domain and run the mmio changes inside of a tasklet? That would be one option. Or you could derive from the approach used for waiting for a response from the device model. Even exiting back to the guest without updating rIP may be possible, provided you have a means to store the continuation information such that when coming back you won't start from the beginning again. Jan
On Wed, Jun 21, 2017 at 05:57:19AM -0600, Jan Beulich wrote: > >>> On 21.06.17 at 13:11, <roger.pau@citrix.com> wrote: > > On Fri, May 19, 2017 at 07:35:39AM -0600, Jan Beulich wrote: > >> >>> On 27.04.17 at 16:35, <roger.pau@citrix.com> wrote: > >> > +int modify_mmio(struct domain *d, gfn_t gfn, mfn_t mfn, unsigned long nr_pages, > >> > + const bool map) > >> > +{ > >> > + int rc; > >> > + > >> > + /* > >> > + * Make sure this function is only used by the hardware domain, because it > >> > + * can take an arbitrary long time, and could DoS the whole system. > >> > + */ > >> > + ASSERT(is_hardware_domain(d)); > >> > >> If that can happen arbitrarily at run time (rather than just at boot, > >> as suggested by the removal of __init), it definitely can't remain as > >> is and will instead need to make use of continuations. I'm therefore > >> unconvinced you really want to move this code instead of simply > >> calling {,un}map_mmio_regions() while taking care of preemption > >> needs. > > > > I'm not sure I know how to use continuations with non-hypercall > > vmexits. Do you have any recommendations about how to do this? pause > > the domain and run the mmio changes inside of a tasklet? > > That would be one option. Or you could derive from the approach > used for waiting for a response from the device model. AFAICT the ioreq code pauses the domain and waits for a reply from the dm, but in that case I would still need the tasklet in order to perform the work (since there's no dm here). > Even exiting > back to the guest without updating rIP may be possible, provided > you have a means to store the continuation information such that > when coming back you won't start from the beginning again. I don't really fancy this since it would mean wasting a lot of time in vmexits/vmenters. Roger.
>>> On 21.06.17 at 14:43, <roger.pau@citrix.com> wrote: > On Wed, Jun 21, 2017 at 05:57:19AM -0600, Jan Beulich wrote: >> >>> On 21.06.17 at 13:11, <roger.pau@citrix.com> wrote: >> > On Fri, May 19, 2017 at 07:35:39AM -0600, Jan Beulich wrote: >> >> >>> On 27.04.17 at 16:35, <roger.pau@citrix.com> wrote: >> >> > +int modify_mmio(struct domain *d, gfn_t gfn, mfn_t mfn, unsigned long > nr_pages, >> >> > + const bool map) >> >> > +{ >> >> > + int rc; >> >> > + >> >> > + /* >> >> > + * Make sure this function is only used by the hardware domain, > because it >> >> > + * can take an arbitrary long time, and could DoS the whole system. >> >> > + */ >> >> > + ASSERT(is_hardware_domain(d)); >> >> >> >> If that can happen arbitrarily at run time (rather than just at boot, >> >> as suggested by the removal of __init), it definitely can't remain as >> >> is and will instead need to make use of continuations. I'm therefore >> >> unconvinced you really want to move this code instead of simply >> >> calling {,un}map_mmio_regions() while taking care of preemption >> >> needs. >> > >> > I'm not sure I know how to use continuations with non-hypercall >> > vmexits. Do you have any recommendations about how to do this? pause >> > the domain and run the mmio changes inside of a tasklet? >> >> That would be one option. Or you could derive from the approach >> used for waiting for a response from the device model. > > AFAICT the ioreq code pauses the domain and waits for a reply from the > dm, but in that case I would still need the tasklet in order to perform > the work (since there's no dm here). Well, that's kind of pausing (it's not an explicit domain_pause(), and you really would mean to pause just the vCPU here). Otoh to prevent hangs we simply call process_pending_softirqs() every once in a while in a few other cases, so maybe doing that would already suffice here. Jan
On Wed, Jun 21, 2017 at 06:51:32AM -0600, Jan Beulich wrote: > >>> On 21.06.17 at 14:43, <roger.pau@citrix.com> wrote: > > On Wed, Jun 21, 2017 at 05:57:19AM -0600, Jan Beulich wrote: > >> >>> On 21.06.17 at 13:11, <roger.pau@citrix.com> wrote: > >> > On Fri, May 19, 2017 at 07:35:39AM -0600, Jan Beulich wrote: > >> >> >>> On 27.04.17 at 16:35, <roger.pau@citrix.com> wrote: > >> >> > +int modify_mmio(struct domain *d, gfn_t gfn, mfn_t mfn, unsigned long > > nr_pages, > >> >> > + const bool map) > >> >> > +{ > >> >> > + int rc; > >> >> > + > >> >> > + /* > >> >> > + * Make sure this function is only used by the hardware domain, > > because it > >> >> > + * can take an arbitrary long time, and could DoS the whole system. > >> >> > + */ > >> >> > + ASSERT(is_hardware_domain(d)); > >> >> > >> >> If that can happen arbitrarily at run time (rather than just at boot, > >> >> as suggested by the removal of __init), it definitely can't remain as > >> >> is and will instead need to make use of continuations. I'm therefore > >> >> unconvinced you really want to move this code instead of simply > >> >> calling {,un}map_mmio_regions() while taking care of preemption > >> >> needs. > >> > > >> > I'm not sure I know how to use continuations with non-hypercall > >> > vmexits. Do you have any recommendations about how to do this? pause > >> > the domain and run the mmio changes inside of a tasklet? > >> > >> That would be one option. Or you could derive from the approach > >> used for waiting for a response from the device model. > > > > AFAICT the ioreq code pauses the domain and waits for a reply from the > > dm, but in that case I would still need the tasklet in order to perform > > the work (since there's no dm here). > > Well, that's kind of pausing (it's not an explicit domain_pause(), > and you really would mean to pause just the vCPU here). Right, so vcpu_pause would do it. > Otoh > to prevent hangs we simply call process_pending_softirqs() > every once in a while in a few other cases, so maybe doing that > would already suffice here. That's what I was doing here in modify_mmio, calling process_pending_softirqs between calls to {map/unmap}_mmio_regions. I could leave modify_identity_mmio as-is and simply call {map/unmap}_mmio_regions from the vPCI header handlers, calling process_pending_softirqs in between. I just moved the helper because it avoids open-coding this again. Roger.
>>> On 21.06.17 at 15:10, <roger.pau@citrix.com> wrote: > On Wed, Jun 21, 2017 at 06:51:32AM -0600, Jan Beulich wrote: >> Otoh >> to prevent hangs we simply call process_pending_softirqs() >> every once in a while in a few other cases, so maybe doing that >> would already suffice here. > > That's what I was doing here in modify_mmio, calling > process_pending_softirqs between calls to {map/unmap}_mmio_regions. > > I could leave modify_identity_mmio as-is and simply call > {map/unmap}_mmio_regions from the vPCI header handlers, calling > process_pending_softirqs in between. I just moved the helper because > it avoids open-coding this again. Oh, indeed. In that case I'd suggest wording the comment in a less scary way. Jan
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c index b8999a053a..9efe4e571b 100644 --- a/xen/arch/x86/hvm/dom0_build.c +++ b/xen/arch/x86/hvm/dom0_build.c @@ -64,27 +64,7 @@ static struct acpi_madt_nmi_source __initdata *nmisrc; static int __init modify_identity_mmio(struct domain *d, unsigned long pfn, unsigned long nr_pages, const bool map) { - int rc; - - for ( ; ; ) - { - rc = (map ? map_mmio_regions : unmap_mmio_regions) - (d, _gfn(pfn), nr_pages, _mfn(pfn)); - if ( rc == 0 ) - break; - if ( rc < 0 ) - { - printk(XENLOG_WARNING - "Failed to identity %smap [%#lx,%#lx) for d%d: %d\n", - map ? "" : "un", pfn, pfn + nr_pages, d->domain_id, rc); - break; - } - nr_pages -= rc; - pfn += rc; - process_pending_softirqs(); - } - - return rc; + return modify_mmio(d, _gfn(pfn), _mfn(pfn), nr_pages, map); } /* Populate a HVM memory range using the biggest possible order. */ diff --git a/xen/common/memory.c b/xen/common/memory.c index 52879e7438..888b3f2f4f 100644 --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -1438,6 +1438,42 @@ int prepare_ring_for_helper( return 0; } +int modify_mmio(struct domain *d, gfn_t gfn, mfn_t mfn, unsigned long nr_pages, + const bool map) +{ + int rc; + + /* + * Make sure this function is only used by the hardware domain, because it + * can take an arbitrary long time, and could DoS the whole system. + */ + ASSERT(is_hardware_domain(d)); + + for ( ; ; ) + { + rc = (map ? map_mmio_regions : unmap_mmio_regions) + (d, gfn, nr_pages, mfn); + if ( rc == 0 ) + break; + if ( rc < 0 ) + { + printk(XENLOG_G_WARNING + "Failed to %smap [%" PRI_gfn ", %" PRI_gfn ") -> " + "[%" PRI_mfn ", %" PRI_mfn ") for d%d: %d\n", + map ? "" : "un", gfn_x(gfn), gfn_x(gfn_add(gfn, nr_pages)), + mfn_x(mfn), mfn_x(mfn_add(mfn, nr_pages)), d->domain_id, + rc); + break; + } + nr_pages -= rc; + mfn = mfn_add(mfn, rc); + gfn = gfn_add(gfn, rc); + process_pending_softirqs(); + } + + return rc; +} + /* * Local variables: * mode: C diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h index 8cd5a6b503..4f398cb847 100644 --- a/xen/include/xen/p2m-common.h +++ b/xen/include/xen/p2m-common.h @@ -13,4 +13,7 @@ int unmap_mmio_regions(struct domain *d, unsigned long nr, mfn_t mfn); +int modify_mmio(struct domain *d, gfn_t gfn, mfn_t mfn, unsigned long nr_pages, + const bool map); + #endif /* _XEN_P2M_COMMON_H */
And also allow it to do non-identity mappings by adding a new parameter. This function will be needed in other parts apart from PVH Dom0 build. While there fix the function to use gfn_t and mfn_t instead of unsigned long for memory addresses. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- Changes since v2: - Use mfn_t and gfn_t. - Remove stray newline. --- xen/arch/x86/hvm/dom0_build.c | 22 +--------------------- xen/common/memory.c | 36 ++++++++++++++++++++++++++++++++++++ xen/include/xen/p2m-common.h | 3 +++ 3 files changed, 40 insertions(+), 21 deletions(-)