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

From: Oleksandr <olekstysh@gmail.com>
To: paul@xen.org, xen-devel@lists.xenproject.org
Cc: "'Oleksandr Tyshchenko'" <oleksandr_tyshchenko@epam.com>,
	"'Jan Beulich'" <jbeulich@suse.com>,
	"'Andrew Cooper'" <andrew.cooper3@citrix.com>,
	"'Roger Pau Monné'" <roger.pau@citrix.com>,
	"'Julien Grall'" <julien@xen.org>,
	"'Stefano Stabellini'" <sstabellini@kernel.org>,
	"'Wei Liu'" <wl@xen.org>, "'Julien Grall'" <julien.grall@arm.com>
Subject: Re: [PATCH V2 01/23] x86/ioreq: Prepare IOREQ feature for making it common
Date: Wed, 4 Nov 2020 11:06:11 +0200	[thread overview]
Message-ID: <3dd55087-0c07-c9f3-e80a-8b136c226475@gmail.com> (raw)
In-Reply-To: <003c01d6a6b0$8c418f50$a4c4adf0$@xen.org>

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