Re: [Xen-devel] [PATCH v6 1/3] xen/pt: fix some pass-thru devices don't work across reboot

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Chao Gao <chao.gao@xxxxxxxxx> 01/28/19 12:59 PM >>>
>On Fri, Jan 25, 2019 at 09:13:49AM -0700, Jan Beulich wrote:
>>>>> On 25.01.19 at 09:26, <chao.gao@xxxxxxxxx> wrote:
>>> --- a/xen/arch/x86/domctl.c
>>> +++ b/xen/arch/x86/domctl.c
>>> @@ -732,7 +732,11 @@ long arch_do_domctl(
>>>              break;
>>>  
>>>          ret = -EPERM;
>>> -        if ( irq <= 0 || !irq_access_permitted(currd, irq) )
>>> +        /*
>>> +         * irq < 0 denotes the corresponding pirq has been forcibly 
>>> unbound.
>>> +         * For this case, bypass permission check to reap the pirq.
>>> +         */
>>> +        if ( !irq || ((irq > 0) && !irq_access_permitted(currd, irq)) )
>>>              break;
>>
>>So why would it be correct to continue into pt_irq_destroy_bind()
>>with irq < 0?  And with an actual XSM policy I'm not sure you'd
>>even make it past xsm_unbind_pt_irq(). If the IRQ was forcibly
>>unbound before, there shouldn't be anything left to clean up?
>
>But some hints are left to denote that a pirq was forcibly unbound.
>See the code snippets below:
>
>''' in unmap_domain_pirq()
>...
>if ( !forced_unbind )
>clear_domain_irq_pirq(d, irq, info);
>else
>{
>info->arch.irq = -irq;
>radix_tree_replace_slot(
>radix_tree_lookup_slot(&d->arch.irq_pirq, irq),
>radix_tree_int_to_ptr(-pirq));
>}
        >...
>'''
>
>and 
>
>''' in pirq_guest_unbind()
>...
>if ( desc == NULL )
>{
>irq = -pirq->arch.irq;
>BUG_ON(irq <= 0);
>desc = irq_to_desc(irq);
>spin_lock_irq(&desc->lock);
        >clear_domain_irq_pirq(d, irq, pirq);
>}
>...
>'''

All understood, but all this new special casing is what we'd like to avoid if
at all possible. Hence the desire to go back to what you had before,
dealing with the error messages that you've observed another way.


>>On the whole I think all the extra additions in v6 only serve to
>>mask the tool stack not needing to do anymore some of what it
>>does, as suggested in a reply to an earlier version. So I guess I
>>agree with Roger that v5 came closer, but may need to be
>>amended by some tool stack adjustment(s).
>
>Yes. What v6 tries to mask here is irq unbinding and irq unmapping invoked
>by qemu and pciback. We need to fix them in pciback and qemu rather than
>tool stack. If we want to leave those error messages alone, we can just
>take Roger's suggestion. Otherwise, we should try to conditional bypass
>irq unbinding and irq unmapping in pciback and qemu and justify it.

FAOD as said before pciback and even more so qemu should be left alone
if at all possible. In particular for qemu, if anything I think the libxc
functions used by qemu could be short circuited, but qemu itself would
better not require any change. For pciback it depends whether things can
be handled transparently in Xen. Accepting error messages to be issued
is imo only a last resort option.


Jan




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel