[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] Hvmloader: Add _STA for PCI hotplug slots



Hi,

Not enough tests are done in system based the patch. 
Windows OS can support PCI hot plug/unplug, PCI hot plug/unplug will cause qemu 
crashes in Redhat6.3/5.8.
After reading the ACPI spec, we modify the patch:
Index: mk_dsdt.c
===================================================================
--- mk_dsdt.c   (revision 90666)
+++ mk_dsdt.c   (working copy)
@@ -437,7 +437,7 @@
         indent(); printf("B0EJ, 32,\n");
         pop_block();
 
-        stmt("OperationRegion", "SRMV, SystemIO, 0xae0c, 0x04");
+        stmt("OperationRegion", "SRMV, SystemIO, 0xae00, 0x04");
         push_block("Field", "SRMV, DWordAcc, NoLock, WriteAsZeros");
         indent(); printf("RMV, 32,\n");
         pop_block();        
@@ -451,10 +451,10 @@
                 } pop_block();
                 push_block("Method", "_STA, 0");{
                    push_block("If", "And(RMV, ShiftLeft(1, %#06x))", slot);
-                      stmt("Return", "0x1F");
+                      stmt("Return", "0x0F");
                    pop_block();
                    push_block("Else", NULL);
-                      stmt("Return", "0x1E");
+                      stmt("Return", "0x00");
                    pop_block();
                 };pop_block();
                 stmt("Name", "_SUN, %i", slot);

based on this patch, PCI hot plug/unplug is supported in Redhat5.8/win2008, but 
the problem still exists in Redhat6.3.

More support are needed, Expecting your reply.

Best Regards,
-Gonglei

