Re: [PATCH] xen/arm: fix SBDF calculation for vPCI MMIO handlers

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Oleksandr,

On 2021/11/1 14:14, Oleksandr Andrushchenko wrote:
> On 29.10.21 10:33, Roger Pau Monné wrote:
> > On Thu, Oct 28, 2021 at 05:55:25PM +0000, Oleksandr Andrushchenko wrote:
> > > On 28.10.21 19:03, Roger Pau Monné wrote:
> > > > On Thu, Oct 28, 2021 at 02:23:34PM +0000, Oleksandr Andrushchenko wrote:
> > > > > On 28.10.21 16:36, Roger Pau Monné wrote:
> > > > > > On Thu, Oct 28, 2021 at 12:09:23PM +0000, Oleksandr Andrushchenko wrote:
> > > > > > > Hi, Julien!
> > > > > > >
> > > > > > > On 27.10.21 20:35, Julien Grall wrote:
> > > > > > > > Hi Oleksandr,
> > > > > > > >
> > > > > > > > On 27/10/2021 09:25, Oleksandr Andrushchenko wrote:
> > > > > > > > > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> > > > > > > > >
> > > > > > > > > While in vPCI MMIO trap handlers for the guest PCI host bridge it is not
> > > > > > > > > enough for SBDF translation to simply call VPCI_ECAM_BDF(info->gpa) as
> > > > > > > > > the base address may not be aligned in the way that the translation
> > > > > > > > > always work. If not adjusted with respect to the base address it may not be
> > > > > > > > > able to properly convert SBDF and crashes:
> > > > > > > > >
> > > > > > > > > (XEN) vpci_mmio_read 0000:65:1a.0 reg 8bc gpa e65d08bc
> > > > > > > > I can't find a printk() that may output this message. Where does this comes 
> > > > > > > > from?
> > > > > > > That was a debug print. I shouldn't have used that in the patch description, but
> > > > > > > probably after "---" to better explain what's happening
> > > > > > > > Anyway, IIUC the guest physical address is 0xe65d08bc which, if I am not 
> > > > > > > > mistaken, doesn't belong to the range advertised for GUEST_VPCI_ECAM.
> > > > > > > This is from dom0 I am working on now.
> > > > > > > > IMHO, the stack trace should come from usptream Xen or need some information to 
> > > > > > > > explain how this was reproduced.
> > > > > > > >
> > > > > > > > > (XEN) Data Abort Trap. Syndrome=0x6
> > > > > > > > > (XEN) Walking Hypervisor VA 0x467a28bc on CPU0 via TTBR 0x00000000481d5000
> > > > > > > > I can understnad that if we don't substract GUEST_VPCI_ECAM, we would (in 
> > > > > > > > theory) not get the correct BDF. But... I don't understand how this would 
> > > > > > > > result to a data abort in the hypervisor.
> > > > > > > >
> > > > > > > > In fact, I think the vPCI code should be resilient enough to not crash if we 
> > > > > > > > pass the wrong BDF.
> > > > > > > Well, there is no (?) easy way to validate SBDF. And this could be a problem if 
> > > > > > > we have a misbehaving
> > > > > > > guest which may force Xen to access the memory beyond that of PCI host bridge
> > > > > > How could that be? The ECAM region exposed to the guest you should be
> > > > > > the same as the physical one for dom0?
> > > > > Ok, I have a Designware PCI hist which has 2 ECAM regions (I am starting to
> > > > > implement the driver for it, so I can be wrong here):
> > > > > - Root Complex ECAM area ("dbi"), it is something like 0x3000 bytes long
> > > > > - "Client" ECAM area ("config")
> > > > > So from Dom0 POV we have 2 ECAM regions and for the guest
> > > > > we always emulate a single big region:
> > > > You need support for multiple ECAM regions. That's how we do it on x86
> > > > PVH dom0. See register_vpci_mmcfg_handler and related machinery.
> > > Is it common for a PCI host bridge to have multiple ECAM regions?
> > > Currently on Arm we were about to support "pci-host-ecam-generic" [1],
> > > e.g. generic ECAM host bridge which normally (?) has a single ECAM
> > > region [2]. But the host bridge I want to support has multiple, so
> > > strictly speaking it is not the one that we implement.
> > It's possible on x86 to have multiple ECAM regions, whether that means
> > multiple host bridges, or host bridges having multiple ECAM regions is
> > unknown to me. It's all reported in the MCFG ACPI table (see PCI
> > Firmware document for the detailed description of MCFG) using the
> > "Configuration Space Base Address Allocation Structure", and there can
> > be multiple of those structures.
> As we are currently supporting generic ECAM host bridge which
> has a single ECAM region I think the existing code we have and
> about to upstream is ok as is for now.
> I own a bridge which has 2 ECAM regions, so I will work towards
> adding its support soon.
> > > Arm folks, do we want this generalization at this moment to align with x86
> > > with this respect?
> > >
> > > We can live with the current approach and when I have my driver implemented
> > > I can send patches to make that generalization.
> > > > > /*
> > > > >      * 256 MB is reserved for VPCI configuration space based on calculation
> > > > >      * 256 buses x 32 devices x 8 functions x 4 KB = 256 MB
> > > > >      */
> > > > > #define GUEST_VPCI_ECAM_BASE    xen_mk_ullong(0x10000000)
> > > > > #define GUEST_VPCI_ECAM_SIZE    xen_mk_ullong(0x10000000)
> > > > >
> > > > > So, we have the base address and size of the emulated ECAM space
> > > > > not connected to the real host bridge
> > > > > > And for domUs you really need to fix vpci_{read,write} to not
> > > > > > passthrough accesses not explicitly handled.
> > > > > Do you mean that we need to validate SBDFs there?
> > > > > This can be tricky if we have a use-case when a PCI device being
> > > > > passed through if not put at 0000:00:0.0, but requested to be, for
> > > > > example, 0000:0d:0.0. So, we need to go over the list of virtual
> > > > > devices and see if SBDF the guest is trying to access is a valid SBDF.
> > > > > Is this what you mean?
> > > > No, you need to prevent accesses to registers not explicitly handled
> > > > by vpci. Ie: do not forward unhandled accesses to
> > > > vpci_{read,wrie}_hw).
> > > I see, so those which have no handlers are not passed to the hardware.
> > > I need to see how to do that
> > Indeed. Without fixing that passthrough to domUs is completely unsafe,
> > as you allow domUs full access to registers not explicitly handled by
> > current vPCI code.
> Well, my understanding is: we can let the guest access whatever
> registers it wants with the following exceptions:
> - "special" registers we already trap in vPCI, e.g. command, BARs
> - we must not let the guest go out of the configuration space of a
> specific PCI device, e.g. prevent it from accessing configuration
> spaces of other devices.
> The rest accesses seem to be ok to me as we do not really want:
> - have handlers and emulate all possible registers
> - we do not want the guest to fail if it accesses a valid register which
> we do not emulate.
I am tring to review your patch, please point out if there is anything
wrong. IIUC, vPCI only emulates some registers, and forward unhandled
accesses to physical device configuration space (if the accesses passed 
the validate.)?
Does that make the context inconsistent in physical device's 
configuration space? For example, one register in physical device
config space is related to another register. But we just emulate
only one in vPCI?


> > Regards, Roger.
> Thanks,
> Oleksandr