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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.08.2023 03:29, Stewart Hildebrand wrote:
> Introduce a handler for the PCI status register, with ability to mask the
> capabilities bit. The status register is write-1-to-clear, so introduce 
> handling
> for this type of register in vPCI.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> ---
> v2->v3:
> * new patch

This being a prereq to the cap list filtering, I think the order of the
patches wants to be inverted.

> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -413,6 +413,17 @@ 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;
> +
> +    if ( header->mask_cap_list )
> +        return pci_conf_read16(pdev->sbdf, reg) & ~(PCI_STATUS_CAP_LIST);

No need for parentheses around a constant.

> +    return pci_conf_read16(pdev->sbdf, reg);

I think this function would better issue the read just in a single place,
and then do any fiddling that may be needed.

> @@ -556,6 +567,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);

Is it really correct to treat the entire register as rw1c, and with write-
through to hardware for all bits? There are reserved bit there, and -
however likely that may seem - it's guesswork whether they would also end
up r/o or rw1c once getting assigned some meaning.

> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -29,6 +29,7 @@ struct vpci_register {
>      unsigned int offset;
>      void *private;
>      struct list_head node;
> +    bool rw1c : 1;
>  };

I'm not the maintainer of this code, so what I say here may be void, but
generally we have blanks to the left and/or right of colons in bitfield
declarations only when we mean to pad for alignment with other nearby
bitfields.

> @@ -205,6 +213,22 @@ 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 _vpci_add_register(vpci, read_handler, write_handler, offset, 
> size,
> +                              data, false);
> +}
> +
> +int vpci_add_rw1c_register(struct vpci *vpci, vpci_read_t *read_handler,
> +                           vpci_write_t *write_handler, unsigned int offset,
> +                           unsigned int size, void *data)
> +{
> +    return _vpci_add_register(vpci, read_handler, write_handler, offset, 
> size,
> +                              data, true);
> +}

I'm always a little irritated by local functions still retaining the
subsystem prefix. Just add_register() for the now static helper would
imo be enough here and overall shorter to read/type.

> @@ -433,9 +452,11 @@ static void vpci_write_helper(const struct pci_dev *pdev,
>  
>      if ( size != r->size )
>      {
> -        uint32_t val;
> +        uint32_t val = 0;
> +
> +        if ( !r->rw1c )
> +            val = r->read(pdev, r->offset, r->private);

Along with the earlier question: Doesn't rw1c need to be a bit mask,
not a single boolean covering the entire register?

> @@ -99,6 +106,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;

I think the intention here is for the colons to align.

Jan