Re: [Xen-devel] [PATCH v10 11/11] vpci/msix: add MSI-X handlers
On Tue, Mar 20, 2018 at 07:10:39AM -0600, Jan Beulich wrote: > >>> On 20.03.18 at 12:43, wrote: > > Since you only had comments on patch 7 and 11 and there's the extra > > fix for the test harness, should I just send those and provide you > > with a git branch that contains the rest? > > Well, if it's not too much hassle for you I'd prefer if you resent the > full series (or, see below, whatever is left at the time you get > around to send it). > > > I can also wait if you want to commit the start of the series > > (provided I get all the relevant Acks) and rebase on top of that. > > I'd leave that up to you; it looks like both tool stack maintainers > are not around right now, so their ack for patch 1 may take a few > more days to arrive. I guess I will resend the full series later so they can review the latest version. Thanks, Roger. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v10 11/11] vpci/msix: add MSI-X handlers
>>> On 20.03.18 at 12:43, wrote: > Since you only had comments on patch 7 and 11 and there's the extra > fix for the test harness, should I just send those and provide you > with a git branch that contains the rest? Well, if it's not too much hassle for you I'd prefer if you resent the full series (or, see below, whatever is left at the time you get around to send it). > I can also wait if you want to commit the start of the series > (provided I get all the relevant Acks) and rebase on top of that. I'd leave that up to you; it looks like both tool stack maintainers are not around right now, so their ack for patch 1 may take a few more days to arrive. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v10 11/11] vpci/msix: add MSI-X handlers
On Tue, Mar 20, 2018 at 11:43:00AM +, Roger Pau Monné wrote: > On Tue, Mar 20, 2018 at 05:03:09AM -0600, Jan Beulich wrote: > > >>> On 16.03.18 at 14:30, wrote: > > > +printk(XENLOG_G_WARNING > > > + "Failed to remove MSIX table [%" PRI_gfn ", %" > > > PRI_gfn "]: %d\n", > > > + PFN_DOWN(start), PFN_DOWN(end), rc); > > > > In cases like this (where you don't use plain start/end anywhere, > > but you do use the same calculation on them twice each), it's > > certainly more efficient for the local variables to be frame numbers > > right away. > > Right, I've fixed it and also changed the printf formatters to lu, I > guess at some point in the series I used to print gfn values. Meant lx not lu... Sorry. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v10 11/11] vpci/msix: add MSI-X handlers
On Tue, Mar 20, 2018 at 05:03:09AM -0600, Jan Beulich wrote:
> >>> On 16.03.18 at 14:30, wrote:
> > +int vpci_msix_arch_print(const struct vpci_msix *msix)
> > +{
> > +unsigned int i;
> > +
> > +for ( i = 0; i < msix->max_entries; i++ )
> > +{
> > +const struct vpci_msix_entry *entry = &msix->entries[i];
> > +
> > +printk("%6u vec=%02x%7s%6s%3sassert%5s%7s dest_id=%lu mask=%u
> > pirq: %d\n",
> > + i, MASK_EXTR(entry->data, MSI_DATA_VECTOR_MASK),
> > + entry->data & MSI_DATA_DELIVERY_LOWPRI ? "lowest" : "fixed",
> > + entry->data & MSI_DATA_TRIGGER_LEVEL ? "level" : "edge",
> > + entry->data & MSI_DATA_LEVEL_ASSERT ? "" : "de",
> > + entry->addr & MSI_ADDR_DESTMODE_LOGIC ? "log" : "phys",
> > + entry->addr & MSI_ADDR_REDIRECTION_LOWPRI ? "lowest" :
> > "fixed",
> > + MASK_EXTR(entry->addr, MSI_ADDR_DEST_ID_MASK),
> > + entry->masked, entry->arch.pirq);
> > +if ( i && !(i % 64) )
> > +{
> > +struct pci_dev *pdev = msix->pdev;
> > +
> > +spin_unlock(&msix->pdev->vpci->lock);
> > +process_pending_softirqs();
> > +/* NB: we assume that pdev cannot go away for an alive domain.
> > */
> > +if ( !pdev->vpci || !spin_trylock(&pdev->vpci->lock) )
> > +return -EBUSY;
> > +msix = pdev->vpci->msix;
>
> I disagree with resuming with a potentially changed msix here: This
> can only lead to confusion of the consumer of the produced output.
OK, I will check that the previous msix pointer matches the current
one.
> > @@ -231,6 +232,23 @@ static int modify_bars(const struct pci_dev *pdev,
> > bool map, bool rom_only)
> > }
> > }
> >
> > +/* Remove any MSIX regions if present. */
> > +for ( i = 0; msix && i < ARRAY_SIZE(msix->tables); i++ )
> > +{
> > +paddr_t start = vmsix_table_addr(pdev->vpci, i);
> > +paddr_t end = start + vmsix_table_size(pdev->vpci, i) - 1;
> > +
> > +rc = rangeset_remove_range(mem, PFN_DOWN(start), PFN_DOWN(end));
> > +if ( rc )
> > +{
> > +printk(XENLOG_G_WARNING
> > + "Failed to remove MSIX table [%" PRI_gfn ", %" PRI_gfn
> > "]: %d\n",
> > + PFN_DOWN(start), PFN_DOWN(end), rc);
>
> In cases like this (where you don't use plain start/end anywhere,
> but you do use the same calculation on them twice each), it's
> certainly more efficient for the local variables to be frame numbers
> right away.
Right, I've fixed it and also changed the printf formatters to lu, I
guess at some point in the series I used to print gfn values.
>
> Considering that I didn't notice this earlier, I won't insist on the
> latter change to be made, i.e. with at least the former issue
> addressed
> Reviewed-by: Jan Beulich
Thanks. FWIW I've fixed both your comments.
Since you only had comments on patch 7 and 11 and there's the extra
fix for the test harness, should I just send those and provide you
with a git branch that contains the rest?
I can also wait if you want to commit the start of the series
(provided I get all the relevant Acks) and rebase on top of that.
Roger.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v10 11/11] vpci/msix: add MSI-X handlers
>>> On 16.03.18 at 14:30, wrote:
> +int vpci_msix_arch_print(const struct vpci_msix *msix)
> +{
> +unsigned int i;
> +
> +for ( i = 0; i < msix->max_entries; i++ )
> +{
> +const struct vpci_msix_entry *entry = &msix->entries[i];
> +
> +printk("%6u vec=%02x%7s%6s%3sassert%5s%7s dest_id=%lu mask=%u pirq:
> %d\n",
> + i, MASK_EXTR(entry->data, MSI_DATA_VECTOR_MASK),
> + entry->data & MSI_DATA_DELIVERY_LOWPRI ? "lowest" : "fixed",
> + entry->data & MSI_DATA_TRIGGER_LEVEL ? "level" : "edge",
> + entry->data & MSI_DATA_LEVEL_ASSERT ? "" : "de",
> + entry->addr & MSI_ADDR_DESTMODE_LOGIC ? "log" : "phys",
> + entry->addr & MSI_ADDR_REDIRECTION_LOWPRI ? "lowest" :
> "fixed",
> + MASK_EXTR(entry->addr, MSI_ADDR_DEST_ID_MASK),
> + entry->masked, entry->arch.pirq);
> +if ( i && !(i % 64) )
> +{
> +struct pci_dev *pdev = msix->pdev;
> +
> +spin_unlock(&msix->pdev->vpci->lock);
> +process_pending_softirqs();
> +/* NB: we assume that pdev cannot go away for an alive domain. */
> +if ( !pdev->vpci || !spin_trylock(&pdev->vpci->lock) )
> +return -EBUSY;
> +msix = pdev->vpci->msix;
I disagree with resuming with a potentially changed msix here: This
can only lead to confusion of the consumer of the produced output.
> @@ -231,6 +232,23 @@ static int modify_bars(const struct pci_dev *pdev, bool
> map, bool rom_only)
> }
> }
>
> +/* Remove any MSIX regions if present. */
> +for ( i = 0; msix && i < ARRAY_SIZE(msix->tables); i++ )
> +{
> +paddr_t start = vmsix_table_addr(pdev->vpci, i);
> +paddr_t end = start + vmsix_table_size(pdev->vpci, i) - 1;
> +
> +rc = rangeset_remove_range(mem, PFN_DOWN(start), PFN_DOWN(end));
> +if ( rc )
> +{
> +printk(XENLOG_G_WARNING
> + "Failed to remove MSIX table [%" PRI_gfn ", %" PRI_gfn
> "]: %d\n",
> + PFN_DOWN(start), PFN_DOWN(end), rc);
In cases like this (where you don't use plain start/end anywhere,
but you do use the same calculation on them twice each), it's
certainly more efficient for the local variables to be frame numbers
right away.
Considering that I didn't notice this earlier, I won't insist on the
latter change to be made, i.e. with at least the former issue
addressed
Reviewed-by: Jan Beulich
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
