Re: [PATCH v9 2/5] x86/ioreq server: Add DMOP to map guest ram with p2m_ioreq_server to an ioreq server.

[Yu Zhang <yu.c.zhang@linux.intel.com>]

On 3/22/2017 10:21 PM, Jan Beulich wrote:
>>>> On 21.03.17 at 03:52, <yu.c.zhang@linux.intel.com> wrote:
>> ---
>>   xen/arch/x86/hvm/dm.c            | 37 ++++++++++++++++++--
>>   xen/arch/x86/hvm/emulate.c       | 65 ++++++++++++++++++++++++++++++++---
>>   xen/arch/x86/hvm/ioreq.c         | 38 +++++++++++++++++++++
>>   xen/arch/x86/mm/hap/nested_hap.c |  2 +-
>>   xen/arch/x86/mm/p2m-ept.c        |  8 ++++-
>>   xen/arch/x86/mm/p2m-pt.c         | 19 +++++++----
>>   xen/arch/x86/mm/p2m.c            | 74 ++++++++++++++++++++++++++++++++++++++++
>>   xen/arch/x86/mm/shadow/multi.c   |  3 +-
>>   xen/include/asm-x86/hvm/ioreq.h  |  2 ++
>>   xen/include/asm-x86/p2m.h        | 26 ++++++++++++--
>>   xen/include/public/hvm/dm_op.h   | 28 +++++++++++++++
>>   xen/include/public/hvm/hvm_op.h  |  8 ++++-
>>   12 files changed, 290 insertions(+), 20 deletions(-)
> Btw., isn't there a libdevicemodel wrapper missing here for this new
> sub-op?

Yes. I planed to add the wrapper code in another patch after this series 
is accepted.
Is this a must in this patchset?

