[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1/6] xen/MSI-X: latch MSI-X table writes
>>> Tue, 16 Jun 2015 15:48:16 +0100 Stefano Stabellini wrote: >On Tue, 16 Jun 2015, Jan Beulich wrote: >> >>> On 16.06.15 at 15:35, Stefano Stabellini wrote: >> > On Fri, 5 Jun 2015, Jan Beulich wrote: I'm sorry for getting back to this only now. >> >> @@ -322,6 +323,13 @@ static int xen_pt_msix_update_one(XenPCI >> >> >> >> pirq = entry->pirq; >> >> >> >> + if (pirq == XEN_PT_UNASSIGNED_PIRQ || s->msix->maskall || >> >> + (entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT)) { >> > >> > I admit I am having difficulties understanding the full purpose of these >> > checks. Please add a comment on them. >> >> The comment would (pointlessly imo) re-state what the code already >> says: >> >> > I guess the intention is only to make changes using the latest values, >> > the ones in entry->latch, when the right conditions are met, otherwise >> > keep using the old values. Is that right? >> > >> > In that case, don't we want to use the latest values on MASKBIT -> >> > !MASKBIT transitions? In general when unmasking? >> >> This is what we want. And with that, the questions you ask further >> down should be answered too: The function gets invoked with the >> pre-change mask flag state in ->latch[], and updates the values >> used for actually setting up when that one has the entry masked >> (or mask-all is set). The actual new value gets written to ->latch[] >> after the call. > >I think this logic is counter-intuitive and prone to confuse the reader. >This change doesn't make sense on its own: when one will read >xen_pt_msix_update_one, won't be able to understand the function without >checking the call sites. > >Could we turn it around to be more obvious? Here check if >!(entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT) and below only >call xen_pt_msix_update_one on MASKBIT -> !MASKBIT transactions? > >Or something like that? That would maybe be doable with just pci_msix_write() as a caller, but not with the one in xen_pt_msix_update() (where we have no transition of the mask bit from set to clear). Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |