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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Oleksandr <olekstysh@xxxxxxxxx>
> Sent: 04 November 2020 09:06
> To: paul@xxxxxxx; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: 'Oleksandr Tyshchenko' <oleksandr_tyshchenko@xxxxxxxx>; 'Jan Beulich' 
> <jbeulich@xxxxxxxx>; 'Andrew
> Cooper' <andrew.cooper3@xxxxxxxxxx>; 'Roger Pau Monné' 
> <roger.pau@xxxxxxxxxx>; 'Julien Grall'
> <julien@xxxxxxx>; 'Stefano Stabellini' <sstabellini@xxxxxxxxxx>; 'Wei Liu' 
> <wl@xxxxxxx>; 'Julien
> Grall' <julien.grall@xxxxxxx>
> Subject: Re: [PATCH V2 01/23] x86/ioreq: Prepare IOREQ feature for making it 
> common
> 
> 
> On 20.10.20 10:13, Paul Durrant wrote:
> 
> Hi Paul.
> 
> Sorry for the late response.
> 
> >> +
> >> +/* Called when target domain is paused */
> >> +static inline void arch_hvm_destroy_ioreq_server(struct hvm_ioreq_server 
> >> *s)
> >> +{
> >> +    p2m_set_ioreq_server(s->target, 0, s);
> >> +}
> >> +
> >> +/*
> >> + * 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
> >> + * introduced, e.g. HVMMEM_ioreq_serverX mapped to ioreq server X. And
> >> + * currently, only write operations are to be forwarded to an ioreq 
> >> server.
> >> + * Support for the emulation of read operations can be added when an ioreq
> >> + * server has such requirement in the future.
> >> + */
> >> +static inline int hvm_map_mem_type_to_ioreq_server(struct domain *d,
> >> +                                                   ioservid_t id,
> >> +                                                   uint32_t type,
> >> +                                                   uint32_t flags)
> >> +{
> >> +    struct hvm_ioreq_server *s;
> >> +    int rc;
> >> +
> >> +    if ( type != HVMMEM_ioreq_server )
> >> +        return -EINVAL;
> >> +
> >> +    if ( flags & ~XEN_DMOP_IOREQ_MEM_ACCESS_WRITE )
> >> +        return -EINVAL;
> >> +
> >> +    spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
> >> +
> >> +    s = get_ioreq_server(d, id);
> >> +
> >> +    rc = -ENOENT;
> >> +    if ( !s )
> >> +        goto out;
> >> +
> >> +    rc = -EPERM;
> >> +    if ( s->emulator != current->domain )
> >> +        goto out;
> >> +
> >> +    rc = p2m_set_ioreq_server(d, flags, s);
> >> +
> >> + 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;
> >> +}
> >> +
> > The above doesn't really feel right to me. It's really an entry point into 
> > the ioreq server code and
> as such I think it ought to be left in the common code. I suggest replacing 
> the p2m_set_ioreq_server()
> function with an arch specific function (also taking the type) which you can 
> then implement here.
> 
> Agree that it ought to be left in the common code.
> 
> However, I am afraid I didn't entirely get you suggestion how this
> function could be split. On Arm struct p2m_domain doesn't contain IOREQ
> fields (p2m->ioreq.XXX), nor p2m_change_entry_type_global() is used, so
> they should be abstracted together with p2m_set_ioreq_server().
> 
> So the whole "if ( rc == 0 && flags == 0 )" check should be folded into
> arch_p2m_set_ioreq_server implementation on x86? This in turn raises a
> question can we put a spin_unlock after.
> 

Hi Oleksandr,

I think the code as it stands is really a bit of a layering violation. I don't 
really see a problem with retaining the ioreq server lock around the call to 
p2m_change_entry_type_global() so I'd just fold that into 
p2m_set_ioreq_server().

  Paul