Re: [Xen-devel] [PATCH 1/2] vpci/msi: split code to bind pirq

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, May 14, 2018 at 08:56:16AM -0600, Jan Beulich wrote:
> >>> On 14.05.18 at 16:15, <roger.pau@xxxxxxxxxx> wrote:
> > On Mon, May 14, 2018 at 06:24:37AM -0600, Jan Beulich wrote:
> >> >>> On 08.05.18 at 11:25, <roger.pau@xxxxxxxxxx> wrote:
> >> > --- a/xen/arch/x86/hvm/vmsi.c
> >> > +++ b/xen/arch/x86/hvm/vmsi.c
> >> > @@ -663,6 +663,42 @@ void vpci_msi_arch_mask(struct vpci_msi *msi, const 
> > struct pci_dev *pdev,
> >> >      vpci_mask_pirq(pdev->domain, msi->arch.pirq + entry, mask);
> >> >  }
> >> >  
> >> > +static int vpci_msi_update(const struct pci_dev *pdev, uint32_t data,
> >> > +                           uint64_t address, unsigned int vectors,
> >> > +                           unsigned int pirq, uint32_t mask)
> >> > +{
> >> > +    unsigned int i;
> >> > +
> >> > +    ASSERT(pcidevs_locked());
> >> > +
> >> > +    for ( i = 0; i < vectors; i++ )
> >> > +    {
> >> > +        uint8_t vector = MASK_EXTR(data, MSI_DATA_VECTOR_MASK);
> >> > +        uint8_t vector_mask = 0xff >> (8 - fls(vectors) + 1);
> >> > +        struct xen_domctl_bind_pt_irq bind = {
> >> > +            .machine_irq = pirq + i,
> >> > +            .irq_type = PT_IRQ_TYPE_MSI,
> >> > +            .u.msi.gvec = (vector & ~vector_mask) |
> >> > +                          ((vector + i) & vector_mask),
> >> > +            .u.msi.gflags = msi_gflags(data, address, (mask >> i) & 1),
> >> > +        };
> >> > +        int rc = pt_irq_create_bind(pdev->domain, &bind);
> >> > +
> >> > +        if ( rc )
> >> > +        {
> >> > +            gdprintk(XENLOG_ERR,
> >> > +                     "%04x:%02x:%02x.%u: failed to bind PIRQ %u: %d\n",
> >> > +                     pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> >> > +                     PCI_FUNC(pdev->devfn), pirq + i, rc);
> >> > +            while ( bind.machine_irq-- )
> >> > +                pt_irq_destroy_bind(pdev->domain, &bind);
> >> 
> >> I realize this is just code movement, but is this while() correct? I think 
> > it
> >> can only be correct if pirq (which bind.machine_irq gets initialized from)
> >> was always zero, yet that doesn't look to be the case.
> >> 
> >> If you agree, I'd prefer fixed code to be moved (read: wants a prereq
> >> patch), or for the fix to be applied while moving the code (suitably
> >> reasoned about in the description).
> > 
> > Right, this should be:
> > 
> > while ( bind.machine_irq-- >= pirq )
> >     pt_irq_destroy_bind(pdev->domain, &bind);
> 
> ">" you presumably mean, due to the post-decrement?

Ended up doing --bind.machine_irq >= pirq, because it seemed clearer
IMO.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel