Re: [Qemu-devel] [PATCH v2 1/3] hw/pci: factor PCI reserve resources to a separate structure

[Liu, Jing2]

Hi Marcel,

On 8/17/2018 11:49 PM, Marcel Apfelbaum wrote:
> Hi Jing,
[...]
> > +/*
> > + * additional resources to reserve on firmware init
> > + */
> > +typedef struct PCIResReserve {
> > +    uint32_t bus_reserve;
> > +    uint64_t io_reserve;
> > +    uint64_t mem_reserve;
>
> The patch looks good to me, I noticed you renamed
> 'mem_no_pref_reserve' to 'mem reserve'.
> I remember we had a lot of discussions about the  naming, so they would
> be clear and consistent with the firmware counterpart.
OK, will change 'mem_no_pref_reserve' to 'mem_no_pref' and also for
others.
> Please add a least a comment in the PCIResReserve.
Will add a comment to the structure definition, and where it's called.

> Also, since you encapsulated the fields into a new struct,
> you could remove the  "_reserve" suffix so we
> remain with clear "bus", "io", "mem" ...
Got it.

Thanks,
Jing

> Thanks,
> Marcel
>
> > +    uint64_t pref32_reserve;
> > +    uint64_t pref64_reserve;
> > +} PCIResReserve;
> > +
> >   int pci_bridge_qemu_reserve_cap_init(PCIDevice *dev, int cap_offset,
> > -                              uint32_t bus_reserve, uint64_t io_reserve,
> > -                              uint64_t mem_non_pref_reserve,
> > -                              uint64_t mem_pref_32_reserve,
> > -                              uint64_t mem_pref_64_reserve,
> > -                              Error **errp);
> > +                               PCIResReserve res_reserve, Error **errp);
> >   #endif /* QEMU_PCI_BRIDGE_H */