Re: [PATCH] xen/vpci: allow BAR write if value is the same

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Stewart Hildebrand <stewart.hildebrand@amd.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH] xen/vpci: allow BAR write if value is the same
Date: Tue, 24 Oct 2023 09:39:19 +0200	[thread overview]
Message-ID: <ZTd0pxpwtEbsCny0@macbook> (raw)
In-Reply-To: <2ef80dad-5ae6-786d-e89f-d0c3af44b485@suse.com>

On Tue, Oct 24, 2023 at 09:09:45AM +0200, Jan Beulich wrote:
> On 23.10.2023 18:36, Stewart Hildebrand wrote:
> > During xl pci-assignable-add, pciback may reset the device while memory decoding
> > (PCI_COMMAND_MEMORY) is enabled. After device reset, memory decoding may be
> > disabled in hardware, and the BARs may be zeroed/reset in hardware. However, Xen
> > vPCI still thinks memory decoding is enabled, and BARs will remain mapped in
> > p2m. In other words, memory decoding may become disabled and BARs reset in
> > hardware, bypassing the respective vPCI command and BAR register handlers.
> > Subsequently, when pciback attempts to restore state to the device, including
> > BARs, it happens to write the BARs before writing the command register.
> > Restoring/writing the BARs silently fails because Xen vPCI mistakenly thinks
> > memory decoding is enabled.
> > 
> > Fix the BAR write by allowing it to succeed if the value written is the same as
> > the Xen vPCI stored value. pciback will subsequently restore the command
> > register and the state of the BARs and memory decoding bit will then be in sync
> > between hardware and vPCI again.
> > 
> > While here, remove a nearby stray newline.
> > 
> > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> > ---
> > Do we need similar handling in rom_write()?
> 
> I think so, if we are to go this route. However, ...
> 
> > We may consider additionally checking during bar_write() if the memory decoding
> > state has become out of sync between hardware and vPCI. During bar_write(), we
> > would check the device's memory decoding bit, compare it to our vPCI state, and
> > invoke modify_bars() if needed. Please let me know your thoughts.
> 
> ... iirc we discussed earlier on that doing resets in Dom0 wants
> communicating to Xen. Any way of resetting by a DomU would likely need
> intercepting. This way the described situation can be avoided altogether.
> We may go further and uniformly intercept resets, carrying them out on
> behalf of Dom0 as well. The main issue is, as with any config-space-
> based interaction with a device, that there may be device specific ways
> of resetting.

I agree with Jan, the plan as I recall was to introduce a new
hypercall to signal Xen of when a device has been reset, I can't
however find that conversation right now.  It would be nice to trap
device reset attempts, but there are some device specific reset
methods that would be too much special casing to accommodate sadly.

The fix here is just papering over the issue, as if the device has
been reset and Xen is not aware of it all the vPCI cached state is out
of date, so it's not only BARs addresses that are stale, but also
possibly MSI and MSI-X.

Thanks, Roger.