All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] Hvmloader: Add _STA for PCI hotplug slots
@ 2013-09-27  6:29 Gonglei (Arei)
  2013-09-27 21:43 ` [Qemu-devel] [Xen-devel] " Konrad Rzeszutek Wilk
  2013-09-27 21:43 ` Konrad Rzeszutek Wilk
  0 siblings, 2 replies; 12+ messages in thread
From: Gonglei (Arei) @ 2013-09-27  6:29 UTC (permalink / raw)
  To: xen-devel
  Cc: Hanweidong (Randy),
	Yanqiangjun, Luonengjun, qemu-devel, Gaowei (UVP),
	Huangweidong (Hardware)

Hi,

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. 

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.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [Xen-devel] Hvmloader: Add _STA for PCI hotplug slots
  2013-09-27  6:29 [Qemu-devel] Hvmloader: Add _STA for PCI hotplug slots Gonglei (Arei)
@ 2013-09-27 21:43 ` Konrad Rzeszutek Wilk
  2013-09-29  0:30   ` Gonglei (Arei)
  2013-09-29  0:30   ` [Qemu-devel] [Xen-devel] " Gonglei (Arei)
  2013-09-27 21:43 ` Konrad Rzeszutek Wilk
  1 sibling, 2 replies; 12+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-09-27 21:43 UTC (permalink / raw)
  To: Gonglei (Arei), anthony.perard, Stefano Stabellini
  Cc: Hanweidong (Randy),
	Yanqiangjun, Luonengjun, qemu-devel, xen-devel, Gaowei (UVP),
	Huangweidong (Hardware)

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@lists.xen.org
> http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Hvmloader: Add _STA for PCI hotplug slots
  2013-09-27  6:29 [Qemu-devel] Hvmloader: Add _STA for PCI hotplug slots Gonglei (Arei)
  2013-09-27 21:43 ` [Qemu-devel] [Xen-devel] " Konrad Rzeszutek Wilk
@ 2013-09-27 21:43 ` Konrad Rzeszutek Wilk
  1 sibling, 0 replies; 12+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-09-27 21:43 UTC (permalink / raw)
  To: Gonglei (Arei), anthony.perard, Stefano Stabellini
  Cc: Hanweidong (Randy),
	Yanqiangjun, Luonengjun, qemu-devel, xen-devel, Gaowei (UVP),
	Huangweidong (Hardware)

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@lists.xen.org
> http://lists.xen.org/xen-devel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [Xen-devel] Hvmloader: Add _STA for PCI hotplug slots
  2013-09-27 21:43 ` [Qemu-devel] [Xen-devel] " Konrad Rzeszutek Wilk
  2013-09-29  0:30   ` Gonglei (Arei)
@ 2013-09-29  0:30   ` Gonglei (Arei)
  2013-10-08 12:58     ` Fabio Fantoni
  2013-10-08 12:58     ` Fabio Fantoni
  1 sibling, 2 replies; 12+ messages in thread
From: Gonglei (Arei) @ 2013-09-29  0:30 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk, anthony.perard, Stefano Stabellini
  Cc: Hanweidong (Randy),
	Yanqiangjun, Luonengjun, qemu-devel, xen-devel, Gaowei (UVP),
	Huangweidong (Hardware)

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Hvmloader: Add _STA for PCI hotplug slots
  2013-09-27 21:43 ` [Qemu-devel] [Xen-devel] " Konrad Rzeszutek Wilk
@ 2013-09-29  0:30   ` Gonglei (Arei)
  2013-09-29  0:30   ` [Qemu-devel] [Xen-devel] " Gonglei (Arei)
  1 sibling, 0 replies; 12+ messages in thread
From: Gonglei (Arei) @ 2013-09-29  0:30 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk, anthony.perard, Stefano Stabellini
  Cc: Hanweidong (Randy),
	Yanqiangjun, Luonengjun, qemu-devel, xen-devel, Gaowei (UVP),
	Huangweidong (Hardware)

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [Xen-devel] Hvmloader: Add _STA for PCI hotplug slots
  2013-09-29  0:30   ` [Qemu-devel] [Xen-devel] " Gonglei (Arei)
@ 2013-10-08 12:58     ` Fabio Fantoni
  2013-10-10 13:32       ` Gonglei (Arei)
  2013-10-10 13:32       ` [Qemu-devel] [Xen-devel] " Gonglei (Arei)
  2013-10-08 12:58     ` Fabio Fantoni
  1 sibling, 2 replies; 12+ messages in thread
From: Fabio Fantoni @ 2013-10-08 12:58 UTC (permalink / raw)
  To: Gonglei (Arei)
  Cc: Yanqiangjun, Konrad Rzeszutek Wilk, Stefano Stabellini,
	Hanweidong (Randy),
	Luonengjun, qemu-devel, xen-devel, anthony.perard, Gaowei (UVP),
	Huangweidong (Hardware)

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Hvmloader: Add _STA for PCI hotplug slots
  2013-09-29  0:30   ` [Qemu-devel] [Xen-devel] " Gonglei (Arei)
  2013-10-08 12:58     ` Fabio Fantoni
@ 2013-10-08 12:58     ` Fabio Fantoni
  1 sibling, 0 replies; 12+ messages in thread
From: Fabio Fantoni @ 2013-10-08 12:58 UTC (permalink / raw)
  To: Gonglei (Arei)
  Cc: Yanqiangjun, Stefano Stabellini, Hanweidong (Randy),
	Luonengjun, qemu-devel, xen-devel, anthony.perard, Gaowei (UVP),
	Huangweidong (Hardware)

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


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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [Xen-devel] Hvmloader: Add _STA for PCI hotplug slots
  2013-10-08 12:58     ` Fabio Fantoni
  2013-10-10 13:32       ` Gonglei (Arei)
