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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 01.12.20 13:03, Alex Bennée wrote:

Hi Alex

> Oleksandr Tyshchenko <olekstysh@xxxxxxxxx> writes:
>
> > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> >
> > As a lot of x86 code can be re-used on Arm later on, this
> > patch makes some preparation to x86/hvm/ioreq.c before moving
> > to the common code. This way we will get a verbatim copy
> <snip>
> > It worth mentioning that a code which checks the return value of
> > p2m_set_ioreq_server() in hvm_map_mem_type_to_ioreq_server() was
> > folded into arch_ioreq_server_map_mem_type() for the clear split.
> > So the p2m_change_entry_type_global() is called with ioreq_server
> > lock held.
> <snip>
> >   
> > +/* 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);
> > -    }
> > -
> It should be noted that p2m holds it's own lock but I'm unfamiliar with
> Xen's locking architecture. Is there anything that prevents another vCPU
> accessing a page that is also being used my ioreq on the first vCPU?
I am not sure that I would be able to provide reasonable explanations here.
All what I understand is that p2m_change_entry_type_global() x86 
specific (we don't have p2m_ioreq_server concept on Arm) and should 
remain as such (not exposed to the common code).
IIRC, I raised a question during V2 review whether we could have ioreq 
server lock around the call to p2m_change_entry_type_global() and didn't 
get objections. I may mistake, but looks like the lock being used
in p2m_change_entry_type_global() is yet another lock for protecting 
page table operations, so unlikely we could get into the trouble calling 
this function with the ioreq server lock held.

> Assuming that deadlock isn't a possibility to my relatively untrained
> eye this looks good to me:
>
> Reviewed-by: Alex Bennée <alex.bennee@xxxxxxxxxx>
Thank you.


--
Regards,

Oleksandr Tyshchenko