Re: [Qemu-devel] [PATCH] ppc/pnv: Add model for Power8 PHB3 PCIe Host bridge

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Jun 26, 2018 at 03:59:28PM +0200, Cédric Le Goater wrote:
> diff --git a/include/hw/pci-host/pnv_phb3_regs.h b/include/hw/pci-host/pnv_phb3_regs.h
> new file mode 100644
> index 000000000000..a1672726b908
> --- /dev/null
> +++ b/include/hw/pci-host/pnv_phb3_regs.h
> @@ -0,0 +1,510 @@
> +/* Copyright (c) 2013-2018, IBM Corporation.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + *      http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> + * implied.
> + *
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#ifndef PCI_HOST_PNV_PHB3_REGS_H
> +#define PCI_HOST_PNV_PHB3_REGS_H
> +
> +/*
> + * Duplicated from target/ppc/cpu.h
> + */
> +#define PPC_BIT(bit)            (0x8000000000000000UL >> (bit))
> +#define PPC_BIT32(bit)          (0x80000000UL >> (bit))
> +#define PPC_BIT8(bit)           (0x80UL >> (bit))
> +#define PPC_BITMASK(bs, be)     ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
> +#define PPC_BITMASK32(bs, be)   ((PPC_BIT32(bs) - PPC_BIT32(be)) | \
> +                                 PPC_BIT32(bs))
> +#define PPC_BITLSHIFT(be)       (63 - (be))
> +#define PPC_BITLSHIFT32(be)     (31 - (be))



> +
> +/* Extract field fname from val */
> +#define GETFIELD(fname, val)                    \
> +        (((val) & fname##_MASK) >> fname##_LSH)
> +
> +/* Set field fname of oval to fval
> + * NOTE: oval isn't modified, the combined result is returned
> + */
> +#define SETFIELD(fname, oval, fval)                     \
> +        (((oval) & ~fname##_MASK) | \
> +         ((((typeof(oval))(fval)) << fname##_LSH) & fname##_MASK))
> +

Pls don't make up macros like these. We can't have each device do it.


> @@ -1031,6 +1110,7 @@ static Property pnv_chip_properties[] = {
>      DEFINE_PROP_UINT64("ram-size", PnvChip, ram_size, 0),
>      DEFINE_PROP_UINT32("nr-cores", PnvChip, nr_cores, 1),
>      DEFINE_PROP_UINT64("cores-mask", PnvChip, cores_mask, 0x0),
> +    DEFINE_PROP_UINT32("num-phbs", PnvChip, num_phbs, 1),
>      DEFINE_PROP_END_OF_LIST(),
>  };

How about instanciating each extra phb using -device?
Seems better than teaching everyone about platform-specific
options.

-- 
MST