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

["Michael S. Tsirkin" <mst@redhat.com>]

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.

-- 
MST