Re: [PATCH qemu] hw/pci-bridge/pxb-cxl: Drop RAS capability from host bridge.

[Michael S. Tsirkin]

On Thu, Feb 15, 2024 at 05:42:12PM +0000, Jonathan Cameron wrote:
> On Thu, 15 Feb 2024 11:11:47 -0500
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Feb 15, 2024 at 03:52:06PM +0000, Jonathan Cameron wrote:
> > > This CXL component isn't allowed to have a RAS capability.
> > > Whilst this should be harmless as software is not expected to look
> > > here, good to clean it up.
> > > 
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> > 
> > Isn't this device migrateable? If yes you need compat
> > handling.
> 
> Not yet. Migrating these is broken in lots of ways :(
> 
> Given they are functional emulation only I've never cared that much.
> We'll need to fix this as part of adding support for virtualization
> use cases which start to make sense when dynamic capacity
> support lands in kernel + QEMU (probably later this year)
> 
> Jonathan


should probably block migration then?

> > 
> > 
> > > ---
> > >  I've been carrying this on my tree for quite a while.
> > >  This wasn't in previous fixes set because it's low priority and
> > >  rebasing it across cleanup series that followed those fixes was
> > >  too fiddly to bother.
> > >  
> > >  include/hw/cxl/cxl_component.h      |  1 +
> > >  hw/cxl/cxl-component-utils.c        | 21 +++++++++++++++++----
> > >  hw/pci-bridge/pci_expander_bridge.c |  2 +-
> > >  3 files changed, 19 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/include/hw/cxl/cxl_component.h 
> > > b/include/hw/cxl/cxl_component.h
> > > index 0e5d35c263..5012fab6f7 100644
> > > --- a/include/hw/cxl/cxl_component.h
> > > +++ b/include/hw/cxl/cxl_component.h
> > > @@ -25,6 +25,7 @@ enum reg_type {
> > >      CXL2_TYPE3_DEVICE,
> > >      CXL2_LOGICAL_DEVICE,
> > >      CXL2_ROOT_PORT,
> > > +    CXL2_RC,
> > >      CXL2_UPSTREAM_PORT,
> > >      CXL2_DOWNSTREAM_PORT,
> > >      CXL3_SWITCH_MAILBOX_CCI,
> > > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > > index 84ab503325..cd116c0401 100644
> > > --- a/hw/cxl/cxl-component-utils.c
> > > +++ b/hw/cxl/cxl-component-utils.c
> > > @@ -297,6 +297,7 @@ void cxl_component_register_init_common(uint32_t 
> > > *reg_state,
> > >          caps = 3;
> > >          break;
> > >      case CXL2_ROOT_PORT:
> > > +    case CXL2_RC:
> > >          /* + Extended Security, + Snoop */
> > >          caps = 5;
> > >          break;
> > > @@ -326,8 +327,19 @@ void cxl_component_register_init_common(uint32_t 
> > > *reg_state,
> > >                         CXL_##reg##_REGISTERS_OFFSET);                    
> > >      \
> > >      } while (0)
> > >  
> > > +    switch (type) {
> > > +    case CXL2_DEVICE:
> > > +    case CXL2_TYPE3_DEVICE:
> > > +    case CXL2_LOGICAL_DEVICE:
> > > +    case CXL2_ROOT_PORT:
> > > +    case CXL2_UPSTREAM_PORT:
> > > +    case CXL2_DOWNSTREAM_PORT:
> > >      init_cap_reg(RAS, 2, CXL_RAS_CAPABILITY_VERSION);
> > > -    ras_init_common(reg_state, write_msk);
> > > +        ras_init_common(reg_state, write_msk);
> > > +        break;
> > > +    default:
> > > +        break;
> > > +    }
> > >  
> > >      init_cap_reg(LINK, 4, CXL_LINK_CAPABILITY_VERSION);
> > >  
> > > @@ -335,9 +347,10 @@ void cxl_component_register_init_common(uint32_t 
> > > *reg_state,
> > >          return;
> > >      }
> > >  
> > > -    init_cap_reg(HDM, 5, CXL_HDM_CAPABILITY_VERSION);
> > > -    hdm_init_common(reg_state, write_msk, type);
> > > -
> > > +    if (type != CXL2_ROOT_PORT) {
> > > +        init_cap_reg(HDM, 5, CXL_HDM_CAPABILITY_VERSION);
> > > +        hdm_init_common(reg_state, write_msk, type);
> > > +    }
> > >      if (caps < 5) {
> > >          return;
> > >      }
> > > diff --git a/hw/pci-bridge/pci_expander_bridge.c 
> > > b/hw/pci-bridge/pci_expander_bridge.c
> > > index 535889f7c2..0411ad31ea 100644
> > > --- a/hw/pci-bridge/pci_expander_bridge.c
> > > +++ b/hw/pci-bridge/pci_expander_bridge.c
> > > @@ -290,7 +290,7 @@ static void pxb_cxl_dev_reset(DeviceState *dev)
> > >      uint32_t *write_msk = cxl_cstate->crb.cache_mem_regs_write_mask;
> > >      int dsp_count = 0;
> > >  
> > > -    cxl_component_register_init_common(reg_state, write_msk, 
> > > CXL2_ROOT_PORT);
> > > +    cxl_component_register_init_common(reg_state, write_msk, CXL2_RC);
> > >      /*
> > >       * The CXL specification allows for host bridges with no HDM decoders
> > >       * if they only have a single root port.
> > > -- 
> > > 2.39.2  
> >