Re: [PATCH v5 4/5] xen/vpci: header: status register handler

[Jan Beulich <jbeulich@xxxxxxxx>]

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