Re: [PATCH V3 01/23] x86/ioreq: Prepare IOREQ feature for making it common

[Oleksandr <olekstysh@xxxxxxxxx>]

Hi Jan


> > > > @@ -1080,6 +1105,24 @@ int hvm_unmap_io_range_from_ioreq_server(struct domain 
> > > > *d, ioservid_t id,
> > > >        return rc;
> > > >    }
> > > >    
> > > > +/* Called with ioreq_server lock held */
> > > > +int arch_ioreq_server_map_mem_type(struct domain *d,
> > > > +                                   struct hvm_ioreq_server *s,
> > > > +                                   uint32_t flags)
> > > > +{
> > > > +    int rc = p2m_set_ioreq_server(d, flags, s);
> > > > +
> > > > +    if ( rc == 0 && flags == 0 )
> > > > +    {
> > > > +        const struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > > > +
> > > > +        if ( read_atomic(&p2m->ioreq.entry_count) )
> > > > +            p2m_change_entry_type_global(d, p2m_ioreq_server, p2m_ram_rw);
> > > > +    }
> > > > +
> > > > +    return rc;
> > > > +}
> > > > +
> > > >    /*
> > > >     * Map or unmap an ioreq server to specific memory type. For now, only
> > > >     * HVMMEM_ioreq_server is supported, and in the future new types can be
> > > > @@ -1112,19 +1155,11 @@ int hvm_map_mem_type_to_ioreq_server(struct domain *d, 
> > > > ioservid_t id,
> > > >        if ( s->emulator != current->domain )
> > > >            goto out;
> > > >    
> > > > -    rc = p2m_set_ioreq_server(d, flags, s);
> > > > +    rc = arch_ioreq_server_map_mem_type(d, s, flags);
> > > >    
> > > >     out:
> > > >        spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock);
> > > >    
> > > > -    if ( rc == 0 && flags == 0 )
> > > > -    {
> > > > -        struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > > > -
> > > > -        if ( read_atomic(&p2m->ioreq.entry_count) )
> > > > -            p2m_change_entry_type_global(d, p2m_ioreq_server, p2m_ram_rw);
> > > > -    }
> > > > -
> > > >        return rc;
> > > >    }
> > > While you mention this change in the description, I'm still
> > > missing justification as to why the change is safe to make. I
> > > continue to think p2m_change_entry_type_global() would better
> > > not be called inside the locked region, if at all possible.
> > Well. I am afraid, I don't have a 100% justification why the change is
> > safe to make as well
> > as I don't see an obvious reason why it is not safe to make (at least I
> > didn't find a possible deadlock scenario while investigating the code).
> > I raised a question earlier whether I can fold this check in, which
> > implied calling p2m_change_entry_type_global() with ioreq_server lock held.
> I'm aware of the earlier discussion. But "didn't find" isn't good
> enough in a case like this, and since it's likely hard to indeed
> prove there's no deadlock possible, I think it's best to avoid
> having to provide such a proof by avoiding the nesting.
Agree here.


> > If there is a concern with calling this inside the locked region
> > (unfortunately still unclear for me at the moment), I will try to find
> > another way how to split hvm_map_mem_type_to_ioreq_server() without
> > potentially unsafe change here *and* exposing
> > p2m_change_entry_type_global() to the common code. Right now, I don't
> > have any ideas how this could be split other than
> > introducing one more hook here to deal with p2m_change_entry_type_global
> > (probably arch_ioreq_server_map_mem_type_complete?), but I don't expect
> > it to be accepted.
> > I appreciate any ideas on that.
> Is there a reason why the simplest solution (two independent
> arch_*() calls) won't do? If so, what are the constraints?
There is no reason.


> Can the first one e.g. somehow indicate what needs to happen
> after the lock was dropped?
I think, yes.


> But the two calls look independent
> right now, so I don't see any complicating factors.
ok, will go "two independent arch hooks" route then.

Thank you for the idea.


--
Regards,

Oleksandr Tyshchenko