* [PATCH v4 1/4] x86/pt: remove hvm_domid field from bind struct
2017-06-01 11:49 [PATCH v4 0/4] x86/dpci: bind legacy PCI interrupts to PVHv2 Dom0 Roger Pau Monne
@ 2017-06-01 11:49 ` Roger Pau Monne
2017-06-01 11:57 ` Wei Liu
2017-06-01 13:17 ` Jan Beulich
2017-06-01 11:49 ` [PATCH v4 2/4] x86/physdev: factor out the code to allocate and map a pirq Roger Pau Monne
` (2 subsequent siblings)
3 siblings, 2 replies; 20+ messages in thread
From: Roger Pau Monne @ 2017-06-01 11:49 UTC (permalink / raw)
To: xen-devel, boris.ostrovsky, konrad.wilk
Cc: Wei Liu, Ian Jackson, Jan Beulich, Roger Pau Monne
This filed is unused and serves no purpose.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reported by: Jan Beulich <JBeulich@suse.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
---
Changes since v3:
- New in this version.
---
tools/libxc/xc_domain.c | 4 ----
xen/include/public/domctl.h | 1 -
2 files changed, 5 deletions(-)
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 00909ad470..a447405903 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -1672,7 +1672,6 @@ int xc_domain_update_msi_irq(
domctl.domain = (domid_t)domid;
bind = &(domctl.u.bind_pt_irq);
- bind->hvm_domid = domid;
bind->irq_type = PT_IRQ_TYPE_MSI;
bind->machine_irq = pirq;
bind->u.msi.gvec = gvec;
@@ -1699,7 +1698,6 @@ int xc_domain_unbind_msi_irq(
domctl.domain = (domid_t)domid;
bind = &(domctl.u.bind_pt_irq);
- bind->hvm_domid = domid;
bind->irq_type = PT_IRQ_TYPE_MSI;
bind->machine_irq = pirq;
bind->u.msi.gvec = gvec;
@@ -1729,7 +1727,6 @@ static int xc_domain_bind_pt_irq_int(
domctl.domain = (domid_t)domid;
bind = &(domctl.u.bind_pt_irq);
- bind->hvm_domid = domid;
bind->irq_type = irq_type;
bind->machine_irq = machine_irq;
switch ( irq_type )
@@ -1788,7 +1785,6 @@ static int xc_domain_unbind_pt_irq_int(
domctl.domain = (domid_t)domid;
bind = &(domctl.u.bind_pt_irq);
- bind->hvm_domid = domid;
bind->irq_type = irq_type;
bind->machine_irq = machine_irq;
switch ( irq_type )
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index e6cf211fe7..02a33def20 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -559,7 +559,6 @@ typedef enum pt_irq_type_e {
struct xen_domctl_bind_pt_irq {
uint32_t machine_irq;
pt_irq_type_t irq_type;
- uint32_t hvm_domid;
union {
struct {
--
2.11.0 (Apple Git-81)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 1/4] x86/pt: remove hvm_domid field from bind struct
2017-06-01 11:49 ` [PATCH v4 1/4] x86/pt: remove hvm_domid field from bind struct Roger Pau Monne
@ 2017-06-01 11:57 ` Wei Liu
2017-06-01 13:17 ` Jan Beulich
1 sibling, 0 replies; 20+ messages in thread
From: Wei Liu @ 2017-06-01 11:57 UTC (permalink / raw)
To: Roger Pau Monne
Cc: Wei Liu, Ian Jackson, Jan Beulich, xen-devel, boris.ostrovsky
On Thu, Jun 01, 2017 at 12:49:11PM +0100, Roger Pau Monne wrote:
> This filed is unused and serves no purpose.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reported by: Jan Beulich <JBeulich@suse.com>
Missing dash.
Acked-by: Wei Liu <wei.liu2@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/4] x86/pt: remove hvm_domid field from bind struct
2017-06-01 11:49 ` [PATCH v4 1/4] x86/pt: remove hvm_domid field from bind struct Roger Pau Monne
2017-06-01 11:57 ` Wei Liu
@ 2017-06-01 13:17 ` Jan Beulich
2017-06-01 14:45 ` Roger Pau Monne
1 sibling, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2017-06-01 13:17 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Ian Jackson, boris.ostrovsky, Wei Liu, xen-devel
>>> On 01.06.17 at 13:49, <roger.pau@citrix.com> wrote:
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -559,7 +559,6 @@ typedef enum pt_irq_type_e {
> struct xen_domctl_bind_pt_irq {
> uint32_t machine_irq;
> pt_irq_type_t irq_type;
> - uint32_t hvm_domid;
>
> union {
> struct {
While there already is a patch pending* which increments the
domctl version, you'd need to do it here too, as we can't be
sure which one lands first. I can of course pull over the hunk
from there and commit it ...
With that addition
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
* https://lists.xenproject.org/archives/html/xen-devel/2017-05/msg02817.html
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 1/4] x86/pt: remove hvm_domid field from bind struct
2017-06-01 13:17 ` Jan Beulich
@ 2017-06-01 14:45 ` Roger Pau Monne
0 siblings, 0 replies; 20+ messages in thread
From: Roger Pau Monne @ 2017-06-01 14:45 UTC (permalink / raw)
To: Jan Beulich; +Cc: Ian Jackson, boris.ostrovsky, Wei Liu, xen-devel
On Thu, Jun 01, 2017 at 07:17:16AM -0600, Jan Beulich wrote:
> >>> On 01.06.17 at 13:49, <roger.pau@citrix.com> wrote:
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -559,7 +559,6 @@ typedef enum pt_irq_type_e {
> > struct xen_domctl_bind_pt_irq {
> > uint32_t machine_irq;
> > pt_irq_type_t irq_type;
> > - uint32_t hvm_domid;
> >
> > union {
> > struct {
>
> While there already is a patch pending* which increments the
> domctl version, you'd need to do it here too, as we can't be
> sure which one lands first. I can of course pull over the hunk
> from there and commit it ...
If you don't mind doing it (and there are no further comments) that's
fine for me.
> With that addition
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Thanks
Roger
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 2/4] x86/physdev: factor out the code to allocate and map a pirq
2017-06-01 11:49 [PATCH v4 0/4] x86/dpci: bind legacy PCI interrupts to PVHv2 Dom0 Roger Pau Monne
2017-06-01 11:49 ` [PATCH v4 1/4] x86/pt: remove hvm_domid field from bind struct Roger Pau Monne
@ 2017-06-01 11:49 ` Roger Pau Monne
2017-06-01 14:20 ` Jan Beulich
2017-06-01 14:40 ` Andrew Cooper
2017-06-01 11:49 ` [PATCH v4 3/4] x86/pt: enable binding of GSIs to a PVH Dom0 Roger Pau Monne
2017-06-01 11:49 ` [PATCH v4 4/4] x86/vioapic: bind interrupts to " Roger Pau Monne
3 siblings, 2 replies; 20+ messages in thread
From: Roger Pau Monne @ 2017-06-01 11:49 UTC (permalink / raw)
To: xen-devel, boris.ostrovsky, konrad.wilk
Cc: Andrew Cooper, Jan Beulich, Roger Pau Monne
Move the code to allocate and map a domain pirq (either GSI or MSI)
into the x86 irq code base, so that it can be used outside of the
physdev ops.
This change shouldn't affect the functionality of the already existing
physdev ops.
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 v3:
- Pass the index parameter to the allocate_pirq function, so that it
can be printed in case of error.
- Remove pointless elses in allocate_pirq.
- Return directly in the last check of allocate_pirq (instead of
storing the error code in the pirq variable.
- allocate_and_map_{gsi/msi}_pirq don't need index to be a pointer.
- Fix the last parameter of the allocate_pirq call in
allocate_and_map_gsi_pirq to use NULL (instead of 0).
- Add an ASSERT_UNREACHABLE if the interrupt type passed to
allocate_and_map_msi_pirq is not of MSI the MSI kind.
- Restore newlines in the physdev_map_pirq switch case.
Changes since v2:
- Factor out the code to allocate the pirq.
- Fix coding style issues.
- Do not take the pci lock to bind a GSI.
- Pass a type parameter to the MSI bind function.
Changes since v1:
- New in this version.
---
xen/arch/x86/irq.c | 159 ++++++++++++++++++++++++++++++++++++++++++++++
xen/arch/x86/physdev.c | 122 ++---------------------------------
xen/include/asm-x86/irq.h | 5 ++
3 files changed, 169 insertions(+), 117 deletions(-)
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 676ba5216f..5184b6144e 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2537,3 +2537,162 @@ bool_t hvm_domain_use_pirq(const struct domain *d, const struct pirq *pirq)
return is_hvm_domain(d) && pirq &&
pirq->arch.hvm.emuirq != IRQ_UNBOUND;
}
+
+static int allocate_pirq(struct domain *d, int index, int pirq, int irq,
+ int type, int *nr)
+{
+ int current_pirq;
+
+ ASSERT(spin_is_locked(&d->event_lock));
+ current_pirq = domain_irq_to_pirq(d, irq);
+ if ( pirq < 0 )
+ {
+ if ( current_pirq )
+ {
+ dprintk(XENLOG_G_ERR, "dom%d: %d:%d already mapped to %d\n",
+ d->domain_id, index, pirq, current_pirq);
+ if ( current_pirq < 0 )
+ return -EBUSY;
+ }
+ else if ( type == MAP_PIRQ_TYPE_MULTI_MSI )
+ {
+ if ( *nr <= 0 || *nr > 32 )
+ return -EDOM;
+ if ( *nr != 1 && !iommu_intremap )
+ return -EOPNOTSUPP;
+
+ while ( *nr & (*nr - 1) )
+ *nr += *nr & -*nr;
+ pirq = get_free_pirqs(d, *nr);
+ if ( pirq < 0 )
+ {
+ while ( (*nr >>= 1) > 1 )
+ if ( get_free_pirqs(d, *nr) > 0 )
+ break;
+ dprintk(XENLOG_G_ERR, "dom%d: no block of %d free pirqs\n",
+ d->domain_id, *nr << 1);
+ }
+ }
+ else
+ {
+ pirq = get_free_pirq(d, type);
+ if ( pirq < 0 )
+ dprintk(XENLOG_G_ERR, "dom%d: no free pirq\n", d->domain_id);
+ }
+ }
+ else if ( current_pirq && pirq != current_pirq )
+ {
+ dprintk(XENLOG_G_ERR, "dom%d: irq %d already mapped to pirq %d\n",
+ d->domain_id, irq, current_pirq);
+ return -EEXIST;
+ }
+
+ return pirq;
+}
+
+int allocate_and_map_gsi_pirq(struct domain *d, int index, int *pirq_p)
+{
+ int irq, pirq, ret;
+
+ if ( index < 0 || index >= nr_irqs_gsi )
+ {
+ dprintk(XENLOG_G_ERR, "dom%d: map invalid irq %d\n", d->domain_id,
+ index);
+ return -EINVAL;
+ }
+
+ irq = domain_pirq_to_irq(current->domain, index);
+ if ( irq <= 0 )
+ {
+ if ( is_hardware_domain(current->domain) )
+ irq = index;
+ else
+ {
+ dprintk(XENLOG_G_ERR, "dom%d: map pirq with incorrect irq!\n",
+ d->domain_id);
+ return -EINVAL;
+ }
+ }
+
+ /* Verify or get pirq. */
+ spin_lock(&d->event_lock);
+ pirq = allocate_pirq(d, index, *pirq_p, irq, MAP_PIRQ_TYPE_GSI, NULL);
+ if ( pirq < 0 )
+ {
+ ret = pirq;
+ goto done;
+ }
+
+ ret = map_domain_pirq(d, pirq, irq, MAP_PIRQ_TYPE_GSI, NULL);
+ if ( ret == 0 )
+ *pirq_p = pirq;
+
+ done:
+ spin_unlock(&d->event_lock);
+
+ return ret;
+}
+
+int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p,
+ int type, struct msi_info *msi)
+{
+ int irq, pirq, ret;
+
+ switch ( type )
+ {
+ case MAP_PIRQ_TYPE_MSI:
+ if ( !msi->table_base )
+ msi->entry_nr = 1;
+ irq = index;
+ if ( irq == -1 )
+ case MAP_PIRQ_TYPE_MULTI_MSI:
+ irq = create_irq(NUMA_NO_NODE);
+
+ if ( irq < nr_irqs_gsi || irq >= nr_irqs )
+ {
+ dprintk(XENLOG_G_ERR, "dom%d: can't create irq for msi!\n",
+ d->domain_id);
+ return -EINVAL;
+ }
+
+ msi->irq = irq;
+ break;
+
+ default:
+ dprintk(XENLOG_G_ERR, "dom%d: wrong pirq type %x\n",
+ d->domain_id, type);
+ ASSERT_UNREACHABLE();
+ return -EINVAL;
+ }
+
+ msi->irq = irq;
+
+ pcidevs_lock();
+ /* Verify or get pirq. */
+ spin_lock(&d->event_lock);
+ pirq = allocate_pirq(d, index, *pirq_p, irq, type, &msi->entry_nr);
+ if ( pirq < 0 )
+ {
+ ret = pirq;
+ goto done;
+ }
+
+ ret = map_domain_pirq(d, pirq, irq, type, msi);
+ if ( ret == 0 )
+ *pirq_p = pirq;
+
+ done:
+ spin_unlock(&d->event_lock);
+ pcidevs_unlock();
+ if ( ret != 0 )
+ switch ( type )
+ {
+ case MAP_PIRQ_TYPE_MSI:
+ if ( index == -1 )
+ case MAP_PIRQ_TYPE_MULTI_MSI:
+ destroy_irq(irq);
+ break;
+ }
+
+ return ret;
+}
diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index eec4a41231..0eb409758f 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -92,8 +92,7 @@ int physdev_map_pirq(domid_t domid, int type, int *index, int *pirq_p,
struct msi_info *msi)
{
struct domain *d = current->domain;
- int pirq, irq, ret = 0;
- void *map_data = NULL;
+ int ret;
if ( domid == DOMID_SELF && is_hvm_domain(d) && has_pirq(d) )
{
@@ -119,135 +118,24 @@ int physdev_map_pirq(domid_t domid, int type, int *index, int *pirq_p,
switch ( type )
{
case MAP_PIRQ_TYPE_GSI:
- if ( *index < 0 || *index >= nr_irqs_gsi )
- {
- dprintk(XENLOG_G_ERR, "dom%d: map invalid irq %d\n",
- d->domain_id, *index);
- ret = -EINVAL;
- goto free_domain;
- }
-
- irq = domain_pirq_to_irq(current->domain, *index);
- if ( irq <= 0 )
- {
- if ( is_hardware_domain(current->domain) )
- irq = *index;
- else {
- dprintk(XENLOG_G_ERR, "dom%d: map pirq with incorrect irq!\n",
- d->domain_id);
- ret = -EINVAL;
- goto free_domain;
- }
- }
+ ret = allocate_and_map_gsi_pirq(d, *index, pirq_p);
break;
case MAP_PIRQ_TYPE_MSI:
if ( !msi->table_base )
msi->entry_nr = 1;
- irq = *index;
- if ( irq == -1 )
+ /* fallthrough */
case MAP_PIRQ_TYPE_MULTI_MSI:
- irq = create_irq(NUMA_NO_NODE);
-
- if ( irq < nr_irqs_gsi || irq >= nr_irqs )
- {
- dprintk(XENLOG_G_ERR, "dom%d: can't create irq for msi!\n",
- d->domain_id);
- ret = -EINVAL;
- goto free_domain;
- }
-
- msi->irq = irq;
- map_data = msi;
+ ret = allocate_and_map_msi_pirq(d, *index, pirq_p, type, msi);
break;
default:
dprintk(XENLOG_G_ERR, "dom%d: wrong map_pirq type %x\n",
d->domain_id, type);
ret = -EINVAL;
- goto free_domain;
- }
-
- pcidevs_lock();
- /* Verify or get pirq. */
- spin_lock(&d->event_lock);
- pirq = domain_irq_to_pirq(d, irq);
- if ( *pirq_p < 0 )
- {
- if ( pirq )
- {
- dprintk(XENLOG_G_ERR, "dom%d: %d:%d already mapped to %d\n",
- d->domain_id, *index, *pirq_p, pirq);
- if ( pirq < 0 )
- {
- ret = -EBUSY;
- goto done;
- }
- }
- else if ( type == MAP_PIRQ_TYPE_MULTI_MSI )
- {
- if ( msi->entry_nr <= 0 || msi->entry_nr > 32 )
- ret = -EDOM;
- else if ( msi->entry_nr != 1 && !iommu_intremap )
- ret = -EOPNOTSUPP;
- else
- {
- while ( msi->entry_nr & (msi->entry_nr - 1) )
- msi->entry_nr += msi->entry_nr & -msi->entry_nr;
- pirq = get_free_pirqs(d, msi->entry_nr);
- if ( pirq < 0 )
- {
- while ( (msi->entry_nr >>= 1) > 1 )
- if ( get_free_pirqs(d, msi->entry_nr) > 0 )
- break;
- dprintk(XENLOG_G_ERR, "dom%d: no block of %d free pirqs\n",
- d->domain_id, msi->entry_nr << 1);
- ret = pirq;
- }
- }
- if ( ret < 0 )
- goto done;
- }
- else
- {
- pirq = get_free_pirq(d, type);
- if ( pirq < 0 )
- {
- dprintk(XENLOG_G_ERR, "dom%d: no free pirq\n", d->domain_id);
- ret = pirq;
- goto done;
- }
- }
- }
- else
- {
- if ( pirq && pirq != *pirq_p )
- {
- dprintk(XENLOG_G_ERR, "dom%d: pirq %d conflicts with irq %d\n",
- d->domain_id, *index, *pirq_p);
- ret = -EEXIST;
- goto done;
- }
- else
- pirq = *pirq_p;
+ break;
}
- ret = map_domain_pirq(d, pirq, irq, type, map_data);
- if ( ret == 0 )
- *pirq_p = pirq;
-
- done:
- spin_unlock(&d->event_lock);
- pcidevs_unlock();
- if ( ret != 0 )
- switch ( type )
- {
- case MAP_PIRQ_TYPE_MSI:
- if ( *index == -1 )
- case MAP_PIRQ_TYPE_MULTI_MSI:
- destroy_irq(irq);
- break;
- }
free_domain:
rcu_unlock_domain(d);
return ret;
diff --git a/xen/include/asm-x86/irq.h b/xen/include/asm-x86/irq.h
index ef625ebb13..182caa44be 100644
--- a/xen/include/asm-x86/irq.h
+++ b/xen/include/asm-x86/irq.h
@@ -200,4 +200,9 @@ bool_t cpu_has_pending_apic_eoi(void);
static inline void arch_move_irqs(struct vcpu *v) { }
+struct msi_info;
+int allocate_and_map_gsi_pirq(struct domain *d, int index, int *pirq_p);
+int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p,
+ int type, struct msi_info *msi);
+
#endif /* _ASM_HW_IRQ_H */
--
2.11.0 (Apple Git-81)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 2/4] x86/physdev: factor out the code to allocate and map a pirq
2017-06-01 11:49 ` [PATCH v4 2/4] x86/physdev: factor out the code to allocate and map a pirq Roger Pau Monne
@ 2017-06-01 14:20 ` Jan Beulich
2017-06-01 14:40 ` Andrew Cooper
1 sibling, 0 replies; 20+ messages in thread
From: Jan Beulich @ 2017-06-01 14:20 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, boris.ostrovsky, xen-devel
>>> On 01.06.17 at 13:49, <roger.pau@citrix.com> wrote:
> Move the code to allocate and map a domain pirq (either GSI or MSI)
> into the x86 irq code base, so that it can be used outside of the
> physdev ops.
>
> This change shouldn't affect the functionality of the already existing
> physdev ops.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/4] x86/physdev: factor out the code to allocate and map a pirq
2017-06-01 11:49 ` [PATCH v4 2/4] x86/physdev: factor out the code to allocate and map a pirq Roger Pau Monne
2017-06-01 14:20 ` Jan Beulich
@ 2017-06-01 14:40 ` Andrew Cooper
2017-06-01 15:20 ` Roger Pau Monne
1 sibling, 1 reply; 20+ messages in thread
From: Andrew Cooper @ 2017-06-01 14:40 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel, boris.ostrovsky, konrad.wilk; +Cc: Jan Beulich
On 01/06/17 12:49, Roger Pau Monne wrote:
> Move the code to allocate and map a domain pirq (either GSI or MSI)
> into the x86 irq code base, so that it can be used outside of the
> physdev ops.
>
> This change shouldn't affect the functionality of the already existing
> physdev ops.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
As you are moving code, please could you make some style fixes (which
can be done on commit if there are no other problems).
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> Changes since v3:
> - Pass the index parameter to the allocate_pirq function, so that it
> can be printed in case of error.
> - Remove pointless elses in allocate_pirq.
> - Return directly in the last check of allocate_pirq (instead of
> storing the error code in the pirq variable.
> - allocate_and_map_{gsi/msi}_pirq don't need index to be a pointer.
> - Fix the last parameter of the allocate_pirq call in
> allocate_and_map_gsi_pirq to use NULL (instead of 0).
> - Add an ASSERT_UNREACHABLE if the interrupt type passed to
> allocate_and_map_msi_pirq is not of MSI the MSI kind.
> - Restore newlines in the physdev_map_pirq switch case.
>
> Changes since v2:
> - Factor out the code to allocate the pirq.
> - Fix coding style issues.
> - Do not take the pci lock to bind a GSI.
> - Pass a type parameter to the MSI bind function.
>
> Changes since v1:
> - New in this version.
> ---
> xen/arch/x86/irq.c | 159 ++++++++++++++++++++++++++++++++++++++++++++++
> xen/arch/x86/physdev.c | 122 ++---------------------------------
> xen/include/asm-x86/irq.h | 5 ++
> 3 files changed, 169 insertions(+), 117 deletions(-)
>
> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> index 676ba5216f..5184b6144e 100644
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -2537,3 +2537,162 @@ bool_t hvm_domain_use_pirq(const struct domain *d, const struct pirq *pirq)
> return is_hvm_domain(d) && pirq &&
> pirq->arch.hvm.emuirq != IRQ_UNBOUND;
> }
> +
> +static int allocate_pirq(struct domain *d, int index, int pirq, int irq,
> + int type, int *nr)
> +{
> + int current_pirq;
> +
> + ASSERT(spin_is_locked(&d->event_lock));
> + current_pirq = domain_irq_to_pirq(d, irq);
> + if ( pirq < 0 )
> + {
> + if ( current_pirq )
> + {
> + dprintk(XENLOG_G_ERR, "dom%d: %d:%d already mapped to %d\n",
> + d->domain_id, index, pirq, current_pirq);
> + if ( current_pirq < 0 )
> + return -EBUSY;
> + }
> + else if ( type == MAP_PIRQ_TYPE_MULTI_MSI )
> + {
> + if ( *nr <= 0 || *nr > 32 )
> + return -EDOM;
> + if ( *nr != 1 && !iommu_intremap )
> + return -EOPNOTSUPP;
> +
> + while ( *nr & (*nr - 1) )
> + *nr += *nr & -*nr;
> + pirq = get_free_pirqs(d, *nr);
> + if ( pirq < 0 )
> + {
> + while ( (*nr >>= 1) > 1 )
> + if ( get_free_pirqs(d, *nr) > 0 )
> + break;
> + dprintk(XENLOG_G_ERR, "dom%d: no block of %d free pirqs\n",
> + d->domain_id, *nr << 1);
> + }
> + }
> + else
> + {
> + pirq = get_free_pirq(d, type);
> + if ( pirq < 0 )
> + dprintk(XENLOG_G_ERR, "dom%d: no free pirq\n", d->domain_id);
> + }
> + }
> + else if ( current_pirq && pirq != current_pirq )
> + {
> + dprintk(XENLOG_G_ERR, "dom%d: irq %d already mapped to pirq %d\n",
> + d->domain_id, irq, current_pirq);
> + return -EEXIST;
> + }
> +
> + return pirq;
> +}
> +
> +int allocate_and_map_gsi_pirq(struct domain *d, int index, int *pirq_p)
> +{
> + int irq, pirq, ret;
> +
> + if ( index < 0 || index >= nr_irqs_gsi )
> + {
> + dprintk(XENLOG_G_ERR, "dom%d: map invalid irq %d\n", d->domain_id,
> + index);
> + return -EINVAL;
> + }
> +
> + irq = domain_pirq_to_irq(current->domain, index);
> + if ( irq <= 0 )
> + {
> + if ( is_hardware_domain(current->domain) )
> + irq = index;
> + else
> + {
> + dprintk(XENLOG_G_ERR, "dom%d: map pirq with incorrect irq!\n",
> + d->domain_id);
> + return -EINVAL;
> + }
> + }
> +
> + /* Verify or get pirq. */
> + spin_lock(&d->event_lock);
> + pirq = allocate_pirq(d, index, *pirq_p, irq, MAP_PIRQ_TYPE_GSI, NULL);
> + if ( pirq < 0 )
> + {
> + ret = pirq;
> + goto done;
> + }
> +
> + ret = map_domain_pirq(d, pirq, irq, MAP_PIRQ_TYPE_GSI, NULL);
> + if ( ret == 0 )
> + *pirq_p = pirq;
> +
> + done:
> + spin_unlock(&d->event_lock);
> +
> + return ret;
> +}
> +
> +int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p,
> + int type, struct msi_info *msi)
> +{
> + int irq, pirq, ret;
> +
> + switch ( type )
> + {
> + case MAP_PIRQ_TYPE_MSI:
> + if ( !msi->table_base )
> + msi->entry_nr = 1;
> + irq = index;
> + if ( irq == -1 )
{
> + case MAP_PIRQ_TYPE_MULTI_MSI:
> + irq = create_irq(NUMA_NO_NODE);
}
> +
> + if ( irq < nr_irqs_gsi || irq >= nr_irqs )
> + {
> + dprintk(XENLOG_G_ERR, "dom%d: can't create irq for msi!\n",
> + d->domain_id);
> + return -EINVAL;
> + }
> +
> + msi->irq = irq;
> + break;
> +
> + default:
> + dprintk(XENLOG_G_ERR, "dom%d: wrong pirq type %x\n",
> + d->domain_id, type);
> + ASSERT_UNREACHABLE();
> + return -EINVAL;
> + }
> +
> + msi->irq = irq;
> +
> + pcidevs_lock();
> + /* Verify or get pirq. */
> + spin_lock(&d->event_lock);
> + pirq = allocate_pirq(d, index, *pirq_p, irq, type, &msi->entry_nr);
> + if ( pirq < 0 )
> + {
> + ret = pirq;
> + goto done;
> + }
> +
> + ret = map_domain_pirq(d, pirq, irq, type, msi);
> + if ( ret == 0 )
> + *pirq_p = pirq;
> +
> + done:
> + spin_unlock(&d->event_lock);
> + pcidevs_unlock();
> + if ( ret != 0 )
{
> + switch ( type )
> + {
> + case MAP_PIRQ_TYPE_MSI:
> + if ( index == -1 )
> + case MAP_PIRQ_TYPE_MULTI_MSI:
> + destroy_irq(irq);
> + break;
> + }
}
> +
> + return ret;
> +}
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 2/4] x86/physdev: factor out the code to allocate and map a pirq
2017-06-01 14:40 ` Andrew Cooper
@ 2017-06-01 15:20 ` Roger Pau Monne
0 siblings, 0 replies; 20+ messages in thread
From: Roger Pau Monne @ 2017-06-01 15:20 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, boris.ostrovsky, Jan Beulich
On Thu, Jun 01, 2017 at 03:40:07PM +0100, Andrew Cooper wrote:
> On 01/06/17 12:49, Roger Pau Monne wrote:
> > Move the code to allocate and map a domain pirq (either GSI or MSI)
> > into the x86 irq code base, so that it can be used outside of the
> > physdev ops.
> >
> > This change shouldn't affect the functionality of the already existing
> > physdev ops.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>
> As you are moving code, please could you make some style fixes (which
> can be done on commit if there are no other problems).
As Jan has already RB this, I suggest the person that commits this
applies the following diff on top (if you both agree it's fine).
---8<---
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 5184b6144e..37b4c7dd5a 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2645,8 +2645,10 @@ int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p,
msi->entry_nr = 1;
irq = index;
if ( irq == -1 )
+ {
case MAP_PIRQ_TYPE_MULTI_MSI:
irq = create_irq(NUMA_NO_NODE);
+ }
if ( irq < nr_irqs_gsi || irq >= nr_irqs )
{
@@ -2685,6 +2687,7 @@ int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p,
spin_unlock(&d->event_lock);
pcidevs_unlock();
if ( ret != 0 )
+ {
switch ( type )
{
case MAP_PIRQ_TYPE_MSI:
@@ -2693,6 +2696,7 @@ int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p,
destroy_irq(irq);
break;
}
+ }
return ret;
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v4 3/4] x86/pt: enable binding of GSIs to a PVH Dom0
2017-06-01 11:49 [PATCH v4 0/4] x86/dpci: bind legacy PCI interrupts to PVHv2 Dom0 Roger Pau Monne
2017-06-01 11:49 ` [PATCH v4 1/4] x86/pt: remove hvm_domid field from bind struct Roger Pau Monne
2017-06-01 11:49 ` [PATCH v4 2/4] x86/physdev: factor out the code to allocate and map a pirq Roger Pau Monne
@ 2017-06-01 11:49 ` Roger Pau Monne
2017-06-01 22:13 ` Boris Ostrovsky
2017-06-01 11:49 ` [PATCH v4 4/4] x86/vioapic: bind interrupts to " Roger Pau Monne
3 siblings, 1 reply; 20+ messages in thread
From: Roger Pau Monne @ 2017-06-01 11:49 UTC (permalink / raw)
To: xen-devel, boris.ostrovsky, konrad.wilk
Cc: Andrew Cooper, Jan Beulich, Roger Pau Monne
Achieve this by expanding pt_irq_create_bind in order to support mapping
interrupts of type PT_IRQ_TYPE_PCI to a PVH Dom0. GSIs bound to Dom0 are always
identity bound, which means the all the fields inside of the u.pci sub-struct
are ignored, and only the machine_irq is actually used in order to determine
which GSI the caller wants to bind.
Also, the hvm_irq_dpci struct is not used by a PVH Dom0, since that's used to
route interrupts and allow different host to guest GSI mappings, which is not
used by a PVH Dom0.
This requires adding some specific handlers for such directly mapped GSIs,
which bypass the PCI interrupt routing done by Xen for HVM guests.
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 v3:
- Rewrite the comment in hvm_gsi_assert.
- Unconditionally set gsi_assert_count to 0 in hvm_gsi_deassert.
- In the pirq timeout function do not defer the EOI for the identity
mapped case.
- Assert that the vIO APIC entry is not masked before checking the
trigger mode.
- In the failure path of pt_irq_create_bind check that girq and digl
are not NULL instead of relying on whether the domain is Dom0.
- In pt_irq_destroy_bind move a condition to the outer if in order to
avoid code indentation.
Changes since v2:
- Turn the assert in hvm_gsi_{de}assert into an assert_unreachable (like it's
done in __hvm_pci_intx_{de}assert.
- Do not increase/decrease gsi_assert_count, instead set it to 1/0.
- Fix a comment grammar error.
- Convert the pt_irq_create_bind asserts for bind type and pirq range into an
error path.
- Reduce the size of the message buffers, 24 should be enough.
- Allow pt_irq_create_bind to unbind hardware domain GSIs.
- s/__hvm_pirq_eoi/hvm_pirq_eoi/.
- Remove ASSERT(pirq_dpci) from hvm_pirq_eoi.
- Remove pirq_dpci local variable from hvm_gsi_eoi (it's used only once).
- s/__hvm_gsi_eoi/hvm_gsi_eoi/.
- Add a comment to document hvm_gsi_assert usage of gsi_assert_count.
Changes since v1:
- Remove the PT_IRQ_TYPE_GSI and instead just use PT_IRQ_TYPE_PCI with a
hardware domain special casing.
- Check the trigger mode of the Dom0 vIO APIC in order to set the shareable
flags in pt_irq_create_bind.
---
xen/arch/x86/hvm/irq.c | 42 +++++++++
xen/drivers/passthrough/io.c | 215 +++++++++++++++++++++++++++++++++----------
xen/include/xen/hvm/irq.h | 6 ++
3 files changed, 212 insertions(+), 51 deletions(-)
diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index 86255847a6..e425df913c 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -126,6 +126,48 @@ void hvm_pci_intx_deassert(
spin_unlock(&d->arch.hvm_domain.irq_lock);
}
+void hvm_gsi_assert(struct domain *d, unsigned int gsi)
+{
+ struct hvm_irq *hvm_irq = hvm_domain_irq(d);
+
+ if ( gsi >= hvm_irq->nr_gsis )
+ {
+ ASSERT_UNREACHABLE();
+ return;
+ }
+
+ /*
+ * __hvm_pci_intx_{de}assert uses a bitfield in pci_intx.i to track the
+ * status of each interrupt line, and Xen does the routing and GSI
+ * assertion based on that. The value of the pci_intx.i bitmap prevents the
+ * same line from triggering multiple times. As we don't use that bitmap
+ * for the hardware domain, Xen needs to rely on gsi_assert_count in order
+ * to know if the GSI is pending or not.
+ */
+ spin_lock(&d->arch.hvm_domain.irq_lock);
+ if ( !hvm_irq->gsi_assert_count[gsi] )
+ {
+ hvm_irq->gsi_assert_count[gsi] = 1;
+ assert_gsi(d, gsi);
+ }
+ spin_unlock(&d->arch.hvm_domain.irq_lock);
+}
+
+void hvm_gsi_deassert(struct domain *d, unsigned int gsi)
+{
+ struct hvm_irq *hvm_irq = hvm_domain_irq(d);
+
+ if ( gsi >= hvm_irq->nr_gsis )
+ {
+ ASSERT_UNREACHABLE();
+ return;
+ }
+
+ spin_lock(&d->arch.hvm_domain.irq_lock);
+ hvm_irq->gsi_assert_count[gsi] = 0;
+ spin_unlock(&d->arch.hvm_domain.irq_lock);
+}
+
void hvm_isa_irq_assert(
struct domain *d, unsigned int isa_irq)
{
diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
index e5a43e508f..a13f790f7e 100644
--- a/xen/drivers/passthrough/io.c
+++ b/xen/drivers/passthrough/io.c
@@ -164,6 +164,25 @@ static void pt_irq_time_out(void *data)
spin_lock(&irq_map->dom->event_lock);
+ if ( irq_map->flags & HVM_IRQ_DPCI_IDENTITY_GSI )
+ {
+ struct pirq *pirq = dpci_pirq(irq_map);
+
+ ASSERT(is_hardware_domain(irq_map->dom));
+ /*
+ * Identity mapped, no need to iterate over the guest GSI list to find
+ * other pirqs sharing the same guest GSI.
+ *
+ * In the identity mapped case the EOI can also be done now, this way
+ * the iteration over the list of domain pirqs is avoided.
+ */
+ hvm_gsi_deassert(irq_map->dom, pirq->pirq);
+ irq_map->flags |= HVM_IRQ_DPCI_EOI_LATCH;
+ pt_irq_guest_eoi(irq_map->dom, irq_map, NULL);
+ spin_unlock(&irq_map->dom->event_lock);
+ return;
+ }
+
dpci = domain_get_irq_dpci(irq_map->dom);
if ( unlikely(!dpci) )
{
@@ -274,10 +293,16 @@ int pt_irq_create_bind(
spin_lock(&d->event_lock);
hvm_irq_dpci = domain_get_irq_dpci(d);
- if ( hvm_irq_dpci == NULL )
+ if ( hvm_irq_dpci == NULL && !is_hardware_domain(d) )
{
unsigned int i;
+ /*
+ * NB: the hardware domain doesn't use a hvm_irq_dpci struct because
+ * it's only allowed to identity map GSIs, and so the data contained in
+ * that struct (used to map guest GSIs into machine GSIs and perform
+ * interrupt routing) it's completely useless to it.
+ */
hvm_irq_dpci = xzalloc(struct hvm_irq_dpci);
if ( hvm_irq_dpci == NULL )
{
@@ -422,35 +447,52 @@ int pt_irq_create_bind(
case PT_IRQ_TYPE_PCI:
case PT_IRQ_TYPE_MSI_TRANSLATE:
{
- unsigned int bus = pt_irq_bind->u.pci.bus;
- unsigned int device = pt_irq_bind->u.pci.device;
- unsigned int intx = pt_irq_bind->u.pci.intx;
- unsigned int guest_gsi = hvm_pci_intx_gsi(device, intx);
- unsigned int link = hvm_pci_intx_link(device, intx);
- struct dev_intx_gsi_link *digl = xmalloc(struct dev_intx_gsi_link);
- struct hvm_girq_dpci_mapping *girq =
- xmalloc(struct hvm_girq_dpci_mapping);
+ struct dev_intx_gsi_link *digl = NULL;
+ struct hvm_girq_dpci_mapping *girq = NULL;
+ unsigned int guest_gsi;
- if ( !digl || !girq )
+ /*
+ * Mapping GSIs for the hardware domain is different than doing it for
+ * an unpriviledged guest, the hardware domain is only allowed to
+ * identity map GSIs, and as such all the data in the u.pci union is
+ * discarded.
+ */
+ if ( !is_hardware_domain(d) )
{
- spin_unlock(&d->event_lock);
- xfree(girq);
- xfree(digl);
- return -ENOMEM;
- }
+ unsigned int link;
+
+ digl = xmalloc(struct dev_intx_gsi_link);
+ girq = xmalloc(struct hvm_girq_dpci_mapping);
- hvm_irq_dpci->link_cnt[link]++;
+ if ( !digl || !girq )
+ {
+ spin_unlock(&d->event_lock);
+ xfree(girq);
+ xfree(digl);
+ return -ENOMEM;
+ }
+
+ girq->bus = digl->bus = pt_irq_bind->u.pci.bus;
+ girq->device = digl->device = pt_irq_bind->u.pci.device;
+ girq->intx = digl->intx = pt_irq_bind->u.pci.intx;
+ list_add_tail(&digl->list, &pirq_dpci->digl_list);
- digl->bus = bus;
- digl->device = device;
- digl->intx = intx;
- list_add_tail(&digl->list, &pirq_dpci->digl_list);
+ guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx);
+ link = hvm_pci_intx_link(digl->device, digl->intx);
- girq->bus = bus;
- girq->device = device;
- girq->intx = intx;
- girq->machine_gsi = pirq;
- list_add_tail(&girq->list, &hvm_irq_dpci->girq[guest_gsi]);
+ hvm_irq_dpci->link_cnt[link]++;
+
+ girq->machine_gsi = pirq;
+ list_add_tail(&girq->list, &hvm_irq_dpci->girq[guest_gsi]);
+ }
+ else
+ {
+ /* MSI_TRANSLATE is not supported by the hardware domain. */
+ if ( pt_irq_bind->irq_type != PT_IRQ_TYPE_PCI ||
+ pirq >= hvm_domain_irq(d)->nr_gsis )
+ return -EINVAL;
+ guest_gsi = pirq;
+ }
/* Bind the same mirq once in the same domain */
if ( !(pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
@@ -472,7 +514,28 @@ 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;
+ 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;
+ }
}
/* Init timer before binding */
@@ -489,9 +552,16 @@ int pt_irq_create_bind(
* IRQ_GUEST is not set. As such we can reset 'dom' directly.
*/
pirq_dpci->dom = NULL;
- list_del(&girq->list);
- list_del(&digl->list);
- hvm_irq_dpci->link_cnt[link]--;
+ if ( girq || digl )
+ {
+ unsigned int link;
+
+ ASSERT(girq && digl);
+ list_del(&girq->list);
+ list_del(&digl->list);
+ link = hvm_pci_intx_link(digl->device, digl->intx);
+ hvm_irq_dpci->link_cnt[link]--;
+ }
pirq_dpci->flags = 0;
pirq_cleanup_check(info, d);
spin_unlock(&d->event_lock);
@@ -504,10 +574,17 @@ 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[24] = "";
+
+ 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);
+
+ printk(XENLOG_G_INFO "d%d: bind: m_gsi=%u g_gsi=%u%s\n",
+ d->domain_id, pirq, guest_gsi, buf);
+ }
break;
}
@@ -554,7 +631,7 @@ int pt_irq_destroy_bind(
hvm_irq_dpci = domain_get_irq_dpci(d);
- if ( hvm_irq_dpci == NULL )
+ if ( hvm_irq_dpci == NULL && !is_hardware_domain(d) )
{
spin_unlock(&d->event_lock);
return -EINVAL;
@@ -563,7 +640,7 @@ int pt_irq_destroy_bind(
pirq = pirq_info(d, machine_gsi);
pirq_dpci = pirq_dpci(pirq);
- if ( pt_irq_bind->irq_type != PT_IRQ_TYPE_MSI )
+ if ( hvm_irq_dpci && pt_irq_bind->irq_type != PT_IRQ_TYPE_MSI )
{
unsigned int bus = pt_irq_bind->u.pci.bus;
unsigned int device = pt_irq_bind->u.pci.device;
@@ -638,11 +715,15 @@ int pt_irq_destroy_bind(
if ( what && iommu_verbose )
{
unsigned int device = pt_irq_bind->u.pci.device;
+ char buf[24] = "";
+
+ if ( !is_hardware_domain(d) )
+ snprintf(buf, ARRAY_SIZE(buf), " dev=%02x.%02x.%u intx=%u",
+ pt_irq_bind->u.pci.bus, PCI_SLOT(device),
+ PCI_FUNC(device), pt_irq_bind->u.pci.intx);
- printk(XENLOG_G_INFO
- "d%d %s unmap: m_irq=%u dev=%02x:%02x.%u intx=%u\n",
- d->domain_id, what, machine_gsi, pt_irq_bind->u.pci.bus,
- PCI_SLOT(device), PCI_FUNC(device), pt_irq_bind->u.pci.intx);
+ printk(XENLOG_G_INFO "d%d %s unmap: m_irq=%u%s\n",
+ d->domain_id, what, machine_gsi, buf);
}
return 0;
@@ -696,7 +777,7 @@ int hvm_do_IRQ_dpci(struct domain *d, struct pirq *pirq)
struct hvm_irq_dpci *dpci = domain_get_irq_dpci(d);
struct hvm_pirq_dpci *pirq_dpci = pirq_dpci(pirq);
- if ( !iommu_enabled || !dpci || !pirq_dpci ||
+ if ( !iommu_enabled || (!is_hardware_domain(d) && !dpci) || !pirq_dpci ||
!(pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
return 0;
@@ -757,7 +838,7 @@ void hvm_dpci_msi_eoi(struct domain *d, int vector)
static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
{
- if ( unlikely(!hvm_domain_irq(d)->dpci) )
+ if ( unlikely(!hvm_domain_irq(d)->dpci) && !is_hardware_domain(d) )
{
ASSERT_UNREACHABLE();
return;
@@ -789,10 +870,17 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
list_for_each_entry ( digl, &pirq_dpci->digl_list, list )
{
+ ASSERT(!(pirq_dpci->flags & HVM_IRQ_DPCI_IDENTITY_GSI));
hvm_pci_intx_assert(d, digl->device, digl->intx);
pirq_dpci->pending++;
}
+ if ( pirq_dpci->flags & HVM_IRQ_DPCI_IDENTITY_GSI )
+ {
+ hvm_gsi_assert(d, pirq->pirq);
+ pirq_dpci->pending++;
+ }
+
if ( pirq_dpci->flags & HVM_IRQ_DPCI_TRANSLATE )
{
/* for translated MSI to INTx interrupt, eoi as early as possible */
@@ -814,17 +902,10 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
spin_unlock(&d->event_lock);
}
-static void __hvm_dpci_eoi(struct domain *d,
- const struct hvm_girq_dpci_mapping *girq,
- const union vioapic_redir_entry *ent)
+static void hvm_pirq_eoi(struct pirq *pirq,
+ const union vioapic_redir_entry *ent)
{
- struct pirq *pirq = pirq_info(d, girq->machine_gsi);
- struct hvm_pirq_dpci *pirq_dpci;
-
- if ( !hvm_domain_use_pirq(d, pirq) )
- hvm_pci_intx_deassert(d, girq->device, girq->intx);
-
- pirq_dpci = pirq_dpci(pirq);
+ struct hvm_pirq_dpci *pirq_dpci = pirq_dpci(pirq);
/*
* No need to get vector lock for timer
@@ -839,6 +920,31 @@ static void __hvm_dpci_eoi(struct domain *d,
pirq_guest_eoi(pirq);
}
+static void __hvm_dpci_eoi(struct domain *d,
+ const struct hvm_girq_dpci_mapping *girq,
+ const union vioapic_redir_entry *ent)
+{
+ struct pirq *pirq = pirq_info(d, girq->machine_gsi);
+
+ if ( !hvm_domain_use_pirq(d, pirq) )
+ hvm_pci_intx_deassert(d, girq->device, girq->intx);
+
+ hvm_pirq_eoi(pirq, ent);
+}
+
+static void hvm_gsi_eoi(struct domain *d, unsigned int gsi,
+ const union vioapic_redir_entry *ent)
+{
+ struct pirq *pirq = pirq_info(d, gsi);
+
+ /* Check if GSI is actually mapped. */
+ if ( !pirq_dpci(pirq) )
+ return;
+
+ hvm_gsi_deassert(d, gsi);
+ hvm_pirq_eoi(pirq, ent);
+}
+
void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi,
const union vioapic_redir_entry *ent)
{
@@ -848,6 +954,13 @@ void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi,
if ( !iommu_enabled )
return;
+ if ( is_hardware_domain(d) )
+ {
+ spin_lock(&d->event_lock);
+ hvm_gsi_eoi(d, guest_gsi, ent);
+ goto unlock;
+ }
+
if ( guest_gsi < NR_ISAIRQS )
{
hvm_dpci_isairq_eoi(d, guest_gsi);
diff --git a/xen/include/xen/hvm/irq.h b/xen/include/xen/hvm/irq.h
index 671a6f2e06..0d2c72c109 100644
--- a/xen/include/xen/hvm/irq.h
+++ b/xen/include/xen/hvm/irq.h
@@ -40,6 +40,7 @@ struct dev_intx_gsi_link {
#define _HVM_IRQ_DPCI_EOI_LATCH_SHIFT 3
#define _HVM_IRQ_DPCI_GUEST_PCI_SHIFT 4
#define _HVM_IRQ_DPCI_GUEST_MSI_SHIFT 5
+#define _HVM_IRQ_DPCI_IDENTITY_GSI_SHIFT 6
#define _HVM_IRQ_DPCI_TRANSLATE_SHIFT 15
#define HVM_IRQ_DPCI_MACH_PCI (1 << _HVM_IRQ_DPCI_MACH_PCI_SHIFT)
#define HVM_IRQ_DPCI_MACH_MSI (1 << _HVM_IRQ_DPCI_MACH_MSI_SHIFT)
@@ -47,6 +48,7 @@ struct dev_intx_gsi_link {
#define HVM_IRQ_DPCI_EOI_LATCH (1 << _HVM_IRQ_DPCI_EOI_LATCH_SHIFT)
#define HVM_IRQ_DPCI_GUEST_PCI (1 << _HVM_IRQ_DPCI_GUEST_PCI_SHIFT)
#define HVM_IRQ_DPCI_GUEST_MSI (1 << _HVM_IRQ_DPCI_GUEST_MSI_SHIFT)
+#define HVM_IRQ_DPCI_IDENTITY_GSI (1 << _HVM_IRQ_DPCI_IDENTITY_GSI_SHIFT)
#define HVM_IRQ_DPCI_TRANSLATE (1 << _HVM_IRQ_DPCI_TRANSLATE_SHIFT)
#define VMSI_DEST_ID_MASK 0xff
@@ -123,6 +125,10 @@ void hvm_isa_irq_assert(
void hvm_isa_irq_deassert(
struct domain *d, unsigned int isa_irq);
+/* Modify state of GSIs. */
+void hvm_gsi_assert(struct domain *d, unsigned int gsi);
+void hvm_gsi_deassert(struct domain *d, unsigned int gsi);
+
int hvm_set_pci_link_route(struct domain *d, u8 link, u8 isa_irq);
int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data);
--
2.11.0 (Apple Git-81)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 3/4] x86/pt: enable binding of GSIs to a PVH Dom0
2017-06-01 11:49 ` [PATCH v4 3/4] x86/pt: enable binding of GSIs to a PVH Dom0 Roger Pau Monne
@ 2017-06-01 22:13 ` Boris Ostrovsky
2017-06-02 8:41 ` Roger Pau Monne
0 siblings, 1 reply; 20+ messages in thread
From: Boris Ostrovsky @ 2017-06-01 22:13 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel, konrad.wilk; +Cc: Andrew Cooper, Jan Beulich
> @@ -696,7 +777,7 @@ int hvm_do_IRQ_dpci(struct domain *d, struct pirq *pirq)
> struct hvm_irq_dpci *dpci = domain_get_irq_dpci(d);
> struct hvm_pirq_dpci *pirq_dpci = pirq_dpci(pirq);
>
> - if ( !iommu_enabled || !dpci || !pirq_dpci ||
> + if ( !iommu_enabled || (!is_hardware_domain(d) && !dpci) || !pirq_dpci ||
> !(pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
> return 0;
We also need to return for !is_hvm_domain(d).
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 3/4] x86/pt: enable binding of GSIs to a PVH Dom0
2017-06-01 22:13 ` Boris Ostrovsky
@ 2017-06-02 8:41 ` Roger Pau Monne
2017-06-02 12:46 ` Boris Ostrovsky
0 siblings, 1 reply; 20+ messages in thread
From: Roger Pau Monne @ 2017-06-02 8:41 UTC (permalink / raw)
To: Boris Ostrovsky; +Cc: xen-devel, Andrew Cooper, Jan Beulich
On Thu, Jun 01, 2017 at 06:13:22PM -0400, Boris Ostrovsky wrote:
>
> > @@ -696,7 +777,7 @@ int hvm_do_IRQ_dpci(struct domain *d, struct pirq *pirq)
> > struct hvm_irq_dpci *dpci = domain_get_irq_dpci(d);
> > struct hvm_pirq_dpci *pirq_dpci = pirq_dpci(pirq);
> >
> > - if ( !iommu_enabled || !dpci || !pirq_dpci ||
> > + if ( !iommu_enabled || (!is_hardware_domain(d) && !dpci) || !pirq_dpci ||
> > !(pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
> > return 0;
>
> We also need to return for !is_hvm_domain(d).
Hm, I see. I guess this affects a PV Dom0 because it's a hardware
domain and doesn't have dpci. I will send a new version of this patch
alone.
In any case, pirq_dpci->flags shouldn't have the HVM_IRQ_DPCI_MAPPED
flag set for PV guests?
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 3/4] x86/pt: enable binding of GSIs to a PVH Dom0
2017-06-02 8:41 ` Roger Pau Monne
@ 2017-06-02 12:46 ` Boris Ostrovsky
2017-06-02 13:58 ` [PATCH v4.1 " Roger Pau Monne
0 siblings, 1 reply; 20+ messages in thread
From: Boris Ostrovsky @ 2017-06-02 12:46 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel, Andrew Cooper, Jan Beulich
On 06/02/2017 04:41 AM, Roger Pau Monne wrote:
> On Thu, Jun 01, 2017 at 06:13:22PM -0400, Boris Ostrovsky wrote:
>>> @@ -696,7 +777,7 @@ int hvm_do_IRQ_dpci(struct domain *d, struct pirq *pirq)
>>> struct hvm_irq_dpci *dpci = domain_get_irq_dpci(d);
>>> struct hvm_pirq_dpci *pirq_dpci = pirq_dpci(pirq);
>>>
>>> - if ( !iommu_enabled || !dpci || !pirq_dpci ||
>>> + if ( !iommu_enabled || (!is_hardware_domain(d) && !dpci) || !pirq_dpci ||
>>> !(pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
>>> return 0;
>> We also need to return for !is_hvm_domain(d).
> Hm, I see. I guess this affects a PV Dom0 because it's a hardware
> domain and doesn't have dpci. I will send a new version of this patch
> alone.
>
> In any case, pirq_dpci->flags shouldn't have the HVM_IRQ_DPCI_MAPPED
> flag set for PV guests?
These flags are part of arch_pirq.hvm and may have garbage on PV. At
least that's what I think I was seeing.
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4.1 3/4] x86/pt: enable binding of GSIs to a PVH Dom0
2017-06-02 12:46 ` Boris Ostrovsky
@ 2017-06-02 13:58 ` Roger Pau Monne
2017-06-07 13:17 ` Jan Beulich
0 siblings, 1 reply; 20+ messages in thread
From: Roger Pau Monne @ 2017-06-02 13:58 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, boris.ostrovsky, Jan Beulich, Roger Pau Monne
Achieve this by expanding pt_irq_create_bind in order to support mapping
interrupts of type PT_IRQ_TYPE_PCI to a PVH Dom0. GSIs bound to Dom0 are always
identity bound, which means the all the fields inside of the u.pci sub-struct
are ignored, and only the machine_irq is actually used in order to determine
which GSI the caller wants to bind.
Also, the hvm_irq_dpci struct is not used by a PVH Dom0, since that's used to
route interrupts and allow different host to guest GSI mappings, which is not
used by a PVH Dom0.
This requires adding some specific handlers for such directly mapped GSIs,
which bypass the PCI interrupt routing done by Xen for HVM guests.
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 v4:
- Add a check to make sure only HVM can use hvm_do_IRQ_dpci.
Changes since v3:
- Rewrite the comment in hvm_gsi_assert.
- Unconditionally set gsi_assert_count to 0 in hvm_gsi_deassert.
- In the pirq timeout function do not defer the EOI for the identity
mapped case.
- Assert that the vIO APIC entry is not masked before checking the
trigger mode.
- In the failure path of pt_irq_create_bind check that girq and digl
are not NULL instead of relying on whether the domain is Dom0.
- In pt_irq_destroy_bind move a condition to the outer if in order to
avoid code indentation.
Changes since v2:
- Turn the assert in hvm_gsi_{de}assert into an assert_unreachable (like it's
done in __hvm_pci_intx_{de}assert.
- Do not increase/decrease gsi_assert_count, instead set it to 1/0.
- Fix a comment grammar error.
- Convert the pt_irq_create_bind asserts for bind type and pirq range into an
error path.
- Reduce the size of the message buffers, 24 should be enough.
- Allow pt_irq_create_bind to unbind hardware domain GSIs.
- s/__hvm_pirq_eoi/hvm_pirq_eoi/.
- Remove ASSERT(pirq_dpci) from hvm_pirq_eoi.
- Remove pirq_dpci local variable from hvm_gsi_eoi (it's used only once).
- s/__hvm_gsi_eoi/hvm_gsi_eoi/.
- Add a comment to document hvm_gsi_assert usage of gsi_assert_count.
Changes since v1:
- Remove the PT_IRQ_TYPE_GSI and instead just use PT_IRQ_TYPE_PCI with a
hardware domain special casing.
- Check the trigger mode of the Dom0 vIO APIC in order to set the shareable
flags in pt_irq_create_bind.
---
xen/arch/x86/hvm/irq.c | 42 +++++++++
xen/drivers/passthrough/io.c | 216 +++++++++++++++++++++++++++++++++----------
xen/include/xen/hvm/irq.h | 6 ++
3 files changed, 213 insertions(+), 51 deletions(-)
diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index 86255847a6..e425df913c 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -126,6 +126,48 @@ void hvm_pci_intx_deassert(
spin_unlock(&d->arch.hvm_domain.irq_lock);
}
+void hvm_gsi_assert(struct domain *d, unsigned int gsi)
+{
+ struct hvm_irq *hvm_irq = hvm_domain_irq(d);
+
+ if ( gsi >= hvm_irq->nr_gsis )
+ {
+ ASSERT_UNREACHABLE();
+ return;
+ }
+
+ /*
+ * __hvm_pci_intx_{de}assert uses a bitfield in pci_intx.i to track the
+ * status of each interrupt line, and Xen does the routing and GSI
+ * assertion based on that. The value of the pci_intx.i bitmap prevents the
+ * same line from triggering multiple times. As we don't use that bitmap
+ * for the hardware domain, Xen needs to rely on gsi_assert_count in order
+ * to know if the GSI is pending or not.
+ */
+ spin_lock(&d->arch.hvm_domain.irq_lock);
+ if ( !hvm_irq->gsi_assert_count[gsi] )
+ {
+ hvm_irq->gsi_assert_count[gsi] = 1;
+ assert_gsi(d, gsi);
+ }
+ spin_unlock(&d->arch.hvm_domain.irq_lock);
+}
+
+void hvm_gsi_deassert(struct domain *d, unsigned int gsi)
+{
+ struct hvm_irq *hvm_irq = hvm_domain_irq(d);
+
+ if ( gsi >= hvm_irq->nr_gsis )
+ {
+ ASSERT_UNREACHABLE();
+ return;
+ }
+
+ spin_lock(&d->arch.hvm_domain.irq_lock);
+ hvm_irq->gsi_assert_count[gsi] = 0;
+ spin_unlock(&d->arch.hvm_domain.irq_lock);
+}
+
void hvm_isa_irq_assert(
struct domain *d, unsigned int isa_irq)
{
diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
index e5a43e508f..81e656f324 100644
--- a/xen/drivers/passthrough/io.c
+++ b/xen/drivers/passthrough/io.c
@@ -164,6 +164,25 @@ static void pt_irq_time_out(void *data)
spin_lock(&irq_map->dom->event_lock);
+ if ( irq_map->flags & HVM_IRQ_DPCI_IDENTITY_GSI )
+ {
+ struct pirq *pirq = dpci_pirq(irq_map);
+
+ ASSERT(is_hardware_domain(irq_map->dom));
+ /*
+ * Identity mapped, no need to iterate over the guest GSI list to find
+ * other pirqs sharing the same guest GSI.
+ *
+ * In the identity mapped case the EOI can also be done now, this way
+ * the iteration over the list of domain pirqs is avoided.
+ */
+ hvm_gsi_deassert(irq_map->dom, pirq->pirq);
+ irq_map->flags |= HVM_IRQ_DPCI_EOI_LATCH;
+ pt_irq_guest_eoi(irq_map->dom, irq_map, NULL);
+ spin_unlock(&irq_map->dom->event_lock);
+ return;
+ }
+
dpci = domain_get_irq_dpci(irq_map->dom);
if ( unlikely(!dpci) )
{
@@ -274,10 +293,16 @@ int pt_irq_create_bind(
spin_lock(&d->event_lock);
hvm_irq_dpci = domain_get_irq_dpci(d);
- if ( hvm_irq_dpci == NULL )
+ if ( hvm_irq_dpci == NULL && !is_hardware_domain(d) )
{
unsigned int i;
+ /*
+ * NB: the hardware domain doesn't use a hvm_irq_dpci struct because
+ * it's only allowed to identity map GSIs, and so the data contained in
+ * that struct (used to map guest GSIs into machine GSIs and perform
+ * interrupt routing) it's completely useless to it.
+ */
hvm_irq_dpci = xzalloc(struct hvm_irq_dpci);
if ( hvm_irq_dpci == NULL )
{
@@ -422,35 +447,52 @@ int pt_irq_create_bind(
case PT_IRQ_TYPE_PCI:
case PT_IRQ_TYPE_MSI_TRANSLATE:
{
- unsigned int bus = pt_irq_bind->u.pci.bus;
- unsigned int device = pt_irq_bind->u.pci.device;
- unsigned int intx = pt_irq_bind->u.pci.intx;
- unsigned int guest_gsi = hvm_pci_intx_gsi(device, intx);
- unsigned int link = hvm_pci_intx_link(device, intx);
- struct dev_intx_gsi_link *digl = xmalloc(struct dev_intx_gsi_link);
- struct hvm_girq_dpci_mapping *girq =
- xmalloc(struct hvm_girq_dpci_mapping);
+ struct dev_intx_gsi_link *digl = NULL;
+ struct hvm_girq_dpci_mapping *girq = NULL;
+ unsigned int guest_gsi;
- if ( !digl || !girq )
+ /*
+ * Mapping GSIs for the hardware domain is different than doing it for
+ * an unpriviledged guest, the hardware domain is only allowed to
+ * identity map GSIs, and as such all the data in the u.pci union is
+ * discarded.
+ */
+ if ( !is_hardware_domain(d) )
{
- spin_unlock(&d->event_lock);
- xfree(girq);
- xfree(digl);
- return -ENOMEM;
- }
+ unsigned int link;
+
+ digl = xmalloc(struct dev_intx_gsi_link);
+ girq = xmalloc(struct hvm_girq_dpci_mapping);
- hvm_irq_dpci->link_cnt[link]++;
+ if ( !digl || !girq )
+ {
+ spin_unlock(&d->event_lock);
+ xfree(girq);
+ xfree(digl);
+ return -ENOMEM;
+ }
+
+ girq->bus = digl->bus = pt_irq_bind->u.pci.bus;
+ girq->device = digl->device = pt_irq_bind->u.pci.device;
+ girq->intx = digl->intx = pt_irq_bind->u.pci.intx;
+ list_add_tail(&digl->list, &pirq_dpci->digl_list);
- digl->bus = bus;
- digl->device = device;
- digl->intx = intx;
- list_add_tail(&digl->list, &pirq_dpci->digl_list);
+ guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx);
+ link = hvm_pci_intx_link(digl->device, digl->intx);
- girq->bus = bus;
- girq->device = device;
- girq->intx = intx;
- girq->machine_gsi = pirq;
- list_add_tail(&girq->list, &hvm_irq_dpci->girq[guest_gsi]);
+ hvm_irq_dpci->link_cnt[link]++;
+
+ girq->machine_gsi = pirq;
+ list_add_tail(&girq->list, &hvm_irq_dpci->girq[guest_gsi]);
+ }
+ else
+ {
+ /* MSI_TRANSLATE is not supported by the hardware domain. */
+ if ( pt_irq_bind->irq_type != PT_IRQ_TYPE_PCI ||
+ pirq >= hvm_domain_irq(d)->nr_gsis )
+ return -EINVAL;
+ guest_gsi = pirq;
+ }
/* Bind the same mirq once in the same domain */
if ( !(pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
@@ -472,7 +514,28 @@ 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;
+ 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;
+ }
}
/* Init timer before binding */
@@ -489,9 +552,16 @@ int pt_irq_create_bind(
* IRQ_GUEST is not set. As such we can reset 'dom' directly.
*/
pirq_dpci->dom = NULL;
- list_del(&girq->list);
- list_del(&digl->list);
- hvm_irq_dpci->link_cnt[link]--;
+ if ( girq || digl )
+ {
+ unsigned int link;
+
+ ASSERT(girq && digl);
+ list_del(&girq->list);
+ list_del(&digl->list);
+ link = hvm_pci_intx_link(digl->device, digl->intx);
+ hvm_irq_dpci->link_cnt[link]--;
+ }
pirq_dpci->flags = 0;
pirq_cleanup_check(info, d);
spin_unlock(&d->event_lock);
@@ -504,10 +574,17 @@ 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[24] = "";
+
+ 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);
+
+ printk(XENLOG_G_INFO "d%d: bind: m_gsi=%u g_gsi=%u%s\n",
+ d->domain_id, pirq, guest_gsi, buf);
+ }
break;
}
@@ -554,7 +631,7 @@ int pt_irq_destroy_bind(
hvm_irq_dpci = domain_get_irq_dpci(d);
- if ( hvm_irq_dpci == NULL )
+ if ( hvm_irq_dpci == NULL && !is_hardware_domain(d) )
{
spin_unlock(&d->event_lock);
return -EINVAL;
@@ -563,7 +640,7 @@ int pt_irq_destroy_bind(
pirq = pirq_info(d, machine_gsi);
pirq_dpci = pirq_dpci(pirq);
- if ( pt_irq_bind->irq_type != PT_IRQ_TYPE_MSI )
+ if ( hvm_irq_dpci && pt_irq_bind->irq_type != PT_IRQ_TYPE_MSI )
{
unsigned int bus = pt_irq_bind->u.pci.bus;
unsigned int device = pt_irq_bind->u.pci.device;
@@ -638,11 +715,15 @@ int pt_irq_destroy_bind(
if ( what && iommu_verbose )
{
unsigned int device = pt_irq_bind->u.pci.device;
+ char buf[24] = "";
+
+ if ( !is_hardware_domain(d) )
+ snprintf(buf, ARRAY_SIZE(buf), " dev=%02x.%02x.%u intx=%u",
+ pt_irq_bind->u.pci.bus, PCI_SLOT(device),
+ PCI_FUNC(device), pt_irq_bind->u.pci.intx);
- printk(XENLOG_G_INFO
- "d%d %s unmap: m_irq=%u dev=%02x:%02x.%u intx=%u\n",
- d->domain_id, what, machine_gsi, pt_irq_bind->u.pci.bus,
- PCI_SLOT(device), PCI_FUNC(device), pt_irq_bind->u.pci.intx);
+ printk(XENLOG_G_INFO "d%d %s unmap: m_irq=%u%s\n",
+ d->domain_id, what, machine_gsi, buf);
}
return 0;
@@ -696,7 +777,8 @@ int hvm_do_IRQ_dpci(struct domain *d, struct pirq *pirq)
struct hvm_irq_dpci *dpci = domain_get_irq_dpci(d);
struct hvm_pirq_dpci *pirq_dpci = pirq_dpci(pirq);
- if ( !iommu_enabled || !dpci || !pirq_dpci ||
+ if ( !is_hvm_domain(d) || !iommu_enabled ||
+ (!is_hardware_domain(d) && !dpci) || !pirq_dpci ||
!(pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
return 0;
@@ -757,7 +839,7 @@ void hvm_dpci_msi_eoi(struct domain *d, int vector)
static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
{
- if ( unlikely(!hvm_domain_irq(d)->dpci) )
+ if ( unlikely(!hvm_domain_irq(d)->dpci) && !is_hardware_domain(d) )
{
ASSERT_UNREACHABLE();
return;
@@ -789,10 +871,17 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
list_for_each_entry ( digl, &pirq_dpci->digl_list, list )
{
+ ASSERT(!(pirq_dpci->flags & HVM_IRQ_DPCI_IDENTITY_GSI));
hvm_pci_intx_assert(d, digl->device, digl->intx);
pirq_dpci->pending++;
}
+ if ( pirq_dpci->flags & HVM_IRQ_DPCI_IDENTITY_GSI )
+ {
+ hvm_gsi_assert(d, pirq->pirq);
+ pirq_dpci->pending++;
+ }
+
if ( pirq_dpci->flags & HVM_IRQ_DPCI_TRANSLATE )
{
/* for translated MSI to INTx interrupt, eoi as early as possible */
@@ -814,17 +903,10 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
spin_unlock(&d->event_lock);
}
-static void __hvm_dpci_eoi(struct domain *d,
- const struct hvm_girq_dpci_mapping *girq,
- const union vioapic_redir_entry *ent)
+static void hvm_pirq_eoi(struct pirq *pirq,
+ const union vioapic_redir_entry *ent)
{
- struct pirq *pirq = pirq_info(d, girq->machine_gsi);
- struct hvm_pirq_dpci *pirq_dpci;
-
- if ( !hvm_domain_use_pirq(d, pirq) )
- hvm_pci_intx_deassert(d, girq->device, girq->intx);
-
- pirq_dpci = pirq_dpci(pirq);
+ struct hvm_pirq_dpci *pirq_dpci = pirq_dpci(pirq);
/*
* No need to get vector lock for timer
@@ -839,6 +921,31 @@ static void __hvm_dpci_eoi(struct domain *d,
pirq_guest_eoi(pirq);
}
+static void __hvm_dpci_eoi(struct domain *d,
+ const struct hvm_girq_dpci_mapping *girq,
+ const union vioapic_redir_entry *ent)
+{
+ struct pirq *pirq = pirq_info(d, girq->machine_gsi);
+
+ if ( !hvm_domain_use_pirq(d, pirq) )
+ hvm_pci_intx_deassert(d, girq->device, girq->intx);
+
+ hvm_pirq_eoi(pirq, ent);
+}
+
+static void hvm_gsi_eoi(struct domain *d, unsigned int gsi,
+ const union vioapic_redir_entry *ent)
+{
+ struct pirq *pirq = pirq_info(d, gsi);
+
+ /* Check if GSI is actually mapped. */
+ if ( !pirq_dpci(pirq) )
+ return;
+
+ hvm_gsi_deassert(d, gsi);
+ hvm_pirq_eoi(pirq, ent);
+}
+
void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi,
const union vioapic_redir_entry *ent)
{
@@ -848,6 +955,13 @@ void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi,
if ( !iommu_enabled )
return;
+ if ( is_hardware_domain(d) )
+ {
+ spin_lock(&d->event_lock);
+ hvm_gsi_eoi(d, guest_gsi, ent);
+ goto unlock;
+ }
+
if ( guest_gsi < NR_ISAIRQS )
{
hvm_dpci_isairq_eoi(d, guest_gsi);
diff --git a/xen/include/xen/hvm/irq.h b/xen/include/xen/hvm/irq.h
index 671a6f2e06..0d2c72c109 100644
--- a/xen/include/xen/hvm/irq.h
+++ b/xen/include/xen/hvm/irq.h
@@ -40,6 +40,7 @@ struct dev_intx_gsi_link {
#define _HVM_IRQ_DPCI_EOI_LATCH_SHIFT 3
#define _HVM_IRQ_DPCI_GUEST_PCI_SHIFT 4
#define _HVM_IRQ_DPCI_GUEST_MSI_SHIFT 5
+#define _HVM_IRQ_DPCI_IDENTITY_GSI_SHIFT 6
#define _HVM_IRQ_DPCI_TRANSLATE_SHIFT 15
#define HVM_IRQ_DPCI_MACH_PCI (1 << _HVM_IRQ_DPCI_MACH_PCI_SHIFT)
#define HVM_IRQ_DPCI_MACH_MSI (1 << _HVM_IRQ_DPCI_MACH_MSI_SHIFT)
@@ -47,6 +48,7 @@ struct dev_intx_gsi_link {
#define HVM_IRQ_DPCI_EOI_LATCH (1 << _HVM_IRQ_DPCI_EOI_LATCH_SHIFT)
#define HVM_IRQ_DPCI_GUEST_PCI (1 << _HVM_IRQ_DPCI_GUEST_PCI_SHIFT)
#define HVM_IRQ_DPCI_GUEST_MSI (1 << _HVM_IRQ_DPCI_GUEST_MSI_SHIFT)
+#define HVM_IRQ_DPCI_IDENTITY_GSI (1 << _HVM_IRQ_DPCI_IDENTITY_GSI_SHIFT)
#define HVM_IRQ_DPCI_TRANSLATE (1 << _HVM_IRQ_DPCI_TRANSLATE_SHIFT)
#define VMSI_DEST_ID_MASK 0xff
@@ -123,6 +125,10 @@ void hvm_isa_irq_assert(
void hvm_isa_irq_deassert(
struct domain *d, unsigned int isa_irq);
+/* Modify state of GSIs. */
+void hvm_gsi_assert(struct domain *d, unsigned int gsi);
+void hvm_gsi_deassert(struct domain *d, unsigned int gsi);
+
int hvm_set_pci_link_route(struct domain *d, u8 link, u8 isa_irq);
int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data);
--
2.11.0 (Apple Git-81)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4.1 3/4] x86/pt: enable binding of GSIs to a PVH Dom0
2017-06-02 13:58 ` [PATCH v4.1 " Roger Pau Monne
@ 2017-06-07 13:17 ` Jan Beulich
2017-06-19 16:45 ` Roger Pau Monne
0 siblings, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2017-06-07 13:17 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, boris.ostrovsky, xen-devel
>>> On 02.06.17 at 15:58, <roger.pau@citrix.com> wrote:
> --- a/xen/drivers/passthrough/io.c
> +++ b/xen/drivers/passthrough/io.c
> @@ -164,6 +164,25 @@ static void pt_irq_time_out(void *data)
>
> spin_lock(&irq_map->dom->event_lock);
>
> + if ( irq_map->flags & HVM_IRQ_DPCI_IDENTITY_GSI )
> + {
> + struct pirq *pirq = dpci_pirq(irq_map);
This could (and hence should) be const. However, ...
> + ASSERT(is_hardware_domain(irq_map->dom));
> + /*
> + * Identity mapped, no need to iterate over the guest GSI list to find
> + * other pirqs sharing the same guest GSI.
> + *
> + * In the identity mapped case the EOI can also be done now, this way
> + * the iteration over the list of domain pirqs is avoided.
> + */
> + hvm_gsi_deassert(irq_map->dom, pirq->pirq);
... this is its only use, so I'm not convinced a local variable is
needed at all.
> @@ -274,10 +293,16 @@ int pt_irq_create_bind(
> spin_lock(&d->event_lock);
>
> hvm_irq_dpci = domain_get_irq_dpci(d);
> - if ( hvm_irq_dpci == NULL )
> + if ( hvm_irq_dpci == NULL && !is_hardware_domain(d) )
Would you mind at once switching to the shorter !hvm_irq_dpci
(also further down), the more that you're using the inverse
without " != NULL" elsewhere?
> {
> unsigned int i;
>
> + /*
> + * NB: the hardware domain doesn't use a hvm_irq_dpci struct because
> + * it's only allowed to identity map GSIs, and so the data contained in
> + * that struct (used to map guest GSIs into machine GSIs and perform
> + * interrupt routing) it's completely useless to it.
"is completely ..."
> @@ -422,35 +447,52 @@ int pt_irq_create_bind(
> case PT_IRQ_TYPE_PCI:
> case PT_IRQ_TYPE_MSI_TRANSLATE:
> {
> - unsigned int bus = pt_irq_bind->u.pci.bus;
> - unsigned int device = pt_irq_bind->u.pci.device;
> - unsigned int intx = pt_irq_bind->u.pci.intx;
> - unsigned int guest_gsi = hvm_pci_intx_gsi(device, intx);
> - unsigned int link = hvm_pci_intx_link(device, intx);
> - struct dev_intx_gsi_link *digl = xmalloc(struct dev_intx_gsi_link);
> - struct hvm_girq_dpci_mapping *girq =
> - xmalloc(struct hvm_girq_dpci_mapping);
> + struct dev_intx_gsi_link *digl = NULL;
> + struct hvm_girq_dpci_mapping *girq = NULL;
> + unsigned int guest_gsi;
>
> - if ( !digl || !girq )
> + /*
> + * Mapping GSIs for the hardware domain is different than doing it for
> + * an unpriviledged guest, the hardware domain is only allowed to
> + * identity map GSIs, and as such all the data in the u.pci union is
> + * discarded.
> + */
> + if ( !is_hardware_domain(d) )
I think I did indicate before that it would feel more safe if you
checked hvm_irq_dpci here (which is NULL if and only if d is
hwdom). At the very least I'd expect a respective ASSERT()
below (but I think the alternative condition here and
ASSERT(is_hardware_domain(d)) in the "else" block would be
better).
> {
> - spin_unlock(&d->event_lock);
> - xfree(girq);
> - xfree(digl);
> - return -ENOMEM;
> - }
> + unsigned int link;
> +
> + digl = xmalloc(struct dev_intx_gsi_link);
> + girq = xmalloc(struct hvm_girq_dpci_mapping);
>
> - hvm_irq_dpci->link_cnt[link]++;
> + if ( !digl || !girq )
> + {
> + spin_unlock(&d->event_lock);
> + xfree(girq);
> + xfree(digl);
> + return -ENOMEM;
> + }
> +
> + girq->bus = digl->bus = pt_irq_bind->u.pci.bus;
> + girq->device = digl->device = pt_irq_bind->u.pci.device;
> + girq->intx = digl->intx = pt_irq_bind->u.pci.intx;
> + list_add_tail(&digl->list, &pirq_dpci->digl_list);
>
> - digl->bus = bus;
> - digl->device = device;
> - digl->intx = intx;
> - list_add_tail(&digl->list, &pirq_dpci->digl_list);
> + guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx);
> + link = hvm_pci_intx_link(digl->device, digl->intx);
>
> - girq->bus = bus;
> - girq->device = device;
> - girq->intx = intx;
> - girq->machine_gsi = pirq;
> - list_add_tail(&girq->list, &hvm_irq_dpci->girq[guest_gsi]);
> + hvm_irq_dpci->link_cnt[link]++;
> +
> + girq->machine_gsi = pirq;
> + list_add_tail(&girq->list, &hvm_irq_dpci->girq[guest_gsi]);
> + }
> + else
> + {
> + /* MSI_TRANSLATE is not supported by the hardware domain. */
s/by/for/ ?
> @@ -472,7 +514,28 @@ 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;
> + struct hvm_vioapic *vioapic = gsi_vioapic(d, guest_gsi,
> + &pin);
const
> @@ -489,9 +552,16 @@ int pt_irq_create_bind(
> * IRQ_GUEST is not set. As such we can reset 'dom'
> directly.
> */
> pirq_dpci->dom = NULL;
> - list_del(&girq->list);
> - list_del(&digl->list);
> - hvm_irq_dpci->link_cnt[link]--;
> + if ( girq || digl )
> + {
> + unsigned int link;
> +
> + ASSERT(girq && digl);
Perhaps even "ASSERT(girq && digl && hvm_irq_dpci)" or follow the
model outlined above for consistency?
> @@ -504,10 +574,17 @@ 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[24] = "";
> +
> + 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);
Perhaps again better "if ( digl )".
> @@ -696,7 +777,8 @@ int hvm_do_IRQ_dpci(struct domain *d, struct pirq *pirq)
> struct hvm_irq_dpci *dpci = domain_get_irq_dpci(d);
> struct hvm_pirq_dpci *pirq_dpci = pirq_dpci(pirq);
>
> - if ( !iommu_enabled || !dpci || !pirq_dpci ||
> + if ( !is_hvm_domain(d) || !iommu_enabled ||
> + (!is_hardware_domain(d) && !dpci) || !pirq_dpci ||
> !(pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
> return 0;
So why again do we suddenly need !is_hvm_domain() here? With
the name of the function there shouldn't be any caller invoking it
for a PV guest.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4.1 3/4] x86/pt: enable binding of GSIs to a PVH Dom0
2017-06-07 13:17 ` Jan Beulich
@ 2017-06-19 16:45 ` Roger Pau Monne
2017-06-20 7:19 ` Jan Beulich
0 siblings, 1 reply; 20+ messages in thread
From: Roger Pau Monne @ 2017-06-19 16:45 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, boris.ostrovsky, xen-devel
On Wed, Jun 07, 2017 at 07:17:16AM -0600, Jan Beulich wrote:
> >>> On 02.06.17 at 15:58, <roger.pau@citrix.com> wrote:
> > --- a/xen/drivers/passthrough/io.c
> > +++ b/xen/drivers/passthrough/io.c
> > @@ -164,6 +164,25 @@ static void pt_irq_time_out(void *data)
> >
> > spin_lock(&irq_map->dom->event_lock);
> >
> > + if ( irq_map->flags & HVM_IRQ_DPCI_IDENTITY_GSI )
> > + {
> > + struct pirq *pirq = dpci_pirq(irq_map);
>
> This could (and hence should) be const. However, ...
>
> > + ASSERT(is_hardware_domain(irq_map->dom));
> > + /*
> > + * Identity mapped, no need to iterate over the guest GSI list to find
> > + * other pirqs sharing the same guest GSI.
> > + *
> > + * In the identity mapped case the EOI can also be done now, this way
> > + * the iteration over the list of domain pirqs is avoided.
> > + */
> > + hvm_gsi_deassert(irq_map->dom, pirq->pirq);
>
> ... this is its only use, so I'm not convinced a local variable is
> needed at all.
Done, I've removed the local variable.
> > @@ -274,10 +293,16 @@ int pt_irq_create_bind(
> > spin_lock(&d->event_lock);
> >
> > hvm_irq_dpci = domain_get_irq_dpci(d);
> > - if ( hvm_irq_dpci == NULL )
> > + if ( hvm_irq_dpci == NULL && !is_hardware_domain(d) )
>
> Would you mind at once switching to the shorter !hvm_irq_dpci
> (also further down), the more that you're using the inverse
> without " != NULL" elsewhere?
Done.
> > {
> > unsigned int i;
> >
> > + /*
> > + * NB: the hardware domain doesn't use a hvm_irq_dpci struct because
> > + * it's only allowed to identity map GSIs, and so the data contained in
> > + * that struct (used to map guest GSIs into machine GSIs and perform
> > + * interrupt routing) it's completely useless to it.
>
> "is completely ..."
Fixed.
> > @@ -422,35 +447,52 @@ int pt_irq_create_bind(
> > case PT_IRQ_TYPE_PCI:
> > case PT_IRQ_TYPE_MSI_TRANSLATE:
> > {
> > - unsigned int bus = pt_irq_bind->u.pci.bus;
> > - unsigned int device = pt_irq_bind->u.pci.device;
> > - unsigned int intx = pt_irq_bind->u.pci.intx;
> > - unsigned int guest_gsi = hvm_pci_intx_gsi(device, intx);
> > - unsigned int link = hvm_pci_intx_link(device, intx);
> > - struct dev_intx_gsi_link *digl = xmalloc(struct dev_intx_gsi_link);
> > - struct hvm_girq_dpci_mapping *girq =
> > - xmalloc(struct hvm_girq_dpci_mapping);
> > + struct dev_intx_gsi_link *digl = NULL;
> > + struct hvm_girq_dpci_mapping *girq = NULL;
> > + unsigned int guest_gsi;
> >
> > - if ( !digl || !girq )
> > + /*
> > + * Mapping GSIs for the hardware domain is different than doing it for
> > + * an unpriviledged guest, the hardware domain is only allowed to
> > + * identity map GSIs, and as such all the data in the u.pci union is
> > + * discarded.
> > + */
> > + if ( !is_hardware_domain(d) )
>
> I think I did indicate before that it would feel more safe if you
> checked hvm_irq_dpci here (which is NULL if and only if d is
> hwdom). At the very least I'd expect a respective ASSERT()
> below (but I think the alternative condition here and
> ASSERT(is_hardware_domain(d)) in the "else" block would be
> better).
I've changed this to:
if ( hvm_irq_dpci )
{
...
}
else
{
ASSERT(is_hardware_domain(d));
...
}
> > {
> > - spin_unlock(&d->event_lock);
> > - xfree(girq);
> > - xfree(digl);
> > - return -ENOMEM;
> > - }
> > + unsigned int link;
> > +
> > + digl = xmalloc(struct dev_intx_gsi_link);
> > + girq = xmalloc(struct hvm_girq_dpci_mapping);
> >
> > - hvm_irq_dpci->link_cnt[link]++;
> > + if ( !digl || !girq )
> > + {
> > + spin_unlock(&d->event_lock);
> > + xfree(girq);
> > + xfree(digl);
> > + return -ENOMEM;
> > + }
> > +
> > + girq->bus = digl->bus = pt_irq_bind->u.pci.bus;
> > + girq->device = digl->device = pt_irq_bind->u.pci.device;
> > + girq->intx = digl->intx = pt_irq_bind->u.pci.intx;
> > + list_add_tail(&digl->list, &pirq_dpci->digl_list);
> >
> > - digl->bus = bus;
> > - digl->device = device;
> > - digl->intx = intx;
> > - list_add_tail(&digl->list, &pirq_dpci->digl_list);
> > + guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx);
> > + link = hvm_pci_intx_link(digl->device, digl->intx);
> >
> > - girq->bus = bus;
> > - girq->device = device;
> > - girq->intx = intx;
> > - girq->machine_gsi = pirq;
> > - list_add_tail(&girq->list, &hvm_irq_dpci->girq[guest_gsi]);
> > + hvm_irq_dpci->link_cnt[link]++;
> > +
> > + girq->machine_gsi = pirq;
> > + list_add_tail(&girq->list, &hvm_irq_dpci->girq[guest_gsi]);
> > + }
> > + else
> > + {
> > + /* MSI_TRANSLATE is not supported by the hardware domain. */
>
> s/by/for/ ?
OK. I guess this is better because d is a target in this context?
> > @@ -472,7 +514,28 @@ 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;
> > + struct hvm_vioapic *vioapic = gsi_vioapic(d, guest_gsi,
> > + &pin);
>
> const
>
> > @@ -489,9 +552,16 @@ int pt_irq_create_bind(
> > * IRQ_GUEST is not set. As such we can reset 'dom'
> > directly.
> > */
> > pirq_dpci->dom = NULL;
> > - list_del(&girq->list);
> > - list_del(&digl->list);
> > - hvm_irq_dpci->link_cnt[link]--;
> > + if ( girq || digl )
> > + {
> > + unsigned int link;
> > +
> > + ASSERT(girq && digl);
>
> Perhaps even "ASSERT(girq && digl && hvm_irq_dpci)" or follow the
> model outlined above for consistency?
I've changed the condition to "if ( hvm_irq_dpci )" and left the ASSERT
as-is.
> > @@ -504,10 +574,17 @@ 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[24] = "";
> > +
> > + 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);
>
> Perhaps again better "if ( digl )".
Yes, I think that's better.
> > @@ -696,7 +777,8 @@ int hvm_do_IRQ_dpci(struct domain *d, struct pirq *pirq)
> > struct hvm_irq_dpci *dpci = domain_get_irq_dpci(d);
> > struct hvm_pirq_dpci *pirq_dpci = pirq_dpci(pirq);
> >
> > - if ( !iommu_enabled || !dpci || !pirq_dpci ||
> > + if ( !is_hvm_domain(d) || !iommu_enabled ||
> > + (!is_hardware_domain(d) && !dpci) || !pirq_dpci ||
> > !(pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
> > return 0;
>
> So why again do we suddenly need !is_hvm_domain() here? With
> the name of the function there shouldn't be any caller invoking it
> for a PV guest.
Sadly there is, in __do_IRQ_guest the following is used:
if ( !hvm_do_IRQ_dpci(d, pirq) )
send_guest_pirq(d, pirq);
Without checking if d is a HVM or PV guest, so the check is needed (or
needs to be moved further up in the call chain into __do_IRQ_guest).
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4.1 3/4] x86/pt: enable binding of GSIs to a PVH Dom0
2017-06-19 16:45 ` Roger Pau Monne
@ 2017-06-20 7:19 ` Jan Beulich
0 siblings, 0 replies; 20+ messages in thread
From: Jan Beulich @ 2017-06-20 7:19 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, boris.ostrovsky, xen-devel
>>> On 19.06.17 at 18:45, <roger.pau@citrix.com> wrote:
> On Wed, Jun 07, 2017 at 07:17:16AM -0600, Jan Beulich wrote:
>> >>> On 02.06.17 at 15:58, <roger.pau@citrix.com> wrote:
>> > + else
>> > + {
>> > + /* MSI_TRANSLATE is not supported by the hardware domain. */
>>
>> s/by/for/ ?
>
> OK. I guess this is better because d is a target in this context?
Yes (as long as my understanding of English prepositions is
correct, which admittedly it often isn't).
>> > @@ -696,7 +777,8 @@ int hvm_do_IRQ_dpci(struct domain *d, struct pirq *pirq)
>> > struct hvm_irq_dpci *dpci = domain_get_irq_dpci(d);
>> > struct hvm_pirq_dpci *pirq_dpci = pirq_dpci(pirq);
>> >
>> > - if ( !iommu_enabled || !dpci || !pirq_dpci ||
>> > + if ( !is_hvm_domain(d) || !iommu_enabled ||
>> > + (!is_hardware_domain(d) && !dpci) || !pirq_dpci ||
>> > !(pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
>> > return 0;
>>
>> So why again do we suddenly need !is_hvm_domain() here? With
>> the name of the function there shouldn't be any caller invoking it
>> for a PV guest.
>
> Sadly there is, in __do_IRQ_guest the following is used:
>
> if ( !hvm_do_IRQ_dpci(d, pirq) )
> send_guest_pirq(d, pirq);
>
> Without checking if d is a HVM or PV guest, so the check is needed (or
> needs to be moved further up in the call chain into __do_IRQ_guest).
Indeed I think callers of hvm_*() functions should make sure they
don't call them on non-HVM domains/vcpus.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 4/4] x86/vioapic: bind interrupts to PVH Dom0
2017-06-01 11:49 [PATCH v4 0/4] x86/dpci: bind legacy PCI interrupts to PVHv2 Dom0 Roger Pau Monne
` (2 preceding siblings ...)
2017-06-01 11:49 ` [PATCH v4 3/4] x86/pt: enable binding of GSIs to a PVH Dom0 Roger Pau Monne
@ 2017-06-01 11:49 ` Roger Pau Monne
2017-06-07 13:20 ` Jan Beulich
3 siblings, 1 reply; 20+ messages in thread
From: Roger Pau Monne @ 2017-06-01 11:49 UTC (permalink / raw)
To: xen-devel, boris.ostrovsky, konrad.wilk
Cc: Andrew Cooper, Jan Beulich, Roger Pau Monne
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 v3:
- Setup the binding after writing the modified RTE fields back into
the vIO APIC struct, or else pt_irq_create_bind will fetch the
wrong trigger mode.
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(+)
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index abcc473c88..64cc75a52a 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)
@@ -190,6 +236,20 @@ static void vioapic_write_redirent(
*pent = ent;
+ 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. */
+ pent->fields.mask = 1;
+ unmasked = 0;
+ }
+ }
+
if ( gsi == 0 )
{
vlapic_adjust_i8259_target(d);
--
2.11.0 (Apple Git-81)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 4/4] x86/vioapic: bind interrupts to PVH Dom0
2017-06-01 11:49 ` [PATCH v4 4/4] x86/vioapic: bind interrupts to " Roger Pau Monne
@ 2017-06-07 13:20 ` Jan Beulich
2017-06-19 17:04 ` Roger Pau Monne
0 siblings, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2017-06-07 13:20 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, boris.ostrovsky, xen-devel
>>> On 01.06.17 at 13:49, <roger.pau@citrix.com> wrote:
> --- 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;
currd or curr_d please. With that
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 4/4] x86/vioapic: bind interrupts to PVH Dom0
2017-06-07 13:20 ` Jan Beulich
@ 2017-06-19 17:04 ` Roger Pau Monne
0 siblings, 0 replies; 20+ messages in thread
From: Roger Pau Monne @ 2017-06-19 17:04 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, boris.ostrovsky, xen-devel
On Wed, Jun 07, 2017 at 07:20:38AM -0600, Jan Beulich wrote:
> >>> On 01.06.17 at 13:49, <roger.pau@citrix.com> wrote:
> > --- 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;
>
> currd or curr_d please. With that
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
I've changed it and added your RB, thanks.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 20+ messages in thread