From: George Dunlap <dunlapg@umich.edu>
To: Tiejun Chen <tiejun.chen@intel.com>
Cc: Keir Fraser <keir@xen.org>,
Ian Campbell <ian.campbell@citrix.com>,
Stefano Stabellini <stefano.stabellini@eu.citrix.com>,
Andrew Cooper <andrew.cooper3@citrix.com>,
Ian Jackson <ian.jackson@eu.citrix.com>,
"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>,
Jan Beulich <jbeulich@suse.com>, Wei Liu <wei.liu2@citrix.com>
Subject: Re: [v8][PATCH 06/16] hvmloader/pci: disable all pci devices conflicting with rdm
Date: Thu, 16 Jul 2015 12:32:47 +0100 [thread overview]
Message-ID: <CAFLBxZZhDHi2XQ9UYkt6gfEiVMNRGxxer3zNptFGVsns2OYOfw@mail.gmail.com> (raw)
In-Reply-To: <1437029582-19564-7-git-send-email-tiejun.chen@intel.com>
On Thu, Jul 16, 2015 at 7:52 AM, Tiejun Chen <tiejun.chen@intel.com> wrote:
> When allocating mmio address for PCI bars, mmio may overlap with
> reserved regions. Currently we just want to disable these associate
> devices simply to avoid conflicts but we will reshape current mmio
> allocation mechanism to fix this completely.
On the whole I still think it would be good to try to relocate BARs if
possible; I would be OK with this if there isn't a better option.
A couple of comments on the patch, however:
>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
> v8:
>
> * Based on current discussion its hard to reshape the original mmio
> allocation mechanism but we haven't a good and simple way to in short term.
> So instead, we don't bring more complicated to intervene that process but
> still check any conflicts to disable all associated devices.
>
> v6 ~ v7:
>
> * Nothing is changed.
>
> v5:
>
> * Rename that field, is_64bar, inside struct bars with flag, and
> then extend to also indicate if this bar is already allocated.
>
> v4:
>
> * We have to re-design this as follows:
>
> #1. Goal
>
> MMIO region should exclude all reserved device memory
>
> #2. Requirements
>
> #2.1 Still need to make sure MMIO region is fit all pci devices as before
>
> #2.2 Accommodate the not aligned reserved memory regions
>
> If I'm missing something let me know.
>
> #3. How to
>
> #3.1 Address #2.1
>
> We need to either of populating more RAM, or of expanding more highmem. But
> we should know just 64bit-bar can work with highmem, and as you mentioned we
> also should avoid expanding highmem as possible. So my implementation is to
> allocate 32bit-bar and 64bit-bar orderly.
>
> 1>. The first allocation round just to 32bit-bar
>
> If we can finish allocating all 32bit-bar, we just go to allocate 64bit-bar
> with all remaining resources including low pci memory.
>
> If not, we need to calculate how much RAM should be populated to allocate the
> remaining 32bit-bars, then populate sufficient RAM as exp_mem_resource to go
> to the second allocation round 2>.
>
> 2>. The second allocation round to the remaining 32bit-bar
>
> We should can finish allocating all 32bit-bar in theory, then go to the third
> allocation round 3>.
>
> 3>. The third allocation round to 64bit-bar
>
> We'll try to first allocate from the remaining low memory resource. If that
> isn't enough, we try to expand highmem to allocate for 64bit-bar. This process
> should be same as the original.
>
> #3.2 Address #2.2
>
> I'm trying to accommodate the not aligned reserved memory regions:
>
> We should skip all reserved device memory, but we also need to check if other
> smaller bars can be allocated if a mmio hole exists between resource->base and
> reserved device memory. If a hole exists between base and reserved device
> memory, lets go out simply to try allocate for next bar since all bars are in
> descending order of size. If not, we need to move resource->base to reserved_end
> just to reallocate this bar.
>
> tools/firmware/hvmloader/pci.c | 87 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 87 insertions(+)
>
> diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
> index 5ff87a7..9e017d5 100644
> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -38,6 +38,90 @@ uint64_t pci_hi_mem_start = 0, pci_hi_mem_end = 0;
> enum virtual_vga virtual_vga = VGA_none;
> unsigned long igd_opregion_pgbase = 0;
>
> +/*
> + * We should check if all valid bars conflict with RDM.
> + *
> + * Here we just need to check mmio bars in the case of non-highmem
> + * since the hypervisor can make sure RDM doesn't involve highmem.
> + */
> +static void disable_conflicting_devices(void)
> +{
> + uint8_t is_64bar;
> + uint32_t devfn, bar_reg, cmd, bar_data;
> + uint16_t vendor_id, device_id;
> + unsigned int bar, i;
> + uint64_t bar_sz;
> + bool is_conflict = false;
> +
> + for ( devfn = 0; devfn < 256; devfn++ )
> + {
> + vendor_id = pci_readw(devfn, PCI_VENDOR_ID);
> + device_id = pci_readw(devfn, PCI_DEVICE_ID);
> + if ( (vendor_id == 0xffff) && (device_id == 0xffff) )
> + continue;
> +
> + /* Check all bars */
> + for ( bar = 0; bar < 7; bar++ )
> + {
> + bar_reg = PCI_BASE_ADDRESS_0 + 4*bar;
> + if ( bar == 6 )
> + bar_reg = PCI_ROM_ADDRESS;
> +
> + bar_data = pci_readl(devfn, bar_reg);
> + bar_data &= PCI_BASE_ADDRESS_MEM_MASK;
> + if ( !bar_data )
> + continue;
> +
> + if ( bar_reg != PCI_ROM_ADDRESS )
> + is_64bar = !!((bar_data & (PCI_BASE_ADDRESS_SPACE |
> + PCI_BASE_ADDRESS_MEM_TYPE_MASK)) ==
> + (PCI_BASE_ADDRESS_SPACE_MEMORY |
> + PCI_BASE_ADDRESS_MEM_TYPE_64));
> +
> + /* Until here we never conflict high memory. */
> + if ( is_64bar && pci_readl(devfn, bar_reg + 4) )
> + continue;
> +
> + /* Just check mmio bars. */
> + if ( ((bar_data & PCI_BASE_ADDRESS_SPACE) ==
> + PCI_BASE_ADDRESS_SPACE_IO) )
> + continue;
> +
> + bar_sz = pci_readl(devfn, bar_reg);
> + bar_sz &= PCI_BASE_ADDRESS_MEM_MASK;
> +
> + for ( i = 0; i < memory_map.nr_map ; i++ )
> + {
> + if ( memory_map.map[i].type != E820_RAM )
Here we're assuming that any region not marked as RAM is an RMRR. Is that true?
In any case, it would be just as strange to have a device BAR overlap
with guest RAM as with an RMRR, wouldn't it?
> + {
> + uint64_t reserved_start, reserved_size;
> + reserved_start = memory_map.map[i].addr;
> + reserved_size = memory_map.map[i].size;
> + if ( check_overlap(bar_data , bar_sz,
> + reserved_start, reserved_size) )
> + {
> + is_conflict = true;
> + /* Now disable the memory or I/O mapping. */
> + printf("pci dev %02x:%x bar %02x : 0x%08x : conflicts "
> + "reserved resource so disable this device.!\n",
> + devfn>>3, devfn&7, bar_reg, bar_data);
> + cmd = pci_readw(devfn, PCI_COMMAND);
> + pci_writew(devfn, PCI_COMMAND, ~cmd);
> + break;
> + }
> + }
> +
> + /* Jump next device. */
> + if ( is_conflict )
> + {
> + is_conflict = false;
> + break;
> + }
This conditional is still inside the memory_map loop; you want it one
loop futher out, in the bar loop, don't you?
Also, if you declare is_conflict inside the devfn loop, rather than in
the main function, then you don't need this "is_conflict=false" here.
It might also be more sensible to use a goto instead; but this is one
where Jan will have a better idea what standard practice will be.
-George
next prev parent reply other threads:[~2015-07-16 11:32 UTC|newest]
Thread overview: 83+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-17 0:45 [v9][PATCH 00/16] Fix RMRR Tiejun Chen
2015-07-17 0:45 ` [v9][PATCH 01/16] xen: introduce XENMEM_reserved_device_memory_map Tiejun Chen
2015-07-17 0:45 ` [v9][PATCH 02/16] xen/vtd: create RMRR mapping Tiejun Chen
2015-07-17 0:45 ` [v9][PATCH 03/16] xen/passthrough: extend hypercall to support rdm reservation policy Tiejun Chen
2015-07-17 6:48 ` Jan Beulich
2015-07-20 1:12 ` Tian, Kevin
2015-07-17 0:45 ` [v9][PATCH 04/16] xen: enable XENMEM_memory_map in hvm Tiejun Chen
2015-07-17 0:45 ` [v9][PATCH 05/16] hvmloader: get guest memory map into memory_map[] Tiejun Chen
2015-07-17 0:45 ` [v9][PATCH 06/16] hvmloader/pci: disable all pci devices conflicting with rdm Tiejun Chen
2015-07-17 13:59 ` Jan Beulich
2015-07-17 14:24 ` Chen, Tiejun
2015-07-17 0:45 ` [v9][PATCH 07/16] hvmloader/e820: construct guest e820 table Tiejun Chen
2015-07-17 7:40 ` Jan Beulich
2015-07-17 9:09 ` Chen, Tiejun
2015-07-17 10:50 ` Jan Beulich
2015-07-17 15:22 ` Chen, Tiejun
2015-07-17 15:31 ` Jan Beulich
2015-07-17 15:54 ` Chen, Tiejun
2015-07-17 16:06 ` Jan Beulich
2015-07-17 16:10 ` Chen, Tiejun
2015-07-18 12:35 ` George Dunlap
2015-07-20 6:19 ` Chen, Tiejun
2015-07-17 9:27 ` Chen, Tiejun
2015-07-17 10:53 ` Jan Beulich
2015-07-17 0:45 ` [v9][PATCH 08/16] tools/libxc: Expose new hypercall xc_reserved_device_memory_map Tiejun Chen
2015-07-17 0:45 ` [v9][PATCH 09/16] tools: extend xc_assign_device() to support rdm reservation policy Tiejun Chen
2015-07-17 0:45 ` [v9][PATCH 10/16] tools: introduce some new parameters to set rdm policy Tiejun Chen
2015-07-17 0:45 ` [v9][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM Tiejun Chen
2015-07-17 0:45 ` [v9][PATCH 12/16] tools: introduce a new parameter to set a predefined rdm boundary Tiejun Chen
2015-07-17 0:45 ` [v9][PATCH 13/16] libxl: construct e820 map with RDM information for HVM guest Tiejun Chen
2015-07-16 6:52 ` [v8][PATCH 00/16] Fix RMRR Tiejun Chen
2015-07-16 6:52 ` [v8][PATCH 01/16] xen: introduce XENMEM_reserved_device_memory_map Tiejun Chen
2015-07-16 6:52 ` [v8][PATCH 02/16] xen/vtd: create RMRR mapping Tiejun Chen
2015-07-16 6:52 ` [v8][PATCH 03/16] xen/passthrough: extend hypercall to support rdm reservation policy Tiejun Chen
2015-07-16 7:40 ` Jan Beulich
2015-07-16 7:48 ` Chen, Tiejun
2015-07-16 7:58 ` Jan Beulich
2015-07-16 11:09 ` George Dunlap
2015-07-16 6:52 ` [v8][PATCH 04/16] xen: enable XENMEM_memory_map in hvm Tiejun Chen
2015-07-16 6:52 ` [v8][PATCH 05/16] hvmloader: get guest memory map into memory_map[] Tiejun Chen
2015-07-16 9:18 ` Jan Beulich
2015-07-16 11:15 ` George Dunlap
2015-07-16 6:52 ` [v8][PATCH 06/16] hvmloader/pci: disable all pci devices conflicting with rdm Tiejun Chen
2015-07-16 11:32 ` George Dunlap [this message]
2015-07-16 11:52 ` Chen, Tiejun
2015-07-16 13:02 ` George Dunlap
2015-07-16 13:21 ` Chen, Tiejun
2015-07-16 13:32 ` Jan Beulich
2015-07-16 13:48 ` Chen, Tiejun
2015-07-16 14:54 ` Jan Beulich
2015-07-16 15:20 ` Chen, Tiejun
2015-07-16 15:39 ` George Dunlap
2015-07-16 16:08 ` Chen, Tiejun
2015-07-16 16:40 ` George Dunlap
2015-07-16 21:24 ` Chen, Tiejun
2015-07-16 16:18 ` George Dunlap
2015-07-16 16:31 ` George Dunlap
2015-07-16 21:15 ` Chen, Tiejun
2015-07-17 9:26 ` George Dunlap
2015-07-17 10:55 ` Jan Beulich
2015-07-16 6:52 ` [v8][PATCH 07/16] hvmloader/e820: construct guest e820 table Tiejun Chen
2015-07-16 11:47 ` George Dunlap
2015-07-16 13:12 ` Chen, Tiejun
2015-07-16 14:29 ` George Dunlap
2015-07-16 15:04 ` Chen, Tiejun
2015-07-16 15:16 ` George Dunlap
2015-07-16 15:29 ` Chen, Tiejun
2015-07-16 15:33 ` George Dunlap
2015-07-16 15:42 ` Chen, Tiejun
2015-07-16 6:52 ` [v8][PATCH 08/16] tools/libxc: Expose new hypercall xc_reserved_device_memory_map Tiejun Chen
2015-07-16 6:52 ` [v8][PATCH 09/16] tools: extend xc_assign_device() to support rdm reservation policy Tiejun Chen
2015-07-16 6:52 ` [v8][PATCH 10/16] tools: introduce some new parameters to set rdm policy Tiejun Chen
2015-07-16 6:52 ` [v8][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM Tiejun Chen
2015-07-16 6:52 ` [v8][PATCH 12/16] tools: introduce a new parameter to set a predefined rdm boundary Tiejun Chen
2015-07-16 6:52 ` [v8][PATCH 13/16] libxl: construct e820 map with RDM information for HVM guest Tiejun Chen
2015-07-22 13:55 ` [v8][PATCH 13/16] libxl: construct e820 map with RDM information for HVM guest [and 1 more messages] Ian Jackson
2015-07-16 6:53 ` [v8][PATCH 14/16] xen/vtd: enable USB device assignment Tiejun Chen
2015-07-16 6:53 ` [v8][PATCH 15/16] xen/vtd: prevent from assign the device with shared rmrr Tiejun Chen
2015-07-16 7:42 ` Jan Beulich
2015-07-16 6:53 ` [v8][PATCH 16/16] tools: parse to enable new rdm policy parameters Tiejun Chen
2015-07-17 0:45 ` [v9][PATCH 14/16] xen/vtd: enable USB device assignment Tiejun Chen
2015-07-17 0:45 ` [v9][PATCH 15/16] xen/vtd: prevent from assign the device with shared rmrr Tiejun Chen
2015-07-17 0:45 ` [v9][PATCH 16/16] tools: parse to enable new rdm policy parameters Tiejun Chen
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=CAFLBxZZhDHi2XQ9UYkt6gfEiVMNRGxxer3zNptFGVsns2OYOfw@mail.gmail.com \
--to=dunlapg@umich.edu \
--cc=andrew.cooper3@citrix.com \
--cc=ian.campbell@citrix.com \
--cc=ian.jackson@eu.citrix.com \
--cc=jbeulich@suse.com \
--cc=keir@xen.org \
--cc=stefano.stabellini@eu.citrix.com \
--cc=tiejun.chen@intel.com \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xen.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).