Re: [PATCH v4 1/5] 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 v4 1/5] vpci: fix updating the command register
Date: Mon, 19 Nov 2018 04:31:11 -0700	[thread overview]
Message-ID: <5BF29EFF02000078001FD6AD@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <20181119110949.73qnubvdtle55jzr@mac>

>>> On 19.11.18 at 12:09, <roger.pau@citrix.com> wrote:
> On Mon, Nov 19, 2018 at 01:26:11AM -0700, Jan Beulich wrote:
>> >>> On 16.11.18 at 15:32, <roger.pau@citrix.com> wrote:
>> > On Fri, Nov 16, 2018 at 05:00:29AM -0700, Jan Beulich wrote:
>> >> >>> On 14.11.18 at 12:57, <roger.pau@citrix.com> wrote:
>> >> > @@ -413,7 +412,7 @@ static void rom_write(const struct pci_dev *pdev, unsigned int reg,
>> >> >          header->rom_enabled = new_enabled;
>> >> >          pci_conf_write32(pdev->seg, pdev->bus, slot, func, reg, val);
>> >> >      }
>> >> > -    else if ( modify_bars(pdev, new_enabled, true) )
>> >> > +    else if ( modify_bars(pdev, new_enabled ? PCI_COMMAND_MEMORY : 0, true) )
>> >> 
>> >> Do you really mean to clear all other defined bits of the command
>> >> register here?
>> > 
>> > This is a ROM BAR write, not a command register write. rom_write
>> > passes PCI_COMMAND_MEMORY merely to signal this is a mapping
>> > operation, but the value would never be written to the command
>> > register, there's an ASSERT(!rom_only) just before the deferred write
>> > of the command register in modify_decoding.
>> 
>> Oh, I see. This is getting more subtle than it already was, so perhaps
>> worth attaching a brief comment here, the more that if anything was
>> wrong with the logic bad behavior would result in release builds?
> 
> It would, I could change the ASSERT to a BUG in modify_decoding if
> that seems more foolproof.

I intentionally did not suggest such a conversion: I'd like us to
stop bringing down the entire host when a problem related to
just one guest is encountered. That said - as long as vPCI is
for the hardware domain only, the transformation (annotated
to indicate it needs adjustment when widening exposure to
DomU-s) would perhaps be acceptable.

Jan

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