Re: [Xen-devel] [PATCH v2] tools/firmware: add ACPI device for Windows laptop/slate mode switch

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 24 March 2017 10:53
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Owen Smith
> <owen.smith@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Ian Jackson
> <Ian.Jackson@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: [PATCH v2] tools/firmware: add ACPI device for Windows
> laptop/slate mode switch
> 
> >>> On 24.03.17 at 11:10, <paul.durrant@xxxxxxxxxx> wrote:
> > @@ -406,6 +407,16 @@ static int construct_secondary_tables(struct
> acpi_ctxt *ctxt,
> >          printf("S4 disabled\n");
> >      }
> >
> > +    if ( config->table_flags & ACPI_HAS_SSDT_CONV )
> > +    {
> > +        ssdt = ctxt->mem_ops.alloc(ctxt, sizeof(ssdt_conv), 16);
> > +        if (!ssdt) return -1;
> 
> This being an optional thing anyway, would it perhaps be better to
> allow guest boot to continue, just logging a message? Otoh, if we
> have no memory here anymore, the guest ought to not get very
> far anyway.
> 

Yes, I think if this fails the guest is probably going to die anyway.

> > +DefinitionBlock ("SSDT_CONV.aml", "SSDT", 2, "Xen", "HVM", 0)
> > +{
> > +    Device (CONV) {
> > +        Method (_HID, 0x0, NotSerialized) {
> > +            Return("ID9001")
> > +        }
> > +        Name (_CID, "PNP0C60")
> > +    }
> > +}
> 
> And that's it? This device doesn't do anything.
> 

Yes, that's it! It is there merely so an in-box Windows driver can bind to it.

> The only other concern I have here is that the abbreviation "conv"
> used throughout the patch is sort of ambiguous. I think it means
> "convertible" here, but without knowing the context it could easily
> be "conventional" or "convenience". Would there be anything
> wrong with spelling it out wherever name length limitations don't
> require it to be just four characters?
> 

I thought that name would be ok since it is the name of the ACPI device itself 
but I could extend the xl.cfg option, bool and flag names to 'acpi_convert' to 
make it more obvious.

  Paul

> Jan


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