* [PATCH v5 0/5] vPCI capabilities filtering
@ 2023-09-01 4:57 Stewart Hildebrand
2023-09-01 4:57 ` [PATCH v5 1/5] xen/pci: convert pci_find_*cap* to pci_sbdf_t Stewart Hildebrand
` (4 more replies)
0 siblings, 5 replies; 10+ messages in thread
From: Stewart Hildebrand @ 2023-09-01 4:57 UTC (permalink / raw)
To: xen-devel
Cc: Stewart Hildebrand, Jan Beulich, Andrew Cooper,
Roger Pau Monné,
Wei Liu, George Dunlap, Julien Grall, Stefano Stabellini,
Paul Durrant, Kevin Tian
This small series enables vPCI to filter which PCI capabilities we expose to a
domU. This series adds vPCI register handlers within
xen/drivers/vpci/header.c:init_bars(), along with some supporting functions.
Note there are minor rebase conflicts with the in-progress vPCI series [1].
These conflicts fall into the category of functions and code being added
adjacent to one another, so are easily resolved. I did not identify any
dependency on the vPCI locking work, and the two series deal with different
aspects of emulating the PCI header.
Future work may involve adding handlers for more registers in the vPCI header,
such as VID/DID, etc. Future work may also involve exposing additional
capabilities to the guest for broader device/driver support.
v4->v5:
* drop ("x86/msi: remove some unused-but-set-variables") as it has been
committed
* add ("xen/pci: update PCI_STATUS_* constants")
* squash ro_mask patch
v3->v4:
* drop "xen/pci: address a violation of MISRA C:2012 Rule 8.3" as it has been
committed
* re-order status register handler and capabilities filtering patches
* split an unrelated change from ("xen/pci: convert pci_find_*cap* to pci_sbdf_t")
into its own patch
* add new patch ("x86/msi: rearrange read_pci_mem_bar slightly") based on
feedback
* add new RFC patch ("xen/vpci: support ro mask")
v2->v3:
* drop RFC "xen/vpci: header: avoid cast for value passed to vpci_read_val"
* minor misra C violation fixup in preparatory patch
* switch to pci_sbdf_t in preparatory patch
* introduce status handler
v1->v2:
* squash helper functions into the patch where they are used to avoid transient
dead code situation
* add new RFC patch, possibly throwaway, to get an idea of what it would look
like to get rid of the (void *)(uintptr_t) cast by introducing a new memory
allocation
[1] https://lists.xenproject.org/archives/html/xen-devel/2023-07/msg01281.html
Stewart Hildebrand (5):
xen/pci: convert pci_find_*cap* to pci_sbdf_t
x86/msi: rearrange read_pci_mem_bar slightly
xen/pci: update PCI_STATUS_* constants
xen/vpci: header: status register handler
xen/vpci: header: filter PCI capabilities
xen/arch/x86/msi.c | 62 +++++---------
xen/drivers/char/ehci-dbgp.c | 3 +-
xen/drivers/passthrough/amd/iommu_detect.c | 2 +-
xen/drivers/passthrough/ats.c | 4 +-
xen/drivers/passthrough/ats.h | 6 +-
xen/drivers/passthrough/msi.c | 6 +-
xen/drivers/passthrough/pci.c | 21 ++---
xen/drivers/passthrough/vtd/quirks.c | 10 +--
xen/drivers/passthrough/vtd/x86/ats.c | 3 +-
xen/drivers/pci/pci.c | 52 +++++++-----
xen/drivers/vpci/header.c | 94 ++++++++++++++++++++++
xen/drivers/vpci/msi.c | 4 +-
xen/drivers/vpci/msix.c | 4 +-
xen/drivers/vpci/vpci.c | 58 +++++++++++--
xen/include/xen/pci.h | 14 ++--
xen/include/xen/pci_regs.h | 10 +++
xen/include/xen/vpci.h | 15 ++++
17 files changed, 255 insertions(+), 113 deletions(-)
base-commit: 6621932264e3e86df3913db4249ecd3eb100b13f
--
2.42.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v5 1/5] xen/pci: convert pci_find_*cap* to pci_sbdf_t
2023-09-01 4:57 [PATCH v5 0/5] vPCI capabilities filtering Stewart Hildebrand
@ 2023-09-01 4:57 ` Stewart Hildebrand
2023-09-01 4:57 ` [PATCH v5 2/5] x86/msi: rearrange read_pci_mem_bar slightly Stewart Hildebrand
` (3 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Stewart Hildebrand @ 2023-09-01 4:57 UTC (permalink / raw)
To: xen-devel
Cc: Stewart Hildebrand, Jan Beulich, Andrew Cooper,
Roger Pau Monné,
Wei Liu, George Dunlap, Julien Grall, Stefano Stabellini,
Paul Durrant, Kevin Tian
Convert pci_find_*cap* functions and call sites to pci_sbdf_t, and remove some
now unused local variables. Also change to more appropriate types on lines that
are already being modified as a result of the pci_sbdf_t conversion.
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
I built with EXTRA_CFLAGS_XEN_CORE="-Wunused-but-set-variable" (and
unfortunately -Wno-error=unused-but-set-variable too) to identify locations of
unneeded local variables as a result of the change to pci_sbdf_t.
v4->v5:
* add Jan's R-b
v3->v4:
* use more appropriate types on lines that are being modified anyway
* remove "no functional change" from commit description
v2->v3:
* new patch
---
xen/arch/x86/msi.c | 40 ++++++----------------
xen/drivers/char/ehci-dbgp.c | 3 +-
xen/drivers/passthrough/amd/iommu_detect.c | 2 +-
xen/drivers/passthrough/ats.c | 4 +--
xen/drivers/passthrough/ats.h | 6 ++--
xen/drivers/passthrough/msi.c | 6 ++--
xen/drivers/passthrough/pci.c | 21 +++++-------
xen/drivers/passthrough/vtd/quirks.c | 10 ++----
xen/drivers/passthrough/vtd/x86/ats.c | 3 +-
xen/drivers/pci/pci.c | 32 +++++++++--------
xen/drivers/vpci/msi.c | 4 +--
xen/drivers/vpci/msix.c | 4 +--
xen/include/xen/pci.h | 11 +++---
13 files changed, 58 insertions(+), 88 deletions(-)
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index 41b82f3e87cb..8d4fd43b10a6 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -283,7 +283,7 @@ static void msi_set_enable(struct pci_dev *dev, int enable)
u8 slot = PCI_SLOT(dev->devfn);
u8 func = PCI_FUNC(dev->devfn);
- pos = pci_find_cap_offset(seg, bus, slot, func, PCI_CAP_ID_MSI);
+ pos = pci_find_cap_offset(dev->sbdf, PCI_CAP_ID_MSI);
if ( pos )
__msi_set_enable(seg, bus, slot, func, pos, enable);
}
@@ -291,12 +291,9 @@ static void msi_set_enable(struct pci_dev *dev, int enable)
static void msix_set_enable(struct pci_dev *dev, int enable)
{
int pos;
- u16 control, seg = dev->seg;
- u8 bus = dev->bus;
- u8 slot = PCI_SLOT(dev->devfn);
- u8 func = PCI_FUNC(dev->devfn);
+ uint16_t control;
- pos = pci_find_cap_offset(seg, bus, slot, func, PCI_CAP_ID_MSIX);
+ pos = pci_find_cap_offset(dev->sbdf, PCI_CAP_ID_MSIX);
if ( pos )
{
control = pci_conf_read16(dev->sbdf, msix_control_reg(pos));
@@ -603,13 +600,10 @@ static int msi_capability_init(struct pci_dev *dev,
struct msi_desc *entry;
int pos;
unsigned int i, mpos;
- u16 control, seg = dev->seg;
- u8 bus = dev->bus;
- u8 slot = PCI_SLOT(dev->devfn);
- u8 func = PCI_FUNC(dev->devfn);
+ uint16_t control;
ASSERT(pcidevs_locked());
- pos = pci_find_cap_offset(seg, bus, slot, func, PCI_CAP_ID_MSI);
+ pos = pci_find_cap_offset(dev->sbdf, PCI_CAP_ID_MSI);
if ( !pos )
return -ENODEV;
control = pci_conf_read16(dev->sbdf, msi_control_reg(pos));
@@ -680,8 +674,8 @@ static u64 read_pci_mem_bar(u16 seg, u8 bus, u8 slot, u8 func, u8 bir, int vf)
{
struct pci_dev *pdev = pci_get_pdev(NULL,
PCI_SBDF(seg, bus, slot, func));
- unsigned int pos = pci_find_ext_capability(seg, bus,
- PCI_DEVFN(slot, func),
+ unsigned int pos = pci_find_ext_capability(PCI_SBDF(seg, bus, slot,
+ func),
PCI_EXT_CAP_ID_SRIOV);
uint16_t ctrl = pci_conf_read16(PCI_SBDF(seg, bus, slot, func),
pos + PCI_SRIOV_CTRL);
@@ -772,8 +766,7 @@ static int msix_capability_init(struct pci_dev *dev,
u8 slot = PCI_SLOT(dev->devfn);
u8 func = PCI_FUNC(dev->devfn);
bool maskall = msix->host_maskall, zap_on_error = false;
- unsigned int pos = pci_find_cap_offset(seg, bus, slot, func,
- PCI_CAP_ID_MSIX);
+ unsigned int pos = pci_find_cap_offset(dev->sbdf, PCI_CAP_ID_MSIX);
if ( !pos )
return -ENODEV;
@@ -1097,12 +1090,7 @@ static void _pci_cleanup_msix(struct arch_msix *msix)
static void __pci_disable_msix(struct msi_desc *entry)
{
struct pci_dev *dev = entry->dev;
- u16 seg = dev->seg;
- u8 bus = dev->bus;
- u8 slot = PCI_SLOT(dev->devfn);
- u8 func = PCI_FUNC(dev->devfn);
- unsigned int pos = pci_find_cap_offset(seg, bus, slot, func,
- PCI_CAP_ID_MSIX);
+ unsigned int pos = pci_find_cap_offset(dev->sbdf, PCI_CAP_ID_MSIX);
u16 control = pci_conf_read16(dev->sbdf,
msix_control_reg(entry->msi_attrib.pos));
bool maskall = dev->msix->host_maskall;
@@ -1206,8 +1194,7 @@ void pci_cleanup_msi(struct pci_dev *pdev)
int pci_reset_msix_state(struct pci_dev *pdev)
{
- unsigned int pos = pci_find_cap_offset(pdev->seg, pdev->bus, pdev->sbdf.dev,
- pdev->sbdf.fn, PCI_CAP_ID_MSIX);
+ unsigned int pos = pci_find_cap_offset(pdev->sbdf, PCI_CAP_ID_MSIX);
ASSERT(pos);
/*
@@ -1229,10 +1216,6 @@ int pci_reset_msix_state(struct pci_dev *pdev)
int pci_msi_conf_write_intercept(struct pci_dev *pdev, unsigned int reg,
unsigned int size, uint32_t *data)
{
- u16 seg = pdev->seg;
- u8 bus = pdev->bus;
- u8 slot = PCI_SLOT(pdev->devfn);
- u8 func = PCI_FUNC(pdev->devfn);
struct msi_desc *entry;
unsigned int pos;
@@ -1240,8 +1223,7 @@ int pci_msi_conf_write_intercept(struct pci_dev *pdev, unsigned int reg,
{
entry = find_msi_entry(pdev, -1, PCI_CAP_ID_MSIX);
pos = entry ? entry->msi_attrib.pos
- : pci_find_cap_offset(seg, bus, slot, func,
- PCI_CAP_ID_MSIX);
+ : pci_find_cap_offset(pdev->sbdf, PCI_CAP_ID_MSIX);
ASSERT(pos);
if ( reg >= pos && reg < msix_pba_offset_reg(pos) + 4 )
diff --git a/xen/drivers/char/ehci-dbgp.c b/xen/drivers/char/ehci-dbgp.c
index 72be4d9cc970..00cbdd5454dd 100644
--- a/xen/drivers/char/ehci-dbgp.c
+++ b/xen/drivers/char/ehci-dbgp.c
@@ -687,7 +687,8 @@ static unsigned int __init __find_dbgp(u8 bus, u8 slot, u8 func)
if ( (class >> 8) != PCI_CLASS_SERIAL_USB_EHCI )
return 0;
- return pci_find_cap_offset(0, bus, slot, func, PCI_CAP_ID_EHCI_DEBUG);
+ return pci_find_cap_offset(PCI_SBDF(0, bus, slot, func),
+ PCI_CAP_ID_EHCI_DEBUG);
}
static unsigned int __init find_dbgp(struct ehci_dbgp *dbgp,
diff --git a/xen/drivers/passthrough/amd/iommu_detect.c b/xen/drivers/passthrough/amd/iommu_detect.c
index 2317fa6a7d8d..cede44e6518f 100644
--- a/xen/drivers/passthrough/amd/iommu_detect.c
+++ b/xen/drivers/passthrough/amd/iommu_detect.c
@@ -27,7 +27,7 @@ static int __init get_iommu_msi_capabilities(
{
int pos;
- pos = pci_find_cap_offset(seg, bus, dev, func, PCI_CAP_ID_MSI);
+ pos = pci_find_cap_offset(PCI_SBDF(seg, bus, dev, func), PCI_CAP_ID_MSI);
if ( !pos )
return -ENODEV;
diff --git a/xen/drivers/passthrough/ats.c b/xen/drivers/passthrough/ats.c
index 253f5c2e1042..0da183d057c5 100644
--- a/xen/drivers/passthrough/ats.c
+++ b/xen/drivers/passthrough/ats.c
@@ -24,11 +24,9 @@ boolean_param("ats", ats_enabled);
int enable_ats_device(struct pci_dev *pdev, struct list_head *ats_list)
{
u32 value;
- u16 seg = pdev->seg;
- u8 bus = pdev->bus, devfn = pdev->devfn;
int pos;
- pos = pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS);
+ pos = pci_find_ext_capability(pdev->sbdf, PCI_EXT_CAP_ID_ATS);
BUG_ON(!pos);
if ( iommu_verbose )
diff --git a/xen/drivers/passthrough/ats.h b/xen/drivers/passthrough/ats.h
index baa5f6a6dc04..f5e1d254e0d3 100644
--- a/xen/drivers/passthrough/ats.h
+++ b/xen/drivers/passthrough/ats.h
@@ -32,7 +32,8 @@ static inline int pci_ats_enabled(int seg, int bus, int devfn)
u32 value;
int pos;
- pos = pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS);
+ pos = pci_find_ext_capability(PCI_SBDF(seg, bus, devfn),
+ PCI_EXT_CAP_ID_ATS);
BUG_ON(!pos);
value = pci_conf_read16(PCI_SBDF(seg, bus, devfn), pos + ATS_REG_CTL);
@@ -45,7 +46,8 @@ static inline int pci_ats_device(int seg, int bus, int devfn)
if ( !ats_enabled )
return 0;
- return pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS);
+ return pci_find_ext_capability(PCI_SBDF(seg, bus, devfn),
+ PCI_EXT_CAP_ID_ATS);
}
#endif /* _ATS_H_ */
diff --git a/xen/drivers/passthrough/msi.c b/xen/drivers/passthrough/msi.c
index fb78e2ebe8a4..13d904692ef8 100644
--- a/xen/drivers/passthrough/msi.c
+++ b/xen/drivers/passthrough/msi.c
@@ -24,8 +24,7 @@ int pdev_msi_init(struct pci_dev *pdev)
INIT_LIST_HEAD(&pdev->msi_list);
- pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
- PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSI);
+ pos = pci_find_cap_offset(pdev->sbdf, PCI_CAP_ID_MSI);
if ( pos )
{
uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
@@ -33,8 +32,7 @@ int pdev_msi_init(struct pci_dev *pdev)
pdev->msi_maxvec = multi_msi_capable(ctrl);
}
- pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
- PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSIX);
+ pos = pci_find_cap_offset(pdev->sbdf, PCI_CAP_ID_MSIX);
if ( pos )
{
struct arch_msix *msix = xzalloc(struct arch_msix);
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index ed1f689227fa..04d00c7c37df 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -361,8 +361,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
break;
case DEV_TYPE_PCIe_ENDPOINT:
- pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn),
- PCI_FUNC(devfn), PCI_CAP_ID_EXP);
+ pos = pci_find_cap_offset(pdev->sbdf, PCI_CAP_ID_EXP);
BUG_ON(!pos);
cap = pci_conf_read16(pdev->sbdf, pos + PCI_EXP_DEVCAP);
if ( cap & PCI_EXP_DEVCAP_PHANTOM )
@@ -565,13 +564,12 @@ struct pci_dev *pci_get_pdev(const struct domain *d, pci_sbdf_t sbdf)
static void pci_enable_acs(struct pci_dev *pdev)
{
int pos;
- u16 cap, ctrl, seg = pdev->seg;
- u8 bus = pdev->bus;
+ uint16_t cap, ctrl;
if ( !is_iommu_enabled(pdev->domain) )
return;
- pos = pci_find_ext_capability(seg, bus, pdev->devfn, PCI_EXT_CAP_ID_ACS);
+ pos = pci_find_ext_capability(pdev->sbdf, PCI_EXT_CAP_ID_ACS);
if (!pos)
return;
@@ -704,7 +702,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
if ( !pdev->info.is_virtfn && !pdev->vf_rlen[0] )
{
- unsigned int pos = pci_find_ext_capability(seg, bus, devfn,
+ unsigned int pos = pci_find_ext_capability(pdev->sbdf,
PCI_EXT_CAP_ID_SRIOV);
uint16_t ctrl = pci_conf_read16(pdev->sbdf, pos + PCI_SRIOV_CTRL);
@@ -916,7 +914,8 @@ enum pdev_type pdev_type(u16 seg, u8 bus, u8 devfn)
{
u16 class_device, creg;
u8 d = PCI_SLOT(devfn), f = PCI_FUNC(devfn);
- int pos = pci_find_cap_offset(seg, bus, d, f, PCI_CAP_ID_EXP);
+ unsigned int pos = pci_find_cap_offset(PCI_SBDF(seg, bus, devfn),
+ PCI_CAP_ID_EXP);
class_device = pci_conf_read16(PCI_SBDF(seg, bus, d, f), PCI_CLASS_DEVICE);
switch ( class_device )
@@ -1184,10 +1183,7 @@ static int hest_match_pci(const struct acpi_hest_aer_common *p,
static bool hest_match_type(const struct acpi_hest_header *hest_hdr,
const struct pci_dev *pdev)
{
- unsigned int pos = pci_find_cap_offset(pdev->seg, pdev->bus,
- PCI_SLOT(pdev->devfn),
- PCI_FUNC(pdev->devfn),
- PCI_CAP_ID_EXP);
+ unsigned int pos = pci_find_cap_offset(pdev->sbdf, PCI_CAP_ID_EXP);
u8 pcie = MASK_EXTR(pci_conf_read16(pdev->sbdf, pos + PCI_EXP_FLAGS),
PCI_EXP_FLAGS_TYPE);
@@ -1258,8 +1254,7 @@ bool pcie_aer_get_firmware_first(const struct pci_dev *pdev)
{
struct aer_hest_parse_info info = { .pdev = pdev };
- return pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
- PCI_FUNC(pdev->devfn), PCI_CAP_ID_EXP) &&
+ return pci_find_cap_offset(pdev->sbdf, PCI_CAP_ID_EXP) &&
apei_hest_parse(aer_hest_parse, &info) >= 0 &&
info.firmware_first;
}
diff --git a/xen/drivers/passthrough/vtd/quirks.c b/xen/drivers/passthrough/vtd/quirks.c
index 5d706a539788..5a56565ea883 100644
--- a/xen/drivers/passthrough/vtd/quirks.c
+++ b/xen/drivers/passthrough/vtd/quirks.c
@@ -495,8 +495,6 @@ int me_wifi_quirk(struct domain *domain, uint8_t bus, uint8_t devfn,
void pci_vtd_quirk(const struct pci_dev *pdev)
{
- int seg = pdev->seg;
- int bus = pdev->bus;
int pos;
bool ff;
u32 val, val2;
@@ -532,12 +530,10 @@ void pci_vtd_quirk(const struct pci_dev *pdev)
/* 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);
+ pos = pci_find_ext_capability(pdev->sbdf, PCI_EXT_CAP_ID_ERR);
if ( !pos )
{
- pos = pci_find_ext_capability(seg, bus, pdev->devfn,
- PCI_EXT_CAP_ID_VNDR);
+ pos = pci_find_ext_capability(pdev->sbdf, PCI_EXT_CAP_ID_VNDR);
while ( pos )
{
val = pci_conf_read32(pdev->sbdf, pos + PCI_VNDR_HEADER);
@@ -546,7 +542,7 @@ void pci_vtd_quirk(const struct pci_dev *pdev)
pos += PCI_VNDR_HEADER;
break;
}
- pos = pci_find_next_ext_capability(seg, bus, pdev->devfn, pos,
+ pos = pci_find_next_ext_capability(pdev->sbdf, pos,
PCI_EXT_CAP_ID_VNDR);
}
ff = 0;
diff --git a/xen/drivers/passthrough/vtd/x86/ats.c b/xen/drivers/passthrough/vtd/x86/ats.c
index 9de419775f90..1f5913bed9d2 100644
--- a/xen/drivers/passthrough/vtd/x86/ats.c
+++ b/xen/drivers/passthrough/vtd/x86/ats.c
@@ -57,8 +57,7 @@ int ats_device(const struct pci_dev *pdev, const struct acpi_drhd_unit *drhd)
return 0;
ats_drhd = find_ats_dev_drhd(drhd->iommu);
- pos = pci_find_ext_capability(pdev->seg, pdev->bus, pdev->devfn,
- PCI_EXT_CAP_ID_ATS);
+ pos = pci_find_ext_capability(pdev->sbdf, PCI_EXT_CAP_ID_ATS);
if ( pos && (ats_drhd == NULL) )
{
diff --git a/xen/drivers/pci/pci.c b/xen/drivers/pci/pci.c
index c73a8c4124af..3569ccb24e9e 100644
--- a/xen/drivers/pci/pci.c
+++ b/xen/drivers/pci/pci.c
@@ -8,25 +8,25 @@
#include <xen/pci.h>
#include <xen/pci_regs.h>
-int pci_find_cap_offset(u16 seg, u8 bus, u8 dev, u8 func, u8 cap)
+unsigned int pci_find_cap_offset(pci_sbdf_t sbdf, unsigned int cap)
{
u8 id;
int max_cap = 48;
u8 pos = PCI_CAPABILITY_LIST;
u16 status;
- status = pci_conf_read16(PCI_SBDF(seg, bus, dev, func), PCI_STATUS);
+ status = pci_conf_read16(sbdf, PCI_STATUS);
if ( (status & PCI_STATUS_CAP_LIST) == 0 )
return 0;
while ( max_cap-- )
{
- pos = pci_conf_read8(PCI_SBDF(seg, bus, dev, func), pos);
+ pos = pci_conf_read8(sbdf, pos);
if ( pos < 0x40 )
break;
pos &= ~3;
- id = pci_conf_read8(PCI_SBDF(seg, bus, dev, func), pos + PCI_CAP_LIST_ID);
+ id = pci_conf_read8(sbdf, pos + PCI_CAP_LIST_ID);
if ( id == 0xff )
break;
@@ -39,19 +39,20 @@ int pci_find_cap_offset(u16 seg, u8 bus, u8 dev, u8 func, u8 cap)
return 0;
}
-int pci_find_next_cap(u16 seg, u8 bus, unsigned int devfn, u8 pos, int cap)
+unsigned int pci_find_next_cap(pci_sbdf_t sbdf, unsigned int pos,
+ unsigned int cap)
{
u8 id;
int ttl = 48;
while ( ttl-- )
{
- pos = pci_conf_read8(PCI_SBDF(seg, bus, devfn), pos);
+ pos = pci_conf_read8(sbdf, pos);
if ( pos < 0x40 )
break;
pos &= ~3;
- id = pci_conf_read8(PCI_SBDF(seg, bus, devfn), pos + PCI_CAP_LIST_ID);
+ id = pci_conf_read8(sbdf, pos + PCI_CAP_LIST_ID);
if ( id == 0xff )
break;
@@ -65,21 +66,21 @@ int pci_find_next_cap(u16 seg, u8 bus, unsigned int devfn, u8 pos, int cap)
/**
* pci_find_ext_capability - Find an extended capability
- * @seg/@bus/@devfn: PCI device to query
+ * @sbdf: 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.
*/
-int pci_find_ext_capability(int seg, int bus, int devfn, int cap)
+unsigned int pci_find_ext_capability(pci_sbdf_t sbdf, unsigned int cap)
{
- return pci_find_next_ext_capability(seg, bus, devfn, 0, cap);
+ return pci_find_next_ext_capability(sbdf, 0, cap);
}
/**
* pci_find_next_ext_capability - Find another extended capability
- * @seg/@bus/@devfn: PCI device to query
+ * @sbdf: PCI device to query
* @start: starting position
* @cap: capability code
*
@@ -87,13 +88,14 @@ int pci_find_ext_capability(int seg, int bus, int devfn, int cap)
* 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)
+unsigned int pci_find_next_ext_capability(pci_sbdf_t sbdf, unsigned int start,
+ unsigned int cap)
{
u32 header;
int ttl = 480; /* 3840 bytes, minimum 8 bytes per capability */
- int pos = max(start, 0x100);
+ unsigned int pos = max(start, 0x100U);
- header = pci_conf_read32(PCI_SBDF(seg, bus, devfn), pos);
+ header = pci_conf_read32(sbdf, pos);
/*
* If we have no capabilities, this is indicated by cap ID,
@@ -109,7 +111,7 @@ int pci_find_next_ext_capability(int seg, int bus, int devfn, int start, int cap
pos = PCI_EXT_CAP_NEXT(header);
if ( pos < 0x100 )
break;
- header = pci_conf_read32(PCI_SBDF(seg, bus, devfn), pos);
+ header = pci_conf_read32(sbdf, pos);
}
return 0;
}
diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
index 2814b63d2be7..a253ccbd7db7 100644
--- a/xen/drivers/vpci/msi.c
+++ b/xen/drivers/vpci/msi.c
@@ -184,9 +184,7 @@ static void cf_check mask_write(
static int cf_check init_msi(struct pci_dev *pdev)
{
- uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
- unsigned int pos = pci_find_cap_offset(pdev->seg, pdev->bus, slot, func,
- PCI_CAP_ID_MSI);
+ unsigned int pos = pci_find_cap_offset(pdev->sbdf, PCI_CAP_ID_MSI);
uint16_t control;
int ret;
diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index 1be861343dba..d1126a417da9 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -659,14 +659,12 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
static int cf_check init_msix(struct pci_dev *pdev)
{
struct domain *d = pdev->domain;
- uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
unsigned int msix_offset, i, max_entries;
uint16_t control;
struct vpci_msix *msix;
int rc;
- msix_offset = pci_find_cap_offset(pdev->seg, pdev->bus, slot, func,
- PCI_CAP_ID_MSIX);
+ msix_offset = pci_find_cap_offset(pdev->sbdf, PCI_CAP_ID_MSIX);
if ( !msix_offset )
return 0;
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 7d8a7cd21301..ea6a4c9abf38 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -193,11 +193,12 @@ int pci_mmcfg_read(unsigned int seg, unsigned int bus,
unsigned int devfn, int reg, int len, u32 *value);
int pci_mmcfg_write(unsigned int seg, unsigned int bus,
unsigned int devfn, int reg, int len, u32 value);
-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 start,
- int cap);
+unsigned int pci_find_cap_offset(pci_sbdf_t sbdf, unsigned int cap);
+unsigned int pci_find_next_cap(pci_sbdf_t sbdf, unsigned int pos,
+ unsigned int cap);
+unsigned int pci_find_ext_capability(pci_sbdf_t sbdf, unsigned int cap);
+unsigned int pci_find_next_ext_capability(pci_sbdf_t sbdf, unsigned int start,
+ unsigned int cap);
const char *parse_pci(const char *, unsigned int *seg, unsigned int *bus,
unsigned int *dev, unsigned int *func);
const char *parse_pci_seg(const char *, unsigned int *seg, unsigned int *bus,
--
2.42.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v5 2/5] x86/msi: rearrange read_pci_mem_bar slightly
2023-09-01 4:57 [PATCH v5 0/5] vPCI capabilities filtering Stewart Hildebrand
2023-09-01 4:57 ` [PATCH v5 1/5] xen/pci: convert pci_find_*cap* to pci_sbdf_t Stewart Hildebrand
@ 2023-09-01 4:57 ` Stewart Hildebrand
2023-09-01 4:57 ` [PATCH v5 3/5] xen/pci: update PCI_STATUS_* constants Stewart Hildebrand
` (2 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Stewart Hildebrand @ 2023-09-01 4:57 UTC (permalink / raw)
To: xen-devel
Cc: Stewart Hildebrand, Jan Beulich, Andrew Cooper,
Roger Pau Monné,
Wei Liu
Use pdev->sbdf instead of the PCI_SBDF macro in calls to pci_* functions
where appropriate. Move NULL check earlier.
Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
v4->v5:
* add Jan's R-b
v3->v4:
* new patch
Suggested-by tag added based on conversation at [1]
[1] https://lists.xenproject.org/archives/html/xen-devel/2023-08/msg01886.html
---
xen/arch/x86/msi.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index 8d4fd43b10a6..a78367d7cf5d 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -674,19 +674,19 @@ static u64 read_pci_mem_bar(u16 seg, u8 bus, u8 slot, u8 func, u8 bir, int vf)
{
struct pci_dev *pdev = pci_get_pdev(NULL,
PCI_SBDF(seg, bus, slot, func));
- unsigned int pos = pci_find_ext_capability(PCI_SBDF(seg, bus, slot,
- func),
- PCI_EXT_CAP_ID_SRIOV);
- uint16_t ctrl = pci_conf_read16(PCI_SBDF(seg, bus, slot, func),
- pos + PCI_SRIOV_CTRL);
- uint16_t num_vf = pci_conf_read16(PCI_SBDF(seg, bus, slot, func),
- pos + PCI_SRIOV_NUM_VF);
- uint16_t offset = pci_conf_read16(PCI_SBDF(seg, bus, slot, func),
- pos + PCI_SRIOV_VF_OFFSET);
- uint16_t stride = pci_conf_read16(PCI_SBDF(seg, bus, slot, func),
- pos + PCI_SRIOV_VF_STRIDE);
-
- if ( !pdev || !pos ||
+ unsigned int pos;
+ uint16_t ctrl, num_vf, offset, stride;
+
+ if ( !pdev )
+ return 0;
+
+ pos = pci_find_ext_capability(pdev->sbdf, PCI_EXT_CAP_ID_SRIOV);
+ ctrl = pci_conf_read16(pdev->sbdf, pos + PCI_SRIOV_CTRL);
+ num_vf = pci_conf_read16(pdev->sbdf, pos + PCI_SRIOV_NUM_VF);
+ offset = pci_conf_read16(pdev->sbdf, pos + PCI_SRIOV_VF_OFFSET);
+ stride = pci_conf_read16(pdev->sbdf, pos + PCI_SRIOV_VF_STRIDE);
+
+ if ( !pos ||
!(ctrl & PCI_SRIOV_CTRL_VFE) ||
!(ctrl & PCI_SRIOV_CTRL_MSE) ||
!num_vf || !offset || (num_vf > 1 && !stride) ||
--
2.42.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v5 3/5] xen/pci: update PCI_STATUS_* constants
2023-09-01 4:57 [PATCH v5 0/5] vPCI capabilities filtering Stewart Hildebrand
2023-09-01 4:57 ` [PATCH v5 1/5] xen/pci: convert pci_find_*cap* to pci_sbdf_t Stewart Hildebrand
2023-09-01 4:57 ` [PATCH v5 2/5] x86/msi: rearrange read_pci_mem_bar slightly Stewart Hildebrand
@ 2023-09-01 4:57 ` Stewart Hildebrand
2023-09-06 8:00 ` Jan Beulich
2023-09-01 4:57 ` [PATCH v5 4/5] xen/vpci: header: status register handler Stewart Hildebrand
2023-09-01 5:13 ` [PATCH v5 5/5] xen/vpci: header: filter PCI capabilities Stewart Hildebrand
4 siblings, 1 reply; 10+ messages in thread
From: Stewart Hildebrand @ 2023-09-01 4:57 UTC (permalink / raw)
To: xen-devel
Cc: Stewart Hildebrand, Andrew Cooper, George Dunlap, Jan Beulich,
Julien Grall, Stefano Stabellini, Wei Liu
Interrupt status introduced in PCI 2.3
Immediate readiness introduced in PCIe 4.0
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v4->v5:
* new patch
---
xen/include/xen/pci_regs.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/xen/include/xen/pci_regs.h b/xen/include/xen/pci_regs.h
index a90aff1712ba..84b18736a85d 100644
--- a/xen/include/xen/pci_regs.h
+++ b/xen/include/xen/pci_regs.h
@@ -50,6 +50,8 @@
#define PCI_COMMAND_INTX_DISABLE 0x400 /* INTx Emulation Disable */
#define PCI_STATUS 0x06 /* 16 bits */
+#define PCI_STATUS_IMM_READY 0x01 /* Immediate Readiness */
+#define PCI_STATUS_INTERRUPT 0x08 /* Interrupt status */
#define PCI_STATUS_CAP_LIST 0x10 /* Support Capability List */
#define PCI_STATUS_66MHZ 0x20 /* Support 66 Mhz PCI 2.1 bus */
#define PCI_STATUS_UDF 0x40 /* Support User Definable Features [obsolete] */
--
2.42.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v5 4/5] xen/vpci: header: status register handler
2023-09-01 4:57 [PATCH v5 0/5] vPCI capabilities filtering Stewart Hildebrand
` (2 preceding siblings ...)
2023-09-01 4:57 ` [PATCH v5 3/5] xen/pci: update PCI_STATUS_* constants Stewart Hildebrand
@ 2023-09-01 4:57 ` Stewart Hildebrand
2023-09-06 8:22 ` Jan Beulich
2023-09-01 5:13 ` [PATCH v5 5/5] xen/vpci: header: filter PCI capabilities Stewart Hildebrand
4 siblings, 1 reply; 10+ messages in thread
From: Stewart Hildebrand @ 2023-09-01 4:57 UTC (permalink / raw)
To: xen-devel
Cc: Stewart Hildebrand, Roger Pau Monné,
Andrew Cooper, George Dunlap, Jan Beulich, Julien Grall,
Stefano Stabellini, Wei Liu
Introduce a handler for the PCI status register, with ability to mask the
capabilities bit. The status register contains reserved bits, read-only bits,
and write-1-to-clear bits, so introduce bitmasks to handle these in vPCI. If a
bit in the bitmask is set, then the special meaning applies:
res_mask: read as zero, write ignore
ro_mask: read normal, write ignore
rw1c_mask: read normal, write 1 to clear
The mask_cap_list flag will be set in a follow-on patch.
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v4->v5:
* add support for res_mask
* add support for ro_mask (squash ro_mask patch)
* add constants for reserved, read-only, and rw1c masks
v3->v4:
* move mask_cap_list setting to the capabilities patch
* single pci_conf_read16 in status_read
* align mask_cap_list bitfield in struct vpci_header
* change to rw1c bit mask instead of treating whole register as rw1c
* drop subsystem prefix on renamed add_register function
v2->v3:
* new patch
---
xen/drivers/vpci/header.c | 18 +++++++++++++++
xen/drivers/vpci/vpci.c | 46 +++++++++++++++++++++++++++++++-------
xen/include/xen/pci_regs.h | 8 +++++++
xen/include/xen/vpci.h | 10 +++++++++
4 files changed, 74 insertions(+), 8 deletions(-)
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 767c1ba718d7..791791e6c9b6 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -413,6 +413,18 @@ static void cf_check cmd_write(
pci_conf_write16(pdev->sbdf, reg, cmd);
}
+static uint32_t cf_check status_read(const struct pci_dev *pdev,
+ unsigned int reg, void *data)
+{
+ struct vpci_header *header = data;
+ uint32_t status = pci_conf_read16(pdev->sbdf, reg);
+
+ if ( header->mask_cap_list )
+ status &= ~PCI_STATUS_CAP_LIST;
+
+ return status;
+}
+
static void cf_check bar_write(
const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data)
{
@@ -544,6 +556,12 @@ static int cf_check init_bars(struct pci_dev *pdev)
if ( rc )
return rc;
+ rc = vpci_add_register_mask(pdev->vpci, status_read, vpci_hw_write16,
+ PCI_STATUS, 2, header, PCI_STATUS_RESERVED_MASK,
+ PCI_STATUS_RO_MASK, PCI_STATUS_RW1C_MASK);
+ if ( rc )
+ return rc;
+
if ( pdev->ignore_bars )
return 0;
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 3bec9a4153da..6e6ad4b80a0d 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -29,6 +29,9 @@ struct vpci_register {
unsigned int offset;
void *private;
struct list_head node;
+ uint32_t res_mask;
+ uint32_t ro_mask;
+ uint32_t rw1c_mask;
};
#ifdef __XEN__
@@ -145,9 +148,16 @@ uint32_t cf_check vpci_hw_read32(
return pci_conf_read32(pdev->sbdf, reg);
}
-int vpci_add_register(struct vpci *vpci, vpci_read_t *read_handler,
- vpci_write_t *write_handler, unsigned int offset,
- unsigned int size, void *data)
+void cf_check vpci_hw_write16(
+ const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data)
+{
+ pci_conf_write16(pdev->sbdf, reg, val);
+}
+
+static int add_register(struct vpci *vpci, vpci_read_t *read_handler,
+ vpci_write_t *write_handler, unsigned int offset,
+ unsigned int size, void *data, uint32_t res_mask,
+ uint32_t ro_mask, uint32_t rw1c_mask)
{
struct list_head *prev;
struct vpci_register *r;
@@ -167,6 +177,9 @@ int vpci_add_register(struct vpci *vpci, vpci_read_t *read_handler,
r->size = size;
r->offset = offset;
r->private = data;
+ r->res_mask = res_mask & (0xffffffffU >> (32 - 8 * size));
+ r->ro_mask = ro_mask & (0xffffffffU >> (32 - 8 * size));
+ r->rw1c_mask = rw1c_mask & (0xffffffffU >> (32 - 8 * size));
spin_lock(&vpci->lock);
@@ -193,6 +206,23 @@ int vpci_add_register(struct vpci *vpci, vpci_read_t *read_handler,
return 0;
}
+int vpci_add_register(struct vpci *vpci, vpci_read_t *read_handler,
+ vpci_write_t *write_handler, unsigned int offset,
+ unsigned int size, void *data)
+{
+ return add_register(vpci, read_handler, write_handler, offset, size, data,
+ 0, 0, 0);
+}
+
+int vpci_add_register_mask(struct vpci *vpci, vpci_read_t *read_handler,
+ vpci_write_t *write_handler, unsigned int offset,
+ unsigned int size, void *data, uint32_t res_mask,
+ uint32_t ro_mask, uint32_t rw1c_mask)
+{
+ return add_register(vpci, read_handler, write_handler, offset, size, data,
+ res_mask, ro_mask, rw1c_mask);
+}
+
int vpci_remove_register(struct vpci *vpci, unsigned int offset,
unsigned int size)
{
@@ -376,6 +406,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size)
}
val = r->read(pdev, r->offset, r->private);
+ val &= ~r->res_mask;
/* Check if the read is in the middle of a register. */
if ( r->offset < emu.offset )
@@ -407,11 +438,6 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size)
/*
* Perform a maybe partial write to a register.
- *
- * Note that this will only work for simple registers, if Xen needs to
- * trap accesses to rw1c registers (like the status PCI header register)
- * the logic in vpci_write will have to be expanded in order to correctly
- * deal with them.
*/
static void vpci_write_helper(const struct pci_dev *pdev,
const struct vpci_register *r, unsigned int size,
@@ -424,9 +450,13 @@ static void vpci_write_helper(const struct pci_dev *pdev,
uint32_t val;
val = r->read(pdev, r->offset, r->private);
+ val &= ~r->res_mask;
+ val &= ~r->rw1c_mask;
data = merge_result(val, data, size, offset);
}
+ data &= ~r->res_mask;
+ data &= ~r->ro_mask;
r->write(pdev, r->offset, data & (0xffffffffU >> (32 - 8 * r->size)),
r->private);
}
diff --git a/xen/include/xen/pci_regs.h b/xen/include/xen/pci_regs.h
index 84b18736a85d..b7cb200969c6 100644
--- a/xen/include/xen/pci_regs.h
+++ b/xen/include/xen/pci_regs.h
@@ -66,6 +66,14 @@
#define PCI_STATUS_REC_MASTER_ABORT 0x2000 /* Set on master abort */
#define PCI_STATUS_SIG_SYSTEM_ERROR 0x4000 /* Set when we drive SERR */
#define PCI_STATUS_DETECTED_PARITY 0x8000 /* Set on parity error */
+#define PCI_STATUS_RESERVED_MASK 0x06
+#define PCI_STATUS_RO_MASK (PCI_STATUS_IMM_READY | PCI_STATUS_INTERRUPT | \
+ PCI_STATUS_CAP_LIST | PCI_STATUS_CAP_LIST | PCI_STATUS_66MHZ | \
+ PCI_STATUS_UDF | PCI_STATUS_FAST_BACK | PCI_STATUS_DEVSEL_MASK)
+#define PCI_STATUS_RW1C_MASK (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)
#define PCI_CLASS_REVISION 0x08 /* High 24 bits are class, low 8 revision */
#define PCI_REVISION_ID 0x08 /* Revision ID */
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 0b8a2a3c745b..0a6c9e19b399 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -37,6 +37,12 @@ int __must_check vpci_add_register(struct vpci *vpci,
vpci_write_t *write_handler,
unsigned int offset, unsigned int size,
void *data);
+int __must_check vpci_add_register_mask(struct vpci *vpci,
+ vpci_read_t *read_handler,
+ vpci_write_t *write_handler,
+ unsigned int offset, unsigned int size,
+ void *data, uint32_t res_mask,
+ uint32_t ro_mask, uint32_t rw1c_mask);
int __must_check vpci_remove_register(struct vpci *vpci, unsigned int offset,
unsigned int size);
@@ -50,6 +56,8 @@ uint32_t cf_check vpci_hw_read16(
const struct pci_dev *pdev, unsigned int reg, void *data);
uint32_t cf_check vpci_hw_read32(
const struct pci_dev *pdev, unsigned int reg, void *data);
+void cf_check vpci_hw_write16(
+ const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data);
/*
* Check for pending vPCI operations on this vcpu. Returns true if the vcpu
@@ -94,6 +102,8 @@ struct vpci {
* upon to know whether BARs are mapped into the guest p2m.
*/
bool bars_mapped : 1;
+ /* Store whether to hide all capabilities from the guest. */
+ bool mask_cap_list : 1;
/* FIXME: currently there's no support for SR-IOV. */
} header;
--
2.42.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v5 5/5] xen/vpci: header: filter PCI capabilities
2023-09-01 4:57 [PATCH v5 0/5] vPCI capabilities filtering Stewart Hildebrand
` (3 preceding siblings ...)
2023-09-01 4:57 ` [PATCH v5 4/5] xen/vpci: header: status register handler Stewart Hildebrand
@ 2023-09-01 5:13 ` Stewart Hildebrand
4 siblings, 0 replies; 10+ messages in thread
From: Stewart Hildebrand @ 2023-09-01 5:13 UTC (permalink / raw)
To: xen-devel
Cc: Stewart Hildebrand, Andrew Cooper, George Dunlap, Jan Beulich,
Julien Grall, Stefano Stabellini, Wei Liu, Roger Pau Monné
Currently, Xen vPCI only supports virtualizing the MSI and MSI-X capabilities.
Hide all other PCI capabilities (including extended capabilities) from domUs for
now, even though there may be certain devices/drivers that depend on being able
to discover certain capabilities.
We parse the physical PCI capabilities linked list and add vPCI register
handlers for the next elements, inserting our own next value, thus presenting a
modified linked list to the domU.
Introduce helper functions vpci_hw_read8 and vpci_read_val. The vpci_read_val
helper function returns a fixed value, which may be used for RAZ registers, or
registers whose value doesn't change.
Introduce pci_find_next_cap_ttl() helper while adapting the logic from
pci_find_next_cap() to suit our needs, and implement the existing
pci_find_next_cap() in terms of the new helper.
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
v4->v5:
* use more appropriate types, continued
* get rid of unnecessary hook function
* add Jan's R-b
v3->v4:
* move mask_cap_list setting to this patch
* leave pci_find_next_cap signature alone
* use more appropriate types
v2->v3:
* get rid of > 0 in loop condition
* implement pci_find_next_cap in terms of new pci_find_next_cap_ttl function so
that hypothetical future callers wouldn't be required to pass &ttl.
* change NULL to (void *)0 for RAZ value passed to vpci_read_val
* change type of ttl to unsigned int
* remember to mask off the low 2 bits of next in the initial loop iteration
* change return type of pci_find_next_cap and pci_find_next_cap_ttl
* avoid wrapping the PCI_STATUS_CAP_LIST condition by using ! instead of == 0
v1->v2:
* change type of ttl to int
* use switch statement instead of if/else
* adapt existing pci_find_next_cap helper instead of rolling our own
* pass ttl as in/out
* "pass through" the lower 2 bits of the next pointer
* squash helper functions into this patch to avoid transient dead code situation
* extended capabilities RAZ/WI
---
xen/drivers/pci/pci.c | 26 +++++++++-----
xen/drivers/vpci/header.c | 76 +++++++++++++++++++++++++++++++++++++++
xen/drivers/vpci/vpci.c | 12 +++++++
xen/include/xen/pci.h | 3 ++
xen/include/xen/vpci.h | 5 +++
5 files changed, 113 insertions(+), 9 deletions(-)
diff --git a/xen/drivers/pci/pci.c b/xen/drivers/pci/pci.c
index 3569ccb24e9e..8799d60c2156 100644
--- a/xen/drivers/pci/pci.c
+++ b/xen/drivers/pci/pci.c
@@ -39,31 +39,39 @@ unsigned int pci_find_cap_offset(pci_sbdf_t sbdf, unsigned int cap)
return 0;
}
-unsigned int pci_find_next_cap(pci_sbdf_t sbdf, unsigned int pos,
- unsigned int cap)
+unsigned int pci_find_next_cap_ttl(pci_sbdf_t sbdf, unsigned int pos,
+ bool (*is_match)(unsigned int),
+ unsigned int cap, unsigned int *ttl)
{
- u8 id;
- int ttl = 48;
+ unsigned int id;
- while ( ttl-- )
+ while ( (*ttl)-- )
{
pos = pci_conf_read8(sbdf, pos);
if ( pos < 0x40 )
break;
- pos &= ~3;
- id = pci_conf_read8(sbdf, pos + PCI_CAP_LIST_ID);
+ id = pci_conf_read8(sbdf, (pos & ~3) + PCI_CAP_LIST_ID);
if ( id == 0xff )
break;
- if ( id == cap )
+ if ( (is_match && is_match(id)) || (!is_match && id == cap) )
return pos;
- pos += PCI_CAP_LIST_NEXT;
+ pos = (pos & ~3) + PCI_CAP_LIST_NEXT;
}
+
return 0;
}
+unsigned int pci_find_next_cap(pci_sbdf_t sbdf, unsigned int pos,
+ unsigned int cap)
+{
+ unsigned int ttl = 48;
+
+ return pci_find_next_cap_ttl(sbdf, pos, NULL, cap, &ttl) & ~3;
+}
+
/**
* pci_find_ext_capability - Find an extended capability
* @sbdf: PCI device to query
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 791791e6c9b6..f7d02a7f7bfa 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -525,6 +525,18 @@ static void cf_check rom_write(
rom->addr = val & PCI_ROM_ADDRESS_MASK;
}
+static bool cf_check vpci_cap_supported(unsigned int id)
+{
+ switch ( id )
+ {
+ case PCI_CAP_ID_MSI:
+ case PCI_CAP_ID_MSIX:
+ return true;
+ default:
+ return false;
+ }
+}
+
static int cf_check init_bars(struct pci_dev *pdev)
{
uint16_t cmd;
@@ -562,6 +574,70 @@ static int cf_check init_bars(struct pci_dev *pdev)
if ( rc )
return rc;
+ if ( !is_hardware_domain(pdev->domain) )
+ {
+ if ( !(pci_conf_read16(pdev->sbdf, PCI_STATUS) & PCI_STATUS_CAP_LIST) )
+ {
+ /* RAZ/WI */
+ rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
+ PCI_CAPABILITY_LIST, 1, (void *)0);
+ if ( rc )
+ return rc;
+ }
+ else
+ {
+ /* Only expose capabilities to the guest that vPCI can handle. */
+ unsigned int next, ttl = 48;
+
+ next = pci_find_next_cap_ttl(pdev->sbdf, PCI_CAPABILITY_LIST,
+ vpci_cap_supported, 0, &ttl);
+
+ rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
+ PCI_CAPABILITY_LIST, 1,
+ (void *)(uintptr_t)next);
+ if ( rc )
+ return rc;
+
+ next &= ~3;
+
+ if ( !next )
+ /*
+ * If we don't have any supported capabilities to expose to the
+ * guest, mask the PCI_STATUS_CAP_LIST bit in the status
+ * register.
+ */
+ header->mask_cap_list = true;
+
+ while ( next && ttl )
+ {
+ unsigned int pos = next;
+
+ next = pci_find_next_cap_ttl(pdev->sbdf,
+ pos + PCI_CAP_LIST_NEXT,
+ vpci_cap_supported, 0, &ttl);
+
+ rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL,
+ pos + PCI_CAP_LIST_ID, 1, NULL);
+ if ( rc )
+ return rc;
+
+ rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
+ pos + PCI_CAP_LIST_NEXT, 1,
+ (void *)(uintptr_t)next);
+ if ( rc )
+ return rc;
+
+ next &= ~3;
+ }
+ }
+
+ /* Extended capabilities RAZ/WI */
+ rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL, 0x100, 4,
+ (void *)0);
+ if ( rc )
+ return rc;
+ }
+
if ( pdev->ignore_bars )
return 0;
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 6e6ad4b80a0d..94cbae312abe 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -136,6 +136,18 @@ static void cf_check vpci_ignored_write(
{
}
+uint32_t cf_check vpci_read_val(
+ const struct pci_dev *pdev, unsigned int reg, void *data)
+{
+ return (uintptr_t)data;
+}
+
+uint32_t cf_check vpci_hw_read8(
+ const struct pci_dev *pdev, unsigned int reg, void *data)
+{
+ return pci_conf_read8(pdev->sbdf, reg);
+}
+
uint32_t cf_check vpci_hw_read16(
const struct pci_dev *pdev, unsigned int reg, void *data)
{
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index ea6a4c9abf38..cceac8654f07 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -194,6 +194,9 @@ int pci_mmcfg_read(unsigned int seg, unsigned int bus,
int pci_mmcfg_write(unsigned int seg, unsigned int bus,
unsigned int devfn, int reg, int len, u32 value);
unsigned int pci_find_cap_offset(pci_sbdf_t sbdf, unsigned int cap);
+unsigned int pci_find_next_cap_ttl(pci_sbdf_t sbdf, unsigned int pos,
+ bool (*is_match)(unsigned int),
+ unsigned int cap, unsigned int *ttl);
unsigned int pci_find_next_cap(pci_sbdf_t sbdf, unsigned int pos,
unsigned int cap);
unsigned int pci_find_ext_capability(pci_sbdf_t sbdf, unsigned int cap);
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 0a6c9e19b399..866bbf656e2b 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -51,7 +51,12 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size);
void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
uint32_t data);
+uint32_t cf_check vpci_read_val(
+ const struct pci_dev *pdev, unsigned int reg, void *data);
+
/* Passthrough handlers. */
+uint32_t cf_check vpci_hw_read8(
+ const struct pci_dev *pdev, unsigned int reg, void *data);
uint32_t cf_check vpci_hw_read16(
const struct pci_dev *pdev, unsigned int reg, void *data);
uint32_t cf_check vpci_hw_read32(
--
2.42.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v5 3/5] xen/pci: update PCI_STATUS_* constants
2023-09-01 4:57 ` [PATCH v5 3/5] xen/pci: update PCI_STATUS_* constants Stewart Hildebrand
@ 2023-09-06 8:00 ` Jan Beulich
0 siblings, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2023-09-06 8:00 UTC (permalink / raw)
To: Stewart Hildebrand
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, xen-devel
On 01.09.2023 06:57, Stewart Hildebrand wrote:
> Interrupt status introduced in PCI 2.3
> Immediate readiness introduced in PCIe 4.0
>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 4/5] xen/vpci: header: status register handler
2023-09-01 4:57 ` [PATCH v5 4/5] xen/vpci: header: status register handler Stewart Hildebrand
@ 2023-09-06 8:22 ` Jan Beulich
2023-09-07 21:29 ` Stewart Hildebrand
0 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2023-09-06 8:22 UTC (permalink / raw)
To: Stewart Hildebrand
Cc: Roger Pau Monné,
Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, xen-devel
On 01.09.2023 06:57, Stewart Hildebrand wrote:
> Introduce a handler for the PCI status register, with ability to mask the
> capabilities bit. The status register contains reserved bits, read-only bits,
> and write-1-to-clear bits, so introduce bitmasks to handle these in vPCI. If a
> bit in the bitmask is set, then the special meaning applies:
>
> res_mask: read as zero, write ignore
> ro_mask: read normal, write ignore
> rw1c_mask: read normal, write 1 to clear
With the last one's name being descriptive of what the behavior is, would
it make sense to name the first one "raz_mask"? (Also a question to Roger
as the maintainer of this code.)
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -413,6 +413,18 @@ static void cf_check cmd_write(
> pci_conf_write16(pdev->sbdf, reg, cmd);
> }
>
> +static uint32_t cf_check status_read(const struct pci_dev *pdev,
> + unsigned int reg, void *data)
> +{
> + struct vpci_header *header = data;
> + uint32_t status = pci_conf_read16(pdev->sbdf, reg);
> +
> + if ( header->mask_cap_list )
> + status &= ~PCI_STATUS_CAP_LIST;
> +
> + return status;
> +}
Do you actually need this function? Can't you ...
> @@ -544,6 +556,12 @@ static int cf_check init_bars(struct pci_dev *pdev)
> if ( rc )
> return rc;
>
> + rc = vpci_add_register_mask(pdev->vpci, status_read, vpci_hw_write16,
> + PCI_STATUS, 2, header, PCI_STATUS_RESERVED_MASK,
> + PCI_STATUS_RO_MASK, PCI_STATUS_RW1C_MASK);
... conditionally OR in PCI_STATUS_CAP_LIST right here? Without
capabilities the CAP_LIST bit becomes kind of reserved anyway.
> @@ -424,9 +450,13 @@ static void vpci_write_helper(const struct pci_dev *pdev,
> uint32_t val;
>
> val = r->read(pdev, r->offset, r->private);
> + val &= ~r->res_mask;
> + val &= ~r->rw1c_mask;
Personally I'd fold these two lines into just one (and similarly below).
> data = merge_result(val, data, size, offset);
> }
>
> + data &= ~r->res_mask;
> + data &= ~r->ro_mask;
This seems risky to me. I'd rather see the same value being written back
for r/o bits, just in case a device doesn't actually implement a bit as
mandated. For reserved flags it's less clear what's best, because in
principle they could also become rw1c once defined.
> --- a/xen/include/xen/pci_regs.h
> +++ b/xen/include/xen/pci_regs.h
> @@ -66,6 +66,14 @@
> #define PCI_STATUS_REC_MASTER_ABORT 0x2000 /* Set on master abort */
> #define PCI_STATUS_SIG_SYSTEM_ERROR 0x4000 /* Set when we drive SERR */
> #define PCI_STATUS_DETECTED_PARITY 0x8000 /* Set on parity error */
> +#define PCI_STATUS_RESERVED_MASK 0x06
I'd recommend separating the "derived" constants by a blank line. I'd
further like to ask that you add two more padding zeros above.
> +#define PCI_STATUS_RO_MASK (PCI_STATUS_IMM_READY | PCI_STATUS_INTERRUPT | \
> + PCI_STATUS_CAP_LIST | PCI_STATUS_CAP_LIST | PCI_STATUS_66MHZ | \
CAP_LIST twice?
Jan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 4/5] xen/vpci: header: status register handler
2023-09-06 8:22 ` Jan Beulich
@ 2023-09-07 21:29 ` Stewart Hildebrand
2023-09-08 6:24 ` Jan Beulich
0 siblings, 1 reply; 10+ messages in thread
From: Stewart Hildebrand @ 2023-09-07 21:29 UTC (permalink / raw)
To: Jan Beulich
Cc: Roger Pau Monné,
Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, xen-devel
On 9/6/23 04:22, Jan Beulich wrote:
> On 01.09.2023 06:57, Stewart Hildebrand wrote:
>> Introduce a handler for the PCI status register, with ability to mask the
>> capabilities bit. The status register contains reserved bits, read-only bits,
>> and write-1-to-clear bits, so introduce bitmasks to handle these in vPCI. If a
>> bit in the bitmask is set, then the special meaning applies:
>>
>> res_mask: read as zero, write ignore
>> ro_mask: read normal, write ignore
>> rw1c_mask: read normal, write 1 to clear
>
> With the last one's name being descriptive of what the behavior is, would
> it make sense to name the first one "raz_mask"? (Also a question to Roger
> as the maintainer of this code.)
Another possible name is rsvdz_mask. See below.
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -413,6 +413,18 @@ static void cf_check cmd_write(
>> pci_conf_write16(pdev->sbdf, reg, cmd);
>> }
>>
>> +static uint32_t cf_check status_read(const struct pci_dev *pdev,
>> + unsigned int reg, void *data)
>> +{
>> + struct vpci_header *header = data;
>> + uint32_t status = pci_conf_read16(pdev->sbdf, reg);
>> +
>> + if ( header->mask_cap_list )
>> + status &= ~PCI_STATUS_CAP_LIST;
>> +
>> + return status;
>> +}
>
> Do you actually need this function? Can't you ...
>
>> @@ -544,6 +556,12 @@ static int cf_check init_bars(struct pci_dev *pdev)
>> if ( rc )
>> return rc;
>>
>> + rc = vpci_add_register_mask(pdev->vpci, status_read, vpci_hw_write16,
>> + PCI_STATUS, 2, header, PCI_STATUS_RESERVED_MASK,
>> + PCI_STATUS_RO_MASK, PCI_STATUS_RW1C_MASK);
>
> ... conditionally OR in PCI_STATUS_CAP_LIST right here? Without
> capabilities the CAP_LIST bit becomes kind of reserved anyway.
Hm. The PCI_STATUS_CAP_LIST bit is a read-only bit according to spec. Given the rationale below, we would want to preserve the value of r/o bits during writes, so this would essentially want to become a RsvdP bit. I wasn't planning on adding support for RsvdP bits here since there aren't any proper RsvdP bits in the status register. If instead we are okay with treating the PCI_STATUS_CAP_LIST bit as RsvdZ, then we could do as you suggest, but I'll plan to keep it as is for now.
>> @@ -424,9 +450,13 @@ static void vpci_write_helper(const struct pci_dev *pdev,
>> uint32_t val;
>>
>> val = r->read(pdev, r->offset, r->private);
>> + val &= ~r->res_mask;
>> + val &= ~r->rw1c_mask;
>
> Personally I'd fold these two lines into just one (and similarly below).
Will do.
>> data = merge_result(val, data, size, offset);
>> }
>>
>> + data &= ~r->res_mask;
>> + data &= ~r->ro_mask;
>
> This seems risky to me. I'd rather see the same value being written back
> for r/o bits, just in case a device doesn't actually implement a bit as
> mandated.
Regarding writes to read-only bits: okay, I'll assume that Xen should take care of preserving the r/o bits (discarding the guests write value for the r/o bits).
If, on the other hand, we would want to rely on the guest to preserve the r/o bits during a write, then the ro_mask would effectively not be doing anything, so may as well be removed.
In either case, for partial register writes, Xen will take care of preserving the r/o bits for the remaining bytes in the register.
I'll make this change for the next version of the series (assuming Xen will handle preserving the r/o bits).
> For reserved flags it's less clear what's best, because in
> principle they could also become rw1c once defined.
Well, it depends on what version of the spec we read.
PCI Local Bus 3.0 spec section 6.1 says: "On writes, software must ensure that the values of reserved bit positions are preserved." PCI Local Bus 3.0 spec section 6.2.3 also says that we can essentially treat the whole status register as rw1c. Huh, that's odd.
PCI Express Base 4.0 spec defines two reserved bit types:
RsvdP: Reserved and Preserved - Reserved for future RW implementations. Software must preserve the value read for writes to bits.
RsvdZ: Reserved and Zero - Reserved for future RW1C implementations. Software must use 0b for writes to bits.
Both RsvdP and RsvdZ are read as zero.
I'm favoring using the definitions in the newer PCI Express Base 4.0 spec since they are clearer.
Regarding writes to rsvdp bits: there are none of these in the status register according to the PCI Express Base 4.0 spec. The older PCI Local Bus 3.0 spec is in conflict with this definition, but at the same time the PCI Local Bus 3.0 spec also conflicts with itself by saying that the entire status register is essentially a rw1c register with reserved bits that should be preserved, which doesn't make sense. The newer PCI Express Base 4.0 spec is clearer, and appears to have resolved this inconsistency, so I'm favoring adhering to the newer PCI Express Base 4.0 spec.
Regarding writes to rsvdz bits: in this case Xen will write zero to the rsvdz bits (discarding the guests write value). This is how the patch already behaves with the res_mask, but I think renaming it to rsvdz_mask will make it more clear, so I'm inclined to rename it for the next revision of the series.
Regarding reads of rsvdp/rsvdz bits: I'm assuming that Xen should return zero to the guest for reads of reserved bits, regardless of what is actually read from the hardware. I think we already are already on the same page about this, I just wanted to explicitly state it for clarity.
>> --- a/xen/include/xen/pci_regs.h
>> +++ b/xen/include/xen/pci_regs.h
>> @@ -66,6 +66,14 @@
>> #define PCI_STATUS_REC_MASTER_ABORT 0x2000 /* Set on master abort */
>> #define PCI_STATUS_SIG_SYSTEM_ERROR 0x4000 /* Set when we drive SERR */
>> #define PCI_STATUS_DETECTED_PARITY 0x8000 /* Set on parity error */
>> +#define PCI_STATUS_RESERVED_MASK 0x06
>
> I'd recommend separating the "derived" constants by a blank line. I'd
> further like to ask that you add two more padding zeros above.
Okay, will do.
>> +#define PCI_STATUS_RO_MASK (PCI_STATUS_IMM_READY | PCI_STATUS_INTERRUPT | \
>> + PCI_STATUS_CAP_LIST | PCI_STATUS_CAP_LIST | PCI_STATUS_66MHZ | \
>
> CAP_LIST twice?
Good catch, it obviously only needs to be there once. I will fix.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 4/5] xen/vpci: header: status register handler
2023-09-07 21:29 ` Stewart Hildebrand
@ 2023-09-08 6:24 ` Jan Beulich
0 siblings, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2023-09-08 6:24 UTC (permalink / raw)
To: Stewart Hildebrand
Cc: Roger Pau Monné,
Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, xen-devel
On 07.09.2023 23:29, Stewart Hildebrand wrote:
> On 9/6/23 04:22, Jan Beulich wrote:
>> On 01.09.2023 06:57, Stewart Hildebrand wrote:
>>> Introduce a handler for the PCI status register, with ability to mask the
>>> capabilities bit. The status register contains reserved bits, read-only bits,
>>> and write-1-to-clear bits, so introduce bitmasks to handle these in vPCI. If a
>>> bit in the bitmask is set, then the special meaning applies:
>>>
>>> res_mask: read as zero, write ignore
>>> ro_mask: read normal, write ignore
>>> rw1c_mask: read normal, write 1 to clear
>>
>> With the last one's name being descriptive of what the behavior is, would
>> it make sense to name the first one "raz_mask"? (Also a question to Roger
>> as the maintainer of this code.)
>
> Another possible name is rsvdz_mask. See below.
Ah, yes, that's better even if for now we won't need rsvdp support.
>>> --- a/xen/drivers/vpci/header.c
>>> +++ b/xen/drivers/vpci/header.c
>>> @@ -413,6 +413,18 @@ static void cf_check cmd_write(
>>> pci_conf_write16(pdev->sbdf, reg, cmd);
>>> }
>>>
>>> +static uint32_t cf_check status_read(const struct pci_dev *pdev,
>>> + unsigned int reg, void *data)
>>> +{
>>> + struct vpci_header *header = data;
>>> + uint32_t status = pci_conf_read16(pdev->sbdf, reg);
>>> +
>>> + if ( header->mask_cap_list )
>>> + status &= ~PCI_STATUS_CAP_LIST;
>>> +
>>> + return status;
>>> +}
>>
>> Do you actually need this function? Can't you ...
>>
>>> @@ -544,6 +556,12 @@ static int cf_check init_bars(struct pci_dev *pdev)
>>> if ( rc )
>>> return rc;
>>>
>>> + rc = vpci_add_register_mask(pdev->vpci, status_read, vpci_hw_write16,
>>> + PCI_STATUS, 2, header, PCI_STATUS_RESERVED_MASK,
>>> + PCI_STATUS_RO_MASK, PCI_STATUS_RW1C_MASK);
>>
>> ... conditionally OR in PCI_STATUS_CAP_LIST right here? Without
>> capabilities the CAP_LIST bit becomes kind of reserved anyway.
>
> Hm. The PCI_STATUS_CAP_LIST bit is a read-only bit according to spec. Given the rationale below, we would want to preserve the value of r/o bits during writes, so this would essentially want to become a RsvdP bit. I wasn't planning on adding support for RsvdP bits here since there aren't any proper RsvdP bits in the status register. If instead we are okay with treating the PCI_STATUS_CAP_LIST bit as RsvdZ, then we could do as you suggest, but I'll plan to keep it as is for now.
I'd say leverage rsvdz (with a comment) for the time being, but let's see
what Roger thinks.
>>> @@ -424,9 +450,13 @@ static void vpci_write_helper(const struct pci_dev *pdev,
>>> uint32_t val;
>>>
>>> val = r->read(pdev, r->offset, r->private);
>>> + val &= ~r->res_mask;
>>> + val &= ~r->rw1c_mask;
>>
>> Personally I'd fold these two lines into just one (and similarly below).
>
> Will do.
>
>>> data = merge_result(val, data, size, offset);
>>> }
>>>
>>> + data &= ~r->res_mask;
>>> + data &= ~r->ro_mask;
>>
>> This seems risky to me. I'd rather see the same value being written back
>> for r/o bits, just in case a device doesn't actually implement a bit as
>> mandated.
>
> Regarding writes to read-only bits: okay, I'll assume that Xen should take care of preserving the r/o bits (discarding the guests write value for the r/o bits).
>
> If, on the other hand, we would want to rely on the guest to preserve the r/o bits during a write, then the ro_mask would effectively not be doing anything, so may as well be removed.
We may never rely on the guest doing anything the way we want it.
> In either case, for partial register writes, Xen will take care of preserving the r/o bits for the remaining bytes in the register.
>
> I'll make this change for the next version of the series (assuming Xen will handle preserving the r/o bits).
>
>> For reserved flags it's less clear what's best, because in
>> principle they could also become rw1c once defined.
>
> Well, it depends on what version of the spec we read.
>
> PCI Local Bus 3.0 spec section 6.1 says: "On writes, software must ensure that the values of reserved bit positions are preserved." PCI Local Bus 3.0 spec section 6.2.3 also says that we can essentially treat the whole status register as rw1c. Huh, that's odd.
>
> PCI Express Base 4.0 spec defines two reserved bit types:
> RsvdP: Reserved and Preserved - Reserved for future RW implementations. Software must preserve the value read for writes to bits.
> RsvdZ: Reserved and Zero - Reserved for future RW1C implementations. Software must use 0b for writes to bits.
> Both RsvdP and RsvdZ are read as zero.
>
> I'm favoring using the definitions in the newer PCI Express Base 4.0 spec since they are clearer.
+1
> Regarding writes to rsvdp bits: there are none of these in the status register according to the PCI Express Base 4.0 spec. The older PCI Local Bus 3.0 spec is in conflict with this definition, but at the same time the PCI Local Bus 3.0 spec also conflicts with itself by saying that the entire status register is essentially a rw1c register with reserved bits that should be preserved, which doesn't make sense. The newer PCI Express Base 4.0 spec is clearer, and appears to have resolved this inconsistency, so I'm favoring adhering to the newer PCI Express Base 4.0 spec.
>
> Regarding writes to rsvdz bits: in this case Xen will write zero to the rsvdz bits (discarding the guests write value). This is how the patch already behaves with the res_mask, but I think renaming it to rsvdz_mask will make it more clear, so I'm inclined to rename it for the next revision of the series.
>
> Regarding reads of rsvdp/rsvdz bits: I'm assuming that Xen should return zero to the guest for reads of reserved bits, regardless of what is actually read from the hardware. I think we already are already on the same page about this, I just wanted to explicitly state it for clarity.
Yes, we are.
Jan
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-09-08 6:24 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-01 4:57 [PATCH v5 0/5] vPCI capabilities filtering Stewart Hildebrand
2023-09-01 4:57 ` [PATCH v5 1/5] xen/pci: convert pci_find_*cap* to pci_sbdf_t Stewart Hildebrand
2023-09-01 4:57 ` [PATCH v5 2/5] x86/msi: rearrange read_pci_mem_bar slightly Stewart Hildebrand
2023-09-01 4:57 ` [PATCH v5 3/5] xen/pci: update PCI_STATUS_* constants Stewart Hildebrand
2023-09-06 8:00 ` Jan Beulich
2023-09-01 4:57 ` [PATCH v5 4/5] xen/vpci: header: status register handler Stewart Hildebrand
2023-09-06 8:22 ` Jan Beulich
2023-09-07 21:29 ` Stewart Hildebrand
2023-09-08 6:24 ` Jan Beulich
2023-09-01 5:13 ` [PATCH v5 5/5] xen/vpci: header: filter PCI capabilities Stewart Hildebrand
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.