From: Simon Gaiser <simon@invisiblethingslab.com>
To: Jason Andryuk <jandryuk@gmail.com>,
Paul Durrant <Paul.Durrant@citrix.com>
Cc: Anthony Perard <anthony.perard@citrix.com>,
"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
Stefano Stabellini <sstabellini@kernel.org>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
"marmarek@invisiblethingslab.com"
<marmarek@invisiblethingslab.com>
Subject: Re: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
Date: Thu, 14 Mar 2019 20:22:49 +0100 [thread overview]
Message-ID: <75347d5f-3b47-3b35-fa96-cd166c240d9e@invisiblethingslab.com> (raw)
In-Reply-To: <CAKf6xpujVs3RPJcb+2vqPZFcYwhdDcjbt=S_+awjPuPFpFHsPw@mail.gmail.com>
[-- Attachment #1.1.1: Type: text/plain, Size: 5352 bytes --]
Jason Andryuk:
> On Wed, Mar 13, 2019 at 11:09 AM Paul Durrant <Paul.Durrant@citrix.com> wrote:
>>
>>> -----Original Message-----
>>> From: Jason Andryuk [mailto:jandryuk@gmail.com]
>>> Sent: 11 March 2019 18:02
>>> To: qemu-devel@nongnu.org
>>> Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Simon Gaiser
>>> <simon@invisiblethingslab.com>; Jason Andryuk <jandryuk@gmail.com>; Stefano Stabellini
>>> <sstabellini@kernel.org>; Anthony Perard <anthony.perard@citrix.com>; Paul Durrant
>>> <Paul.Durrant@citrix.com>
>>> Subject: [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE
>>>
>>> From: Simon Gaiser <simon@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?
Not sure which one it was. Probably the USB controller or the SD host
controller. As your example below shows this is not so uncommon.
> 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 <simon@invisiblethingslab.com>
>>> Signed-off-by: Jason Andryuk <jandryuk@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
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2019-03-14 19:23 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-11 18:02 [PATCH 0/6] Xen stubdom support Jason Andryuk
2019-03-11 18:02 ` [PATCH 1/6] xen: Introduce -xen-stubdom option Jason Andryuk
2019-03-11 18:06 ` Paolo Bonzini
2019-03-11 19:46 ` Jason Andryuk
2019-03-11 18:02 ` [PATCH 2/6] xen: Move xenstore initialization to common location Jason Andryuk
2019-03-13 15:01 ` Paul Durrant
2019-03-13 18:11 ` Jason Andryuk
2019-03-14 14:00 ` Paul Durrant
2019-03-11 18:02 ` [PATCH 3/6] xen: Skip backend initialization for stubdom Jason Andryuk
2019-03-13 15:04 ` Paul Durrant
2019-03-11 18:02 ` [PATCH 4/6] xen: Set HVM_PARAM_DM_DOMAIN for stubdom on older Xen Jason Andryuk
2019-03-11 18:02 ` [PATCH 5/6] xen-pt: Hide MSI-X from xen stubdoms Jason Andryuk
2019-03-12 12:04 ` Roger Pau Monné
2019-03-12 12:38 ` Marek Marczykowski-Górecki
2019-03-12 13:58 ` Jason Andryuk
2019-03-12 14:13 ` Roger Pau Monné
2019-03-12 15:15 ` Jason Andryuk
2019-03-13 2:15 ` Jason Andryuk
2019-03-12 14:29 ` Marek Marczykowski-Górecki
2019-03-11 18:02 ` [PATCH 6/6] xen-pt: Round pci regions sizes to XEN_PAGE_SIZE Jason Andryuk
2019-03-13 15:09 ` Paul Durrant
2019-03-14 18:15 ` Jason Andryuk
2019-03-14 19:22 ` Simon Gaiser [this message]
2019-03-14 19:37 ` Andrew Cooper
2019-03-15 9:12 ` Paul Durrant
2019-03-14 20:45 ` Simon Gaiser
2019-03-15 9:17 ` Paul Durrant
2019-03-15 16:28 ` Andrew Cooper
2019-03-20 17:28 ` Jason Andryuk
2019-03-22 3:09 ` Roger Pau Monné
2019-03-22 19:43 ` Jason Andryuk
2020-01-13 19:01 ` [Xen-devel] " Jason Andryuk
2020-01-14 10:04 ` Roger Pau Monné
2020-01-14 14:41 ` Jason Andryuk
2020-01-14 18:04 ` Roger Pau Monné
2020-01-15 8:33 ` Durrant, Paul
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=75347d5f-3b47-3b35-fa96-cd166c240d9e@invisiblethingslab.com \
--to=simon@invisiblethingslab.com \
--cc=Paul.Durrant@citrix.com \
--cc=anthony.perard@citrix.com \
--cc=jandryuk@gmail.com \
--cc=marmarek@invisiblethingslab.com \
--cc=qemu-devel@nongnu.org \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).