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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 02.12.20 10:00, Jan Beulich wrote:

Hi Jan

> On 01.12.2020 19:53, Oleksandr wrote:
> > On 01.12.20 13:03, Alex Bennée wrote:
> > > Oleksandr Tyshchenko <olekstysh@xxxxxxxxx> writes:
> > > > @@ -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.
> Not getting objections doesn't mean much. Personally I don't recall
> such a question, but this doesn't mean much.
Sorry for not being clear here. Discussion happened at [1] when I was 
asked to move hvm_map_mem_type_to_ioreq_server() to the common code.

>   The important thing
> here is that you properly justify this change in the description (I
> didn't look at this version of the patch as a whole yet, so quite
> likely you actually do). This is because you need to guarantee that
> you don't introduce any lock order violations by this.
Yes, almost all changes in this patch are mostly mechanical and leave 
things as they are.
The change with p2m_change_entry_type_global() requires an additional 
attention, so decided to put emphasis on touching that
in the description and add a comment in the code that it is called with 
ioreq_server lock held.

[1] 
https://patchwork.kernel.org/project/xen-devel/patch/1602780274-29141-2-git-send-email-olekstysh@xxxxxxxxx/#23734839
--
Regards,

Oleksandr Tyshchenko