Re: [PATCH v3 09/11] vpci/header: Reset the command register when adding devices

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

On Thu, Sep 30, 2021 at 10:52:21AM +0300, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> 
> Reset the command register when passing through a PCI device:
> it is possible that when passing through a PCI device its memory
> decoding bits in the command register are already set. Thus, a
> guest OS may not write to the command register to update memory
> decoding, so guest mappings (guest's view of the BARs) are
> left not updated.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
> ---
> Since v1:
>  - do not write 0 to the command register, but respect host settings.
> ---
>  xen/drivers/vpci/header.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 754aeb5a584f..70d911b147e1 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -451,8 +451,7 @@ static void cmd_write(const struct pci_dev *pdev, 
> unsigned int reg,
>          pci_conf_write16(pdev->sbdf, reg, cmd);
>  }
>  
> -static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg,
> -                            uint32_t cmd, void *data)
> +static uint32_t emulate_cmd_reg(const struct pci_dev *pdev, uint32_t cmd)
>  {
>      /* TODO: Add proper emulation for all bits of the command register. */
>  
> @@ -467,14 +466,20 @@ static void guest_cmd_write(const struct pci_dev *pdev, 
> unsigned int reg,
>              cmd |= PCI_COMMAND_INTX_DISABLE;
>          else
>          {
> -            uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg);
> +            uint16_t current_cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);

Either we keep reg here or we drop the parameter altogether from the
function prototype. Having one caller pass 0 while the other passing
PCI_COMMAND is confusing. The more that the parameter is now
effectively unused.

>  
>              if ( current_cmd & PCI_COMMAND_INTX_DISABLE )
>                  cmd |= PCI_COMMAND_INTX_DISABLE;
>          }
>      }
>  
> -    cmd_write(pdev, reg, cmd, data);
> +    return cmd;
> +}
> +
> +static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg,
> +                            uint32_t cmd, void *data)
> +{
> +    cmd_write(pdev, reg, emulate_cmd_reg(pdev, cmd), data);
>  }
>  
>  static void bar_write(const struct pci_dev *pdev, unsigned int reg,
> @@ -793,6 +798,10 @@ int vpci_bar_add_handlers(const struct domain *d, const 
> struct pci_dev *pdev)
>          gdprintk(XENLOG_ERR,
>                   "%pp: failed to add BAR handlers for dom%pd: %d\n",
>                   &pdev->sbdf, d, rc);
> +
> +    /* Reset the command register with respect to host settings. */
> +    pci_conf_write16(pdev->sbdf, PCI_COMMAND, emulate_cmd_reg(pdev, 0));

I think we likely want to unset the memory and IO decoding bits from
the command register, as the guest view of the BAR address is
currently forced to 0, and not mapped into the guest p2m.

Thanks, Roger.