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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.03.17 at 10:05, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> On 3/23/2017 4:57 PM, Jan Beulich wrote:
>>>>> On 23.03.17 at 04:23, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
>>> On 3/22/2017 10:21 PM, Jan Beulich wrote:
>>>>>>> On 21.03.17 at 03:52, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
>>>>> @@ -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?

I don't follow. I've specifically suggested that you don't change
the code, but simply state clearly the requirement that patches
2...5 of this series should all go in at the same time. I don't mind
you making changes, but the risk then is that further round trips
may be required because of there being new issues with the
changes you may do.

>>>>> --- 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"?

Well, you must have had a reason to add it. To be honest, I don't
care too much about the nested code (as it's far from production
ready anyway), so leaving the code above untouched would be
fine with me, but taking care of adjustments to nested code where
they're actually needed would be even better. So the preferred
option is for you to explain why you've done the change above,
and why you think it's correct/needed. The next best option might
be to drop the change.

>>>>> --- 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?

Because you don't call p2m_get_ioreq_server() here (discarding
the return value, but using the flags).

> 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...

Sure, all understood. I wasn't really suggesting to drop the locked
accesses, but instead I was using this to illustrate the non-locked
access (and hence the inconsistency with other code) here. As
said - if there's a good reason not to call the function here, I'm all
ears.

Jan

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