Re: [Xen-devel] [PATCH v3 8/8] libxl, hvmloader: Don't relocate memory for MMIO hole

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 21 Jun 2013, George Dunlap wrote:
> On 20/06/13 18:38, Stefano Stabellini wrote:
> > On Thu, 20 Jun 2013, George Dunlap wrote:
> > > At the moment, qemu-xen can't handle memory being relocated by
> > > hvmloader.  This may happen if a device with a large enough memory
> > > region is passed through to the guest.  At the moment, if this
> > > happens, then at some point in the future qemu will crash and the
> > > domain will hang.  (qemu-traditional is fine.)
> > > 
> > > It's too late in the release to do a proper fix, so we try to do
> > > damage control.
> > > 
> > > hvmloader already has mechanisms to relocate memory to 64-bit space
> > > if it can't make a big enough MMIO hole.  By default this is 2GiB; if
> > > we just refuse to make the hole bigger if it will overlap with guest
> > > memory, then the relocation will happen by default.
> > > 
> > > v3:
> > >   - Fix polarity of comparison
> > >   - Move diagnostic messages to another patch
> > >   - Tested with xen platform pci device hacked to have different BAR sizes
> > >     {256MiB, 1GiB} x {qemu-xen, qemu-traditional} x various memory
> > >     configurations
> > >   - Add comment explaining why we default to "allow"
> > >   - Remove cast to bool
> > > v2:
> > >   - style fixes
> > >   - fix and expand comment on the MMIO hole loop
> > >   - use "%d" rather than "%s" -> (...)?"1":"0"
> > >   - use bool instead of uint8_t
> > >   - Move 64-bit bar relocate detection to another patch
> > >   - Add more diagnostic messages
> > > 
> > > Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> > > CC: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > > CC: Ian Jackson <ian.jackson@xxxxxxxxxx>
> > > CC: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> > > CC: Hanweidong <hanweidong@xxxxxxxxxx>
> > > CC: Keir Fraser <keir@xxxxxxx>
> > > CC: Keir Fraser <keir@xxxxxxx>
> > > ---
> > >   tools/firmware/hvmloader/pci.c          |   49
> > > +++++++++++++++++++++++++++++--
> > >   tools/libxl/libxl_dm.c                  |    6 ++++
> > >   xen/include/public/hvm/hvm_xs_strings.h |    1 +
> > >   3 files changed, 54 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tools/firmware/hvmloader/pci.c
> > > b/tools/firmware/hvmloader/pci.c
> > > index 3108c8a..2364177 100644
> > > --- a/tools/firmware/hvmloader/pci.c
> > > +++ b/tools/firmware/hvmloader/pci.c
> > > @@ -27,6 +27,8 @@
> > >     #include <xen/memory.h>
> > >   #include <xen/hvm/ioreq.h>
> > > +#include <xen/hvm/hvm_xs_strings.h>
> > > +#include <stdbool.h>
> > >     unsigned long pci_mem_start = PCI_MEM_START;
> > >   unsigned long pci_mem_end = PCI_MEM_END;
> > > @@ -57,6 +59,32 @@ void pci_setup(void)
> > >       } *bars = (struct bars *)scratch_start;
> > >       unsigned int i, nr_bars = 0;
> > >   +    const char *s;
> > > +    /*
> > > +     * Do we allow hvmloader to relocate guest memory in order to
> > > +     * increase the size of the lowmem MMIO hole?  Defaulting to 1
> > > +     * here will mean that non-libxl toolstacks (including xend and
> > > +     * home-grown ones) will experience this series as "no change".
> > > +     * It does mean that those using qemu-xen will still experience
> > > +     * the bug (described below); but it also means that those using
> > > +     * qemu-traditional will *not* experience any change; and it also
> > > +     * means that there is a work-around for those using qemu-xen,
> > > +     * namely switching to qemu-traditional.
> > > +     *
> > > +     * If we defaulted to 0, and failing to resize the hole caused any
> > > +     * problems with qemu-traditional, then there is no work-around.
> > > +     *
> > > +     * Since xend can't talk to qemu-traditional, I think this is the
> > > +     * option that will have the least impact.
> > > +     */
> > > +    bool allow_memory_relocate = 1;
> > > +
> > > +    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
> > > +    if ( s )
> > > +        allow_memory_relocate = strtoll(s, NULL, 0);
> > Considering that strtoll retuns a long long, are we sure that this
> > allocation does what we want for all the possible long long values
> > that can be returned?
> > 
> > For example, if strtoll returns -1, do we want allow_memory_relocate to
> > be set to true?
> 
> The only valid values here are "0" and "1"; everything else is undefined.

This code doesn't do what you say: "0" means false and everything else
means true. The undefined values are treated as true. Is that what we
want?

In order to do what you say you would need:

bool allow_memory_relocate = 1;
int i;
i = strtoll(s, NULL, 0);
if (i == 0 || i == 1)
    allow_memory_relocate = i;
else
    printf("WARNING");


> Look, the bike shed is already painted, the brushes have been washed and put
> away.  Leave it be. :-)

I am leaving strtoll be. I think it would be easier not to use to strtoll
but I don't mind.
But if we do use strtoll then we need to handle all the possible return
values appropriately.

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