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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

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

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