re-work PCI config setup API (#198)

Split up vfu_pci_setup_config_hdr(): individual "helpers" like vfu_pci_set_id()
are much simpler to use than making the user specify the values in
header-formatted structs; and this way if we want to add additional helpers, we
won't need to modify the existing functions.

Signed-off-by: John Levon <john.levon@nutanix.com>
Reviewed-by: Thanos Makatos <thanos.makatos@nutanix.com>

8 files changed, 94 insertions, 69 deletions

diff --git a/include/libvfio-user.h b/include/libvfio-user.h
index 75878c1..e091db9 100644
--- a/include/libvfio-user.h
+++ b/include/libvfio-user.h
@@ -550,34 +550,50 @@ typedef enum {
 } vfu_pci_type_t;
 
 /**
- * Setup PCI configuration space header data. This function must be called only
+ * Initialize the context for a PCI device. This function must be called only
  * once per libvfio-user context.
  *
+ * This function initializes a buffer for the PCI config space, accessible via
+ * vfu_pci_get_config_space().
+ *
+ * Returns 0 on success, or -1 on error, setting errno.
+ *
  * @vfu_ctx: the libvfio-user context
- * @id: Device and vendor ID
- * @ss: Subsystem vendor and device ID
- * @cc: Class code
  * @pci_type: PCI type (convention PCI, PCI-X mode 1, PCI-X mode2, PCI-Express)
+ * @hdr_type: PCI header type. Only PCI_HEADER_TYPE_NORMAL is supported.
  * @revision: PCI/PCI-X/PCIe revision
+ */
+int
+vfu_pci_init(vfu_ctx_t *vfu_ctx, vfu_pci_type_t pci_type,
+             int hdr_type, int revision __attribute__((unused)));
+
+/*
+ * Set the Vendor ID, Device ID, Subsystem Vendor ID, and Subsystem ID fields of
+ * the PCI config header (PCI3 6.2.1, 6.2.4).
  *
- * @returns 0 on success, -1 on failure and sets errno.
- * TODO: Check other PCI header registers suitable to be filled by device.
- *       Or should we pass whole vfu_pci_hdr_t to be filled by user.
+ * This must always be called for PCI devices, after vfu_pci_init().
+ */
+void
+vfu_pci_set_id(vfu_ctx_t *vfu_ctx, uint16_t vid, uint16_t did,
+               uint16_t ssvid, uint16_t ssid);
 
+/*
+ * Set the class code fields (base, sub-class, and programming interface) of the
+ * PCI config header (PCI3 6.2.1).
+ *
+ * If this function is not called, the fields are initialized to zero.
  */
-int
-vfu_pci_setup_config_hdr(vfu_ctx_t *vfu_ctx, vfu_pci_hdr_id_t id,
-                         vfu_pci_hdr_ss_t ss, vfu_pci_hdr_cc_t cc,
-                         vfu_pci_type_t pci_type,
-                         int revision __attribute__((unused)));
+void
+vfu_pci_set_class(vfu_ctx_t *vfu_ctx, uint8_t base, uint8_t sub, uint8_t pi);
+
 
 /*
  * Returns a pointer to the PCI configuration space.
  *
  * PCI config space consists of an initial 64-byte vfu_pci_hdr_t, plus
- * additional space, either containing capabilities, or device-specific
- * configuration.  Standard config space is 256 bytes; extended config space is
- * 4096 bytes.
+ * additional space, containing capabilities and/or device-specific
+ * configuration.  Standard config space is 256 bytes (PCI_CFG_SPACE_SIZE);
+ * extended config space is 4096 bytes (PCI_CFG_SPACE_EXP_SIZE).
  */
 vfu_pci_config_space_t *
 vfu_pci_get_config_space(vfu_ctx_t *vfu_ctx);
diff --git a/lib/pci.c b/lib/pci.c
index f28e7b0..01b8a27 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -339,27 +339,15 @@ pci_config_space_access(vfu_ctx_t *vfu_ctx, char *buf, size_t count,
 }
 
 int
-vfu_pci_setup_config_hdr(vfu_ctx_t *vfu_ctx, vfu_pci_hdr_id_t id,
-                         vfu_pci_hdr_ss_t ss, vfu_pci_hdr_cc_t cc,
-                         vfu_pci_type_t pci_type,
-                         int revision __attribute__((unused)))
+vfu_pci_init(vfu_ctx_t *vfu_ctx, vfu_pci_type_t pci_type,
+             int hdr_type, int revision UNUSED)
 {
     vfu_pci_config_space_t *config_space;
     size_t size;
 
     assert(vfu_ctx != NULL);
 
-    /*
-     * TODO there no real reason why we shouldn't allow this, we should just
-     * clean up and redo it.
-     */
-    if (vfu_ctx->pci.config_space != NULL) {
-        vfu_log(vfu_ctx, LOG_ERR, "PCI configuration space header already setup");
-        return ERROR(EEXIST);
-    }
-
-    vfu_ctx->pci.type = pci_type;
-    switch (vfu_ctx->pci.type) {
+    switch (pci_type) {
     case VFU_PCI_TYPE_CONVENTIONAL:
     case VFU_PCI_TYPE_PCI_X_1:
         size = PCI_CFG_SPACE_SIZE;
@@ -369,25 +357,56 @@ vfu_pci_setup_config_hdr(vfu_ctx_t *vfu_ctx, vfu_pci_hdr_id_t id,
         size = PCI_CFG_SPACE_EXP_SIZE;
         break;
     default:
-        vfu_log(vfu_ctx, LOG_ERR, "invalid PCI type %d", pci_type);
+        vfu_log(vfu_ctx, LOG_ERR, "invalid PCI type %u", pci_type);
         return ERROR(EINVAL);
     }
 
+    if (hdr_type != PCI_HEADER_TYPE_NORMAL) {
+        vfu_log(vfu_ctx, LOG_ERR, "invalid PCI header type %d", hdr_type);
+        return ERROR(EINVAL);
+    }
+
+    /*
+     * TODO there no real reason why we shouldn't allow this, we should just
+     * clean up and redo it.
+     */
+    if (vfu_ctx->pci.config_space != NULL) {
+        vfu_log(vfu_ctx, LOG_ERR,
+                "PCI configuration space header already setup");
+        return ERROR(EEXIST);
+    }
+
     // Allocate a buffer for the config space.
     config_space = calloc(1, size);
     if (config_space == NULL) {
         return ERROR(ENOMEM);
     }
 
-    config_space->hdr.id = id;
-    config_space->hdr.ss = ss;
-    config_space->hdr.cc = cc;
+    vfu_ctx->pci.type = pci_type;
     vfu_ctx->pci.config_space = config_space;
     vfu_ctx->reg_info[VFU_PCI_DEV_CFG_REGION_IDX].size = size;
 
     return 0;
 }
 
+void
+vfu_pci_set_id(vfu_ctx_t *vfu_ctx, uint16_t vid, uint16_t did,
+               uint16_t ssvid, uint16_t ssid)
+{
+    vfu_ctx->pci.config_space->hdr.id.vid = vid;
+    vfu_ctx->pci.config_space->hdr.id.did = did;
+    vfu_ctx->pci.config_space->hdr.ss.vid = ssvid;
+    vfu_ctx->pci.config_space->hdr.ss.sid = ssid;
+}
+
+void
+vfu_pci_set_class(vfu_ctx_t *vfu_ctx, uint8_t base, uint8_t sub, uint8_t pi)
+{
+    vfu_ctx->pci.config_space->hdr.cc.bcc = base;
+    vfu_ctx->pci.config_space->hdr.cc.scc = sub;
+    vfu_ctx->pci.config_space->hdr.cc.pi = pi;
+}
+
 int
 vfu_pci_setup_caps(vfu_ctx_t *vfu_ctx, vfu_cap_t **caps, int nr_caps)
 {
diff --git a/samples/client.c b/samples/client.c
index 7568afe..dbff008 100644
--- a/samples/client.c
+++ b/samples/client.c
@@ -983,7 +983,7 @@ int main(int argc, char *argv[])
      */
     negotiate(sock, &server_max_fds, &pgsize);
 
-    /* try to access a bogus region, we should het an error */
+    /* try to access a bogus region, we should get an error */
     ret = access_region(sock, 0xdeadbeef, false, 0, &ret, sizeof ret);
     if (ret != -EINVAL) {
         errx(EXIT_FAILURE,
@@ -1006,10 +1006,11 @@ int main(int argc, char *argv[])
         errx(EXIT_FAILURE, "failed to read PCI configuration space: %s\n",
              strerror(-ret));
     }
-    assert(config_space.id.raw == 0xdeadbeef);
-    assert(config_space.ss.raw == 0xcafebabe);
-    assert(config_space.cc.pi == 0xab && config_space.cc.scc == 0xcd
-           && config_space.cc.bcc == 0xef);
+
+    assert(config_space.id.vid == 0xdead);
+    assert(config_space.id.did == 0xbeef);
+    assert(config_space.ss.vid == 0xcafe);
+    assert(config_space.ss.sid == 0xbabe);
 
     /* XXX VFIO_USER_DEVICE_RESET */
     send_device_reset(sock);
diff --git a/samples/gpio-pci-idio-16.c b/samples/gpio-pci-idio-16.c
index 08f2cca..350d11f 100644
--- a/samples/gpio-pci-idio-16.c
+++ b/samples/gpio-pci-idio-16.c
@@ -75,9 +75,6 @@ main(int argc, char *argv[])
     char opt;
     struct sigaction act = { .sa_handler = _sa_handler };
     vfu_ctx_t *vfu_ctx;
-    vfu_pci_hdr_id_t id = { .vid = 0x494F, .did = 0x0DC8 };
-    vfu_pci_hdr_ss_t ss = { .vid = 0x0, .sid = 0x0 };
-    vfu_pci_hdr_cc_t cc = { { 0 } };
 
     while ((opt = getopt(argc, argv, "v")) != -1) {
         switch (opt) {
@@ -114,13 +111,14 @@ main(int argc, char *argv[])
         err(EXIT_FAILURE, "failed to setup log");
     }
 
-    ret = vfu_pci_setup_config_hdr(vfu_ctx, id, ss, cc, 
-                                   VFU_PCI_TYPE_CONVENTIONAL, 0);
+    ret = vfu_pci_init(vfu_ctx, VFU_PCI_TYPE_CONVENTIONAL,
+                       PCI_HEADER_TYPE_NORMAL, 0);
     if (ret < 0) {
-        fprintf(stderr, "failed to setup pci header\n");
-        goto out;
+        err(EXIT_FAILURE, "vfu_pci_init() failed");
     }
 
+    vfu_pci_set_id(vfu_ctx, 0x494f, 0x0dc8, 0x0, 0x0);
+
     ret = vfu_setup_region(vfu_ctx, VFU_PCI_DEV_BAR2_REGION_IDX, 0x100,
                            &bar2_access, VFU_REGION_FLAG_RW, NULL, 0, -1);
     if (ret < 0) {
diff --git a/samples/lspci.c b/samples/lspci.c
index 61861d6..e2fa6fc 100644
--- a/samples/lspci.c
+++ b/samples/lspci.c
@@ -42,9 +42,6 @@ int main(void)
     int i, j;
     char *buf;
     const int bytes_per_line = 0x10;
-    vfu_pci_hdr_id_t id = { 0 };
-    vfu_pci_hdr_ss_t ss = { 0 };
-    vfu_pci_hdr_cc_t cc = { { 0 } };
     vfu_cap_t pm = { .pm = { .hdr.id = PCI_CAP_ID_PM, .pmcs.nsfrst = 0x1 } };
     vfu_cap_t *vsc = alloca(sizeof(*vsc) + 0xd);
     vfu_cap_t *caps[2] = { &pm, vsc };
@@ -54,8 +51,9 @@ int main(void)
     if (vfu_ctx == NULL) {
         err(EXIT_FAILURE, "failed to create libvfio-user context");
     }
-    if (vfu_pci_setup_config_hdr(vfu_ctx, id, ss, cc, VFU_PCI_TYPE_CONVENTIONAL, 0) < 0) {
-        err(EXIT_FAILURE, "failed to setup PCI configuration space header");
+    if (vfu_pci_init(vfu_ctx, VFU_PCI_TYPE_CONVENTIONAL,
+                     PCI_HEADER_TYPE_NORMAL, 0) < 0) {
+        err(EXIT_FAILURE, "vfu_pci_init() failed");
     }
     vsc->vsc.hdr.id = PCI_CAP_ID_VNDR;
     vsc->vsc.size = 0x10;
diff --git a/samples/null.c b/samples/null.c
index 23d727a..d05d0fc 100644
--- a/samples/null.c
+++ b/samples/null.c
@@ -81,10 +81,6 @@ int main(int argc, char **argv)
 {
     int ret;
     pthread_t thread;
-    vfu_pci_hdr_id_t id = { 0 };
-    vfu_pci_hdr_ss_t ss = { 0 };
-    vfu_pci_hdr_cc_t cc = { { 0 } };
-
 
     if (argc != 2) {
         errx(EXIT_FAILURE, "missing vfio-user socket path");
@@ -101,8 +97,10 @@ int main(int argc, char **argv)
         err(EXIT_FAILURE, "failed to setup log");
     }
 
-    ret = vfu_pci_setup_config_hdr(vfu_ctx, id, ss, cc,
-                                   VFU_PCI_TYPE_CONVENTIONAL, 0);
+    if (vfu_pci_init(vfu_ctx, VFU_PCI_TYPE_CONVENTIONAL,
+                     PCI_HEADER_TYPE_NORMAL, 0) < 0) {
+        err(EXIT_FAILURE, "vfu_pci_init() failed");
+    }
 
     ret = pthread_create(&thread, NULL, null_drive, vfu_ctx);
     if (ret != 0) {
diff --git a/samples/server.c b/samples/server.c
index 88b9591..fa9bd7b 100644
--- a/samples/server.c
+++ b/samples/server.c
@@ -375,9 +375,6 @@ int main(int argc, char *argv[])
         }
     };
     vfu_ctx_t *vfu_ctx;
-    vfu_pci_hdr_id_t id = {.raw = 0xdeadbeef};
-    vfu_pci_hdr_ss_t ss = {.raw = 0xcafebabe};
-    vfu_pci_hdr_cc_t cc = {.pi = 0xab, .scc = 0xcd, .bcc = 0xef};
     FILE *fp;
 
     while ((opt = getopt(argc, argv, "v")) != -1) {
@@ -416,12 +413,14 @@ int main(int argc, char *argv[])
         err(EXIT_FAILURE, "failed to setup log");
     }
 
-    ret = vfu_pci_setup_config_hdr(vfu_ctx, id, ss, cc,
-                                   VFU_PCI_TYPE_CONVENTIONAL, 0);
+    ret = vfu_pci_init(vfu_ctx, VFU_PCI_TYPE_CONVENTIONAL,
+                       PCI_HEADER_TYPE_NORMAL, 0);
     if (ret < 0) {
-        err(EXIT_FAILURE, "failed to setup PCI header");
+        err(EXIT_FAILURE, "vfu_pci_init() failed") ;
     }
 
+    vfu_pci_set_id(vfu_ctx, 0xdead, 0xbeef, 0xcafe, 0xbabe);
+
     ret = vfu_setup_region(vfu_ctx, VFU_PCI_DEV_BAR0_REGION_IDX, sizeof(time_t),
                            &bar0_access, VFU_REGION_FLAG_RW, NULL, 0, -1);
     if (ret < 0) {
diff --git a/test/unit-tests.c b/test/unit-tests.c
index 9063038..a304e02 100644
--- a/test/unit-tests.c
+++ b/test/unit-tests.c
@@ -516,9 +516,6 @@ static void
 test_vfu_ctx_create(void **state __attribute__((unused)))
 {
     vfu_ctx_t *vfu_ctx = NULL;
-    vfu_pci_hdr_id_t id = { 0 };
-    vfu_pci_hdr_ss_t ss = { 0 };
-    vfu_pci_hdr_cc_t cc = { { 0 } };
     vfu_cap_t pm = {.pm = {.hdr.id = PCI_CAP_ID_PM}};
     vfu_cap_t *caps[] = { &pm };
 
@@ -527,9 +524,8 @@ test_vfu_ctx_create(void **state __attribute__((unused)))
     assert_non_null(vfu_ctx);
     assert_int_equal(1, vfu_ctx->irq_count[VFU_DEV_ERR_IRQ]);
     assert_int_equal(1, vfu_ctx->irq_count[VFU_DEV_REQ_IRQ]);
-    assert_int_equal(0,
-                     vfu_pci_setup_config_hdr(vfu_ctx, id, ss, cc,
-                                              VFU_PCI_TYPE_CONVENTIONAL, 0));
+    assert_int_equal(0, vfu_pci_init(vfu_ctx, VFU_PCI_TYPE_CONVENTIONAL,
+                        PCI_HEADER_TYPE_NORMAL, 0));
     assert_int_equal(0, vfu_pci_setup_caps(vfu_ctx, caps, 1));
     assert_int_equal(0, vfu_realize_ctx(vfu_ctx));
 }