Re: [Xen-devel] [PATCH v2 8/9] vpci/msi: add MSI handlers

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Mon, Apr 24, 2017 at 04:31:57PM +0100, Julien Grall wrote:
> Hi Roger,
> 
> On 20/04/17 16:17, Roger Pau Monne wrote:
> > diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
> > new file mode 100644
> > index 0000000000..aea6c68907
> > --- /dev/null
> > +++ b/xen/drivers/vpci/msi.c
> > @@ -0,0 +1,469 @@
> > +/*
> > + * Handlers for accesses to the MSI capability structure.
> > + *
> > + * Copyright (C) 2017 Citrix Systems R&D
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms and conditions of the GNU General Public
> > + * License, version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public
> > + * License along with this program; If not, see 
> > <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <xen/sched.h>
> > +#include <xen/vpci.h>
> > +#include <asm/msi.h>
> > +#include <xen/keyhandler.h>
> > +
> > +static void vpci_msi_mask_pirq(int pirq, bool mask)
> > +{
> > +        struct pirq *pinfo = pirq_info(current->domain, pirq);
> 
> We don't have pirq on ARM and don't plan to introduce it for MSI as
> interrupt will be handled directly by a virtual interrupt controller (see
> the vITS series [1]).
> 
> It would be nice if you can get the vPCI architecture agnostic. We would be
> to help here.
>
> > +        struct irq_desc *desc;
> > +        unsigned long flags;
> > +        int irq;
> > +
> > +        ASSERT(pinfo);
> > +        irq = pinfo->arch.irq;
> > +        ASSERT(irq < nr_irqs);
> > +
> > +        desc = irq_to_desc(irq);
> 
> Similarly we don't have irq_desc for MSI.

OK, I've moved all the arch-specific functions into vmsi.c, and introduced a
vpci_arch_msi struct in order to store the PIRQ on x86.

> > +        ASSERT(desc);
> > +
> > +        spin_lock_irqsave(&desc->lock, flags);
> > +        guest_mask_msi_irq(desc, mask);
> > +        spin_unlock_irqrestore(&desc->lock, flags);
> > +}
> > +
> 
> [...]
> 
> > +static int vpci_init_msi(struct pci_dev *pdev)
> > +{
> > +    uint8_t seg = pdev->seg, bus = pdev->bus;
> > +    uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
> > +    struct vpci_msi *msi = NULL;
> > +    unsigned int msi_offset;
> > +    uint16_t control;
> > +    int rc;
> > +
> > +    msi_offset = pci_find_cap_offset(seg, bus, slot, func, PCI_CAP_ID_MSI);
> > +    if ( !msi_offset )
> > +        return 0;
> > +
> > +    if ( !dom0_msi )
> 
> I would introduce an helper to allow per-architecture decision. Likely on
> ARM MSI will be enabled by default.

dom0_msi is also enabled by default on x86.

> > diff --git a/xen/include/asm-x86/hvm/io.h b/xen/include/asm-x86/hvm/io.h
> > index 0434aca706..899e37ae0f 100644
> > --- a/xen/include/asm-x86/hvm/io.h
> > +++ b/xen/include/asm-x86/hvm/io.h
> > @@ -126,6 +126,10 @@ void hvm_dpci_eoi(struct domain *d, unsigned int 
> > guest_irq,
> >  void msix_write_completion(struct vcpu *);
> >  void msixtbl_init(struct domain *d);
> > 
> > +/* Get the vector/flags from a MSI address/data fields. */
> > +unsigned int msi_vector(uint16_t data);
> > +unsigned int msi_flags(uint16_t data, uint64_t addr);
> 
> Should not those 2 helpers go in msi.h?

The other guest-related msi functions are in io.h, msi.h seems to only contain
functions that deal with the hardware itself (although I could be wrong).

Thanks, Roger.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel