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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.09.2023 04:16, Stewart Hildebrand wrote:
> @@ -544,6 +545,18 @@ static int cf_check init_bars(struct pci_dev *pdev)
>      if ( rc )
>          return rc;
>  
> +    /*
> +     * If mask_cap_list is true, PCI_STATUS_CAP_LIST will be set in both
> +     * rsvdz_mask and ro_mask, and thus will effectively behave as rsvdp
> +     * (reserved/RAZ and preserved on write).
> +     */
> +    rc = vpci_add_register_mask(pdev->vpci, vpci_hw_read16, vpci_hw_write16,
> +                                PCI_STATUS, 2, header, PCI_STATUS_RSVDZ_MASK 
> |
> +                                (mask_cap_list ? PCI_STATUS_CAP_LIST : 0),
> +                                PCI_STATUS_RO_MASK, PCI_STATUS_RW1C_MASK);

While not formally disallowed by ./CODING_STYLE, I think this kind of argument
wrapping is bad practice. If an argument is too long for the current line, it
should start on a fresh one. Otherwise at the very least I think we'd want the
continuation to stand out, by parenthesizing the entire argument, thus leading
to one deeper indentation on the continuing line(s). (This may even be useful
to do when starting on a fresh line.)

> @@ -155,7 +165,8 @@ 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) ||
> +         (rsvdz_mask & rw1c_mask) )
>          return -EINVAL;

What about rsvdz_mask and ro_mask? They better wouldn't overlap either
(requiring an adjustment to the code you add to init_bars()).

> @@ -407,26 +439,25 @@ 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;
> +
>      ASSERT(size <= r->size);
>  
> -    if ( size != r->size )
> +    if ( (size != r->size) || r->ro_mask )
>      {
> -        uint32_t val;
> -
>          val = r->read(pdev, r->offset, r->private);
> +        val &= ~r->rw1c_mask;
>          data = merge_result(val, data, size, offset);
>      }
>  
> +    data &= ~r->rsvdz_mask & ~r->ro_mask;

~(r->rsvdz_mask | r->ro_mask) ? But maybe that's indeed just me ...

Jan