From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH v3 1/1] Add pci_hole_min_size Date: Tue, 01 Jul 2014 07:56:53 -0400 Message-ID: <53B2A205.3080807@terremark.com> References: <1403804869-3461-1-git-send-email-dslutz@verizon.com> <1403804869-3461-2-git-send-email-dslutz@verizon.com> <53AD48B8020000780001DDC2@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53AD48B8020000780001DDC2@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , Don Slutz Cc: Boris Ostrovsky , xen-devel@lists.xen.org, Ian Jackson , Ian Campbell , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org On 06/27/14 04:34, Jan Beulich wrote: >>>> On 26.06.14 at 19:47, wrote: >> --- a/tools/firmware/hvmloader/config.h >> +++ b/tools/firmware/hvmloader/config.h >> @@ -53,7 +53,6 @@ extern struct bios_config ovmf_config; >> #define PCI_ISA_IRQ_MASK 0x0c20U /* ISA IRQs 5,10,11 are PCI connected */ >> >> /* MMIO hole: Hardcoded defaults, which can be dynamically expanded. */ >> -#define PCI_MEM_START 0xf0000000 >> #define PCI_MEM_END 0xfc000000 > This pair already shows that ... [skip next comment for continuation] > >> --- a/tools/firmware/hvmloader/pci.c >> +++ b/tools/firmware/hvmloader/pci.c >> @@ -28,9 +28,10 @@ >> #include >> #include >> #include >> +#include >> #include >> >> -unsigned long pci_mem_start = PCI_MEM_START; >> +unsigned long pci_mem_start = HVM_BELOW_4G_RAM_END; > Perhaps (also related to the comment below) better to use > HVM_BELOW_4G_MMIO_START here, even if right now both evaluate > to the same number. Will do. > >> @@ -237,6 +243,26 @@ void pci_setup(void) >> pci_writew(devfn, PCI_COMMAND, cmd); >> } >> >> + if ( pci_hole_min_size ) >> + { >> + uint64_t max_ram_below_4g = (1ULL << 32) - pci_hole_min_size; > ... this isn't really correct: The space immediately below 4G is used for > MMIO unrelated to PCI. Hence, to not confuse people, I'd really like > the variable to be properly named (whether the xl config option should > also get renamed I'm not sure - the xl maintainers will know), i.e. > mmio_hole_min_size. I think it is better to have at most 2 names. So either s/pci_hole_min_size/mmio_hole_min_size/ or switch to max_ram_below_4g. >> + >> + if ( max_ram_below_4g >= HVM_BELOW_4G_RAM_END ) >> + { >> + printf("max_ram_below_4g=0x"PRIllx >> + " too big for pci_hole_min_size=0x"PRIllx"\n", >> + PRIllx_arg(max_ram_below_4g), >> + PRIllx_arg(pci_hole_min_size)); > Probably worth making clear in the logged message that the option > therefore is being ignored? Sure. >> + } >> + else >> + { >> + pci_mem_start = max_ram_below_4g; >> + printf("pci_mem_start=0x%lx (was 0x%x) for pci_hole_min_size=%lu\n", >> + pci_mem_start, HVM_BELOW_4G_RAM_END, >> + (long)pci_hole_min_size); >> + } >> + } > Furthermore the whole concept collides with the auto-growing of > the hole immediately following this code. Both logically (both having > a similar purpose) and, what's worse, functionally in that the > shifting done there requires that ((1ULL << 32) - pci_mem_start) > be a power of 2. An option might be to skip this auto-sizing if the > option was given. I am fine with disabling the auto-growing (since as far as I know this is bug #28 ( #28 - support PCI hole resize in qemu-xen ) Which this change is a kind of work around. I would also drop the _min part of the name (i.e. use mmio_hole_size or max_ram_below_4g). -Don Slutz > Jan >