Re: [Xen-devel] [PATCH v5 05/11] mm: move modify_identity_mmio to global file and drop __init

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.09.17 at 13:27, <roger.pau@xxxxxxxxxx> wrote:
> On Tue, Sep 12, 2017 at 03:04:02AM -0600, Jan Beulich wrote:
>> >>> On 12.09.17 at 09:49, <roger.pau@xxxxxxxxxx> wrote:
>> > On Tue, Sep 05, 2017 at 09:01:57AM -0600, Jan Beulich wrote:
>> >> >>> On 14.08.17 at 16:28, <roger.pau@xxxxxxxxxx> wrote:
>> >> > +int modify_mmio(struct domain *d, gfn_t gfn, mfn_t mfn, unsigned long 
>> > nr_pages,
>> >> > +                bool map)
>> >> > +{
>> >> > +    int rc;
>> >> > +
>> >> > +    /*
>> >> > +     * ATM this function should only be used by the hardware domain
>> >> > +     * because it doesn't support preemption/continuation, and as such
>> >> > +     * can take a non-negligible amount of time. Note that it 
>> >> > periodically
>> >> > +     * calls process_pending_softirqs in order to avoid stalling the 
>> > 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_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();
>> >> 
>> >> With the __init dropped, this become questionable: We shouldn't
>> >> do this arbitrarily; runtime use should instead force a hypercall
>> >> continuation (assuming that's the context it's going to be used in).
>> > 
>> > This will be used by the PCI emulation code, which is a vmexit but not
>> > an hypercall.
>> > 
>> > I have a plan to add continuations, but I would rather do it as part
>> > of using the PCI emulation for DomUs.
>> 
>> In which case please retain the __init while moving the function,
>> so there's no latent bug here in case someone else wants to
>> call this function in other than boot time context. The __init
>> should be dropped only together with making the softirq
>> processing here conditional, using some suitable other mechanism
>> post-boot.
> 
> This will already be used in non-boot context with this series. From
> the discussion that we had in v3 I though it was fine to use
> process_pending_softirqs as long as it was limited to Dom0:
> 
> https://lists.xenproject.org/archives/html/xen-devel/2017-06/msg02411.html 

I don't think it was a good idea to agree - we shouldn't special
case Dom0 in this regard.

Jan


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