[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] RE: [PATCH] xen/arm: fix SBDF calculation for vPCI MMIO handlers
Hi Oleksandr, > -----Original Message----- > From: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx> > Sent: 2021年11月2日 15:47 > To: Wei Chen <Wei.Chen@xxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx> > Cc: Julien Grall <julien@xxxxxxx>; Bertrand Marquis > <Bertrand.Marquis@xxxxxxx>; sstabellini@xxxxxxxxxx; Rahul Singh > <Rahul.Singh@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH] xen/arm: fix SBDF calculation for vPCI MMIO handlers > > Hi, > > On 02.11.21 09:37, Wei Chen wrote: > > 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.)? > Right > > Does that make the context inconsistent in physical device's > configuration space? > It is always consistent for the hardware domain and some parts of it are > emulated > for guests > > For example, one register in physical device > > config space is related to another register. But we just emulate > > only one in vPCI? > So, we trap for all domains and emulate for guests, e.g. hardware domain's > view on the > registers is consistent. For guests we emulate: > - PCI_COMMAND - not to allow INTx as we do not support that on Arm > - BARs - we emulate guest's view on these according to the memory spaces > of the emulated host bridge, so the real BARs still have physical values, > but > guests see emulated ones > > Hope this helps Thanks, it's very helpful! > > > > > >>> > >>> Regards, Roger. > >>> > >> Thanks, > >> Oleksandr > >>
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |