Re: [Xen-devel] [PATCH v3 1/1] Add pci_hole_min_size

[Don Slutz <dslutz@xxxxxxxxxxx>]

On 06/27/14 04:34, Jan Beulich wrote:
> > > > On 26.06.14 at 19:47, <dslutz@xxxxxxxxxxx> 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 <xen/memory.h>
> >   #include <xen/hvm/ioreq.h>
> >   #include <xen/hvm/hvm_xs_strings.h>
> > +#include <xen/hvm/e820.h>
> >   #include <stdbool.h>
> >   
> > -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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel