Re: [PATCH v4 4/6] xen/vpci: header: status register handler

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.08.2023 19:56, Stewart Hildebrand wrote:
> --- 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;
> +}

Imo we also cannot validly pass through any of the reserved bits. Doing so
is an option only once we know what purpose they might gain. (In this
context I notice our set of PCI_STATUS_* constants isn't quite up-to-date.)

> @@ -544,6 +556,11 @@ static int cf_check init_bars(struct pci_dev *pdev)
>      if ( rc )
>          return rc;
>  
> +    rc = vpci_add_rw1c_register(pdev->vpci, status_read, vpci_hw_write16,
> +                                PCI_STATUS, 2, header, 0xF900);

Rather than a literal number, imo this wants to be an OR of the respective
PCI_STATUS_* constants (which, if you like, could of course be consolidated
into a new PCI_STATUS_RW1C_MASK, to help readability).

> @@ -167,6 +174,7 @@ int vpci_add_register(struct vpci *vpci, vpci_read_t 
> *read_handler,
>      r->size = size;
>      r->offset = offset;
>      r->private = data;
> +    r->rw1c_mask = rw1c_mask;

To avoid surprises with ...

> @@ -424,6 +443,7 @@ static void vpci_write_helper(const struct pci_dev *pdev,
>          uint32_t val;
>  
>          val = r->read(pdev, r->offset, r->private);
> +        val &= ~r->rw1c_mask;
>          data = merge_result(val, data, size, offset);

... the user of this field, should you either assert that no bits beyond
the field size are set, or simply mask to the respective number of bits?

Jan