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

[Fabio Fantoni <fabio.fantoni@m2r.biz>]

Il 29/09/2013 02:30, Gonglei (Arei) ha scritto:
>> -----Original Message-----
>> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
>> Sent: Saturday, September 28, 2013 5:43 AM
>> To: Gonglei (Arei); anthony.perard@citrix.com; Stefano Stabellini
>> Cc: xen-devel@lists.xen.org; Hanweidong (Randy); Yanqiangjun; Luonengjun;
>> qemu-devel@nongnu.org; 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@lists.xen.org
>>> http://lists.xen.org/xen-devel
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel