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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 07.09.21 11:00, Jan Beulich wrote:
> 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.

But then I am wondering to what extent we need to emulate the command

register? We have the following bits in the command register:

#define  PCI_COMMAND_IO        0x1    /* Enable response in I/O space */
#define  PCI_COMMAND_MEMORY    0x2    /* Enable response in Memory space */
#define  PCI_COMMAND_MASTER    0x4    /* Enable bus mastering */
#define  PCI_COMMAND_SPECIAL    0x8    /* Enable response to special cycles */
#define  PCI_COMMAND_INVALIDATE    0x10    /* Use memory write and invalidate */
#define  PCI_COMMAND_VGA_PALETTE 0x20    /* Enable palette snooping */
#define  PCI_COMMAND_PARITY    0x40    /* Enable parity checking */
#define  PCI_COMMAND_WAIT     0x80    /* Enable address/data stepping */
#define  PCI_COMMAND_SERR    0x100    /* Enable SERR */
#define  PCI_COMMAND_FAST_BACK    0x200    /* Enable back-to-back writes */
#define  PCI_COMMAND_INTX_DISABLE 0x400 /* INTx Emulation Disable */

We want the guest to access directly at least I/O and memory decoding and bus 
mastering

bits, but how do we emulate the rest? Do you mean we can match the rest to what 
host

uses for the device, like PCI_COMMAND_INTX_DISABLE bit? If so, as per my 
understanding,

those bits get set/cleared when a device is enabled, e.g. by Linux 
kernel/device driver for example.

So, if we have a hidden PCI device which can be assigned to a guest and it is 
literally untouched

(not enabled in Dom0) then I think there will be no such reference as "host 
assigned values" as

most probably the command register will remain in its after reset state.

Thus, I am not quite sure the command register can easily be emulated.

Please correct me if my understanding is wrong here.

>   Reporting the
> emulated register as zero initially is probably also quite fine (to
> match, as you say, mandated reset state).
>
> Jan
>
Thank you,

Oleksandr