Skip to content

Commit

Permalink
x86/dpci: remove the dpci EOI timer
Browse files Browse the repository at this point in the history 
Current interrupt pass though code will setup a timer for each
interrupt injected to the guest that requires an EOI from the guest.
Such timer would perform two actions if the guest doesn't EOI the
interrupt before a given period of time. The first one is deasserting
the virtual line, the second is perform an EOI of the physical
interrupt source if it requires such.

The deasserting of the guest virtual line is wrong, since it messes
with the interrupt status of the guest. This seems to have been done
in order to compensate for missing deasserts when certain interrupt
controller actions are performed. The original motivation of the
introduction of the timer was to fix issues when a GSI was shared
between different guests. We believe that other changes in the
interrupt handling code (ie: proper propagation of EOI related actions
to dpci) will have fixed such errors now.

Performing an EOI of the physical interrupt source is redundant, since
there's already a timer that takes care of this for all interrupts,
not just the HVM dpci ones, see irq_guest_action_t struct eoi_timer
field.

Since both of the actions performed by the dpci timer are not
required, remove it altogether.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
  • Loading branch information
@royger @jbeulich
royger authored and jbeulich committed Apr 20, 2021
1 parent 2d494f2 commit 730d0f6
Show file tree
Hide file tree
Showing 4 changed files with 2 additions and 104 deletions.
3 changes: 0 additions & 3 deletions xen/drivers/passthrough/vtd/x86/hvm.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,7 @@ static int _hvm_dpci_isairq_eoi(struct domain *d,
{
hvm_pci_intx_deassert(d, digl->device, digl->intx);
if ( --pirq_dpci->pending == 0 )
{
stop_timer(&pirq_dpci->timer);
pirq_guest_eoi(dpci_pirq(pirq_dpci));
}
}
}

Expand Down
95 changes: 2 additions & 93 deletions xen/drivers/passthrough/x86/hvm.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,77 +136,6 @@ static void pt_pirq_softirq_reset(struct hvm_pirq_dpci *pirq_dpci)
pirq_dpci->masked = 0;
}

bool pt_irq_need_timer(uint32_t flags)
{
return !(flags & (HVM_IRQ_DPCI_GUEST_MSI | HVM_IRQ_DPCI_TRANSLATE |
HVM_IRQ_DPCI_NO_EOI));
}

static int pt_irq_guest_eoi(struct domain *d, struct hvm_pirq_dpci *pirq_dpci,
void *arg)
{
if ( __test_and_clear_bit(_HVM_IRQ_DPCI_EOI_LATCH_SHIFT,
&pirq_dpci->flags) )
{
pirq_dpci->masked = 0;
pirq_dpci->pending = 0;
pirq_guest_eoi(dpci_pirq(pirq_dpci));
}

return 0;
}

static void pt_irq_time_out(void *data)
{
struct hvm_pirq_dpci *irq_map = data;
const struct hvm_irq_dpci *dpci;
const struct dev_intx_gsi_link *digl;

spin_lock(&irq_map->dom->event_lock);

if ( irq_map->flags & HVM_IRQ_DPCI_IDENTITY_GSI )
{
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, dpci_pirq(irq_map)->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) )
{
ASSERT_UNREACHABLE();
spin_unlock(&irq_map->dom->event_lock);
return;
}
list_for_each_entry ( digl, &irq_map->digl_list, list )
{
unsigned int guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx);
const struct hvm_girq_dpci_mapping *girq;

list_for_each_entry ( girq, &dpci->girq[guest_gsi], list )
{
struct pirq *pirq = pirq_info(irq_map->dom, girq->machine_gsi);

pirq_dpci(pirq)->flags |= HVM_IRQ_DPCI_EOI_LATCH;
}
hvm_pci_intx_deassert(irq_map->dom, digl->device, digl->intx);
}

pt_pirq_iterate(irq_map->dom, pt_irq_guest_eoi, NULL);

spin_unlock(&irq_map->dom->event_lock);
}

struct hvm_irq_dpci *domain_get_irq_dpci(const struct domain *d)
{
if ( !d || !is_hvm_domain(d) )
Expand Down Expand Up @@ -568,15 +497,10 @@ int pt_irq_create_bind(
}
}

