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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.09.2021 09:43, Oleksandr Andrushchenko wrote:
> 
> 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.

Yes, writes to the register will need emulating for DomU. Reporting the
emulated register as zero initially is probably also quite fine (to
match, as you say, mandated reset state).

Jan