Re: [PATCH v1 2/3] vpci/rebar: Remove registers when init_rebar() fails

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Mar 27, 2025 at 03:32:13PM +0800, Jiqian Chen wrote:
> When init_rebar() fails, the new codes will try to hide Rebar
> capability, so it can't rely on vpci_deassign_device() to remove
> all Rebar related registers anymore, those registers must be
> cleaned up in failure path of init_rebar.
> 
> To do that, use a vpci_register array to record all Rebar registers
> and call vpci_remove_register() to remove registers.
> 
> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> ---
>  xen/drivers/vpci/rebar.c | 33 ++++++++++++++++++++-------------
>  xen/drivers/vpci/vpci.c  | 14 --------------
>  xen/include/xen/vpci.h   | 15 +++++++++++++++
>  3 files changed, 35 insertions(+), 27 deletions(-)
> 
> diff --git a/xen/drivers/vpci/rebar.c b/xen/drivers/vpci/rebar.c
> index 7c53ee031887..5f2f9978feb9 100644
> --- a/xen/drivers/vpci/rebar.c
> +++ b/xen/drivers/vpci/rebar.c
> @@ -51,8 +51,11 @@ static void cf_check rebar_ctrl_write(const struct pci_dev 
> *pdev,
>  
>  static int cf_check init_rebar(struct pci_dev *pdev)
>  {
> +    int rc = 0;
>      uint32_t ctrl;
>      unsigned int nbars;
> +    unsigned int reg_index = 0;
> +    struct vpci_register registers[VPCI_CAP_MAX_REGISTER];

I'm not sure I like this approach much, as it seems to be quite
cumbersome.  Iff we really want to go that route I would recommend
that you use a much lighter structure here, struct vpci_register has a
bunch of fields that are not used at all by the purposes here.  You
just want a struct with and offset and a size fields.

>      unsigned int rebar_offset = pci_find_ext_capability(pdev->sbdf,
>                                                          
> PCI_EXT_CAP_ID_REBAR);
>  
> @@ -70,17 +73,17 @@ static int cf_check init_rebar(struct pci_dev *pdev)
>      nbars = MASK_EXTR(ctrl, PCI_REBAR_CTRL_NBAR_MASK);
>      for ( unsigned int i = 0; i < nbars; i++ )
>      {
> -        int rc;
> +        const unsigned int offset = rebar_offset + PCI_REBAR_CTRL(i);
>          struct vpci_bar *bar;
>          unsigned int index;
>  
> -        ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL(i));
> +        ctrl = pci_conf_read32(pdev->sbdf, offset);
>          index = ctrl & PCI_REBAR_CTRL_BAR_IDX;
>          if ( index >= PCI_HEADER_NORMAL_NR_BARS )
>          {
>              printk(XENLOG_ERR "%pd %pp: too big BAR number %u in 
> REBAR_CTRL\n",
>                     pdev->domain, &pdev->sbdf, index);
> -            continue;
> +            goto fail;
>          }
>  
>          bar = &pdev->vpci->header.bars[index];
> @@ -88,24 +91,19 @@ static int cf_check init_rebar(struct pci_dev *pdev)
>          {
>              printk(XENLOG_ERR "%pd %pp: BAR%u is not in memory space\n",
>                     pdev->domain, &pdev->sbdf, index);
> -            continue;
> +            goto fail;
>          }
>  
>          rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rebar_ctrl_write,
> -                               rebar_offset + PCI_REBAR_CTRL(i), 4, bar);
> +                               offset, 4, bar);
>          if ( rc )
>          {
>              printk(XENLOG_ERR "%pd %pp: BAR%u fail to add reg of REBAR_CTRL 
> rc=%d\n",
>                     pdev->domain, &pdev->sbdf, index, rc);
> -            /*
> -             * Ideally we would hide the ReBar capability on error, but code
> -             * for doing so still needs to be written. Use continue instead
> -             * to keep any already setup register hooks, as returning an
> -             * error will cause the hardware domain to get unmediated access
> -             * to all device registers.
> -             */
> -            continue;
> +            goto fail;
>          }
> +        registers[reg_index].offset = offset;
> +        registers[reg_index++].size = 4;
>  
>          bar->resizable_sizes =
>              MASK_EXTR(pci_conf_read32(pdev->sbdf,
> @@ -117,6 +115,15 @@ static int cf_check init_rebar(struct pci_dev *pdev)
>      }
>  
>      return 0;
> +
> + fail:
> +    for ( unsigned int i = 0; i < reg_index; i++ )
> +        if ( vpci_remove_register(pdev->vpci,
> +                                  registers[i].offset,
> +                                  registers[i].size) )
> +            continue;

Keep in mind it's fine to remove registers that are not there, iow you
could possibly do:

for ( unsigned int i = 0; i < nbars; i++ )
    if ( vpci_remove_register(pdev->vpci, rebar_offset + PCI_REBAR_CTRL(i),
                              4) )
        continue;

And it would be fine IMO, without the need to store exactly which
registers have been added.  It's not like there's much that can be
done from vpci_remove_register() failing in this context.

In fact you can remove the __must_check from vpci_remove_register(), I
don't think it's helpful at all.

> +
> +    return rc;
>  }
>  REGISTER_VPCI_EXTEND_CAP(PCI_EXT_CAP_ID_REBAR, init_rebar, 
> VPCI_PRIORITY_LOW);
>  
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index a8362e46e097..ea81d8cc9604 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -21,20 +21,6 @@
>  #include <xen/vpci.h>
>  #include <xen/vmap.h>
>  
> -/* Internal struct to store the emulated PCI registers. */
> -struct vpci_register {
> -    vpci_read_t *read;
> -    vpci_write_t *write;
> -    unsigned int size;
> -    unsigned int offset;
> -    void *private;
> -    struct list_head node;
> -    uint32_t ro_mask;
> -    uint32_t rw1c_mask;
> -    uint32_t rsvdp_mask;
> -    uint32_t rsvdz_mask;
> -};
> -
>  #ifdef __XEN__
>  extern vpci_capability_t *const __start_vpci_array[];
>  extern vpci_capability_t *const __end_vpci_array[];
> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> index fa13397ae409..19a036c22165 100644
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -214,6 +214,21 @@ struct vpci_vcpu {
>      bool rom_only : 1;
>  };
>  
> +#define VPCI_CAP_MAX_REGISTER 10

That 10 is kind of arbitrary...

Thanks, Roger.