Re: [PATCH v8 1/2] xen/vpci: header: status register handler

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, Nov 28, 2023 at 02:44:24PM -0500, Stewart Hildebrand wrote:
> Introduce a handler for the PCI status register, with ability to mask
> the capabilities bit. The status register contains RsvdZ bits,
> read-only bits, and write-1-to-clear bits. Additionally, we use RsvdP to
> mask the capabilities bit. Introduce bitmasks to handle these in vPCI.
> If a bit in the bitmask is set, then the special meaning applies:
> 
>   ro_mask: read normal, guest write ignore (preserve on write to hardware)
>   rw1c_mask: read normal, write 1 to clear
>   rsvdp_mask: read as zero, guest write ignore (preserve on write to hardware)
>   rsvdz_mask: read as zero, guest write ignore (write zero to hardware)
> 
> The RO/RW1C/RsvdP/RsvdZ naming and definitions were borrowed from the
> PCI Express Base 6.1 specification. RsvdP/RsvdZ bits help Xen enforce
> our view of the world. Xen preserves the value of read-only bits on
> write to hardware, discarding the guests write value. This is done in
> case hardware wrongly implements R/O bits as R/W.
> 
> The mask_cap_list flag will be set in a follow-on patch.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>

Thanks for adding the tests, this is looking very good, just a couple
of cosmetics comments mostly, and a question whether we should refuse
masks that have bit set outside the register size instead of
attempting to adjust them.