/* Init timer before binding */
if ( pt_irq_need_timer(pirq_dpci->flags) )
init_timer(&pirq_dpci->timer, pt_irq_time_out, pirq_dpci, 0);
/* Deal with gsi for legacy devices */
rc = pirq_guest_bind(d->vcpu[0], info, share);
if ( unlikely(rc) )
{
if ( pt_irq_need_timer(pirq_dpci->flags) )
kill_timer(&pirq_dpci->timer);
/*
* There is no path for __do_IRQ to schedule softirq as
* IRQ_GUEST is not set. As such we can reset 'dom' directly.
Expand Down Expand Up @@ -743,8 +667,6 @@ int pt_irq_destroy_bind(
{
pirq_guest_unbind(d, pirq);
msixtbl_pt_unregister(d, pirq);
if ( pt_irq_need_timer(pirq_dpci->flags) )
kill_timer(&pirq_dpci->timer);
pirq_dpci->flags = 0;
/*
* See comment in pt_irq_create_bind's PT_IRQ_TYPE_MSI before the
Expand Down Expand Up @@ -934,16 +856,6 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
__msi_pirq_eoi(pirq_dpci);
goto out;
}

/*
* Set a timer to see if the guest can finish the interrupt or not. For
* example, the guest OS may unmask the PIC during boot, before the
* guest driver is loaded. hvm_pci_intx_assert() may succeed, but the
* guest will never deal with the irq, then the physical interrupt line
* will never be deasserted.
*/
ASSERT(pt_irq_need_timer(pirq_dpci->flags));
set_timer(&pirq_dpci->timer, NOW() + PT_IRQ_TIME_OUT);
}

out:
Expand All @@ -967,10 +879,10 @@ static void hvm_pirq_eoi(struct pirq *pirq)
* since interrupt is still not EOIed
*/
if ( --pirq_dpci->pending ||
!pt_irq_need_timer(pirq_dpci->flags) )
/* When the interrupt source is MSI no Ack should be performed. */
(pirq_dpci->flags & HVM_IRQ_DPCI_TRANSLATE) )
return;

stop_timer(&pirq_dpci->timer);
pirq_guest_eoi(pirq);
}

Expand Down Expand Up @@ -1042,9 +954,6 @@ static int pci_clean_dpci_irq(struct domain *d,

pirq_guest_unbind(d, dpci_pirq(pirq_dpci));

if ( pt_irq_need_timer(pirq_dpci->flags) )
kill_timer(&pirq_dpci->timer);

list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
{
list_del(&digl->list);
Expand Down
3 changes: 0 additions & 3 deletions xen/include/asm-x86/hvm/irq.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,6 @@ struct dev_intx_gsi_link {
#define _HVM_IRQ_DPCI_MACH_PCI_SHIFT 0
#define _HVM_IRQ_DPCI_MACH_MSI_SHIFT 1
#define _HVM_IRQ_DPCI_MAPPED_SHIFT 2
#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
Expand All @@ -125,7 +124,6 @@ struct dev_intx_gsi_link {
#define HVM_IRQ_DPCI_MACH_PCI (1u << _HVM_IRQ_DPCI_MACH_PCI_SHIFT)
#define HVM_IRQ_DPCI_MACH_MSI (1u << _HVM_IRQ_DPCI_MACH_MSI_SHIFT)
#define HVM_IRQ_DPCI_MAPPED (1u << _HVM_IRQ_DPCI_MAPPED_SHIFT)
#define HVM_IRQ_DPCI_EOI_LATCH (1u << _HVM_IRQ_DPCI_EOI_LATCH_SHIFT)
#define HVM_IRQ_DPCI_GUEST_PCI (1u << _HVM_IRQ_DPCI_GUEST_PCI_SHIFT)
#define HVM_IRQ_DPCI_GUEST_MSI (1u << _HVM_IRQ_DPCI_GUEST_MSI_SHIFT)
#define HVM_IRQ_DPCI_IDENTITY_GSI (1u << _HVM_IRQ_DPCI_IDENTITY_GSI_SHIFT)
Expand Down Expand Up @@ -170,7 +168,6 @@ struct hvm_pirq_dpci {
struct list_head digl_list;
struct domain *dom;
struct hvm_gmsi_info gmsi;
struct timer timer;
struct list_head softirq_list;
};

Expand Down
5 changes: 0 additions & 5 deletions xen/include/xen/iommu.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,11 +183,6 @@ int pt_irq_destroy_bind(struct domain *, const struct xen_domctl_bind_pt_irq *);
void hvm_dpci_isairq_eoi(struct domain *d, unsigned int isairq);
struct hvm_irq_dpci *domain_get_irq_dpci(const struct domain *);
void free_hvm_irq_dpci(struct hvm_irq_dpci *dpci);
#ifdef CONFIG_HVM
bool pt_irq_need_timer(uint32_t flags);
#else
static inline bool pt_irq_need_timer(unsigned int flags) { return false; }
#endif

struct msi_desc;
struct msi_msg;
Expand Down

0 comments on commit 730d0f6

Please sign in to comment.