Re: [PATCH v9 4/5] x86/ioreq server: Asynchronously reset outstanding p2m_ioreq_server entries.

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

On 3/23/2017 5:00 PM, Jan Beulich wrote:
>>>> On 23.03.17 at 04:23, <yu.c.zhang@linux.intel.com> wrote:
>> On 3/22/2017 10:29 PM, Jan Beulich wrote:
>>>>>> On 21.03.17 at 03:52, <yu.c.zhang@linux.intel.com> wrote:
>>>> --- a/xen/arch/x86/hvm/ioreq.c
>>>> +++ b/xen/arch/x86/hvm/ioreq.c
>>>> @@ -949,6 +949,14 @@ int hvm_map_mem_type_to_ioreq_server(struct domain *d, ioservid_t id,
>>>>    
>>>>        spin_unlock_recursive(&d->arch.hvm_domain.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);
>>>> +    }
>>> If you do this after dropping the lock, don't you risk a race with
>>> another server mapping the type to itself?
>> I believe it's OK. Remaining p2m_ioreq_server entries still needs to be
>> cleaned anyway.
> Are you refusing a new server mapping the type before being
> done with the cleanup?

No. I meant even a new server is mapped, we can still sweep the p2m 
table later asynchronously.
But this reminds me other point - will a dm op be interrupted by another 
one, or should it?
Since we have patch 5/5 which sweep the p2m table right after the unmap 
happens, maybe
we should refuse any mapping requirement if there's remaining 
p2m_ioreq_server entries.

>
>>>> --- a/xen/arch/x86/mm/p2m-ept.c
>>>> +++ b/xen/arch/x86/mm/p2m-ept.c
>>>> @@ -544,6 +544,12 @@ static int resolve_misconfig(struct p2m_domain *p2m,
>>>> unsigned long gfn)
>>>>                        e.ipat = ipat;
>>>>                        if ( e.recalc && p2m_is_changeable(e.sa_p2mt) )
>>>>                        {
>>>> +                         if ( e.sa_p2mt == p2m_ioreq_server )
>>>> +                         {
>>>> +                             p2m->ioreq.entry_count--;
>>>> +                             ASSERT(p2m->ioreq.entry_count >= 0);
>>> If you did the ASSERT() first (using > 0), you wouldn't need the
>>> type be a signed one, doubling the valid value range (even if
>>> right now the full 64 bits can't be used anyway, but it would be
>>> one less thing to worry about once we get 6-level page tables).
>> Well, entry_count counts only for 4K pages, so even if the guest
>> physical address
>> width is extended up to 64 bit in the future, entry_count will not
>> exceed 2^52(
>> 2^64/2^12).
> Oh, true. Still I'd prefer if you used an unsigned type for a count
> when that's easily possible.

Got it. :)

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


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