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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 20 Jun 2013, George Dunlap wrote:
> On 19/06/13 18:18, Stefano Stabellini wrote:
> > On Tue, 18 Jun 2013, George Dunlap wrote:
> > > +    const char *s;
> > > +    bool allow_memory_relocate = 1;
> > Arguably the default should be 0, given that the default device model is
> > qemu-xen that cannot cope with memory relocation.
> 
> OK, so time to think a bit harder about this. This will only matter if someone
> is using this hvmloader with a non-libxl toolstack which includes xend, or a
> home-grown one.
> 
> * If we default to 1, then:
>  - VMs running qemu-traditional will be have exactly as before
>  - VMs running qemu-xen will have the risk of crashing mysteriously.
>  - If qemu-xen is the default, then there is a work-around: run
> qemu-traditional

If we are talking about xend, then there is no point in thinking about
qemu-xen, given that xend cannot use it.


> * If we default to 0, then:
>  - VMs running qemu-xen will be fine
>  - VMs running qemu-traditional may have strange problems; we haven't tested
> relocating things into 64-bit with qemu-tradiational.

This is not true, hvmloader even before your changes relocates bar above
64-bit. It just does it in a more limited way.


>  - There is no work-around available; if the device either can't be relocated,
> or the OS / qemu can't handle the relocation, then the user is just hosed.

Given that we are talking about xend, we are forced to run
qemu-xen-traditional, so there is no workaround.


> Furthermore, I think xm defaults to qemu-traditional, right?  Does xm even
> know how to drive qemu-xen?  If it does default to qemu-traditional,
> defaulting to 0 will pretty much guarantee a whole slew of currently-working
> configurations will be affected (perhaps break, perhaps not).
> 
> Overall I think defaulting to 1 is probably the lowest-risk option.

Defaulting to 1 makes sure that the behaviour of xend remains the same.
OK.


> > > +    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
> > > +    if ( s )
> > > +        allow_memory_relocate = (bool)strtoll(s, NULL, 0);
> > > +    printf("Relocating guest memory for lowmem MMIO space %s\n",
> > > +           allow_memory_relocate?"enabled":"disabled");
> > It doesn't take a strtoll to parse a boolean.
> 
> As discussed in v1, strtoll is the only "XtoY" function available in
> hvmloader. :-)  The only other option would be to explicitly compare for "1"
> or "0" (or do some half-baked *s-'0' thing).

What's wrong with testing for '0' or '1'?


> This does make me think though -- what is the semantics of casting to a bool?
> Is it !!, or will it essentially clip off the high bits?  (e.g., would "2"
> become "1", or "0"?)

I think that is !!


> > >       /* Program PCI-ISA bridge with appropriate link routes. */
> > >       isa_irq = 0;
> > >       for ( link = 0; link < 4; link++ )
> > > @@ -209,14 +220,38 @@ void pci_setup(void)
> > >           pci_writew(devfn, PCI_COMMAND, cmd);
> > >       }
> > >   -    while ( (mmio_total > (pci_mem_end - pci_mem_start)) &&
> > > -            ((pci_mem_start << 1) != 0) )
> > > +    /*
> > > +     * At the moment qemu-xen can't deal with relocated memory regions.
> > > +     * It's too close to the release to make a proper fix; for now,
> > > +     * only allow the MMIO hole to grow large enough to move guest memory
> > > +     * if we're running qemu-traditional.  Items that don't fit will be
> > > +     * relocated into the 64-bit address space.
> > I would avoid mentioning release issues in a comment within the code.
> 
> I guess it depends on whether we intend to keep this change or to get rid of
> it once the 4.4. window opens. If we intend to get rid of it, then I think the
> comment should stay; if for some reason someone comes along later and and
> wants to know, "Can I change this?" Knowing that it was only meant to be a
> temporary measure is important information.
> 
> Really, I'm of the opinion that if KVM is using SeaBIOS's pci routines, we
> should just move do the same.  No sense in duplicating the effort for
> something like this.

I would never make any changes with the assumption that they are going
to be reverted. There is nothing more lasting than a temporary
workaround. The comment should stay and explain why we are using it, but
mentioning the release (what release? when was it?) is pointless.

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