>> @@ -177,8 +178,64 @@ static int hvmemul_do_io(
>>           break;
>>       case X86EMUL_UNHANDLEABLE:
>>       {
>> -        struct hvm_ioreq_server *s =
>> -            hvm_select_ioreq_server(curr->domain, &p);
>> +        /*
>> +         * Xen isn't emulating the instruction internally, so see if
>> +         * there's an ioreq server that can handle it. Rules:
>> +         *
>> +         * - PIO and "normal" MMIO run through hvm_select_ioreq_server()
>> +         * to choose the ioreq server by range. If no server is found,
>> +         * the access is ignored.
>> +         *
>> +         * - p2m_ioreq_server accesses are handled by the designated
>> +         * ioreq_server for the domain, but there are some corner
>> +         * cases:
>> +         *
>> +         *   - If the domain ioreq_server is NULL, assume there is a
>> +         *   race between the unbinding of ioreq server and guest fault
>> +         *   so re-try the instruction.
> And that retry won't come back here because of? (The answer
> should not include any behavior added by subsequent patches.)

You got me. :)
In this patch, retry will come back here. It should be after patch 4 or 
patch 5 that the retry
will be ignored(p2m type changed back to p2m_ram_rw after the unbinding).

>> +         */
>> +        struct hvm_ioreq_server *s = NULL;
>> +        p2m_type_t p2mt = p2m_invalid;
>> +
>> +        if ( is_mmio )
>> +        {
>> +            unsigned long gmfn = paddr_to_pfn(addr);
>> +
>> +            get_gfn_query_unlocked(currd, gmfn, &p2mt);
>> +
>> +            if ( p2mt == p2m_ioreq_server )
>> +            {
>> +                unsigned int flags;
>> +
>> +                /*
>> +                 * Value of s could be stale, when we lost a race
>> +                 * with dm_op which unmaps p2m_ioreq_server from the
>> +                 * ioreq server. Yet there's no cheap way to avoid
>> +                 * this, so device model need to do the check.
>> +                 */
>> +                s = p2m_get_ioreq_server(currd, &flags);
>> +
>> +                /*
>> +                 * If p2mt is ioreq_server but ioreq_server is NULL,
>> +                 * we probably lost a race with unbinding of ioreq
>> +                 * server, just retry the access.
>> +                 */
> This repeats the earlier comment - please settle on where to state
> this, but don't say the exact same thing twice within a few lines of
> code.

Thanks, will remove this comment.

>> +                if ( s == NULL )
>> +                {
>> +                    rc = X86EMUL_RETRY;
>> +                    vio->io_req.state = STATE_IOREQ_NONE;
>> +                    break;
>> +                }
>> +            }
>> +        }
>> +
>> +        /*
>> +         * Value of s could be stale, when we lost a race with dm_op
>> +         * which unmaps this PIO/MMIO address from the ioreq server.
>> +         * The device model side need to do the check.
> I think "will do" would be more natural here, or add "anyway" to
> the end of the sentence.
>

Got it. Thanks.

>> @@ -914,6 +916,42 @@ int hvm_unmap_io_range_from_ioreq_server(struct domain *d, ioservid_t id,
>>       return rc;
>>   }
>>   
>> +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;
>> +
>> +    /* For now, only HVMMEM_ioreq_server is supported. */
>> +    if ( type != HVMMEM_ioreq_server )
>> +        return -EINVAL;
>> +
>> +    /* For now, only write emulation is supported. */
>> +    if ( flags & ~(XEN_DMOP_IOREQ_MEM_ACCESS_WRITE) )
> Stray parentheses.

Got it.
>> --- a/xen/arch/x86/mm/hap/nested_hap.c
>> +++ b/xen/arch/x86/mm/hap/nested_hap.c
>> @@ -172,7 +172,7 @@ nestedhap_walk_L0_p2m(struct p2m_domain *p2m, paddr_t L1_gpa, paddr_t *L0_gpa,
>>       if ( *p2mt == p2m_mmio_direct )
>>           goto direct_mmio_out;
>>       rc = NESTEDHVM_PAGEFAULT_MMIO;
>> -    if ( *p2mt == p2m_mmio_dm )
>> +    if ( *p2mt == p2m_mmio_dm || *p2mt == p2m_ioreq_server )
> Btw., how does this addition match up with the rc value being
> assigned right before the if()?

Well returning a NESTEDHVM_PAGEFAULT_MMIO in such case will trigger 
handle_mmio() later in
hvm_hap_nested_page_fault(). Guess that is what we expected.

>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -131,6 +131,13 @@ static void ept_p2m_type_to_flags(struct p2m_domain *p2m, ept_entry_t *entry,
>>               entry->r = entry->w = entry->x = 1;
>>               entry->a = entry->d = !!cpu_has_vmx_ept_ad;
>>               break;
>> +        case p2m_ioreq_server:
>> +            entry->r = 1;
>> +            entry->w = !(p2m->ioreq.flags & XEN_DMOP_IOREQ_MEM_ACCESS_WRITE);
> Is this effectively open coded p2m_get_ioreq_server() actually
> okay? If so, why does the function need to be used elsewhere,
> instead of doing direct, lock-free accesses?

Maybe your comments is about whether it is necessary to use the lock in 
p2m_get_ioreq_server()?
I still believe so, it does not only protect the value of ioreq server, 
but also the flag together with it.

Besides, it is used not only in the emulation process, but also the 
hypercall to set the mem type.
So the lock can still provide some kind protection against the 
p2m_set_ioreq_server() - even it does
not always do so.

>> +void p2m_destroy_ioreq_server(const struct domain *d,
>> +                              const struct hvm_ioreq_server *s)
>> +{
>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> +
>> +    spin_lock(&p2m->ioreq.lock);
>> +
>> +    if ( p2m->ioreq.server == s )
>> +    {
>> +        p2m->ioreq.server = NULL;
>> +        p2m->ioreq.flags = 0;
>> +    }
>> +
>> +    spin_unlock(&p2m->ioreq.lock);
>> +}
> Is this function really needed? I.e. can't the caller simply call
> p2m_set_ioreq_server(d, 0, s) instead?

You are right, we can use p2m_set_ioreq_server(d, 0, s). :)

Yu
> Jan
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel