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

["Gonglei (Arei)" <arei.gonglei@xxxxxxxxxx>]

> -----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.
> >
> > 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