Re: [Qemu-devel] [PATCH 2/4] spapr-pci: introduce a finish_realize() callback

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 03/13/2014 12:56 PM, Andreas Färber wrote:
> Am 21.11.2013 05:08, schrieb Alexey Kardashevskiy:
>> The spapr-pci PHB initializes IOMMU for emulataed devices only.
> 
> "emulated"
> 
>> The upcoming VFIO support will do it different. However both emulated
>> and VFIO PHB types share most of the initialization code.
>> For the type specific things a new finish_realize() callback is
>> introduced.
>>
>> This introduces sPAPRPHBClass derived from PCIHostBridgeClass and
>> adds the callback pointer.
>>
>> This implements finish_realize() for emulated devices.
>>
>> This changes initialization steps order to have the finish_realize()
>> call at the end of the spapr_finalize().
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> Changes:
>> v5:
>> * this is a new patch in the series, it was a part of a previous patch
>> ---
>>  hw/ppc/spapr_pci.c          | 46 +++++++++++++++++++++++++++++----------------
>>  include/hw/pci-host/spapr.h | 18 ++++++++++++++++--
>>  2 files changed, 46 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index aeb012d..963841c 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -500,6 +500,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>>      SysBusDevice *s = SYS_BUS_DEVICE(dev);
>>      sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(s);
>>      PCIHostState *phb = PCI_HOST_BRIDGE(s);
>> +    sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(s);
>>      const char *busname;
>>      char *namebuf;
>>      int i;
>> @@ -609,22 +610,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>>                             PCI_DEVFN(0, 0), PCI_NUM_PINS, TYPE_PCI_BUS);
>>      phb->bus = bus;
>>  
>> -    sphb->dma_window_start = 0;
>> -    sphb->dma_window_size = 0x40000000;
>> -    sphb->tcet = spapr_tce_new_table(dev, sphb->dma_liobn,
>> -                                     sphb->dma_window_size);
>> -    if (!sphb->tcet) {
>> -        error_setg(errp, "Unable to create TCE table for %s",
>> -                   sphb->dtbusname);
>> -        return;
>> -    }
>> -    address_space_init(&sphb->iommu_as, spapr_tce_get_iommu(sphb->tcet),
>> -                       sphb->dtbusname);
>> -
>> -    pci_setup_iommu(bus, spapr_pci_dma_iommu, sphb);
>> -
>> -    pci_bus_set_route_irq_fn(bus, spapr_route_intx_pin_to_irq);
>> -
>>      QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
>>  
>>      /* Initialize the LSI table */
>> @@ -639,6 +624,32 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>>  
>>          sphb->lsi_table[i].irq = irq;
>>      }
>> +
>> +    pci_setup_iommu(sphb->parent_obj.bus, spapr_pci_dma_iommu, sphb);
> 
> Accessing ->parent_obj anywhere but in VMState is very likely wrong,
> here it is definitely.

This is just a victim of multiple cut-n-paste's. My bad, missed it... It
should be:
pci_setup_iommu(bus, spapr_pci_dma_iommu, sphb);


> Also due to time pressure I'm not comfortable taking this into 2.0-rc0
> and would like to see how this works for the second implementation.

Repost it? Or I better wait for something (what?)? Thanks.


> 
> Regards,
> Andreas
> 
>> +
>> +    pci_bus_set_route_irq_fn(bus, spapr_route_intx_pin_to_irq);
>> +
>> +    if (!info->finish_realize) {
>> +        error_setg(errp, "finish_realize not defined");
>> +        return;
>> +    }
>> +
>> +    info->finish_realize(sphb, errp);
>> +}
>> +
>> +static void spapr_phb_finish_realize(sPAPRPHBState *sphb, Error **errp)
>> +{
>> +    sphb->dma_window_start = 0;
>> +    sphb->dma_window_size = 0x40000000;
>> +    sphb->tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn,
>> +                                     sphb->dma_window_size);
>> +    if (!sphb->tcet) {
>> +        error_setg(errp, "Unable to create TCE table for %s",
>> +                   sphb->dtbusname);
>> +        return ;
>> +    }
>> +    address_space_init(&sphb->iommu_as, spapr_tce_get_iommu(sphb->tcet),
>> +                       sphb->dtbusname);
>>  }
>>  
>>  static void spapr_phb_reset(DeviceState *qdev)
>> @@ -722,12 +733,14 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
>>  {
>>      PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> +    sPAPRPHBClass *spc = SPAPR_PCI_HOST_BRIDGE_CLASS(klass);
>>  
>>      hc->root_bus_path = spapr_phb_root_bus_path;
>>      dc->realize = spapr_phb_realize;
>>      dc->props = spapr_phb_properties;
>>      dc->reset = spapr_phb_reset;
>>      dc->vmsd = &vmstate_spapr_pci;
>> +    spc->finish_realize = spapr_phb_finish_realize;
>>  }
>>  
>>  static const TypeInfo spapr_phb_info = {
>> @@ -735,6 +748,7 @@ static const TypeInfo spapr_phb_info = {
>>      .parent        = TYPE_PCI_HOST_BRIDGE,
>>      .instance_size = sizeof(sPAPRPHBState),
>>      .class_init    = spapr_phb_class_init,
>> +    .class_size    = sizeof(sPAPRPHBClass),
>>  };
>>  
>>  PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
>> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
>> index 970b4a9..0f428a1 100644
>> --- a/include/hw/pci-host/spapr.h
>> +++ b/include/hw/pci-host/spapr.h
>> @@ -34,7 +34,21 @@
>>  #define SPAPR_PCI_HOST_BRIDGE(obj) \
>>      OBJECT_CHECK(sPAPRPHBState, (obj), TYPE_SPAPR_PCI_HOST_BRIDGE)
>>  
>> -typedef struct sPAPRPHBState {
>> +#define SPAPR_PCI_HOST_BRIDGE_CLASS(klass) \
>> +     OBJECT_CLASS_CHECK(sPAPRPHBClass, (klass), TYPE_SPAPR_PCI_HOST_BRIDGE)
>> +#define SPAPR_PCI_HOST_BRIDGE_GET_CLASS(obj) \
>> +     OBJECT_GET_CLASS(sPAPRPHBClass, (obj), TYPE_SPAPR_PCI_HOST_BRIDGE)
>> +
>> +typedef struct sPAPRPHBClass sPAPRPHBClass;
>> +typedef struct sPAPRPHBState sPAPRPHBState;
>> +
>> +struct sPAPRPHBClass {
>> +    PCIHostBridgeClass parent_class;
>> +
>> +    void (*finish_realize)(sPAPRPHBState *sphb, Error **errp);
>> +};
>> +
>> +struct sPAPRPHBState {
>>      PCIHostState parent_obj;
>>  
>>      int32_t index;
>> @@ -62,7 +76,7 @@ typedef struct sPAPRPHBState {
>>      } msi_table[SPAPR_MSIX_MAX_DEVS];
>>  
>>      QLIST_ENTRY(sPAPRPHBState) list;
>> -} sPAPRPHBState;
>> +};
>>  
>>  #define SPAPR_PCI_BASE_BUID          0x800000020000000ULL
>>  
>>
> 
> 


-- 
Alexey