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 <[email protected]>
> ---
> 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
