Re: [RFC PATCH 2/2] acpi/pci_root: negotiate CXL _OSC

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Thu, 2022-03-17 at 16:10 +0000, Jonathan Cameron wrote:
> On Wed, 16 Mar 2022 18:27:04 -0600
> Vishal Verma <vishal.l.verma@intel.com> wrote:
> 
> > Add full support for negotiating _OSC as defined in the CXL 2.0 spec, as
> > applicable to CXL-enabled platforms. Advertise support for the CXL
> > features we support - 'CXL 2.0 port/device register access', 'Protocol
> > Error Reporting', and 'CL Native Hot Plug'. Request control for 'CXL
> > Memory Error Reporting'. The requests are dependent on CONFIG_* based
> > pre-requisites, and prior PCI enabling, similar to how the standard PCI
> > _OSC bits are determined.
> > 
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Robert Moore <robert.moore@intel.com>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> Hi Vishal,
> 
> A few minor queries inline.
> 
> Jonathan

Thanks for reviewing Jonathan - fixed up most of the things, see below.

> 
[..]
> >  
> > +static u32 acpi_osc_ctx_get_pci_control(struct acpi_osc_context *context)
> > +{
> > +       return *((u32 *)(context->ret.pointer + 8));
> 
> Use the defines for the offsets? sizeof(u32) * OSC_CONTROL_DWORD for example
> 
> > +}
> > +
> > +static u32 acpi_osc_ctx_get_cxl_control(struct acpi_osc_context *context)
> > +{
> > +       return *((u32 *)(context->ret.pointer + 16));
> 
> As above + sizeof(u32) * OSC_CXL_CONTROL_DWORD)

Makes sense, done.

> 
> > +}
> > +
> >  static acpi_status acpi_pci_run_osc(struct acpi_pci_root *root,
> > -                                   const u32 *capbuf, u32 *retval)
> > +                                   const u32 *capbuf, u32 *pci_control,
> > +                                   u32 *cxl_control)
> >  {
> >         struct acpi_osc_context context = {
> >                 .uuid_str = to_uuid(root),
> > @@ -212,18 +246,20 @@ static acpi_status acpi_pci_run_osc(struct acpi_pci_root *root,
> >  
> >         status = acpi_run_osc(root->device->handle, &context);
> >         if (ACPI_SUCCESS(status)) {
> > -               *retval = *((u32 *)(context.ret.pointer + 8));
> > +               *pci_control = acpi_osc_ctx_get_pci_control(&context);
> > +               if (is_cxl(root))
> > +                       *cxl_control = acpi_osc_ctx_get_cxl_control(&context);
> >                 kfree(context.ret.pointer);
> >         }
> >         return status;
> >  }
> >  
> > -static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root,
> > -                                       u32 support,
> > -                                       u32 *control)
> > +static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root, u32 support,
> > +                                     u32 *control, u32 cxl_support,
> > +                                     u32 *cxl_control)
> >  {
> >         acpi_status status;
> > -       u32 result, capbuf[6];
> > +       u32 pci_result, cxl_result, capbuf[8];
> 
> Nice to set capbuf size off one of the defines if possible, though I'm not
> sure why it is 8 (or why it was 6 before for that mater).  I think it should be 5.

Yep, I'm not sure why these were 6. I've added a new define and set it
to 5. Perhaps someone from ACPI might comment if there was a reason for
the extra padding. Rafael or Robert?

> 
> >  
> >         support |= root->osc_support_set;
> >  
> > @@ -231,11 +267,21 @@ static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root,
> >         capbuf[OSC_SUPPORT_DWORD] = support;
> >         capbuf[OSC_CONTROL_DWORD] = *control | root->osc_control_set;
> >  
> > +       if (is_cxl(root)) {
> > +               cxl_support |= root->cxl_osc_support_set;
> > +               capbuf[OSC_CXL_SUPPORT_DWORD] = cxl_support;
> > +               capbuf[OSC_CXL_CONTROL_DWORD] = *cxl_control | root->cxl_osc_control_set;
> > +       }
> > +
> >  retry:
> > -       status = acpi_pci_run_osc(root, capbuf, &result);
> > +       status = acpi_pci_run_osc(root, capbuf, &pci_result, &cxl_result);
> >         if (ACPI_SUCCESS(status)) {
> >                 root->osc_support_set = support;
> > -               *control = result;
> > +               *control = pci_result;
> > +               if (is_cxl(root)) {
> > +                       root->cxl_osc_support_set = cxl_support;
> > +                       *cxl_control = cxl_result;
> > +               }
> >         } else if (is_cxl(root)) {
> >                 /*
> >                  * CXL _OSC is optional on CXL 1.1 hosts. Fall back to PCIe _OSC
> > @@ -358,6 +404,8 @@ EXPORT_SYMBOL_GPL(acpi_get_pci_dev);
> >   * @handle: ACPI handle of a PCI root bridge (or PCIe Root Complex).
> >   * @mask: Mask of _OSC bits to request control of, place to store control mask.
> >   * @support: _OSC supported capability.
> > + * @cxl_mask: Mask of CXL _OSC control bits, place to store control mask.
> > + * @cxl_support: CXL _OSC supported capability.
> >   *
> >   * Run _OSC query for @mask and if that is successful, compare the returned
> >   * mask of control bits with @req.  If all of the @req bits are set in the
> > @@ -368,12 +416,14 @@ EXPORT_SYMBOL_GPL(acpi_get_pci_dev);
> >   * _OSC bits the BIOS has granted control of, but its contents are meaningless
> >   * on failure.
> >   **/
> > -static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 support)
> > +static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask,
> > +                                           u32 support, u32 *cxl_mask,
> > +                                           u32 cxl_support)
> >  {
> >         u32 req = OSC_PCI_EXPRESS_CAPABILITY_CONTROL;
> >         struct acpi_pci_root *root;
> >         acpi_status status;
> > -       u32 ctrl, capbuf[6];
> > +       u32 ctrl, cxl_ctrl, capbuf[8];
> 
> As above, why 8?
> 
> >  
> >         if (!mask)
> >                 return AE_BAD_PARAMETER;
> > @@ -385,20 +435,35 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 s
> >         ctrl   = *mask;
> >         *mask |= root->osc_control_set;
> >  
> > +       if (is_cxl(root)) {
> > +               cxl_ctrl   = *cxl_mask;
> 
> Odd spacing

Fixed.