> -----Original Message-----
> From: Fabio Fantoni [mailto:fabio.fantoni@xxxxxxx]
> Sent: Tuesday, October 08, 2013 8:58 PM
> To: Gonglei (Arei)
> Cc: Konrad Rzeszutek Wilk; anthony.perard@xxxxxxxxxx; Stefano Stabellini;
> Hanweidong (Randy); Yanqiangjun; Luonengjun; qemu-devel@xxxxxxxxxx;
> xen-devel@xxxxxxxxxxxxx; Gaowei (UVP); Huangweidong (Hardware)
> Subject: Re: [Xen-devel] Hvmloader: Add _STA for PCI hotplug slots
> 
> Il 29/09/2013 02:30, Gonglei (Arei) ha scritto:
> >> -----Original Message-----
> >> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@xxxxxxxxxx]
> >> Sent: Saturday, September 28, 2013 5:43 AM
> >> To: Gonglei (Arei); anthony.perard@xxxxxxxxxx; Stefano Stabellini
> >> Cc: xen-devel@xxxxxxxxxxxxx; Hanweidong (Randy); Yanqiangjun;
> Luonengjun;
> >> qemu-devel@xxxxxxxxxx; Gaowei (UVP); Huangweidong (Hardware)
> >> Subject: Re: [Xen-devel] Hvmloader: Add _STA for PCI hotplug slots
> >>
> >> On Fri, Sep 27, 2013 at 06:29:20AM +0000, Gonglei (Arei) wrote:
> >>> Hi,
> >> Hey,
> >>
> >> (CCing Stefano and Anthony).
> >>
> >>> In Xen platform, after using upstream qemu, the all of pci devices will 
> >>> show
> >> hotplug in the windows guest.
> >>> In this situation, the windows guest may occur blue screen when VM' user
> >> click the icon of VGA card for trying unplug VGA card.
> >>> However, we don't hope VM's user can do such dangerous operation, and
> >> showing all pci devices inside the guest OS is unfriendly.
> >>> In addition, I find the traditional qemu have not this problem, and KVM
> also.
> 
> Is there any news about this patch please?
> 
> >>>
> >>> On the KVM platform, the seabios will read the RMV bits of pci slot
> (according
> >> the 0xae08 I/O port register),
> >>> then modify the SSDT table.
> >>>
> >>> The key steps as follows:
> >>> In Seabios:
> >>> #define PCI_RMV_BASE 0xae0c    // 0xae08 I/O port register
> >>> static void* build_ssdt(void)
> >>> {
> >>>   ...
> >>>   // build Device object for each slot
> >>>   u32 rmvc_pcrm = inl(PCI_RMV_BASE);
> >>>   ...
> >>> }
> >>>
> >>> In upstream Qemu, read 0xae0c I/O port register function:
> >>> static uint64_t pci_read(void *opaque, hwaddr addr, unsigned int size)
> >>> {
> >>>      ...
> >>>   case PCI_RMV_BASE - PCI_HOTPLUG_ADDR:
> >>>          val = s->pci0_hotplug_enable;
> >>>          break;
> >>> }
> >>> s->pci0_hotplug_enable is set by the follow function:
> >>>
> >>> static void piix4_update_hotplug(PIIX4PMState *s)
> >>> {
> >>>   ...
> >>>   s->pci0_hotplug_enable = ~0;
> >>>      s->pci0_slot_device_present = 0;
> >>>
> >>>      QTAILQ_FOREACH_SAFE(kid, &bus->children, sibling, next) {
> >>>          DeviceState *qdev = kid->child;
> >>>          PCIDevice *pdev = PCI_DEVICE(qdev);
> >>>          PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pdev);
> >>>          int slot = PCI_SLOT(pdev->devfn);
> >>>
> >>>           //setting by PCIDeviceClass *k->no_hotplug
> >>>          if (pc->no_hotplug) {
> >>>              s->pci0_hotplug_enable &= ~(1U << slot);
> >>>          }
> >>>
> >>>          s->pci0_slot_device_present |= (1U << slot);
> >>>      }
> >>> }
> >>>
> >>> But, on the XEN platform, ACPI DSDT tables is produced by the hvmloader,
> >>> more details in this patchï
> >>>
> >>
> http://xen.1045712.n5.nabble.com/xen-unstable-hvmloader-acpi-dsdt-Fix-PCI-
> >> hotplug-with-the-new-qemu-xen-td4947152.html
> >>> # Node ID 1a912ce93b506a185b54fd97986214e6eff8a0bc
> >>> # Parent  6bc03e22f921aadfa7e5cebe92100cb01377947d
> >>> hvmloader/acpi/dsdt: Fix PCI hotplug with the new qemu-xen.
> >> oddly enough you did not CC the author of said patch?
> >>
> >> I am doing that for you.
> > That's my mistake, thank you so much!
> >>> The ACPI PIIX4 device in QEMU upstream as not the same behavior to
> >>> handle PCI hotplug. This patch introduce the necessary change to the
> >>> DSDT ACPI table to behave as expceted by the new QEMU.
> >>>
> >>> To switch to this new DSDT table version, there is a new option
> >>> --dm-version to mk_dsdt.
> >>>
> >>> Change are inspired by SeaBIOS DSDT source code.
> >>>
> >>> There is few things missing with the new QEMU:
> >>>    - QEMU provide the plugged/unplugged status only per slot (and not
> >>>      per func like qemu-xen-traditionnal.
> >>>    - I did not include the _STA ACPI method that give the status of a
> >>>      device (present, functionning properly) because qemu-xen does not
> >>>      handle it.
> >>>    - I did not include the _RMV method that say if the device can be
> >>>      removed,
> >>>      because the IO port of QEMU that give this status always return
> >>>      true. In
> >>>      SeaBIOS table, they have a specific _RMV method for VGA, ISA that
> >>>      return
> >>>      false. But I'm not sure that we can do the same in Xen.
> >>>
> >>>
> >>> now, I add the _STA method, return the different value according the
> 0xae08
> >> I/O port register on read,
> >>> a pci device allow hotplug return 0x1f, a pci device don't allow return 
> >>> 0x1e.
> >>> Then the pci devices which don't allow hotplug will not show inside the
> guest
> >> OS.
> >>
> >> So you are saying that the 'the IO port of QEMU that give this status 
> >> always
> >> return
> >> true. " is no longer correct?
> >>
> > Yes, now the RMV bits of pci slot is set by the PCIDeviceClass's no_hotplug
> attribute, such as:
> >
> > static void cirrus_vga_class_init(ObjectClass *klass, void *data)
> > {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> >
> >      k->no_hotplug = 1;
> >      ... ...
> > }
> >
> > If k->no_hotplug equal 1, the corresponding slot of PIIX4PMState
> pci0_hotplug_enable bit will be cleared.
> > Otherwise the slot's pci0_hotplug_enable bit will be set.
> >
> >>> Index: tools/firmware/hvmloader/acpi/mk_dsdt.c
> >>>
> >>
> ================================================================
> >> ===
> >>> --- tools/firmware/hvmloader/acpi/mk_dsdt.c       (revision 1105)
> >>> +++ tools/firmware/hvmloader/acpi/mk_dsdt.c       (working copy)
> >>> @@ -437,6 +437,10 @@
> >>>           indent(); printf("B0EJ, 32,\n");
> >>>           pop_block();
> >>>
> >>> +        stmt("OperationRegion", "SRMV, SystemIO, 0xae0c, 0x04");
> >>> +        push_block("Field", "SRMV, DWordAcc, NoLock,
> WriteAsZeros");
> >>> +        indent(); printf("RMV, 32,\n");
> >>> +        pop_block();
> >>>           /* hotplug_slot */
> >>>           for (slot = 1; slot <= 31; slot++) {
> >>>               push_block("Device", "S%i", slot); {
> >>> @@ -445,6 +449,14 @@
> >>>                       stmt("Store", "ShiftLeft(1, %#06x), B0EJ",
> slot);
> >>>                       stmt("Return", "0x0");
> >>>                   } pop_block();
> >>> +                push_block("Method", "_STA, 0");{
> >>> +                   push_block("If", "And(RMV, ShiftLeft(1, %#06x))",
> >> slot);
> >>> +                      stmt("Return", "0x1F");
> >>> +                   pop_block();
> >>> +                   push_block("Else", NULL);
> >>> +                      stmt("Return", "0x1E");
> >>> +                   pop_block();
> >>> +                };pop_block();
> >>>                   stmt("Name", "_SUN, %i", slot);
> >>>               } pop_block();
> >>>           }
> >>>
> >>> Have you any ideasïExpecting your reply, thanks in advance!
> >>>
> >>> Best regards,
> >>> -Gonglei
> >>> _______________________________________________
> >>> Xen-devel mailing list
> >>> Xen-devel@xxxxxxxxxxxxx
> >>> http://lists.xen.org/xen-devel
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@xxxxxxxxxxxxx
> > http://lists.xen.org/xen-devel

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.