Re: [Xen-devel] [PATCH v3 3/9] xen/mm: move modify_identity_mmio to global file and drop __init

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.06.17 at 13:11, <roger.pau@xxxxxxxxxx> wrote:
> On Fri, May 19, 2017 at 07:35:39AM -0600, Jan Beulich wrote:
>> >>> On 27.04.17 at 16:35, <roger.pau@xxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel