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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.02.2022 14:30, 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:

Perhaps, but then I think ...

> --- 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);
> +
>       return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, cmd, false) : 0;

... you also want to update cmd, thus avoiding the call to modify_bars().

And btw, from an abstract pov the same is true for I/O decoding: I
realize that you mean to leave I/O port BARs aside for the moment, but I
think the command register handling could very well take care of both.

Which quickly gets us to the bus master enable bit: I think that one
should initially be off too. Making me wonder: Doesn't the PCI spec
define what the reset state of this register is? If so, that's what I
think we want to put in place for DomU-s.

Jan