* [PATCH 0/3] fixes (read: workarounds) for XSA-59
@ 2014-04-03 9:33 Jan Beulich
2014-04-03 9:39 ` [PATCH 1/3] VT-d: suppress UR signaling for server chipsets Jan Beulich
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Jan Beulich @ 2014-04-03 9:33 UTC (permalink / raw)
To: xen-devel
Cc: Asit K Mallick, xiantao.zhang, Keir Fraser, Jun Nakajima,
Donald D Dugger
Finally, after a long period of silence and then back and forth, here is
what came out of the discussion with Intel.
1: VT-d: suppress UR signaling for server chipsets
2: VT-d: suppress UR signaling for desktop chipsets
3: passthrough: allow to suppress SERR and PERR signaling altogether
Signed-off-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 1/3] VT-d: suppress UR signaling for server chipsets 2014-04-03 9:33 [PATCH 0/3] fixes (read: workarounds) for XSA-59 Jan Beulich @ 2014-04-03 9:39 ` Jan Beulich 2014-04-07 12:12 ` Andrew Cooper 2014-04-03 9:40 ` [PATCH 2/3] VT-d: suppress UR signaling for desktop chipsets Jan Beulich 2014-04-03 9:41 ` [PATCH 3/3] passthrough: allow to suppress SERR and PERR signaling altogether Jan Beulich 2 siblings, 1 reply; 13+ messages in thread From: Jan Beulich @ 2014-04-03 9:39 UTC (permalink / raw) To: xen-devel Cc: Asit K Mallick, xiantao.zhang, Keir Fraser, Jun Nakajima, Donald D Dugger [-- Attachment #1: Type: text/plain, Size: 7313 bytes --] Unspported Requests can be signaled for malformed writes to the MSI address region, e.g. due to buggy or malicious DMA set up to that region. These should normally result in IOMMU faults, but don't on the server chipsets dealt with here. IDs 0xe00, 0xe01, and 0xe04 ... 0xe0b (Ivytown) aren't needed here - Intel confirmed the issue to be fixed in hardware there. This is CVE-2013-3495 / XSA-59. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/drivers/passthrough/vtd/quirks.c +++ b/xen/drivers/passthrough/vtd/quirks.c @@ -390,12 +390,67 @@ void __init pci_vtd_quirk(struct pci_dev int bus = pdev->bus; int dev = PCI_SLOT(pdev->devfn); int func = PCI_FUNC(pdev->devfn); - int id, val; + int pos; + u32 val; - id = pci_conf_read32(seg, bus, dev, func, 0); - if ( id == 0x342e8086 || id == 0x3c288086 ) + if ( pci_conf_read16(seg, bus, dev, func, PCI_VENDOR_ID) != 0x8086 ) + return; + + switch ( pci_conf_read16(seg, bus, dev, func, PCI_DEVICE_ID) ) { + case 0x342e: /* Tylersburg chipset (Nehalem / Westmere systems) */ + case 0x3c28: /* Sandybridge */ val = pci_conf_read32(seg, bus, dev, func, 0x1AC); pci_conf_write32(seg, bus, dev, func, 0x1AC, val | (1 << 31)); + break; + + /* Tylersburg (EP)/Boxboro (MP) chipsets (NHM-EP/EX, WSM-EP/EX) */ + case 0x3400 ... 0x3407: /* host bridges */ + case 0x3408 ... 0x3411: case 0x3420 ... 0x3421: /* root ports */ + /* JasperForest (Intel Xeon Processor C5500/C3500 */ + case 0x3700 ... 0x370f: /* host bridges */ + case 0x3720 ... 0x3724: /* root ports */ + /* Sandybridge-EP (Romley) */ + case 0x3c00: /* host bridge */ + case 0x3c01 ... 0x3c0b: /* root ports */ + pos = pci_find_ext_capability(seg, bus, pdev->devfn, + PCI_EXT_CAP_ID_ERR); + if ( !pos ) + { + pos = pci_find_ext_capability(seg, bus, pdev->devfn, + PCI_EXT_CAP_ID_VNDR); + while ( pos ) + { + val = pci_conf_read32(seg, bus, dev, func, pos + PCI_VNDR_HEADER); + if ( PCI_VNDR_HEADER_ID(val) == 4 && PCI_VNDR_HEADER_REV(val) == 1 ) + { + pos += PCI_VNDR_HEADER; + break; + } + pos = pci_find_next_ext_capability(seg, bus, pdev->devfn, pos, + PCI_EXT_CAP_ID_VNDR); + } + } + if ( !pos ) + { + printk(XENLOG_WARNING "%04x:%02x:%02x.%u without AER capability?\n", + seg, bus, dev, func); + break; + } + + val = pci_conf_read32(seg, bus, dev, func, pos + PCI_ERR_UNCOR_MASK); + pci_conf_write32(seg, bus, dev, func, pos + PCI_ERR_UNCOR_MASK, + val | PCI_ERR_UNC_UNSUP); + val = pci_conf_read32(seg, bus, dev, func, pos + PCI_ERR_COR_MASK); + pci_conf_write32(seg, bus, dev, func, pos + PCI_ERR_COR_MASK, + val | PCI_ERR_COR_ADV_NFAT); + + /* XPUNCERRMSK Send Completion with Unsupported Request */ + val = pci_conf_read32(seg, bus, dev, func, 0x20c); + pci_conf_write32(seg, bus, dev, func, 0x20c, val | (1 << 4)); + + printk(XENLOG_INFO "Masked UR signaling on %04x:%02x:%02x.%u\n", + seg, bus, dev, func); + break; } } --- a/xen/drivers/pci/pci.c +++ b/xen/drivers/pci/pci.c @@ -66,23 +66,33 @@ int pci_find_next_cap(u16 seg, u8 bus, u /** * pci_find_ext_capability - Find an extended capability - * @dev: PCI device to query + * @seg/@bus/@devfn: PCI device to query * @cap: capability code * * Returns the address of the requested extended capability structure * within the device's PCI configuration space or 0 if the device does - * not support it. Possible values for @cap: - * - * %PCI_EXT_CAP_ID_ERR Advanced Error Reporting - * %PCI_EXT_CAP_ID_VC Virtual Channel - * %PCI_EXT_CAP_ID_DSN Device Serial Number - * %PCI_EXT_CAP_ID_PWR Power Budgeting + * not support it. */ int pci_find_ext_capability(int seg, int bus, int devfn, int cap) { + return pci_find_next_ext_capability(seg, bus, devfn, 0, cap); +} + +/** + * pci_find_next_ext_capability - Find another extended capability + * @seg/@bus/@devfn: PCI device to query + * @pos: starting position + * @cap: capability code + * + * Returns the address of the requested extended capability structure + * within the device's PCI configuration space or 0 if the device does + * not support it. + */ +int pci_find_next_ext_capability(int seg, int bus, int devfn, int start, int cap) +{ u32 header; int ttl = 480; /* 3840 bytes, minimum 8 bytes per capability */ - int pos = 0x100; + int pos = max(start, 0x100); header = pci_conf_read32(seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), pos); @@ -92,9 +102,10 @@ int pci_find_ext_capability(int seg, int */ if ( (header == 0) || (header == -1) ) return 0; + ASSERT(start != pos || PCI_EXT_CAP_ID(header) == cap); while ( ttl-- > 0 ) { - if ( PCI_EXT_CAP_ID(header) == cap ) + if ( PCI_EXT_CAP_ID(header) == cap && pos != start ) return pos; pos = PCI_EXT_CAP_NEXT(header); if ( pos < 0x100 ) --- a/xen/include/xen/pci.h +++ b/xen/include/xen/pci.h @@ -140,6 +140,7 @@ int pci_mmcfg_write(unsigned int seg, un int pci_find_cap_offset(u16 seg, u8 bus, u8 dev, u8 func, u8 cap); int pci_find_next_cap(u16 seg, u8 bus, unsigned int devfn, u8 pos, int cap); int pci_find_ext_capability(int seg, int bus, int devfn, int cap); +int pci_find_next_ext_capability(int seg, int bus, int devfn, int pos, int cap); const char *parse_pci(const char *, unsigned int *seg, unsigned int *bus, unsigned int *dev, unsigned int *func); --- a/xen/include/xen/pci_regs.h +++ b/xen/include/xen/pci_regs.h @@ -431,6 +431,7 @@ #define PCI_EXT_CAP_ID_VC 2 #define PCI_EXT_CAP_ID_DSN 3 #define PCI_EXT_CAP_ID_PWR 4 +#define PCI_EXT_CAP_ID_VNDR 11 #define PCI_EXT_CAP_ID_ACS 13 #define PCI_EXT_CAP_ID_ARI 14 #define PCI_EXT_CAP_ID_ATS 15 @@ -459,6 +460,7 @@ #define PCI_ERR_COR_BAD_DLLP 0x00000080 /* Bad DLLP Status */ #define PCI_ERR_COR_REP_ROLL 0x00000100 /* REPLAY_NUM Rollover */ #define PCI_ERR_COR_REP_TIMER 0x00001000 /* Replay Timer Timeout */ +#define PCI_ERR_COR_ADV_NFAT 0x00002000 /* Advisory Non-Fatal */ #define PCI_ERR_COR_MASK 20 /* Correctable Error Mask */ /* Same bits as above */ #define PCI_ERR_CAP 24 /* Advanced Error Capabilities */ @@ -510,6 +512,12 @@ #define PCI_PWR_CAP 12 /* Capability */ #define PCI_PWR_CAP_BUDGET(x) ((x) & 1) /* Included in system budget */ +/* Vendor-Specific (VSEC, PCI_EXT_CAP_ID_VNDR) */ +#define PCI_VNDR_HEADER 4 /* Vendor-Specific Header */ +#define PCI_VNDR_HEADER_ID(x) ((x) & 0xffff) +#define PCI_VNDR_HEADER_REV(x) (((x) >> 16) & 0xf) +#define PCI_VNDR_HEADER_LEN(x) (((x) >> 20) & 0xfff) + /* * Hypertransport sub capability types * [-- Attachment #2: VT-d-mask-UR-host-bridge-server.patch --] [-- Type: text/plain, Size: 7360 bytes --] VT-d: suppress UR signaling for server chipsets Unspported Requests can be signaled for malformed writes to the MSI address region, e.g. due to buggy or malicious DMA set up to that region. These should normally result in IOMMU faults, but don't on the server chipsets dealt with here. IDs 0xe00, 0xe01, and 0xe04 ... 0xe0b (Ivytown) aren't needed here - Intel confirmed the issue to be fixed in hardware there. This is CVE-2013-3495 / XSA-59. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/drivers/passthrough/vtd/quirks.c +++ b/xen/drivers/passthrough/vtd/quirks.c @@ -390,12 +390,67 @@ void __init pci_vtd_quirk(struct pci_dev int bus = pdev->bus; int dev = PCI_SLOT(pdev->devfn); int func = PCI_FUNC(pdev->devfn); - int id, val; + int pos; + u32 val; - id = pci_conf_read32(seg, bus, dev, func, 0); - if ( id == 0x342e8086 || id == 0x3c288086 ) + if ( pci_conf_read16(seg, bus, dev, func, PCI_VENDOR_ID) != 0x8086 ) + return; + + switch ( pci_conf_read16(seg, bus, dev, func, PCI_DEVICE_ID) ) { + case 0x342e: /* Tylersburg chipset (Nehalem / Westmere systems) */ + case 0x3c28: /* Sandybridge */ val = pci_conf_read32(seg, bus, dev, func, 0x1AC); pci_conf_write32(seg, bus, dev, func, 0x1AC, val | (1 << 31)); + break; + + /* Tylersburg (EP)/Boxboro (MP) chipsets (NHM-EP/EX, WSM-EP/EX) */ + case 0x3400 ... 0x3407: /* host bridges */ + case 0x3408 ... 0x3411: case 0x3420 ... 0x3421: /* root ports */ + /* JasperForest (Intel Xeon Processor C5500/C3500 */ + case 0x3700 ... 0x370f: /* host bridges */ + case 0x3720 ... 0x3724: /* root ports */ + /* Sandybridge-EP (Romley) */ + case 0x3c00: /* host bridge */ + case 0x3c01 ... 0x3c0b: /* root ports */ + pos = pci_find_ext_capability(seg, bus, pdev->devfn, + PCI_EXT_CAP_ID_ERR); + if ( !pos ) + { + pos = pci_find_ext_capability(seg, bus, pdev->devfn, + PCI_EXT_CAP_ID_VNDR); + while ( pos ) + { + val = pci_conf_read32(seg, bus, dev, func, pos + PCI_VNDR_HEADER); + if ( PCI_VNDR_HEADER_ID(val) == 4 && PCI_VNDR_HEADER_REV(val) == 1 ) + { + pos += PCI_VNDR_HEADER; + break; + } + pos = pci_find_next_ext_capability(seg, bus, pdev->devfn, pos, + PCI_EXT_CAP_ID_VNDR); + } + } + if ( !pos ) + { + printk(XENLOG_WARNING "%04x:%02x:%02x.%u without AER capability?\n", + seg, bus, dev, func); + break; + } + + val = pci_conf_read32(seg, bus, dev, func, pos + PCI_ERR_UNCOR_MASK); + pci_conf_write32(seg, bus, dev, func, pos + PCI_ERR_UNCOR_MASK, + val | PCI_ERR_UNC_UNSUP); + val = pci_conf_read32(seg, bus, dev, func, pos + PCI_ERR_COR_MASK); + pci_conf_write32(seg, bus, dev, func, pos + PCI_ERR_COR_MASK, + val | PCI_ERR_COR_ADV_NFAT); + + /* XPUNCERRMSK Send Completion with Unsupported Request */ + val = pci_conf_read32(seg, bus, dev, func, 0x20c); + pci_conf_write32(seg, bus, dev, func, 0x20c, val | (1 << 4)); + + printk(XENLOG_INFO "Masked UR signaling on %04x:%02x:%02x.%u\n", + seg, bus, dev, func); + break; } } --- a/xen/drivers/pci/pci.c +++ b/xen/drivers/pci/pci.c @@ -66,23 +66,33 @@ int pci_find_next_cap(u16 seg, u8 bus, u /** * pci_find_ext_capability - Find an extended capability - * @dev: PCI device to query + * @seg/@bus/@devfn: PCI device to query * @cap: capability code * * Returns the address of the requested extended capability structure * within the device's PCI configuration space or 0 if the device does - * not support it. Possible values for @cap: - * - * %PCI_EXT_CAP_ID_ERR Advanced Error Reporting - * %PCI_EXT_CAP_ID_VC Virtual Channel - * %PCI_EXT_CAP_ID_DSN Device Serial Number - * %PCI_EXT_CAP_ID_PWR Power Budgeting + * not support it. */ int pci_find_ext_capability(int seg, int bus, int devfn, int cap) { + return pci_find_next_ext_capability(seg, bus, devfn, 0, cap); +} + +/** + * pci_find_next_ext_capability - Find another extended capability + * @seg/@bus/@devfn: PCI device to query + * @pos: starting position + * @cap: capability code + * + * Returns the address of the requested extended capability structure + * within the device's PCI configuration space or 0 if the device does + * not support it. + */ +int pci_find_next_ext_capability(int seg, int bus, int devfn, int start, int cap) +{ u32 header; int ttl = 480; /* 3840 bytes, minimum 8 bytes per capability */ - int pos = 0x100; + int pos = max(start, 0x100); header = pci_conf_read32(seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), pos); @@ -92,9 +102,10 @@ int pci_find_ext_capability(int seg, int */ if ( (header == 0) || (header == -1) ) return 0; + ASSERT(start != pos || PCI_EXT_CAP_ID(header) == cap); while ( ttl-- > 0 ) { - if ( PCI_EXT_CAP_ID(header) == cap ) + if ( PCI_EXT_CAP_ID(header) == cap && pos != start ) return pos; pos = PCI_EXT_CAP_NEXT(header); if ( pos < 0x100 ) --- a/xen/include/xen/pci.h +++ b/xen/include/xen/pci.h @@ -140,6 +140,7 @@ int pci_mmcfg_write(unsigned int seg, un int pci_find_cap_offset(u16 seg, u8 bus, u8 dev, u8 func, u8 cap); int pci_find_next_cap(u16 seg, u8 bus, unsigned int devfn, u8 pos, int cap); int pci_find_ext_capability(int seg, int bus, int devfn, int cap); +int pci_find_next_ext_capability(int seg, int bus, int devfn, int pos, int cap); const char *parse_pci(const char *, unsigned int *seg, unsigned int *bus, unsigned int *dev, unsigned int *func); --- a/xen/include/xen/pci_regs.h +++ b/xen/include/xen/pci_regs.h @@ -431,6 +431,7 @@ #define PCI_EXT_CAP_ID_VC 2 #define PCI_EXT_CAP_ID_DSN 3 #define PCI_EXT_CAP_ID_PWR 4 +#define PCI_EXT_CAP_ID_VNDR 11 #define PCI_EXT_CAP_ID_ACS 13 #define PCI_EXT_CAP_ID_ARI 14 #define PCI_EXT_CAP_ID_ATS 15 @@ -459,6 +460,7 @@ #define PCI_ERR_COR_BAD_DLLP 0x00000080 /* Bad DLLP Status */ #define PCI_ERR_COR_REP_ROLL 0x00000100 /* REPLAY_NUM Rollover */ #define PCI_ERR_COR_REP_TIMER 0x00001000 /* Replay Timer Timeout */ +#define PCI_ERR_COR_ADV_NFAT 0x00002000 /* Advisory Non-Fatal */ #define PCI_ERR_COR_MASK 20 /* Correctable Error Mask */ /* Same bits as above */ #define PCI_ERR_CAP 24 /* Advanced Error Capabilities */ @@ -510,6 +512,12 @@ #define PCI_PWR_CAP 12 /* Capability */ #define PCI_PWR_CAP_BUDGET(x) ((x) & 1) /* Included in system budget */ +/* Vendor-Specific (VSEC, PCI_EXT_CAP_ID_VNDR) */ +#define PCI_VNDR_HEADER 4 /* Vendor-Specific Header */ +#define PCI_VNDR_HEADER_ID(x) ((x) & 0xffff) +#define PCI_VNDR_HEADER_REV(x) (((x) >> 16) & 0xf) +#define PCI_VNDR_HEADER_LEN(x) (((x) >> 20) & 0xfff) + /* * Hypertransport sub capability types * [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] VT-d: suppress UR signaling for server chipsets 2014-04-03 9:39 ` [PATCH 1/3] VT-d: suppress UR signaling for server chipsets Jan Beulich @ 2014-04-07 12:12 ` Andrew Cooper 2014-04-07 13:11 ` Jan Beulich 0 siblings, 1 reply; 13+ messages in thread From: Andrew Cooper @ 2014-04-07 12:12 UTC (permalink / raw) To: Jan Beulich Cc: Keir Fraser, Asit K Mallick, Donald D Dugger, Jun Nakajima, xen-devel, xiantao.zhang [-- Attachment #1.1: Type: text/plain, Size: 7723 bytes --] On 03/04/14 10:39, Jan Beulich wrote: > Unspported Requests can be signaled for malformed writes to the MSI > address region, e.g. due to buggy or malicious DMA set up to that > region. These should normally result in IOMMU faults, but don't on > the server chipsets dealt with here. > > IDs 0xe00, 0xe01, and 0xe04 ... 0xe0b (Ivytown) aren't needed here - > Intel confirmed the issue to be fixed in hardware there. > > This is CVE-2013-3495 / XSA-59. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/drivers/passthrough/vtd/quirks.c > +++ b/xen/drivers/passthrough/vtd/quirks.c > @@ -390,12 +390,67 @@ void __init pci_vtd_quirk(struct pci_dev > int bus = pdev->bus; > int dev = PCI_SLOT(pdev->devfn); > int func = PCI_FUNC(pdev->devfn); > - int id, val; > + int pos; > + u32 val; > > - id = pci_conf_read32(seg, bus, dev, func, 0); > - if ( id == 0x342e8086 || id == 0x3c288086 ) > + if ( pci_conf_read16(seg, bus, dev, func, PCI_VENDOR_ID) != 0x8086 ) Can this using the new PCI_VENDOR_ID_INTEL? ~Andrew > + return; > + > + switch ( pci_conf_read16(seg, bus, dev, func, PCI_DEVICE_ID) ) > { > + case 0x342e: /* Tylersburg chipset (Nehalem / Westmere systems) */ > + case 0x3c28: /* Sandybridge */ > val = pci_conf_read32(seg, bus, dev, func, 0x1AC); > pci_conf_write32(seg, bus, dev, func, 0x1AC, val | (1 << 31)); > + break; > + > + /* Tylersburg (EP)/Boxboro (MP) chipsets (NHM-EP/EX, WSM-EP/EX) */ > + case 0x3400 ... 0x3407: /* host bridges */ > + case 0x3408 ... 0x3411: case 0x3420 ... 0x3421: /* root ports */ > + /* JasperForest (Intel Xeon Processor C5500/C3500 */ > + case 0x3700 ... 0x370f: /* host bridges */ > + case 0x3720 ... 0x3724: /* root ports */ > + /* Sandybridge-EP (Romley) */ > + case 0x3c00: /* host bridge */ > + case 0x3c01 ... 0x3c0b: /* root ports */ > + pos = pci_find_ext_capability(seg, bus, pdev->devfn, > + PCI_EXT_CAP_ID_ERR); > + if ( !pos ) > + { > + pos = pci_find_ext_capability(seg, bus, pdev->devfn, > + PCI_EXT_CAP_ID_VNDR); > + while ( pos ) > + { > + val = pci_conf_read32(seg, bus, dev, func, pos + PCI_VNDR_HEADER); > + if ( PCI_VNDR_HEADER_ID(val) == 4 && PCI_VNDR_HEADER_REV(val) == 1 ) > + { > + pos += PCI_VNDR_HEADER; > + break; > + } > + pos = pci_find_next_ext_capability(seg, bus, pdev->devfn, pos, > + PCI_EXT_CAP_ID_VNDR); > + } > + } > + if ( !pos ) > + { > + printk(XENLOG_WARNING "%04x:%02x:%02x.%u without AER capability?\n", > + seg, bus, dev, func); > + break; > + } > + > + val = pci_conf_read32(seg, bus, dev, func, pos + PCI_ERR_UNCOR_MASK); > + pci_conf_write32(seg, bus, dev, func, pos + PCI_ERR_UNCOR_MASK, > + val | PCI_ERR_UNC_UNSUP); > + val = pci_conf_read32(seg, bus, dev, func, pos + PCI_ERR_COR_MASK); > + pci_conf_write32(seg, bus, dev, func, pos + PCI_ERR_COR_MASK, > + val | PCI_ERR_COR_ADV_NFAT); > + > + /* XPUNCERRMSK Send Completion with Unsupported Request */ > + val = pci_conf_read32(seg, bus, dev, func, 0x20c); > + pci_conf_write32(seg, bus, dev, func, 0x20c, val | (1 << 4)); > + > + printk(XENLOG_INFO "Masked UR signaling on %04x:%02x:%02x.%u\n", > + seg, bus, dev, func); > + break; > } > } > --- a/xen/drivers/pci/pci.c > +++ b/xen/drivers/pci/pci.c > @@ -66,23 +66,33 @@ int pci_find_next_cap(u16 seg, u8 bus, u > > /** > * pci_find_ext_capability - Find an extended capability > - * @dev: PCI device to query > + * @seg/@bus/@devfn: PCI device to query > * @cap: capability code > * > * Returns the address of the requested extended capability structure > * within the device's PCI configuration space or 0 if the device does > - * not support it. Possible values for @cap: > - * > - * %PCI_EXT_CAP_ID_ERR Advanced Error Reporting > - * %PCI_EXT_CAP_ID_VC Virtual Channel > - * %PCI_EXT_CAP_ID_DSN Device Serial Number > - * %PCI_EXT_CAP_ID_PWR Power Budgeting > + * not support it. > */ > int pci_find_ext_capability(int seg, int bus, int devfn, int cap) > { > + return pci_find_next_ext_capability(seg, bus, devfn, 0, cap); > +} > + > +/** > + * pci_find_next_ext_capability - Find another extended capability > + * @seg/@bus/@devfn: PCI device to query > + * @pos: starting position > + * @cap: capability code > + * > + * Returns the address of the requested extended capability structure > + * within the device's PCI configuration space or 0 if the device does > + * not support it. > + */ > +int pci_find_next_ext_capability(int seg, int bus, int devfn, int start, int cap) > +{ > u32 header; > int ttl = 480; /* 3840 bytes, minimum 8 bytes per capability */ > - int pos = 0x100; > + int pos = max(start, 0x100); > > header = pci_conf_read32(seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), pos); > > @@ -92,9 +102,10 @@ int pci_find_ext_capability(int seg, int > */ > if ( (header == 0) || (header == -1) ) > return 0; > + ASSERT(start != pos || PCI_EXT_CAP_ID(header) == cap); > > while ( ttl-- > 0 ) { > - if ( PCI_EXT_CAP_ID(header) == cap ) > + if ( PCI_EXT_CAP_ID(header) == cap && pos != start ) > return pos; > pos = PCI_EXT_CAP_NEXT(header); > if ( pos < 0x100 ) > --- a/xen/include/xen/pci.h > +++ b/xen/include/xen/pci.h > @@ -140,6 +140,7 @@ int pci_mmcfg_write(unsigned int seg, un > int pci_find_cap_offset(u16 seg, u8 bus, u8 dev, u8 func, u8 cap); > int pci_find_next_cap(u16 seg, u8 bus, unsigned int devfn, u8 pos, int cap); > int pci_find_ext_capability(int seg, int bus, int devfn, int cap); > +int pci_find_next_ext_capability(int seg, int bus, int devfn, int pos, int cap); > const char *parse_pci(const char *, unsigned int *seg, unsigned int *bus, > unsigned int *dev, unsigned int *func); > > --- a/xen/include/xen/pci_regs.h > +++ b/xen/include/xen/pci_regs.h > @@ -431,6 +431,7 @@ > #define PCI_EXT_CAP_ID_VC 2 > #define PCI_EXT_CAP_ID_DSN 3 > #define PCI_EXT_CAP_ID_PWR 4 > +#define PCI_EXT_CAP_ID_VNDR 11 > #define PCI_EXT_CAP_ID_ACS 13 > #define PCI_EXT_CAP_ID_ARI 14 > #define PCI_EXT_CAP_ID_ATS 15 > @@ -459,6 +460,7 @@ > #define PCI_ERR_COR_BAD_DLLP 0x00000080 /* Bad DLLP Status */ > #define PCI_ERR_COR_REP_ROLL 0x00000100 /* REPLAY_NUM Rollover */ > #define PCI_ERR_COR_REP_TIMER 0x00001000 /* Replay Timer Timeout */ > +#define PCI_ERR_COR_ADV_NFAT 0x00002000 /* Advisory Non-Fatal */ > #define PCI_ERR_COR_MASK 20 /* Correctable Error Mask */ > /* Same bits as above */ > #define PCI_ERR_CAP 24 /* Advanced Error Capabilities */ > @@ -510,6 +512,12 @@ > #define PCI_PWR_CAP 12 /* Capability */ > #define PCI_PWR_CAP_BUDGET(x) ((x) & 1) /* Included in system budget */ > > +/* Vendor-Specific (VSEC, PCI_EXT_CAP_ID_VNDR) */ > +#define PCI_VNDR_HEADER 4 /* Vendor-Specific Header */ > +#define PCI_VNDR_HEADER_ID(x) ((x) & 0xffff) > +#define PCI_VNDR_HEADER_REV(x) (((x) >> 16) & 0xf) > +#define PCI_VNDR_HEADER_LEN(x) (((x) >> 20) & 0xfff) > + > /* > * Hypertransport sub capability types > * > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel [-- Attachment #1.2: Type: text/html, Size: 8299 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] VT-d: suppress UR signaling for server chipsets 2014-04-07 12:12 ` Andrew Cooper @ 2014-04-07 13:11 ` Jan Beulich 0 siblings, 0 replies; 13+ messages in thread From: Jan Beulich @ 2014-04-07 13:11 UTC (permalink / raw) To: Andrew Cooper Cc: Keir Fraser, Asit K Mallick, Donald D Dugger, Jun Nakajima, xen-devel, xiantao.zhang >>> On 07.04.14 at 14:12, <andrew.cooper3@citrix.com> wrote: > On 03/04/14 10:39, Jan Beulich wrote: >> Unspported Requests can be signaled for malformed writes to the MSI >> address region, e.g. due to buggy or malicious DMA set up to that >> region. These should normally result in IOMMU faults, but don't on >> the server chipsets dealt with here. >> >> IDs 0xe00, 0xe01, and 0xe04 ... 0xe0b (Ivytown) aren't needed here - >> Intel confirmed the issue to be fixed in hardware there. >> >> This is CVE-2013-3495 / XSA-59. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- a/xen/drivers/passthrough/vtd/quirks.c >> +++ b/xen/drivers/passthrough/vtd/quirks.c >> @@ -390,12 +390,67 @@ void __init pci_vtd_quirk(struct pci_dev >> int bus = pdev->bus; >> int dev = PCI_SLOT(pdev->devfn); >> int func = PCI_FUNC(pdev->devfn); >> - int id, val; >> + int pos; >> + u32 val; >> >> - id = pci_conf_read32(seg, bus, dev, func, 0); >> - if ( id == 0x342e8086 || id == 0x3c288086 ) >> + if ( pci_conf_read16(seg, bus, dev, func, PCI_VENDOR_ID) != 0x8086 ) > > Can this using the new PCI_VENDOR_ID_INTEL? Yes, it could. The patch simply predates that addition by many months. Plus a little more care will be needed during the backport. But I guess I'll do it anyway. Jan ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] VT-d: suppress UR signaling for desktop chipsets 2014-04-03 9:33 [PATCH 0/3] fixes (read: workarounds) for XSA-59 Jan Beulich 2014-04-03 9:39 ` [PATCH 1/3] VT-d: suppress UR signaling for server chipsets Jan Beulich @ 2014-04-03 9:40 ` Jan Beulich 2014-04-07 12:21 ` Andrew Cooper 2014-04-03 9:41 ` [PATCH 3/3] passthrough: allow to suppress SERR and PERR signaling altogether Jan Beulich 2 siblings, 1 reply; 13+ messages in thread From: Jan Beulich @ 2014-04-03 9:40 UTC (permalink / raw) To: xen-devel Cc: Asit K Mallick, xiantao.zhang, Keir Fraser, Jun Nakajima, Donald D Dugger [-- Attachment #1: Type: text/plain, Size: 2071 bytes --] Unsupported Requests can be signaled for malformed writes to the MSI address region, e.g. due to buggy or malicious DMA set up to that region. These should normally result in IOMMU faults, but don't on the desktop chipsets dealt with here. This is CVE-2013-3495 / XSA-59. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/drivers/passthrough/vtd/quirks.c +++ b/xen/drivers/passthrough/vtd/quirks.c @@ -392,6 +392,8 @@ void __init pci_vtd_quirk(struct pci_dev int func = PCI_FUNC(pdev->devfn); int pos; u32 val; + u64 bar; + paddr_t pa; if ( pci_conf_read16(seg, bus, dev, func, PCI_VENDOR_ID) != 0x8086 ) return; @@ -452,5 +454,33 @@ void __init pci_vtd_quirk(struct pci_dev printk(XENLOG_INFO "Masked UR signaling on %04x:%02x:%02x.%u\n", seg, bus, dev, func); break; + + case 0x100: case 0x104: case 0x108: /* Sandybridge */ + case 0x150: case 0x154: case 0x158: /* Ivybridge */ + case 0xa04: /* Haswell ULT */ + case 0xc00: case 0xc04: case 0xc08: /* Haswell */ + bar = pci_conf_read32(seg, bus, dev, func, 0x6c); + bar = (bar << 32) | pci_conf_read32(seg, bus, dev, func, 0x68); + pa = bar & 0x7fffff000; /* bits 12...38 */ + if ( (bar & 1) && pa && + page_is_ram_type(paddr_to_pfn(pa), RAM_TYPE_RESERVED) ) + { + u32 __iomem *va = ioremap(pa, PAGE_SIZE); + + if ( va ) + { + __set_bit(0x1c8 * 8 + 20, va); + iounmap(va); + printk(XENLOG_INFO "Masked UR signaling on %04x:%02x:%02x.%u\n", + seg, bus, dev, func); + } + else + printk(XENLOG_ERR "Could not map %"PRIpaddr" for %04x:%02x:%02x.%u\n", + pa, seg, bus, dev, func); + } + else + printk(XENLOG_WARNING "Bogus DMIBAR %#"PRIx64" on %04x:%02x:%02x.%u\n", + bar, seg, bus, dev, func); + break; } } [-- Attachment #2: VT-d-mask-UR-host-bridge-desktop.patch --] [-- Type: text/plain, Size: 2117 bytes --] VT-d: suppress UR signaling for desktop chipsets Unsupported Requests can be signaled for malformed writes to the MSI address region, e.g. due to buggy or malicious DMA set up to that region. These should normally result in IOMMU faults, but don't on the desktop chipsets dealt with here. This is CVE-2013-3495 / XSA-59. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/drivers/passthrough/vtd/quirks.c +++ b/xen/drivers/passthrough/vtd/quirks.c @@ -392,6 +392,8 @@ void __init pci_vtd_quirk(struct pci_dev int func = PCI_FUNC(pdev->devfn); int pos; u32 val; + u64 bar; + paddr_t pa; if ( pci_conf_read16(seg, bus, dev, func, PCI_VENDOR_ID) != 0x8086 ) return; @@ -452,5 +454,33 @@ void __init pci_vtd_quirk(struct pci_dev printk(XENLOG_INFO "Masked UR signaling on %04x:%02x:%02x.%u\n", seg, bus, dev, func); break; + + case 0x100: case 0x104: case 0x108: /* Sandybridge */ + case 0x150: case 0x154: case 0x158: /* Ivybridge */ + case 0xa04: /* Haswell ULT */ + case 0xc00: case 0xc04: case 0xc08: /* Haswell */ + bar = pci_conf_read32(seg, bus, dev, func, 0x6c); + bar = (bar << 32) | pci_conf_read32(seg, bus, dev, func, 0x68); + pa = bar & 0x7fffff000; /* bits 12...38 */ + if ( (bar & 1) && pa && + page_is_ram_type(paddr_to_pfn(pa), RAM_TYPE_RESERVED) ) + { + u32 __iomem *va = ioremap(pa, PAGE_SIZE); + + if ( va ) + { + __set_bit(0x1c8 * 8 + 20, va); + iounmap(va); + printk(XENLOG_INFO "Masked UR signaling on %04x:%02x:%02x.%u\n", + seg, bus, dev, func); + } + else + printk(XENLOG_ERR "Could not map %"PRIpaddr" for %04x:%02x:%02x.%u\n", + pa, seg, bus, dev, func); + } + else + printk(XENLOG_WARNING "Bogus DMIBAR %#"PRIx64" on %04x:%02x:%02x.%u\n", + bar, seg, bus, dev, func); + break; } } [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] VT-d: suppress UR signaling for desktop chipsets 2014-04-03 9:40 ` [PATCH 2/3] VT-d: suppress UR signaling for desktop chipsets Jan Beulich @ 2014-04-07 12:21 ` Andrew Cooper 0 siblings, 0 replies; 13+ messages in thread From: Andrew Cooper @ 2014-04-07 12:21 UTC (permalink / raw) To: Jan Beulich Cc: Keir Fraser, Asit K Mallick, Donald D Dugger, Jun Nakajima, xen-devel, xiantao.zhang [-- Attachment #1.1: Type: text/plain, Size: 2356 bytes --] On 03/04/14 10:40, Jan Beulich wrote: > Unsupported Requests can be signaled for malformed writes to the MSI > address region, e.g. due to buggy or malicious DMA set up to that > region. These should normally result in IOMMU faults, but don't on > the desktop chipsets dealt with here. > > This is CVE-2013-3495 / XSA-59. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > > --- a/xen/drivers/passthrough/vtd/quirks.c > +++ b/xen/drivers/passthrough/vtd/quirks.c > @@ -392,6 +392,8 @@ void __init pci_vtd_quirk(struct pci_dev > int func = PCI_FUNC(pdev->devfn); > int pos; > u32 val; > + u64 bar; > + paddr_t pa; > > if ( pci_conf_read16(seg, bus, dev, func, PCI_VENDOR_ID) != 0x8086 ) > return; > @@ -452,5 +454,33 @@ void __init pci_vtd_quirk(struct pci_dev > printk(XENLOG_INFO "Masked UR signaling on %04x:%02x:%02x.%u\n", > seg, bus, dev, func); > break; > + > + case 0x100: case 0x104: case 0x108: /* Sandybridge */ > + case 0x150: case 0x154: case 0x158: /* Ivybridge */ > + case 0xa04: /* Haswell ULT */ > + case 0xc00: case 0xc04: case 0xc08: /* Haswell */ > + bar = pci_conf_read32(seg, bus, dev, func, 0x6c); > + bar = (bar << 32) | pci_conf_read32(seg, bus, dev, func, 0x68); > + pa = bar & 0x7fffff000; /* bits 12...38 */ > + if ( (bar & 1) && pa && > + page_is_ram_type(paddr_to_pfn(pa), RAM_TYPE_RESERVED) ) > + { > + u32 __iomem *va = ioremap(pa, PAGE_SIZE); > + > + if ( va ) > + { > + __set_bit(0x1c8 * 8 + 20, va); > + iounmap(va); > + printk(XENLOG_INFO "Masked UR signaling on %04x:%02x:%02x.%u\n", > + seg, bus, dev, func); > + } > + else > + printk(XENLOG_ERR "Could not map %"PRIpaddr" for %04x:%02x:%02x.%u\n", > + pa, seg, bus, dev, func); > + } > + else > + printk(XENLOG_WARNING "Bogus DMIBAR %#"PRIx64" on %04x:%02x:%02x.%u\n", > + bar, seg, bus, dev, func); > + break; > } > } > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel [-- Attachment #1.2: Type: text/html, Size: 3206 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] passthrough: allow to suppress SERR and PERR signaling altogether 2014-04-03 9:33 [PATCH 0/3] fixes (read: workarounds) for XSA-59 Jan Beulich 2014-04-03 9:39 ` [PATCH 1/3] VT-d: suppress UR signaling for server chipsets Jan Beulich 2014-04-03 9:40 ` [PATCH 2/3] VT-d: suppress UR signaling for desktop chipsets Jan Beulich @ 2014-04-03 9:41 ` Jan Beulich 2014-04-07 10:05 ` Andrew Cooper 2014-04-07 12:47 ` Andrew Cooper 2 siblings, 2 replies; 13+ messages in thread From: Jan Beulich @ 2014-04-03 9:41 UTC (permalink / raw) To: xen-devel Cc: Asit K Mallick, xiantao.zhang, Keir Fraser, Jun Nakajima, Donald D Dugger [-- Attachment #1: Type: text/plain, Size: 5093 bytes --] 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); + pci_conf_write16(seg, bus, dev, func, PCI_STATUS, val); + } + } + + 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 [-- Attachment #2: pt-suppress-SERR.patch --] [-- Type: text/plain, Size: 5158 bytes --] passthrough: allow to suppress SERR and PERR signaling altogether 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); + pci_conf_write16(seg, bus, dev, func, PCI_STATUS, val); + } + } + + 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 [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] passthrough: allow to suppress SERR and PERR signaling altogether 2014-04-03 9:41 ` [PATCH 3/3] passthrough: allow to suppress SERR and PERR signaling altogether Jan Beulich @ 2014-04-07 10:05 ` Andrew Cooper 2014-04-07 10:21 ` Jan Beulich 2014-04-07 12:47 ` Andrew Cooper 1 sibling, 1 reply; 13+ messages in thread From: Andrew Cooper @ 2014-04-07 10:05 UTC (permalink / raw) To: Jan Beulich, xen-devel Cc: Asit K Mallick, Donald D Dugger, Keir Fraser, Jun Nakajima, xiantao.zhang [-- Attachment #1.1: Type: text/plain, Size: 5476 bytes --] On 03/04/2014 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> Please patch xen-command-line.markdown. ~Andrew > --- 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); > + pci_conf_write16(seg, bus, dev, func, PCI_STATUS, val); > + } > + } > + > + 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: 6032 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] passthrough: allow to suppress SERR and PERR signaling altogether 2014-04-07 10:05 ` Andrew Cooper @ 2014-04-07 10:21 ` Jan Beulich 0 siblings, 0 replies; 13+ messages in thread From: Jan Beulich @ 2014-04-07 10:21 UTC (permalink / raw) To: Andrew Cooper Cc: KeirFraser, Asit K Mallick, Donald D Dugger, Jun Nakajima, xen-devel, xiantao.zhang >>> On 07.04.14 at 12:05, <andrew.cooper3@citrix.com> wrote: > On 03/04/2014 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> > > Please patch xen-command-line.markdown. Oops, yes, of course: --- a/docs/misc/xen-command-line.markdown +++ b/docs/misc/xen-command-line.markdown @@ -790,6 +790,14 @@ Defaults to booting secondary processors Default: `on` +### pci +> `= {no-}serr | {no-}perr` + +Disable signaling of SERR (system errors) and/or PERR (parity errors) +on all PCI devices. + +Default: Signaling left as set by firmware. + ### pci-phantom > `=[<seg>:]<bus>:<device>,<stride>` ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] passthrough: allow to suppress SERR and PERR signaling altogether 2014-04-03 9:41 ` [PATCH 3/3] passthrough: allow to suppress SERR and PERR signaling altogether Jan Beulich 2014-04-07 10:05 ` Andrew Cooper @ 2014-04-07 12:47 ` Andrew Cooper 2014-04-07 13:05 ` Jan Beulich 1 sibling, 1 reply; 13+ messages in thread From: Andrew Cooper @ 2014-04-07 12:47 UTC (permalink / raw) To: Jan Beulich Cc: Keir Fraser, Asit K Mallick, Donald D Dugger, Jun Nakajima, xen-devel, xiantao.zhang [-- 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] passthrough: allow to suppress SERR and PERR signaling altogether 2014-04-07 12:47 ` Andrew Cooper @ 2014-04-07 13:05 ` Jan Beulich 2014-04-07 13:17 ` Andrew Cooper 0 siblings, 1 reply; 13+ messages in thread From: Jan Beulich @ 2014-04-07 13:05 UTC (permalink / raw) To: Andrew Cooper Cc: Keir Fraser, Asit K Mallick, Donald D Dugger, Jun Nakajima, xen-devel, xiantao.zhang >>> On 07.04.14 at 14:47, <andrew.cooper3@citrix.com> wrote: > On 03/04/14 10:41, Jan Beulich wrote: >> + 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. It's simply to have an indication that the status register was written (and that certain bits may have got cleared). >> + 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" Hmm, the intention is to clear all status fields that can be cleared, and the if() around the write is just to avoid the printk() and the write if possible. PCI_STATUS_CHECK already includes all changeable bits, and I'd expect any of the few that are currently reserved to get added here, should they attain a meaning of a writable one. Jan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] passthrough: allow to suppress SERR and PERR signaling altogether 2014-04-07 13:05 ` Jan Beulich @ 2014-04-07 13:17 ` Andrew Cooper 2014-04-07 13:43 ` Jan Beulich 0 siblings, 1 reply; 13+ messages in thread From: Andrew Cooper @ 2014-04-07 13:17 UTC (permalink / raw) To: Jan Beulich Cc: Keir Fraser, Asit K Mallick, Donald D Dugger, Jun Nakajima, xen-devel, xiantao.zhang On 07/04/14 14:05, Jan Beulich wrote: >>>> On 07.04.14 at 14:47, <andrew.cooper3@citrix.com> wrote: >> On 03/04/14 10:41, Jan Beulich wrote: >>> + 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. > It's simply to have an indication that the status register was written > (and that certain bits may have got cleared). Then at the very least it should be ..."status %04x -> %04x\n", .... val, val & PCI_STATUS_CHECK) to identify which status bits are being cleared. > >>> + 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" > Hmm, the intention is to clear all status fields that can be cleared, and > the if() around the write is just to avoid the printk() and the write if > possible. PCI_STATUS_CHECK already includes all changeable bits, and > I'd expect any of the few that are currently reserved to get added > here, should they attain a meaning of a writable one. > > Jan > For forward compatibility, we must not assume anything about currently reserved bits which Xen doesn't know about. If PCI_STATUS_CHECK is intended to be extended with future clearable bits, perhaps naming it "PCI_STATUS_CLEARABLE" would be clearer. ~Andrew ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] passthrough: allow to suppress SERR and PERR signaling altogether 2014-04-07 13:17 ` Andrew Cooper @ 2014-04-07 13:43 ` Jan Beulich 0 siblings, 0 replies; 13+ messages in thread From: Jan Beulich @ 2014-04-07 13:43 UTC (permalink / raw) To: Andrew Cooper Cc: KeirFraser, Asit K Mallick, Donald D Dugger, Jun Nakajima, xen-devel, xiantao.zhang >>> On 07.04.14 at 15:17, <andrew.cooper3@citrix.com> wrote: > On 07/04/14 14:05, Jan Beulich wrote: >>>>> On 07.04.14 at 14:47, <andrew.cooper3@citrix.com> wrote: >>> On 03/04/14 10:41, Jan Beulich wrote: >>>> + 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. >> It's simply to have an indication that the status register was written >> (and that certain bits may have got cleared). > > Then at the very least it should be ..."status %04x -> %04x\n", .... > val, val & PCI_STATUS_CHECK) to identify which status bits are being > cleared. This obviously makes sense only when also changing the write operation to use val & PCI_STATUS_CHECK, and then it would seem to make more sense to print val & ~PCI_STATUS_CHECK as the second value. >>>> + 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" >> Hmm, the intention is to clear all status fields that can be cleared, and >> the if() around the write is just to avoid the printk() and the write if >> possible. PCI_STATUS_CHECK already includes all changeable bits, and >> I'd expect any of the few that are currently reserved to get added >> here, should they attain a meaning of a writable one. > > For forward compatibility, we must not assume anything about currently > reserved bits which Xen doesn't know about. In general this is correct; I'm not so certain in the particular case here. For one, the code doesn't get used by default, but only when asked for on the command line. If the code was found to have a problem, just don't specify the command line option. And then, as said before, the intention is to get the status register into a clean state. It's a status register, so we're not going to corrupt device state by clearing a few too many bits, and we log which bits were set at the time we did the clearing. Jan ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-04-07 13:43 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-04-03 9:33 [PATCH 0/3] fixes (read: workarounds) for XSA-59 Jan Beulich 2014-04-03 9:39 ` [PATCH 1/3] VT-d: suppress UR signaling for server chipsets Jan Beulich 2014-04-07 12:12 ` Andrew Cooper 2014-04-07 13:11 ` Jan Beulich 2014-04-03 9:40 ` [PATCH 2/3] VT-d: suppress UR signaling for desktop chipsets Jan Beulich 2014-04-07 12:21 ` Andrew Cooper 2014-04-03 9:41 ` [PATCH 3/3] passthrough: allow to suppress SERR and PERR signaling altogether Jan Beulich 2014-04-07 10:05 ` Andrew Cooper 2014-04-07 10:21 ` Jan Beulich 2014-04-07 12:47 ` Andrew Cooper 2014-04-07 13:05 ` Jan Beulich 2014-04-07 13:17 ` Andrew Cooper 2014-04-07 13:43 ` Jan Beulich
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).