Re: [PATCH 8/9] vpci/header: Reset the command register when adding devices

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 06.09.21 17:55, Jan Beulich wrote:
> On 03.09.2021 12:08, Oleksandr Andrushchenko wrote:
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -811,6 +811,16 @@ int vpci_bar_add_handlers(const struct domain *d, 
>> struct pci_dev *pdev)
>>           gprintk(XENLOG_ERR,
>>               "%pp: failed to add BAR handlers for dom%d\n", &pdev->sbdf,
>>               d->domain_id);
>> +
>> +    /*
>> +     * Reset the command register: it is possible that when passing
>> +     * through a PCI device its memory decoding bits in the command
>> +     * register are already set. Thus, a guest OS may not write to the
>> +     * command register to update memory decoding, so guest mappings
>> +     * (guest's view of the BARs) are left not updated.
>> +     */
>> +    pci_conf_write16(pdev->sbdf, PCI_COMMAND, 0);
> Can you really blindly write 0 here? What about bits that have to be
> under host control, e.g. INTX_DISABLE? I can see that you may want to
> hand off with I/O and memory decoding off and bus mastering disabled,
> but for every other bit (including reserved ones) I'd expect separate
> justification (in the commit message).
According to "PCI LOCAL BUS SPECIFICATION, REV. 3.0" I have at hand,
section "6.2.2 Device Control" says that the reset state of the command
register is typically 0, so this is why I chose to write 0 here, e.g.
make the command register as if it is after the reset.

With respect to host control: we currently do not really emulate command
register which probably was ok for x86 PVH Dom0 and this might not be the
case now as we add DomU's. That being said: in my implementation guest can
alter command register as it wants without restrictions.
If we see it does need proper emulation then we would need adding that as
well (is not part of this series though).

Meanwhile, I agree that we can only reset IO space, memory space and bus
master bits and leave the rest untouched. But again, without proper command
register emulation guests can still set what they want.


Please let me know your opinion on how we can proceed.

>
> Jan
>
Thank you,

Oleksandr