Re: [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default

[Igor Mammedov <imammedo@redhat.com>]

On Sun, 28 Jul 2013 12:13:17 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Sun, Jul 28, 2013 at 10:21:56AM +0200, Igor Mammedov wrote:
> > On Sun, 28 Jul 2013 10:57:12 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Sun, Jul 28, 2013 at 09:29:13AM +0200, Igor Mammedov wrote:
> > > > It turns out that some 32 bit windows guests crash
> > > > if 64 bit PCI hole size is >2G.
> > > > Limit it to 2G for piix and q35 by default.
> > > > User may override default boundaries by using
> > > > "pci_hole64_end " property.
> > > > 
> > > > Examples:
> > > > -global i440FX-pcihost.pci_hole64_end=0xffffffffffffffff
> > > > 
> > > > -global q35-pcihost.pci_hole64_end=0xffffffffffffffff
> > > 
> > > IMO that's pretty bad as user interfaces go.
> > > In particular if you are not careful you can make
> > > end < start.
> > > Why not set the size, and include a patch that
> > > let people write "1G" or "2G" for convenience?
> > sure as convenience why not, on top of this patches.
> >  
> > > 
> > > > Reported-by: Igor Mammedov <imammedo@redhat.com>,
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > >  hw/i386/pc.c              | 59 +++++++++++++++++++++++++++++------------------
> > > >  hw/i386/pc_piix.c         | 14 +----------
> > > >  hw/pci-host/piix.c        | 37 +++++++++++++++++++++++++----
> > > >  hw/pci-host/q35.c         | 32 +++++++++++++++----------
> > > >  include/hw/i386/pc.h      |  5 ++--
> > > >  include/hw/pci-host/q35.h |  1 +
> > > >  6 files changed, 94 insertions(+), 54 deletions(-)
> > > > 
> > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > > index b0b98a8..acaeb6c 100644
> > > > --- a/hw/i386/pc.c
> > > > +++ b/hw/i386/pc.c
> > > > @@ -1003,15 +1003,28 @@ typedef struct PcRomPciInfo {
> > > >  static void pc_fw_cfg_guest_info(PcGuestInfo *guest_info)
> > > >  {
> > > >      PcRomPciInfo *info;
> > > > +    Object *pci_info;
> > > > +
> > > >      if (!guest_info->has_pci_info || !guest_info->fw_cfg) {
> > > >          return;
> > > >      }
> > > > +    pci_info = object_resolve_path("/machine/i440fx", NULL);
> > > > +    if (!pci_info) {
> > > > +        pci_info = object_resolve_path("/machine/q35", NULL);
> > > > +        if (!pci_info) {
> > > > +            return;
> > > > +        }
> > > > +    }
> > > 
> > > 
> > > So is the path /machine/i440fx? /machine/i440FX?
> > > /machine/i440FX-pcihost?
> > > There's no way to check this code is right without
> > > actually running it.
> > that drawback of dynamic lookup,
> > QOM paths supposed to be stable.
> > 
> > > 
> > > How about i44fx_get_pci_info so we can have this
> > > knowledge of paths localized in specific chipset code?
> > I've seen objections from afaerber about this approach, so dropped
> > this idea.
> 
> Could we lookup TYPE_PCI_HOST? This will make pc code
> independent of specific machine.
sure, thanks for idea.

> 
> -- 
> MST


-- 
Regards,
  Igor