@ 2013-10-10 13:32       ` Gonglei (Arei)
  2013-10-10 13:52         ` Jan Beulich
  2013-10-10 13:52         ` Jan Beulich
  1 sibling, 2 replies; 12+ messages in thread
From: Gonglei (Arei) @ 2013-10-10 13:32 UTC (permalink / raw)
  To: Fabio Fantoni
  Cc: Yanqiangjun, Konrad Rzeszutek Wilk, Stefano Stabellini,
	Hanweidong (Randy),
	Luonengjun, qemu-devel, xen-devel, anthony.perard, Gaowei (UVP),
	Huangweidong (Hardware)

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@m2r.biz]
> Sent: Tuesday, October 08, 2013 8:58 PM
> To: Gonglei (Arei)
> Cc: Konrad Rzeszutek Wilk; anthony.perard@citrix.com; Stefano Stabellini;
> Hanweidong (Randy); Yanqiangjun; Luonengjun; qemu-devel@nongnu.org;
> xen-devel@lists.xen.org; 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@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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Hvmloader: Add _STA for PCI hotplug slots
  2013-10-08 12:58     ` Fabio Fantoni
@ 2013-10-10 13:32       ` Gonglei (Arei)
  2013-10-10 13:32       ` [Qemu-devel] [Xen-devel] " Gonglei (Arei)
  1 sibling, 0 replies; 12+ messages in thread
From: Gonglei (Arei) @ 2013-10-10 13:32 UTC (permalink / raw)
  To: Fabio Fantoni
  Cc: Yanqiangjun, Stefano Stabellini, Hanweidong (Randy),
	Luonengjun, qemu-devel, xen-devel, anthony.perard, Gaowei (UVP),
	Huangweidong (Hardware)

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@m2r.biz]
> Sent: Tuesday, October 08, 2013 8:58 PM
> To: Gonglei (Arei)
> Cc: Konrad Rzeszutek Wilk; anthony.perard@citrix.com; Stefano Stabellini;
> Hanweidong (Randy); Yanqiangjun; Luonengjun; qemu-devel@nongnu.org;
> xen-devel@lists.xen.org; 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@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

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [Xen-devel] Hvmloader: Add _STA for PCI hotplug slots
  2013-10-10 13:32       ` [Qemu-devel] [Xen-devel] " Gonglei (Arei)
@ 2013-10-10 13:52         ` Jan Beulich
  2013-10-10 13:52         ` Jan Beulich
  1 sibling, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2013-10-10 13:52 UTC (permalink / raw)
  To: Gonglei (Arei)
  Cc: Yanqiangjun, Hanweidong (Randy),
	Stefano Stabellini, Luonengjun, qemu-devel, xen-devel,
	Fabio Fantoni, anthony.perard, Gaowei (UVP),
	Huangweidong (Hardware)

>>> On 10.10.13 at 15:32, "Gonglei (Arei)" <arei.gonglei@huawei.com> wrote:
> --- 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");

Is this change really necessary? And if so, why would nothing else
require adjustment (i.e. is the value either arbitrary, or used only
here and nowhere else, and also is guaranteed to not conflict with
anything else)?

Jan

>          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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Hvmloader: Add _STA for PCI hotplug slots
  2013-10-10 13:32       ` [Qemu-devel] [Xen-devel] " Gonglei (Arei)
  2013-10-10 13:52         ` Jan Beulich
@ 2013-10-10 13:52         ` Jan Beulich
  1 sibling, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2013-10-10 13:52 UTC (permalink / raw)
  To: Gonglei (Arei)
  Cc: Yanqiangjun, Hanweidong (Randy),
	Stefano Stabellini, Luonengjun, qemu-devel, xen-devel,
	Fabio Fantoni, anthony.perard, Gaowei (UVP),
	Huangweidong (Hardware)

>>> On 10.10.13 at 15:32, "Gonglei (Arei)" <arei.gonglei@huawei.com> wrote:
> --- 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");

Is this change really necessary? And if so, why would nothing else
require adjustment (i.e. is the value either arbitrary, or used only
here and nowhere else, and also is guaranteed to not conflict with
anything else)?

Jan

>          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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Hvmloader: Add _STA for PCI hotplug slots
@ 2013-09-27  6:29 Gonglei (Arei)
  0 siblings, 0 replies; 12+ messages in thread
From: Gonglei (Arei) @ 2013-09-27  6:29 UTC (permalink / raw)
  To: xen-devel
  Cc: Hanweidong (Randy),
	Yanqiangjun, Luonengjun, qemu-devel, Gaowei (UVP),
	Huangweidong (Hardware)

Hi,

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. 

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.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2013-10-10 13:53 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-09-27  6:29 [Qemu-devel] Hvmloader: Add _STA for PCI hotplug slots Gonglei (Arei)
2013-09-27 21:43 ` [Qemu-devel] [Xen-devel] " Konrad Rzeszutek Wilk
2013-09-29  0:30   ` Gonglei (Arei)
2013-09-29  0:30   ` [Qemu-devel] [Xen-devel] " Gonglei (Arei)
2013-10-08 12:58     ` Fabio Fantoni
2013-10-10 13:32       ` Gonglei (Arei)
2013-10-10 13:32       ` [Qemu-devel] [Xen-devel] " Gonglei (Arei)
2013-10-10 13:52         ` Jan Beulich
2013-10-10 13:52         ` Jan Beulich
2013-10-08 12:58     ` Fabio Fantoni
2013-09-27 21:43 ` Konrad Rzeszutek Wilk
  -- strict thread matches above, loose matches on Subject: below --
2013-09-27  6:29 Gonglei (Arei)

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.