Re: [PATCH v3 4/7] vpci: fix updating the command register

From: "Jan Beulich" <JBeulich@suse.com>
To: Roger Pau Monne <roger.pau@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	Julien Grall <julien.grall@arm.com>,
	xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v3 4/7] vpci: fix updating the command register
Date: Wed, 07 Nov 2018 08:00:17 -0700	[thread overview]
Message-ID: <5BE2FE0102000078001F92FA@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <20181107104759.igndnmfbxv7bdoah@mac.citrite.net>

>>> On 07.11.18 at 11:47, <roger.pau@citrix.com> wrote:
> On Mon, Nov 05, 2018 at 09:46:22AM -0700, Jan Beulich wrote:
>> >>> On 30.10.18 at 16:41, <roger.pau@citrix.com> wrote:
>> > When switching the memory decoding bit in the command register the
>> > rest of the changes where dropped, leading to only the memory decoding
>> > bit being updated.
>> > 
>> > Fix this by unconditionally writing the guest-requested command except
>> > for the memory decoding bit, which will be updated once the p2m
>> > changes are performed.
>> 
>> Are you convinced there are no devices (or drivers) with errata
>> requiring the write to happen in a single step?
> 
> That I certainly don't know. On Xen we already toggle the memory
> decoding bit if required when sizing the BARs during initialization,
> which will likely also trigger such a bug if it exist.

Toggling a single bit is still slightly different from taking off a
certain bit from a value to be written. Only the second write
(toggling just that one bit) matches behavior we already have.

>> Remember that
>> the register value immediately becomes visible to other (v)CPUs.
> 
> But the vCPU that performed the write would be blocked and only
> released once all the bits have been updated. While other vCPUs could
> indeed see a partly updated command value without the memory decode
> bit set I'm not sure this is harmful, well behaved OSes should wait
> for the write to complete in any case.

They should, yes. Anyway, this isn't an argument against the patch
in its current shape, just something which came to mind while reviewing.

>> > --- a/xen/drivers/vpci/header.c
>> > +++ b/xen/drivers/vpci/header.c
>> > @@ -333,8 +333,10 @@ static void cmd_write(const struct pci_dev *pdev, unsigned int reg,
>> >           * hoping the guest will realize and try again.
>> >           */
>> >          modify_bars(pdev, cmd & PCI_COMMAND_MEMORY, false);
>> > -    else
>> > -        pci_conf_write16(pdev->seg, pdev->bus, slot, func, reg, cmd);
>> > +
>> > +    /* Write the new command without updating the memory decoding bit. */
>> > +    cmd = (cmd & ~PCI_COMMAND_MEMORY) | (current_cmd & PCI_COMMAND_MEMORY);
>> > +    pci_conf_write16(pdev->seg, pdev->bus, slot, func, reg, cmd);
>> 
>> Furthermore, if the mapping didn't get deferred, aren't you
>> discarding the new decode bit value here, as written by
>> modify_bars() -> apply_map() -> modify_decoding()?
> 
> The map should always be deferred when called from cmd_write because
> this handler will be accessed exclusively by the guest, in which case
> system_state >= SYS_STATE_active.

Would you mind clarifying this in a comment, or by way of an added
ASSERT()?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel