| Message ID | 20170517151516.52421-4-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 17.05.17 at 17:15, <roger.pau@citrix.com> wrote: > Changes since v2: > - s/vioapic_dom0_map_gsi/vioapic_hwdom_map_gsi/. > - Don't set hvm_domid in xen_domctl_bind_pt_irq_t (it's ignored). The implication of the respective earlier comment was for there to first be a prereq patch added removing this dead field. Otherwise not setting the field is a latent bug. > --- a/xen/arch/x86/hvm/vioapic.c > +++ b/xen/arch/x86/hvm/vioapic.c > @@ -158,6 +158,52 @@ static int vioapic_read( > return X86EMUL_OKAY; > } > > +static int vioapic_hwdom_map_gsi(unsigned int gsi, unsigned int trig, > + unsigned int pol) > +{ > + struct domain *d = current->domain; > + xen_domctl_bind_pt_irq_t pt_irq_bind = { > + .irq_type = PT_IRQ_TYPE_PCI, > + .machine_irq = gsi, Actually you still set the field, just that this is no implicit. Hence the latent bug reduces to just the hwdom != Dom0 case, but anyway. Jan
On Tue, May 30, 2017 at 04:05:39AM -0600, Jan Beulich wrote: > >>> On 17.05.17 at 17:15, <roger.pau@citrix.com> wrote: > > Changes since v2: > > - s/vioapic_dom0_map_gsi/vioapic_hwdom_map_gsi/. > > - Don't set hvm_domid in xen_domctl_bind_pt_irq_t (it's ignored). > > The implication of the respective earlier comment was for there to > first be a prereq patch added removing this dead field. Otherwise > not setting the field is a latent bug. I've added a pre-patch to get rid of hvm_domid in the bind struct. > > --- a/xen/arch/x86/hvm/vioapic.c > > +++ b/xen/arch/x86/hvm/vioapic.c > > @@ -158,6 +158,52 @@ static int vioapic_read( > > return X86EMUL_OKAY; > > } > > > > +static int vioapic_hwdom_map_gsi(unsigned int gsi, unsigned int trig, > > + unsigned int pol) > > +{ > > + struct domain *d = current->domain; > > + xen_domctl_bind_pt_irq_t pt_irq_bind = { > > + .irq_type = PT_IRQ_TYPE_PCI, > > + .machine_irq = gsi, > > Actually you still set the field, just that this is no implicit. Hence the > latent bug reduces to just the hwdom != Dom0 case, but anyway. What do you mean by implicit? Is that because I'm passing d to the bind function? Roger.
>>> On 31.05.17 at 16:48, <roger.pau@citrix.com> wrote: > On Tue, May 30, 2017 at 04:05:39AM -0600, Jan Beulich wrote: >> >>> On 17.05.17 at 17:15, <roger.pau@citrix.com> wrote: >> > Changes since v2: >> > - s/vioapic_dom0_map_gsi/vioapic_hwdom_map_gsi/. >> > - Don't set hvm_domid in xen_domctl_bind_pt_irq_t (it's ignored). >> >> The implication of the respective earlier comment was for there to >> first be a prereq patch added removing this dead field. Otherwise >> not setting the field is a latent bug. > > I've added a pre-patch to get rid of hvm_domid in the bind struct. > >> > --- a/xen/arch/x86/hvm/vioapic.c >> > +++ b/xen/arch/x86/hvm/vioapic.c >> > @@ -158,6 +158,52 @@ static int vioapic_read( >> > return X86EMUL_OKAY; >> > } >> > >> > +static int vioapic_hwdom_map_gsi(unsigned int gsi, unsigned int trig, >> > + unsigned int pol) >> > +{ >> > + struct domain *d = current->domain; >> > + xen_domctl_bind_pt_irq_t pt_irq_bind = { >> > + .irq_type = PT_IRQ_TYPE_PCI, >> > + .machine_irq = gsi, >> >> Actually you still set the field, just that this is no implicit. Hence the >> latent bug reduces to just the hwdom != Dom0 case, but anyway. > > What do you mean by implicit? Is that because I'm passing d to the > bind function? For one I see that me having misspelled thing ("is now implicit") may have caused misunderstanding. But what I meant anyway was that by omitting the initializer you implicitly initialize the field to zero, which is correct if and only if Dom0 == hwdom. But the discussion is moot with the field gone. Jan
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c index abcc473c88..4093082c31 100644 --- a/xen/arch/x86/hvm/vioapic.c +++ b/xen/arch/x86/hvm/vioapic.c @@ -158,6 +158,52 @@ static int vioapic_read( return X86EMUL_OKAY; } +static int vioapic_hwdom_map_gsi(unsigned int gsi, unsigned int trig, + unsigned int pol) +{ + struct domain *d = current->domain; + xen_domctl_bind_pt_irq_t pt_irq_bind = { + .irq_type = PT_IRQ_TYPE_PCI, + .machine_irq = gsi, + }; + int ret, pirq = gsi; + + ASSERT(is_hardware_domain(d)); + + /* Interrupt has been unmasked, bind it now. */ + ret = mp_register_gsi(gsi, trig, pol); + if ( ret == -EEXIST ) + return 0; + if ( ret ) + { + gprintk(XENLOG_WARNING, "vioapic: error registering GSI %u: %d\n", + gsi, ret); + return ret; + } + + ret = allocate_and_map_gsi_pirq(d, &pirq, &pirq); + if ( ret ) + { + gprintk(XENLOG_WARNING, "vioapic: error mapping GSI %u: %d\n", + gsi, ret); + return ret; + } + + pcidevs_lock(); + ret = pt_irq_create_bind(d, &pt_irq_bind); + if ( ret ) + { + gprintk(XENLOG_WARNING, "vioapic: error binding GSI %u: %d\n", + gsi, ret); + spin_lock(&d->event_lock); + unmap_domain_pirq(d, pirq); + spin_unlock(&d->event_lock); + } + pcidevs_unlock(); + + return ret; +} + static void vioapic_write_redirent( struct hvm_vioapic *vioapic, unsigned int idx, int top_word, uint32_t val) @@ -188,6 +234,20 @@ static void vioapic_write_redirent( unmasked = unmasked && !ent.fields.mask; } + if ( is_hardware_domain(d) && unmasked ) + { + int ret; + + ret = vioapic_hwdom_map_gsi(gsi, ent.fields.trig_mode, + ent.fields.polarity); + if ( ret ) + { + /* Mask the entry again. */ + ent.fields.mask = 1; + unmasked = 0; + } + } + *pent = ent; if ( gsi == 0 )
Add the glue in order to bind the PVH Dom0 GSI from bare metal. This is done when Dom0 unmasks the vIO APIC pins, by fetching the current pin settings and setting up the PIRQ, which will then be bound to Dom0. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- Changes since v2: - s/vioapic_dom0_map_gsi/vioapic_hwdom_map_gsi/. - Don't set hvm_domid in xen_domctl_bind_pt_irq_t (it's ignored). - s/gdprintk/gprintk/. - Change the logic of the error paths and remove the labels. Changes since v1: - Mask the pin on error (instead of panicking). - Factor out the Dom0 specific code into a function. - Use the newly introduced allocate_and_map_gsi_pirq instead of physdev_map_pirq. --- xen/arch/x86/hvm/vioapic.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+)