| Message ID | 20170327104429.99992-6-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 27.03.17 at 12:44, <roger.pau@citrix.com> wrote: > --- a/xen/arch/x86/hvm/vioapic.c > +++ b/xen/arch/x86/hvm/vioapic.c > @@ -199,6 +199,34 @@ static void vioapic_write_redirent( > unmasked = unmasked && !ent.fields.mask; > } > > + if ( is_hardware_domain(d) && unmasked ) > + { > + xen_domctl_bind_pt_irq_t pt_irq_bind = { > + .irq_type = PT_IRQ_TYPE_GSI, > + .machine_irq = gsi, > + .u.gsi.gsi = gsi, > + .hvm_domid = DOMID_SELF, > + }; > + int ret, pirq = gsi; > + > + /* Interrupt has been unmasked, bind it now. */ > + ret = mp_register_gsi(gsi, ent.fields.trig_mode, ent.fields.polarity); > + if ( ret && ret != -EEXIST ) > + { > + gdprintk(XENLOG_WARNING, > + "%s: error registering GSI %u: %d\n", __func__, gsi, ret); > + } > + if ( !ret ) > + { > + ret = physdev_map_pirq(DOMID_SELF, MAP_PIRQ_TYPE_GSI, &pirq, &pirq, > + NULL); With this call you basically admit that PVH can't really do without physdev ops, just that you hide it behind IO-APIC RTE writes. Along the lines of the comment on the previous patch I wonder though whether you really need to use this function, i.e. whether you can't instead get away with little more than the call to map_domain_pirq() which that function does. > + BUG_ON(ret); You absolutely don't want to bring down the entire system if a failure occurs here or ... > + ret = pt_irq_create_bind(d, &pt_irq_bind); > + BUG_ON(ret); ... here. Probably the best you can do besides issuing a log message is to mask the RTE. Jan
On Tue, Apr 18, 2017 at 06:35:57AM -0600, Jan Beulich wrote: > >>> On 27.03.17 at 12:44, <roger.pau@citrix.com> wrote: > > --- a/xen/arch/x86/hvm/vioapic.c > > +++ b/xen/arch/x86/hvm/vioapic.c > > @@ -199,6 +199,34 @@ static void vioapic_write_redirent( > > unmasked = unmasked && !ent.fields.mask; > > } > > > > + if ( is_hardware_domain(d) && unmasked ) > > + { > > + xen_domctl_bind_pt_irq_t pt_irq_bind = { > > + .irq_type = PT_IRQ_TYPE_GSI, > > + .machine_irq = gsi, > > + .u.gsi.gsi = gsi, > > + .hvm_domid = DOMID_SELF, > > + }; > > + int ret, pirq = gsi; > > + > > + /* Interrupt has been unmasked, bind it now. */ > > + ret = mp_register_gsi(gsi, ent.fields.trig_mode, ent.fields.polarity); > > + if ( ret && ret != -EEXIST ) > > + { > > + gdprintk(XENLOG_WARNING, > > + "%s: error registering GSI %u: %d\n", __func__, gsi, ret); > > + } > > + if ( !ret ) > > + { > > + ret = physdev_map_pirq(DOMID_SELF, MAP_PIRQ_TYPE_GSI, &pirq, &pirq, > > + NULL); > > With this call you basically admit that PVH can't really do without > physdev ops, just that you hide it behind IO-APIC RTE writes. Yes, I just want to get rid of the physdev hypercalls for PVH Dom0, not the physdev ops inside of Xen. > Along the lines of the comment on the previous patch I wonder > though whether you really need to use this function, i.e. > whether you can't instead get away with little more than the > call to map_domain_pirq() which that function does. Well, I could certainly open-code part of physdev_map_pirq here, AFAICT I just need to make sure the GSI is not used, and bind it to Dom0, but that's just duplicating the code inside of physdev_map_pirq. For MSI/MSI-X I certainly need to use physdev_map_pirq, because the logic in that case is more complex, so I will have to end up making physdev_map_pirq available to external callers in any case. > > > + BUG_ON(ret); > > You absolutely don't want to bring down the entire system if a > failure occurs here or ... > > > + ret = pt_irq_create_bind(d, &pt_irq_bind); > > + BUG_ON(ret); > > ... here. Probably the best you can do besides issuing a log > message is to mask the RTE. Right, those are leftovers from when I was still debugging this. Thanks, Roger.
>>> On 18.04.17 at 15:44, <roger.pau@citrix.com> wrote: > On Tue, Apr 18, 2017 at 06:35:57AM -0600, Jan Beulich wrote: >> >>> On 27.03.17 at 12:44, <roger.pau@citrix.com> wrote: >> > --- a/xen/arch/x86/hvm/vioapic.c >> > +++ b/xen/arch/x86/hvm/vioapic.c >> > @@ -199,6 +199,34 @@ static void vioapic_write_redirent( >> > unmasked = unmasked && !ent.fields.mask; >> > } >> > >> > + if ( is_hardware_domain(d) && unmasked ) >> > + { >> > + xen_domctl_bind_pt_irq_t pt_irq_bind = { >> > + .irq_type = PT_IRQ_TYPE_GSI, >> > + .machine_irq = gsi, >> > + .u.gsi.gsi = gsi, >> > + .hvm_domid = DOMID_SELF, >> > + }; >> > + int ret, pirq = gsi; >> > + >> > + /* Interrupt has been unmasked, bind it now. */ >> > + ret = mp_register_gsi(gsi, ent.fields.trig_mode, ent.fields.polarity); >> > + if ( ret && ret != -EEXIST ) >> > + { >> > + gdprintk(XENLOG_WARNING, >> > + "%s: error registering GSI %u: %d\n", __func__, gsi, ret); >> > + } >> > + if ( !ret ) >> > + { >> > + ret = physdev_map_pirq(DOMID_SELF, MAP_PIRQ_TYPE_GSI, &pirq, &pirq, >> > + NULL); >> >> With this call you basically admit that PVH can't really do without >> physdev ops, just that you hide it behind IO-APIC RTE writes. > > Yes, I just want to get rid of the physdev hypercalls for PVH Dom0, not the > physdev ops inside of Xen. > >> Along the lines of the comment on the previous patch I wonder >> though whether you really need to use this function, i.e. >> whether you can't instead get away with little more than the >> call to map_domain_pirq() which that function does. > > Well, I could certainly open-code part of physdev_map_pirq here, AFAICT I just > need to make sure the GSI is not used, and bind it to Dom0, but that's just > duplicating the code inside of physdev_map_pirq. > > For MSI/MSI-X I certainly need to use physdev_map_pirq, because the logic in > that case is more complex, so I will have to end up making physdev_map_pirq > available to external callers in any case. Hmm, ugly. I'd really like to keep them local as they are - as their name says - physdev-op specific. Would it perhaps be possible to split out GSI and MSI specific helpers from it (which you could then call from here and from the MSI code you're going to add), or would that still leave too much code duplication? Jan
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c index c349a3ee61..ca3aab16ef 100644 --- a/xen/arch/x86/hvm/vioapic.c +++ b/xen/arch/x86/hvm/vioapic.c @@ -199,6 +199,34 @@ static void vioapic_write_redirent( unmasked = unmasked && !ent.fields.mask; } + if ( is_hardware_domain(d) && unmasked ) + { + xen_domctl_bind_pt_irq_t pt_irq_bind = { + .irq_type = PT_IRQ_TYPE_GSI, + .machine_irq = gsi, + .u.gsi.gsi = gsi, + .hvm_domid = DOMID_SELF, + }; + int ret, pirq = gsi; + + /* Interrupt has been unmasked, bind it now. */ + ret = mp_register_gsi(gsi, ent.fields.trig_mode, ent.fields.polarity); + if ( ret && ret != -EEXIST ) + { + gdprintk(XENLOG_WARNING, + "%s: error registering GSI %u: %d\n", __func__, gsi, ret); + } + if ( !ret ) + { + ret = physdev_map_pirq(DOMID_SELF, MAP_PIRQ_TYPE_GSI, &pirq, &pirq, + NULL); + BUG_ON(ret); + + ret = pt_irq_create_bind(d, &pt_irq_bind); + BUG_ON(ret); + } + } + *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 using the newly introduced PT_IRQ_TYPE_GSI bind type. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- xen/arch/x86/hvm/vioapic.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)