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/23/2017 4:57 PM, Jan Beulich wrote:
>>>> On 23.03.17 at 04:23, <yu.c.zhang@linux.intel.com> wrote:
>> 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?
> I think so, or else the code you add is effectively dead. We should
> avoid encouraging people to bypass libxc.

OK. I'll try to add another patch to do so, along with the existing 
ones. Thanks.
>>>> @@ -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).
> In which case I think we shouldn't insist on you to change things, but
> you should spell out very clearly that this patch should not go in
> without the others going in at the same time.

So maybe it would be better we leave the retry part to a later patch, 
say patch 4/5 or patch 5/5,
and return unhandleable in this patch?

>>>> --- 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.
> That's probably what is expected, but it's no MMIO which we're
> doing in that case. And note that we've stopped abusing
> handle_mmio() for non-MMIO purposes a little while ago (commit
> 3dd00f7b56 ["x86/HVM: restrict permitted instructions during
> special purpose emulation"]).

OK. So what about we just remove this "*p2mt == p2m_ioreq_server"?

>>>> --- 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.
> The question, fundamentally, is about consistency: The same
> access model should be followed universally, unless there is an
> explicit reason for an exception.

Sorry, I do not quite understand. Why the consistency is broken?
I think this lock at least protects the ioreq server and the flag. The 
only exception
is the one you mentioned - s could become stale which we agreed to let 
the device
model do the check. Without this lock, things would become more complex 
- more
race conditions...

Thanks
Yu
> Jan
>


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