Re: [PATCH 3/3] passthrough: allow to suppress SERR and PERR signaling altogether

[Andrew Cooper <andrew.cooper3@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5660 bytes --]

On 03/04/14 10:41, Jan Beulich wrote:
> This is just to have a workaround at hand in case other chipsets (not
> covered by the previous two patches) also have similar issues.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -154,6 +154,112 @@ static void __init parse_phantom_dev(cha
>  }
>  custom_param("pci-phantom", parse_phantom_dev);
>  
> +static u16 __read_mostly command_mask;
> +static u16 __read_mostly bridge_ctl_mask;
> +
> +/*
> + * The 'pci' parameter controls certain PCI device aspects.
> + * Optional comma separated value may contain:
> + *
> + *   serr                       don't suppress system errors (default)
> + *   no-serr                    suppress system errors
> + *   perr                       don't suppress parity errors (default)
> + *   no-perr                    suppress parity errors
> + */
> +static void __init parse_pci_param(char *s)
> +{
> +    char *ss;
> +
> +    do {
> +        bool_t on = !!strncmp(s, "no-", 3);
> +        u16 cmd_mask = 0, brctl_mask = 0;
> +
> +        if ( !on )
> +            s += 3;
> +
> +        ss = strchr(s, ',');
> +        if ( ss )
> +            *ss = '\0';
> +
> +        if ( !strcmp(s, "serr") )
> +        {
> +            cmd_mask = PCI_COMMAND_SERR;
> +            brctl_mask = PCI_BRIDGE_CTL_SERR | PCI_BRIDGE_CTL_DTMR_SERR;
> +        }
> +        else if ( !strcmp(s, "perr") )
> +        {
> +            cmd_mask = PCI_COMMAND_PARITY;
> +            brctl_mask = PCI_BRIDGE_CTL_PARITY;
> +        }
> +
> +        if ( on )
> +        {
> +            command_mask &= ~cmd_mask;
> +            bridge_ctl_mask &= ~brctl_mask;
> +        }
> +        else
> +        {
> +            command_mask |= cmd_mask;
> +            bridge_ctl_mask |= brctl_mask;
> +        }
> +
> +        s = ss + 1;
> +    } while ( ss );
> +}
> +custom_param("pci", parse_pci_param);
> +
> +static void check_pdev(const struct pci_dev *pdev)
> +{
> +#define PCI_STATUS_CHECK \
> +    (PCI_STATUS_PARITY | PCI_STATUS_SIG_TARGET_ABORT | \
> +     PCI_STATUS_REC_TARGET_ABORT | PCI_STATUS_REC_MASTER_ABORT | \
> +     PCI_STATUS_SIG_SYSTEM_ERROR | PCI_STATUS_DETECTED_PARITY)
> +    u16 seg = pdev->seg;
> +    u8 bus = pdev->bus;
> +    u8 dev = PCI_SLOT(pdev->devfn);
> +    u8 func = PCI_FUNC(pdev->devfn);
> +    u16 val;
> +
> +    if ( command_mask )
> +    {
> +        val = pci_conf_read16(seg, bus, dev, func, PCI_COMMAND);
> +        if ( val & command_mask )
> +            pci_conf_write16(seg, bus, dev, func, PCI_COMMAND,
> +                             val & ~command_mask);
> +        val = pci_conf_read16(seg, bus, dev, func, PCI_STATUS);
> +        if ( val & PCI_STATUS_CHECK )
> +        {
> +            printk(XENLOG_INFO "%04x:%02x:%02x.%u status %04x\n",
> +                   seg, bus, dev, func, val);

What is the purpose of this printk?  From the text alone it is not obvious.

> +            pci_conf_write16(seg, bus, dev, func, PCI_STATUS, val);

I dont think this code has any right to clear status bits other than the
ones it is checking for, so the write should be "val & PCI_STATUS_CHECK"

~Andrew

> +        }
> +    }
> +
> +    switch ( pci_conf_read8(seg, bus, dev, func, PCI_HEADER_TYPE) & 0x7f )
> +    {
> +    case PCI_HEADER_TYPE_BRIDGE:
> +        if ( !bridge_ctl_mask )
> +            break;
> +        val = pci_conf_read16(seg, bus, dev, func, PCI_BRIDGE_CONTROL);
> +        if ( val & bridge_ctl_mask )
> +            pci_conf_write16(seg, bus, dev, func, PCI_BRIDGE_CONTROL,
> +                             val & ~bridge_ctl_mask);
> +        val = pci_conf_read16(seg, bus, dev, func, PCI_SEC_STATUS);
> +        if ( val & PCI_STATUS_CHECK )
> +        {
> +            printk(XENLOG_INFO "%04x:%02x:%02x.%u secondary status %04x\n",
> +                   seg, bus, dev, func, val);
> +            pci_conf_write16(seg, bus, dev, func, PCI_SEC_STATUS, val);
> +        }
> +        break;
> +
> +    case PCI_HEADER_TYPE_CARDBUS:
> +        /* TODO */
> +        break;
> +    }
> +#undef PCI_STATUS_CHECK
> +}
> +
>  static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>  {
>      struct pci_dev *pdev;
> @@ -252,6 +358,8 @@ static struct pci_dev *alloc_pdev(struct
>              break;
>      }
>  
> +    check_pdev(pdev);
> +
>      return pdev;
>  }
>  
> @@ -566,6 +674,8 @@ int pci_add_device(u16 seg, u8 bus, u8 d
>                     seg, bus, slot, func, ctrl);
>      }
>  
> +    check_pdev(pdev);
> +
>      ret = 0;
>      if ( !pdev->domain )
>      {
> --- a/xen/include/xen/pci_regs.h
> +++ b/xen/include/xen/pci_regs.h
> @@ -125,7 +125,7 @@
>  #define  PCI_IO_RANGE_TYPE_16	0x00
>  #define  PCI_IO_RANGE_TYPE_32	0x01
>  #define  PCI_IO_RANGE_MASK	(~0x0fUL)
> -#define PCI_SEC_STATUS		0x1e	/* Secondary status register, only bit 14 used */
> +#define PCI_SEC_STATUS		0x1e	/* Secondary status register */
>  #define PCI_MEMORY_BASE		0x20	/* Memory range behind */
>  #define PCI_MEMORY_LIMIT	0x22
>  #define  PCI_MEMORY_RANGE_TYPE_MASK 0x0fUL
> @@ -152,6 +152,7 @@
>  #define  PCI_BRIDGE_CTL_MASTER_ABORT	0x20  /* Report master aborts */
>  #define  PCI_BRIDGE_CTL_BUS_RESET	0x40	/* Secondary bus reset */
>  #define  PCI_BRIDGE_CTL_FAST_BACK	0x80	/* Fast Back2Back enabled on secondary interface */
> +#define  PCI_BRIDGE_CTL_DTMR_SERR	0x800	/* SERR upon discard timer expiry */
>  
>  /* Header type 2 (CardBus bridges) */
>  #define PCI_CB_CAPABILITY_LIST	0x14
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 6436 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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