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 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);

Will fix before moving the code.

Thanks, Roger.

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