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

Re: [Xen-devel] [PATCH v4] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching



On 05/09/2014 12:00 PM, Konrad Rzeszutek Wilk wrote:
On Fri, May 09, 2014 at 10:38:49AM -0400, Ross Philipson wrote:
On 05/09/2014 09:31 AM, Ian Campbell wrote:
On Fri, 2014-05-09 at 09:25 -0400, Konrad Rzeszutek Wilk wrote:
On Fri, May 09, 2014 at 11:26:35AM +0100, Ian Campbell wrote:
On Fri, 2014-05-09 at 10:15 +0000, Gonglei (Arei) wrote:
Hi,

First, please forgive me for my bad English.
It's so sad.

-----Original Message-----
From: Ian Campbell [mailto:Ian.Campbell@xxxxxxxxxx]
Sent: Friday, May 09, 2014 5:57 PM
To: Gonglei (Arei)
Cc: Jan Beulich; xen-devel@xxxxxxxxxxxxx; anthony.perard@xxxxxxxxxx;
stefano.stabellini@xxxxxxxxxxxxx; johannes.krampf@xxxxxxxxxxxxxx; Gaowei
(UVP); Hanweidong (Randy); Huangweidong (C); kevin@xxxxxxxxxxxx;
fabio.fantoni@xxxxxxx; qemu-devel@xxxxxxxxxx; mst@xxxxxxxxxx
Subject: Re: [PATCH v4] Hvmloader: Modify ACPI to only supply _EJ0 methods
for PCIslots that support hotplug by runtime patching

On Fri, 2014-05-09 at 09:45 +0000, Gonglei (Arei) wrote:
And it also seem pretty pointless to send a v4 without addressing
all comments you got on v3.

I don't think so. I have absorbed Ian's all suggestion on v3. And for other
questions have been answered too, in despite of is me or not.

Actually you haven't answered "Why is runtime patching the only
option here?" which was originally phrased as:
Which appears to involve an awful lot of jumping through hoops... Please
can you explain why it is necessary, as opposed to e.g. using a dynamic
set of SSDTs?

Ian, I understand your mean now, which consider our method to address
this issue is maybe unnecessary, right? And you suggest us to use a dynamic
set of SSDTs.

Really what I'm asking is what set of constraints and requirements led
you to this particular solution.

I think the method seems complicated, and I'd therefore like to know why
it was preferred over other alternatives, or perhaps why it is the only
option.

TBH I don't know more about the dynamic SSDTs, if you have any details,
tell me please, thanks in advance!

I'm not an ACPI expert, but AIUI an SSDT is essentially a little piece
of DSDT which is grafted onto the main DSDT at runtime by the OSPM. They
make it somewhat easier for BIOS (or ACPI table) authors to include or
exclude functionality at runtime, perhaps on a physical system in
response to a user changing something in the BIOS setup screens. In Xen
we appear to use SSDTs for HPET, TPM and S3/S4 functionality, depending
on the guest configuration
(hvmloader/acpi/build.c:construct_secondary_tables()).

Can it be used to patch the DSDT? Or were you (Ian) thinking that the bulk
of the ACPI PCI stuff can be moved there ?

I think it can "shadow" or extend existing DSDT stuff, I don't think it
can patch as sych. But here we want to dynamically add an entire method
I think? (or hide, but I don't think that is possible).

Yea the SSDTs extend the DSDT. The DSDT is loaded to create the name space
and then SSDTs are loaded and added to the name space. If you need to make
runtime modifications like this, it is much easier to do it in an SSDT as
you suggest. What I don't know is whether you could extend say a device, as
in this case, with with a single method in a separate SSDT. I have never
really seen something like that before.

WRT to hide vs. remove, I believe the intent is to effectively remove the
eject method from a given device by renaming it. It could simply be removed
making the device not eject-able but I think they are trying to avoid having
to recalculate and update the checksum on the DSDT.

As to whether this has to be done at runtime, I don't know. If it does, I
wrote a lib that can generate basically any (rev2) AML on the fly. We used
it e.g. to generate WMI functionality in an SSDT at runtime. I would be
happy to share if it is useful.

So we could just then gat the _EJ0 functionality based on values that
are present (or not) in the SSDT ?

Something like this in SSDT:

Name (EJA, Package(), {0,1,1,1,..})

(to be generated by hvmloader)

And in the DSDT do a similar mechanims that the CPU hotplug does.
Something like this for EJ0 for function SLOT 0->7:
        Store (ToBuffer (EJA), Local0)
        Store (DerefOf (Index(Local0, Zero)), Local1)
        /* First eight bits in local1 */
        And (Local1, One, Local2)
        /* EJA[0:7] & 1 is stored in Local2 */
        Store(Local2, "\\_GPE.DPT1")
        /* In the debug port it goes. */
        If (LNotEqual(Local2, 1) {
                // Don't do the EJ0
        }

Well something like this might work as long as the OSPM checked to see if the device did in fact eject after calling this. If it just assumes the eject succeeds, things could go badly.

If something like this works, why not go a step farther and just invent an OpRegion, maybe in IO space, and have each _EJ0 method ask at runtime?




How would this work with the 'secondary emulator' patches that
do all of this PCI hotplug in the hypervisor?

It should just mean that the magic I/O port protocol becomes backed by
Xen, although now you mentioned it I suppose it will be necessary for
Xen to know the answer now ;-)

(CC-ing the author
of said patches).

  I thought I did earlier, perhaps I forgot or changed my mind.

Ian.


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

-----
No virus found in this message.
Checked by AVG - www.avg.com
Version: 2014.0.4570 / Virus Database: 3931/7443 - Release Date: 05/05/14



--
Ross Philipson

-----
No virus found in this message.
Checked by AVG - www.avg.com
Version: 2014.0.4570 / Virus Database: 3931/7443 - Release Date: 05/05/14



--
Ross Philipson

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


 


Rackspace

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