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

[Oleksandr <olekstysh@xxxxxxxxx>]

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.
I am wondering would it be acceptable to replace 
hvm_map_mem_type_to_ioreq_server by 
arch_hvm_map_mem_type_to_ioreq_server here and have the following in the 
common code:
int hvm_map_mem_type_to_ioreq_server(struct domain *d,
                                     ioservid_t id,
                                     uint32_t type,
                                     uint32_t flags)
{
    return arch_hvm_map_mem_type_to_ioreq_server(d, id, type, flags);
}


> The rest of the patch looks ok.
Thank you.

--
Regards,

Oleksandr Tyshchenko