Re: [Xen-devel] [PATCH v2 2/3] x86/pt: enable binding of GSIs to a PVH Dom0

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.05.17 at 17:55, <roger.pau@xxxxxxxxxx> wrote:
> On Fri, May 12, 2017 at 07:42:51AM -0600, Jan Beulich wrote:
>> >>> On 19.04.17 at 17:11, <roger.pau@xxxxxxxxxx> wrote:
>> > Note that currently there's no support for unbinding this interrupts.
>> 
>> Do you plan to deal with that before this changes goes in? Aiui this
>> not working means you can't pass through devices with pin based
>> interrupts once Dom0 chose to bind to them. Otoh hand you modify
>> pt_irq_destroy_bind(), so I'm a little puzzled ...
> 
> Yes, I modify pt_irq_destroy_bind to return EOPNOTSUPP when trying to unbind
> such an interrupt. I can implement the unbind, but it's not going to be used
> ATM.

Is it not? I can see the mentioned pass-through case to be of no
interest, but wouldn't a well behaved kernel perhaps unmap IRQs
while shutting down?

>> > +    spin_lock(&d->arch.hvm_domain.irq_lock);
>> > +    if ( !hvm_irq->gsi_assert_count[gsi] )
>> > +    {
>> > +        hvm_irq->gsi_assert_count[gsi]++;
>> 
>> Why is this an increment instead of a simple write of 1? Or the
>> other way around - why is this not always incrementing, just like
>> __hvm_pci_intx_assert() does? (Symmetric questions then for
>> hvm_gsi_deassert()).
> 
> __hvm_pci_intx_{de}assert has an array that tracks the status of each 
> interrupt
> line, and Xen does the routing based on that (the __test_and_clear_bit at the
> top of __hvm_pci_intx_assert). That prevents the same line from triggering
> multiple times, which is not available here, and thus Xen needs to rely on
> gsi_assert_count in order to know if the GSI is pending or not.
> 
> Switched to use a set to 1/0 instead of the increment, which I agree makes 
> this
> clearer.

And altogether this likely would benefit from a comment put
somewhere.

>> > @@ -504,10 +567,18 @@ int pt_irq_create_bind(
>> >          spin_unlock(&d->event_lock);
>> >  
>> >          if ( iommu_verbose )
>> > -            printk(XENLOG_G_INFO
>> > -                   "d%d: bind: m_gsi=%u g_gsi=%u dev=%02x.%02x.%u 
>> > intx=%u\n",
>> > -                   d->domain_id, pirq, guest_gsi, bus,
>> > -                   PCI_SLOT(device), PCI_FUNC(device), intx);
>> > +        {
>> > +            char buf[50];
>> > +
>> > +            if ( !is_hardware_domain(d) )
>> > +                snprintf(buf, ARRAY_SIZE(buf), " dev=%02x.%02x.%u 
>> > intx=%u",
>> > +                         digl->bus, PCI_SLOT(digl->device),
>> > +                         PCI_FUNC(digl->device), digl->intx);
>> 
>> The buffer array seems heavily over-sized - my counting gives at best
>> slightly over 20 characters you actually need.
> 
> AFAICT max length should be 21, would you be fine with me setting it to 24 to
> be safe?

Sure.

Jan


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