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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 03.02.22 16:04, Jan Beulich wrote:
> 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.
The spec I have says that all bits are typically 0 after reset.
So, it seems to be reasonable to just write 0 to PCI_COMMAND
> Jan
>
Thank you,
Oleksandr