Re: [PATCH v5 06/14] vpci/header: implement guest BAR register handlers

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

On Thu, Feb 03, 2022 at 01:30:26PM +0000, Oleksandr Andrushchenko wrote:
> 
> 
> On 03.02.22 14:54, Jan Beulich wrote:
> > On 03.02.2022 13:45, Oleksandr Andrushchenko wrote:
> >>>> Also memory decoding needs to be initially disabled when used by
> >>>> guests, in order to prevent the BAR being placed on top of a RAM
> >>>> region. The guest physmap will be different from the host one, so it's
> >>>> possible for BARs to end up placed on top of RAM regions initially
> >>>> until the firmware or OS places them at a suitable address.
> >>> Agree, memory decoding must be disabled
> >> Isn't it already achieved by the toolstack resetting the PCI device
> >> while assigning  it to a guest?
> > Iirc the tool stack would reset a device only after getting it back from
> > a DomU. When coming straight from Dom0 or DomIO, no reset would be
> > performed. Furthermore, (again iirc) there are cases where there's no
> > known (standard) way to reset a device. Assigning such to a guest when
> > it previously was owned by another one is risky (and hence needs an
> > admin knowing what they're doing), but may be acceptable in particular
> > when e.g. simply rebooting a guest.
> >
> > IOW - I don't think you can rely on the bit being in a particular state.
> So, you mean something like:
> 
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 7695158e6445..9ebd57472da8 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -808,6 +808,14 @@ static int init_bars(struct pci_dev *pdev)
>               return rc;
>       }
> 
> +    /*
> +     * Memory decoding needs to be initially disabled when used by
> +     * guests, in order to prevent the BAR being placed on top of a RAM
> +     * region.
> +     */
> +    if ( !is_hwdom )
> +        pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd & ~PCI_COMMAND_MEMORY);

Memory decoding is already disabled here, so you just need to avoid
enabling it, for example:

    /*
     * Memory decoding needs to be initially disabled when used by
     * guests, in order to prevent the BARs being mapped at gfn 0 by
     * default.
     */
    if ( !is_hwdom )
        cmd &= ~PCI_COMMAND_MEMORY;

>       return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, cmd, false) : 0;

This is important here because guest_reg won't be set (ie: will be set
to 0) so if for some reason memory decoding was enabled you would end
up with BARs mappings overlapping at gfn 0.

Thanks, Roger.