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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 03.02.22 16:05, Roger Pau Monné wrote:
> 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.
Then the patch [1] will do what we need to be done for the guest I guess
I am thinking to still have it in the series which will solve exactly the 
problem
we are trying to solve
>
> Thanks, Roger.
[1] 
https://patchwork.kernel.org/project/xen-devel/patch/20211125110251.2877218-11-andr2000@xxxxxxxxx/