[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

  • To: Jan Beulich <jbeulich@xxxxxxxx>, Oleksandr Andrushchenko <andr2000@xxxxxxxxx>
  • From: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>
  • Date: Tue, 7 Sep 2021 08:18:34 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=epam.com; dmarc=pass action=none header.from=epam.com; dkim=pass header.d=epam.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=g9FL1dpPHRZyfhmVFJHfBWM1lqcz7ec+X2EeUF80IHM=; b=GFtRRX5L56B5MHIbu7t5W6adWjo5EgbAd3N9xb5AbiCijZGEsPbLm1bg90xzukZAYXZhkHtNTQvQfpyKfEwUQHAQc1SQpFpVNPsZnfRri4bWLpRmXxxKfPGZjOZUsoJzTa4gGX8WFv8GIMalsdAXYHDHDRAC6s6ds3W7fXhyzssyA4g3nXu/EiWqwzEHuI45rDSEWTNErHvFy2gHf/P00akRtDMD8aDJLpQWiF4dAwVdYiu76bgDQfc+z2D32hRnHCbxB5VeMWYdvwCPP27Yh5GnK1XoCgcYQutkYNCsnVIrLxwgxVRA9Ll01oRojey2DlmUfxSgfmxQXEqAt776gA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=lTSnc8k0z9+pblZq22nD0t1fgXmhduQuhih1IY1gGEZl9W/2EI1a8b5PJVRc5N5tw2dVx6UrWE7xqxcy7pm/glHLya2OP2+3XlSVSIwRZxhLbJrt6SQIO6xjmPOltCfVOFp1hyf7QZ1P4T4M/pts5B5A8QR0oVrh62VlsW31zT2l6nZsU3raScdiRO9Si8qKGyi6mrdhzJoleqWxJwYBA2lWl5Hsdz0QXboq5wPml3FNjwZSdcKwRfRX+h3F5/94tXNsBryoXomC9uUMBFYRnEIdPunBYzAt2YwN2IhdNSEizzSiFEa7iqrCoWAcQo/9qG6SchK0FylxksP+91t9Mw==
  • Authentication-results: suse.com; dkim=none (message not signed) header.d=none;suse.com; dmarc=none action=none header.from=epam.com;
  • Cc: "julien@xxxxxxx" <julien@xxxxxxx>, "sstabellini@xxxxxxxxxx" <sstabellini@xxxxxxxxxx>, Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Artem Mygaiev <Artem_Mygaiev@xxxxxxxx>, "roger.pau@xxxxxxxxxx" <roger.pau@xxxxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Rahul Singh <rahul.singh@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Tue, 07 Sep 2021 08:18:46 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHXoKxjFI8WSqpeJkWkpzZfft/zi6uXHR6AgAEZm4CAAASsgIAABSgA
  • Thread-topic: [PATCH 8/9] vpci/header: Reset the command register when adding devices

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 

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

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

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,




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.