Re: [PATCH v12 10/15] vpci/header: emulate PCI_COMMAND register for guests

From: Jan Beulich <jbeulich@suse.com>
To: "Stewart Hildebrand" <stewart.hildebrand@amd.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Cc: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Volodymyr Babchuk <volodymyr_babchuk@epam.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v12 10/15] vpci/header: emulate PCI_COMMAND register for guests
Date: Thu, 1 Feb 2024 09:14:52 +0100	[thread overview]
Message-ID: <86b25777-788c-4b9a-8166-a6f8174bedc9@suse.com> (raw)
In-Reply-To: <48a16cb3-9cb9-4b7d-a950-f190a9a64bc2@amd.com>

On 01.02.2024 05:50, Stewart Hildebrand wrote:
> On 1/25/24 10:43, Jan Beulich wrote:
>> On 09.01.2024 22:51, Stewart Hildebrand wrote:
>>> --- a/xen/drivers/vpci/header.c
>>> +++ b/xen/drivers/vpci/header.c
>>> @@ -168,6 +168,9 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
>>>      if ( !rom_only )
>>>      {
>>>          pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
>>> +        /* Show DomU that we updated P2M */
>>> +        header->guest_cmd &= ~PCI_COMMAND_MEMORY;
>>> +        header->guest_cmd |= cmd & PCI_COMMAND_MEMORY;
>>>          header->bars_mapped = map;
>>>      }
>>
>> I don't follow what the comment means to say. The bit in question has no
>> real connection to the P2M, and the guest also may have no notion of the
>> underlying hypervisor's internals. Likely connected to ...
> 
> Indeed. If the comment survives to v13, I'll update it to:
> 
>         /* Now that we updated P2M, show DomU change to PCI_COMMAND_MEMORY */
> 
>>
>>> @@ -524,9 +527,26 @@ static void cf_check cmd_write(
>>>  {
>>>      struct vpci_header *header = data;
>>>  
>>> +    if ( !is_hardware_domain(pdev->domain) )
>>> +    {
>>> +        const struct vpci *vpci = pdev->vpci;
>>> +
>>> +        if ( (vpci->msi && vpci->msi->enabled) ||
>>> +             (vpci->msix && vpci->msix->enabled) )
>>> +            cmd |= PCI_COMMAND_INTX_DISABLE;
>>> +
>>> +        /*
>>> +         * Do not show change to PCI_COMMAND_MEMORY bit until we finish
>>> +         * modifying P2M mappings.
>>> +         */
>>> +        header->guest_cmd = (cmd & ~PCI_COMMAND_MEMORY) |
>>> +                            (header->guest_cmd & PCI_COMMAND_MEMORY);
>>> +    }
>>
>> ... the comment here, but then shouldn't it be that the guest can't even
>> issue a 2nd cfg space access until the present write has been carried out?
>> Otherwise I'd be inclined to claim that such a partial update is unlikely
>> to be spec-conformant.
> 
> Due to the raise_softirq() call added in
> 
>   3e568fa9e19c ("vpci: fix deferral of long operations")
> 
> my current understanding is: when the guest toggles memory decoding, the guest vcpu doesn't resume execution until vpci_process_pending() and modify_decoding() have finished. So I think the guest should see a consistent state of the register, unless it was trying to read from a different vcpu than the one doing the writing.
> 
> Regardless, if the guest did have an opportunity to successfully read the partially updated state of the register, I'm not really spotting what part of the spec that would be a violation of. PCIe 6.1 has this description regarding the bit: "When this bit is Set" and "When this bit is Clear" the device will decode (or not) memory accesses. The spec doesn't seem to distinguish whether the host or the device itself is the one to set/clear the bit. One might even try to argue the opposite: allowing the bit to be toggled before the device reflects the change would be a violation of spec. Since the spec is ambiguous in this regard, I don't think either argument is particularly strong.
> 
> Chesterton's fence: the logic for deferring the update of PCI_COMMAND_MEMORY in guest_cmd was added between v10 and v11 of this series. I went back to look at the review comments on v10 [1], but the rationale is still not entirely clear to me.

Indeed. The only sentence possibly hinting in such a direction would imo
have been "I'm kind of unsure whether we want to fake the guest view by
returning what the guest writes." It's unclear to me whether it really
was meant that way.

> At the end of the day, with the information I have at hand, I suspect it would be fine either way (whether updating guest_cmd is deferred or not). If no other info comes to light, I'm leaning toward not deferring because it would be simpler to update the bit right away in cmd_write().

I'm not sure it would be fine either way. Config space writes aren't
posted writes, so they complete synchronously. IOW whatever internal
state updates are needed in the device, they ought to have finished by
the time the write completes.

> [1] https://lore.kernel.org/xen-devel/ZVy73iJ3E8nJHvgf@macbook.local/
> 
>>[...]
>>> --- a/xen/drivers/vpci/msix.c
>>> +++ b/xen/drivers/vpci/msix.c
>>> @@ -135,6 +135,13 @@ static void cf_check control_write(
>>>          }
>>>      }
>>>  
>>> +    /* Make sure domU doesn't enable INTx while enabling MSI-X. */
>>> +    if ( new_enabled && !msix->enabled && !is_hardware_domain(pdev->domain) )
>>> +    {
>>> +        pci_intx(pdev, false);
>>> +        pdev->vpci->header.guest_cmd |= PCI_COMMAND_INTX_DISABLE;
>>> +    }
>>
>> ... the similar code here has it.
>>
>> In both cases, is it really appropriate to set the bit in guest view?
> 
> I added this based on Roger's comment at [2]. Roger, what do you think? I don't believe QEMU updates the guest view in this manner.
> 
> [2] https://lore.kernel.org/xen-devel/ZLqI65gmNj1XDBm4@MacBook-Air-de-Roger.local/

Leaving this for Roger to answer.

Jan