> ---
> v7->v8:
> * move PCI_STATUS_UDF to rsvdz_mask (per PCI Express Base 6 spec)
> * add support for rsvdp bits
> * add tests for ro/rw1c/rsvdp/rsvdz bits in tools/tests/vpci/main.c
> * dropped R-b tag [1] since the patch has changed moderately since the last rev
> 
> [1] https://lists.xenproject.org/archives/html/xen-devel/2023-09/msg00909.html
> 
> v6->v7:
> * re-work args passed to vpci_add_register_mask() (called in init_bars())
> * also check for overlap of (rsvdz_mask & ro_mask) in add_register()
> * slightly adjust masking operation in vpci_write_helper()
> 
> v5->v6:
> * remove duplicate PCI_STATUS_CAP_LIST in constant definition
> * style fixup in constant definitions
> * s/res_mask/rsvdz_mask/
> * combine a new masking operation into single line
> * preserve r/o bits on write
> * get rid of status_read. Instead, use rsvdz_mask for conditionally masking
>   PCI_STATUS_CAP_LIST bit
> * add comment about PCI_STATUS_CAP_LIST and rsvdp behavior
> * add sanity checks in add_register
> * move mask_cap_list from struct vpci_header to local variable
> 
> 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
> ---
>  tools/tests/vpci/main.c    | 98 ++++++++++++++++++++++++++++++++++++++
>  xen/drivers/vpci/header.c  | 12 +++++
>  xen/drivers/vpci/vpci.c    | 62 +++++++++++++++++++-----
>  xen/include/xen/pci_regs.h |  9 ++++
>  xen/include/xen/vpci.h     |  9 ++++
>  5 files changed, 178 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/tests/vpci/main.c b/tools/tests/vpci/main.c
> index b9a0a6006bb9..b0bb993be297 100644
> --- a/tools/tests/vpci/main.c
> +++ b/tools/tests/vpci/main.c
> @@ -70,6 +70,26 @@ static void vpci_write32(const struct pci_dev *pdev, unsigned int reg,
>      *(uint32_t *)data = val;
>  }
>  
> +struct mask_data {
> +    uint32_t val;
> +    uint32_t rw1c_mask;
> +};
> +
> +static uint32_t vpci_read32_mask(const struct pci_dev *pdev, unsigned int reg,
> +                                 void *data)
> +{
> +    struct mask_data *md = data;

Newline, and possibly const for md.

> +    return md->val;
> +}
> +
> +static void vpci_write32_mask(const struct pci_dev *pdev, unsigned int reg,
> +                              uint32_t val, void *data)
> +{
> +    struct mask_data *md = data;

Newline.

> +    md->val  = val | (md->val & md->rw1c_mask);
> +    md->val &= ~(val & md->rw1c_mask);
> +}
> +
>  #define VPCI_READ(reg, size, data) ({                           \
>      data = vpci_read((pci_sbdf_t){ .sbdf = 0 }, reg, size);     \
>  })
> @@ -94,9 +114,20 @@ static void vpci_write32(const struct pci_dev *pdev, unsigned int reg,
>      assert(!vpci_add_register(test_pdev.vpci, fread, fwrite, off, size,     \
>                                &store))
>  
> +#define VPCI_ADD_REG_MASK(fread, fwrite, off, size, store,                     \
> +                          ro_mask, rw1c_mask, rsvdp_mask, rsvdz_mask)          \
> +    assert(!vpci_add_register_mask(test_pdev.vpci, fread, fwrite, off, size,   \
> +                                   &store,                                     \
> +                                   ro_mask, rw1c_mask, rsvdp_mask, rsvdz_mask))
> +
>  #define VPCI_ADD_INVALID_REG(fread, fwrite, off, size)                      \
>      assert(vpci_add_register(test_pdev.vpci, fread, fwrite, off, size, NULL))
>  
> +#define VPCI_ADD_INVALID_REG_MASK(fread, fwrite, off, size,                   \
> +                                  ro_mask, rw1c_mask, rsvdp_mask, rsvdz_mask) \
> +    assert(vpci_add_register_mask(test_pdev.vpci, fread, fwrite, off, size,   \
> +                                  NULL, ro_mask, rw1c_mask, rsvdp_mask, rsvdz_mask))
> +
>  #define VPCI_REMOVE_REG(off, size)                                          \
>      assert(!vpci_remove_register(test_pdev.vpci, off, size))
>  
> @@ -154,6 +185,7 @@ main(int argc, char **argv)
>      uint16_t r20[2] = { };
>      uint32_t r24 = 0;
>      uint8_t r28, r30;
> +    struct mask_data r32;
>      unsigned int i;
>      int rc;
>  
> @@ -213,6 +245,14 @@ main(int argc, char **argv)
>      /* Try to add a register with missing handlers. */
>      VPCI_ADD_INVALID_REG(NULL, NULL, 8, 2);
>  
> +    /* Try to add registers with the same bits set in multiple masks. */
> +    VPCI_ADD_INVALID_REG_MASK(vpci_read32, vpci_write32, 8, 4, 1, 1, 0, 0);
> +    VPCI_ADD_INVALID_REG_MASK(vpci_read32, vpci_write32, 8, 4, 1, 0, 1, 0);
> +    VPCI_ADD_INVALID_REG_MASK(vpci_read32, vpci_write32, 8, 4, 1, 0, 0, 1);
> +    VPCI_ADD_INVALID_REG_MASK(vpci_read32, vpci_write32, 8, 4, 0, 1, 1, 0);
> +    VPCI_ADD_INVALID_REG_MASK(vpci_read32, vpci_write32, 8, 4, 0, 1, 0, 1);
> +    VPCI_ADD_INVALID_REG_MASK(vpci_read32, vpci_write32, 8, 4, 0, 0, 1, 1);
> +
>      /* Read/write of unset register. */
>      VPCI_READ_CHECK(8, 4, 0xffffffff);
>      VPCI_READ_CHECK(8, 2, 0xffff);
> @@ -287,6 +327,64 @@ main(int argc, char **argv)
>      VPCI_ADD_REG(vpci_read8, vpci_write8, 30, 1, r30);
>      VPCI_WRITE_CHECK(28, 4, 0xffacffdc);
>  
> +    /*
> +     * Test ro/rw1c/rsvdp/rsvdz masks.
> +     *
> +     * 32     24     16      8      0
> +     *  +---------------------------+
> +     *  |            r32            | 32
> +     *  +---------------------------+

Might be even better to clarify which region is using each mask:

32     24     16      8      0
 +------+------+------+------+
 |rsvdz |rsvdp | rw1c |  ro  | 32
 +------+------+------+------+

> +     *
> +     */
> +    r32.rw1c_mask = 0x0000ff00U;
> +    VPCI_ADD_REG_MASK(vpci_read32_mask, vpci_write32_mask, 32, 4, r32,
> +                      0x000000ffU   /* RO    */,
> +                      r32.rw1c_mask /* RW1C  */,
> +                      0x00ff0000U   /* RsvdP */,
> +                      0xff000000U   /* RsvdZ */);
> +
> +    /* ro */
> +    r32.val = 0x0f0f0f0fU;
> +    VPCI_READ_CHECK(32, 1, 0x0f);
> +    VPCI_WRITE(32, 1, 0x5a);
> +    VPCI_READ_CHECK(32, 1, 0x0f);
> +    assert(r32.val == 0x000f0f0fU);
> +
> +    /* rw1c */
> +    r32.val = 0x0f0f0f0fU;
> +    VPCI_READ_CHECK(33, 1, 0x0f);
> +    VPCI_WRITE(33, 1, 0x5a);
> +    VPCI_READ_CHECK(33, 1, 0x05);
> +    assert(r32.val == 0x000f050fU);
> +
> +    /* rsvdp */
> +    r32.val = 0x0f0f0f0fU;
> +    VPCI_READ_CHECK(34, 1, 0);
> +    VPCI_WRITE(34, 1, 0x5a);
> +    VPCI_READ_CHECK(34, 1, 0);
> +    assert(r32.val == 0x000f0f0fU);
> +
> +    /* rsvdz */
> +    r32.val = 0x0f0f0f0fU;
> +    VPCI_READ_CHECK(35, 1, 0);
> +    VPCI_WRITE(35, 1, 0x5a);
> +    VPCI_READ_CHECK(35, 1, 0);
> +    assert(r32.val == 0x000f0f0fU);
> +
> +    /* write all 0's */
> +    r32.val = 0x0f0f0f0fU;
> +    VPCI_READ_CHECK(32, 4, 0x00000f0fU);
> +    VPCI_WRITE(32, 4, 0);
> +    VPCI_READ_CHECK(32, 4, 0x00000f0fU);
> +    assert(r32.val == 0x000f0f0fU);
> +
> +    /* write all 1's */
> +    r32.val = 0x0f0f0f0fU;
> +    VPCI_READ_CHECK(32, 4, 0x00000f0fU);
> +    VPCI_WRITE(32, 4, 0xffffffffU);
> +    VPCI_READ_CHECK(32, 4, 0x0000000fU);
> +    assert(r32.val == 0x000f000fU);
> +
>      /* Finally try to remove a couple of registers. */
>      VPCI_REMOVE_REG(28, 1);
>      VPCI_REMOVE_REG(24, 4);
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 767c1ba718d7..351318121e48 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -521,6 +521,7 @@ static int cf_check init_bars(struct pci_dev *pdev)
>      struct vpci_header *header = &pdev->vpci->header;
>      struct vpci_bar *bars = header->bars;
>      int rc;
> +    bool mask_cap_list = false;
>  
>      switch ( pci_conf_read8(pdev->sbdf, PCI_HEADER_TYPE) & 0x7f )
>      {
> @@ -544,6 +545,17 @@ static int cf_check init_bars(struct pci_dev *pdev)
>      if ( rc )
>          return rc;
>  
> +    /* Utilize rsvdp_mask to hide PCI_STATUS_CAP_LIST from the guest. */
> +    rc = vpci_add_register_mask(pdev->vpci, vpci_hw_read16, vpci_hw_write16,
> +                                PCI_STATUS, 2, NULL,
> +                                PCI_STATUS_RO_MASK &
> +                                    ~(mask_cap_list ? PCI_STATUS_CAP_LIST : 0),
> +                                PCI_STATUS_RW1C_MASK,
> +                                mask_cap_list ? PCI_STATUS_CAP_LIST : 0,
> +                                PCI_STATUS_RSVDZ_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..96187b70141b 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -29,6 +29,10 @@ struct vpci_register {
>      unsigned int offset;
>      void *private;
>      struct list_head node;
> +    uint32_t ro_mask;
> +    uint32_t rw1c_mask;
> +    uint32_t rsvdp_mask;
> +    uint32_t rsvdz_mask;
>  };
>  
>  #ifdef __XEN__
> @@ -145,9 +149,17 @@ 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 ro_mask,
> +                        uint32_t rw1c_mask, uint32_t rsvdp_mask,
> +                        uint32_t rsvdz_mask)
>  {
>      struct list_head *prev;
>      struct vpci_register *r;
> @@ -155,7 +167,10 @@ int vpci_add_register(struct vpci *vpci, vpci_read_t *read_handler,
>      /* Some sanity checks. */
>      if ( (size != 1 && size != 2 && size != 4) ||
>           offset >= PCI_CFG_SPACE_EXP_SIZE || (offset & (size - 1)) ||
> -         (!read_handler && !write_handler) )
> +         (!read_handler && !write_handler) || (ro_mask & rw1c_mask) ||
> +         (ro_mask & rsvdp_mask) || (ro_mask & rsvdz_mask) ||
> +         (rw1c_mask & rsvdp_mask) || (rw1c_mask & rsvdz_mask) ||
> +         (rsvdp_mask & rsvdz_mask) )

It would also be helpful to check that the masks don't have bits set
above the given register size, ie:

if ( size != 4 &&
     (ro_mask | rw1c_mask | rsvdp_mask | rsvdz_mask) >> (size * 8) )
    return -EINVAL;

>          return -EINVAL;
>  
>      r = xmalloc(struct vpci_register);
> @@ -167,6 +182,10 @@ int vpci_add_register(struct vpci *vpci, vpci_read_t *read_handler,
>      r->size = size;
>      r->offset = offset;
>      r->private = data;
> +    r->ro_mask = ro_mask & (0xffffffffU >> (32 - 8 * size));
> +    r->rw1c_mask = rw1c_mask & (0xffffffffU >> (32 - 8 * size));
> +    r->rsvdp_mask = rsvdp_mask & (0xffffffffU >> (32 - 8 * size));
> +    r->rsvdz_mask = rsvdz_mask & (0xffffffffU >> (32 - 8 * size));

Oh, you adjust the masks to match the expected width.  I think it
might be more sensible to instead make sure the caller has provided
appropriate masks, as providing a mask that doesn't match the register
size likely points out to issues in the caller.

>  
>      spin_lock(&vpci->lock);
>  
> @@ -193,6 +212,24 @@ 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, 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 ro_mask,
> +                           uint32_t rw1c_mask, uint32_t rsvdp_mask,
> +                           uint32_t rsvdz_mask)
> +{
> +    return add_register(vpci, read_handler, write_handler, offset, size, data,
> +                        ro_mask, rw1c_mask, rsvdp_mask, rsvdz_mask);
> +}
> +
>  int vpci_remove_register(struct vpci *vpci, unsigned int offset,
>                           unsigned int size)
>  {
> @@ -376,6 +413,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->rsvdp_mask | r->rsvdz_mask);
>  
>          /* Check if the read is in the middle of a register. */
>          if ( r->offset < emu.offset )
> @@ -407,26 +445,26 @@ 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,
>                                unsigned int offset, uint32_t data)
>  {
> +    uint32_t val = 0;

Nit: might be clearer to name this 'current': it's easy to get
confused whether val or data holds the user-provided input.

> +    uint32_t preserved_mask = r->ro_mask | r->rsvdp_mask;
> +
>      ASSERT(size <= r->size);
>  
> -    if ( size != r->size )
> +    if ( (size != r->size) || preserved_mask )
>      {
> -        uint32_t val;
> -
>          val = r->read(pdev, r->offset, r->private);
> +        val &= ~r->rw1c_mask;
>          data = merge_result(val, data, size, offset);
>      }
>  
> +    data &= ~(preserved_mask | r->rsvdz_mask);
> +    data |= val & preserved_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..9909b27425a5 100644
> --- a/xen/include/xen/pci_regs.h
> +++ b/xen/include/xen/pci_regs.h
> @@ -66,6 +66,15 @@
>  #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_RSVDZ_MASK		0x0046 /* Includes PCI_STATUS_UDF */
> +
> +#define  PCI_STATUS_RO_MASK (PCI_STATUS_IMM_READY | PCI_STATUS_INTERRUPT | \
> +    PCI_STATUS_CAP_LIST | PCI_STATUS_66MHZ | 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 d743d96a10b8..8e8e42372ec1 100644
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -37,6 +37,13 @@ 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 ro_mask,
> +                                        uint32_t rw1c_mask, uint32_t rsvdp_mask,
> +                                        uint32_t rsvdz_mask);

Instead of exporting two functions, you could export only
vpci_add_register_mask() and make vpci_add_register() a static inline
defined in the header as a wrapper around vpci_add_register_mask().

Thanks, Roger.