On Wed, Mar 13, 2019 at 11:09 AM Paul Durrant <paul.durr...@citrix.com> wrote:
>
> > -----Original Message-----
> > From: Jason Andryuk [mailto:jandr...@gmail.com]
> > Sent: 11 March 2019 18:02
> > To: qemu-de...@nongnu.org
> > Cc: xen-devel@lists.xenproject.org; marma...@invisiblethingslab.com; Simon
> > Gaiser
> > <si...@invisiblethingslab.com>; Jason Andryuk <jandr...@gmail.com>; Stefano
> > Stabellini
> > <sstabell...@kernel.org>; Anthony Perard <anthony.per...@citrix.com>; Paul
> > Durrant
> > <paul.durr...@citrix.com>
> > Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
> >
> > From: Simon Gaiser <si...@invisiblethingslab.com>
> >
> > If a pci memory region has a size < XEN_PAGE_SIZE it can get located at
> > an address which is not page aligned.
>
> IIRC the PCI spec says that the minimum memory region size should be at least
> 4k. Should we even be tolerating BARs smaller than that?
>
> Paul
>
Hi, Paul.
Simon found this, so it affects a real device. Simon, do you recall
which device was affected?
I think BARs only need to be power-of-two size and aligned, and 4k is
not a minimum. 16bytes may be a minimum, but I don't know what the
spec says.
On an Ivy Bridge system, here are some of the devices with BARs smaller than 4K:
00:16.0 Communication controller: Intel Corporation 7 Series/C210
Series Chipset Family MEI Controller #1 (rev 04)
Memory at d0735000 (64-bit, non-prefetchable) [disabled] [size=16]
00:1d.0 USB controller: Intel Corporation 7 Series/C210 Series Chipset
Family USB Enhanced Host Controller #1 (rev 04) (prog-if 20 [EHCI])
Memory at d0739000 (32-bit, non-prefetchable) [disabled] [size=1K]
00:1f.3 SMBus: Intel Corporation 7 Series/C210 Series Chipset Family
SMBus Controller (rev 04)
Memory at d0734000 (64-bit, non-prefetchable) [disabled] [size=256]
02:00.0 System peripheral: JMicron Technology Corp. SD/MMC Host
Controller (rev 30)
Memory at d0503000 (32-bit, non-prefetchable) [disabled] [size=256]
These examples are all 4K aligned, so this is not an issue on this machine.
Reviewing the code, I'm now wondering if the following in
hw/xen/xen_pt.c:xen_pt_region_update is wrong: rc =
xc_domain_memory_mapping(xen_xc, xen_domid,
XEN_PFN(guest_addr + XC_PAGE_SIZE - 1),
XEN_PFN(machine_addr + XC_PAGE_SIZE - 1),
XEN_PFN(size + XC_PAGE_SIZE - 1),
op);
If a bar of size 0x100 is at 0xd0500800, then the machine_addr passed
in would be 0xd0501000 which is past the actual location. Should the
call arguments just be XEN_PFN(guest_addr) & XEN_PFN(machine_addr)?
BARs smaller than a page would also be a problem if BARs for different
devices shared the same page.
Regards,
Jason
> > This breaks the memory mapping via
> > xc_domain_memory_mapping since this function is page based and the
> > "offset" is therefore lost.
> >
> > Without this patch you will see error like this in the stubdom log:
> >
> > [00:05.0] xen_pt_bar_read: Error: Should not read BAR through QEMU.
> > @0x0000000000000004
> >
> > QubesOS/qubes-issues#2849
> >
> > Signed-off-by: Simon Gaiser <si...@invisiblethingslab.com>
> > Signed-off-by: Jason Andryuk <jandr...@gmail.com>
> > ---
> > hw/xen/xen_pt.c | 10 +++++++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> > index 5539d56c3a..7f680442ee 100644
> > --- a/hw/xen/xen_pt.c
> > +++ b/hw/xen/xen_pt.c
> > @@ -449,9 +449,10 @@ static int
> > xen_pt_register_regions(XenPCIPassthroughState *s, uint16_t *cmd)
> > /* Register PIO/MMIO BARs */
> > for (i = 0; i < PCI_ROM_SLOT; i++) {
> > XenHostPCIIORegion *r = &d->io_regions[i];
> > + pcibus_t r_size = r->size;
> > uint8_t type;
> >
> > - if (r->base_addr == 0 || r->size == 0) {
> > + if (r->base_addr == 0 || r_size == 0) {
> > continue;
> > }
> >
> > @@ -469,15 +470,18 @@ static int
> > xen_pt_register_regions(XenPCIPassthroughState *s, uint16_t *cmd)
> > type |= PCI_BASE_ADDRESS_MEM_TYPE_64;
> > }
> > *cmd |= PCI_COMMAND_MEMORY;
> > +
> > + /* Round up to a full page for the hypercall. */
> > + r_size = (r_size + XC_PAGE_SIZE - 1) & XC_PAGE_MASK;
> > }
> >
> > memory_region_init_io(&s->bar[i], OBJECT(s), &ops, &s->dev,
> > - "xen-pci-pt-bar", r->size);
> > + "xen-pci-pt-bar", r_size);
> > pci_register_bar(&s->dev, i, type, &s->bar[i]);
> >
> > XEN_PT_LOG(&s->dev, "IO region %i registered (size=0x%08"PRIx64
> > " base_addr=0x%08"PRIx64" type: %#x)\n",
> > - i, r->size, r->base_addr, type);
> > + i, r_size, r->base_addr, type);
> > }
> >
> > /* Register expansion ROM address */
> > --
> > 2.20.1
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
