Re: [PATCH v9 11/16] vpci/header: reset the command register when adding devices

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

On Tue, Aug 29, 2023 at 11:19:45PM +0000, Volodymyr Babchuk wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> 
> Reset the command register when assigning a PCI device to a guest:
> according to the PCI spec the PCI_COMMAND register is typically all 0's
> after reset, but this might not be true for the guest as it needs
> to respect host's settings.
> For that reason, do not write 0 to the PCI_COMMAND register directly,
> but go through the corresponding emulation layer (cmd_write), which
> will take care about the actual bits written. Also, honor value of
> PCI_COMMAND_VGA_PALETTE value, which is set by firmware.

I think this is likely dangerous, it would be better IMO to simply
make sure the value presented to the guest is all zeros, and that the
vPCI cached state is consistent with that.

> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> ---
> Since v9:
> - Honor PCI_COMMAND_VGA_PALETTE bit
> Since v6:
> - use cmd_write directly without introducing emulate_cmd_reg
> - update commit message with more description on all 0's in PCI_COMMAND
> Since v5:
> - updated commit message
> Since v1:
>  - do not write 0 to the command register, but respect host settings.
> ---
>  xen/drivers/vpci/header.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index e351db4620..1d243eeaf9 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -762,6 +762,12 @@ static int cf_check init_bars(struct pci_dev *pdev)
>          return -EOPNOTSUPP;
>      }
>  
> +    /* Reset the command register for guests. We want to preserve only
> +     * PCI_COMMAND_VGA_PALETTE as it is configured by firmware */

Wrong comment style, and PCI_COMMAND_VGA_PALETTE is likely to be gone
anyway after we perform a FLR of the device anyway?

> +    cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
> +    if ( !is_hwdom )
> +        cmd_write(pdev, PCI_COMMAND, cmd & PCI_COMMAND_VGA_PALETTE, header);

Such cmd_write() call might trigger an attempt to change the guest
physmap if you are toggling the Memory Enabled bit from 1 -> 0, and
that would fail because the guest doesn't have BAR p2m mappings setup
yet, those are done at the end of the function by the call to
modify_bars().

Thanks, Roger.