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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.06.17 at 10:26, <roger.pau@xxxxxxxxxx> wrote:
> On Thu, Jun 22, 2017 at 03:40:52AM -0600, Jan Beulich wrote:
>> >>> On 20.06.17 at 11:15, <roger.pau@xxxxxxxxxx> wrote:
>> > @@ -472,7 +514,29 @@ int pt_irq_create_bind(
>> >                  pirq_dpci->flags = HVM_IRQ_DPCI_MAPPED |
>> >                                     HVM_IRQ_DPCI_MACH_PCI |
>> >                                     HVM_IRQ_DPCI_GUEST_PCI;
>> > -                share = BIND_PIRQ__WILL_SHARE;
>> > +                if ( !is_hardware_domain(d) )
>> > +                    share = BIND_PIRQ__WILL_SHARE;
>> > +                else
>> > +                {
>> > +                    unsigned int pin;
>> > +                    const struct hvm_vioapic *vioapic = gsi_vioapic(d,
>> > +                                                                    
>> > guest_gsi,
>> > +                                                                    &pin);
>> > +
>> > +                    if ( !vioapic )
>> > +                    {
>> > +                        ASSERT_UNREACHABLE();
>> > +                        return -EINVAL;
>> > +                    }
>> > +                    pirq_dpci->flags |= HVM_IRQ_DPCI_IDENTITY_GSI;
>> > +                    /*
>> > +                     * Check if the corresponding vIO APIC pin is 
>> > configured
>> > +                     * level or edge trigger, level triggered interrupts 
>> > will
>> > +                     * be marked as shareable.
>> > +                     */
>> > +                    ASSERT(!vioapic->redirtbl[pin].fields.mask);
>> > +                    share = vioapic->redirtbl[pin].fields.trig_mode;
>> > +                }
>> 
>> I'm sorry for paying attention to this only now, but the vIO-APIC
>> code here really should be converted into a
>> vioapic_get_trigger_mode() helper living in vioapic.[ch]. The same
>> I think actually holds for the two uses of gsi_vioapic(), and that
>> function should really have been kept static to vioapic.c.
> 
> We will need a couple more of helper functions then,
> vioapic_irq_positive_edge for example reads the trigger mode, but it
> also checks and sets the IRR and pt_irq_vector gets the vector.
> 
> Maybe a vioapic_get_redirection_entry?

That may be a middle ground, yet I'd personally prefer to have
separate helpers (maybe Andrew could break ties here). If you
use this more generic one, please make sure you either return
the entry by value, or a const pointer to it (to document that
callers aren't supposed to fiddle with the live value).

Jan


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