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

[Julien Grall <julien.grall@xxxxxxx>]

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.

> +        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.
[...]

> 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?

> +
>  enum stdvga_cache_state {
>      STDVGA_CACHE_UNINITIALIZED,
>      STDVGA_CACHE_ENABLED,
> diff --git a/xen/include/asm-x86/msi.h b/xen/include/asm-x86/msi.h
> index a5de6a1328..dcbec8cf04 100644
> --- a/xen/include/asm-x86/msi.h
> +++ b/xen/include/asm-x86/msi.h
> @@ -251,4 +251,6 @@ void ack_nonmaskable_msi_irq(struct irq_desc *);
>  void end_nonmaskable_msi_irq(struct irq_desc *, u8 vector);
>  void set_msi_affinity(struct irq_desc *, const cpumask_t *);
>
> +extern bool dom0_msi;
> +
>  #endif /* __ASM_MSI_H */
Cheers,

[1] 
https://lists.xenproject.org/archives/html/xen-devel/2017-04/msg01672.html
--
Julien Grall

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