Re: [Qemu-devel] [PATCH v3 4/8] Extend the command-line to provide memory latency and bandwidth information

[Eduardo Habkost]

On Tue, Apr 02, 2019 at 11:46:37AM +0200, Igor Mammedov wrote:
> On Tue, 2 Apr 2019 12:42:37 +0800
> Tao Xu <address@hidden> wrote:
> 
> > On 2/6/19 6:11 PM, Igor Mammedov wrote:
> > > On Thu, 31 Jan 2019 15:16:54 +0800
> > > Tao Xu <address@hidden> wrote:
> > >   
> > >> From: Liu Jingqi <address@hidden>
> > >>
> > >> Add -numa hmat-lb option to provide System Locality Latency and
> > >> Bandwidth Information. These memory attributes help to build
> > >> System Locality Latency and Bandwidth Information Structure(s)
> > >> in ACPI Heterogeneous Memory Attribute Table (HMAT).  
> > > Maybe instead of adding/using more globals since the rest of numa.c was
> > > written so, it's time to introduce NumaMachine type which inherits
> > > form base Machine and extends it with numa extensions.
> > > 
> > > You don't have to refactor all old numa code for that (just the parts
> > > you use in HMAT) and later we could gradually refactor the rest of
> > > numa handling.
> > >   
> > Hi Igor,
> > 
> > I am sorry for replying after so long, because I am preparing the next 
> > version of patch and confusing about the implementation of NumaMachine.
> > If NumaMachine inherits from Machine, and then the PCmachine and other 
> > platform machine inherits from NumaMachine (Implementation a)? Or 
> > PCmachine and other platform machine inherits from Machine 
> > (Implementation b)?
> > 
> > Implementation a:           Implementation b:
> > Machine                         Machine
> > │                               │
> > └── NumaMachine                 ├── NumaMachine
> >      │                           │
> >      ├── PCMachine               ├── PCMachine
> >      │                           │
> >      └── none-PCMachine          └── none-PCMachine
> 
> I'd imagine 'a', where none-PCMachine is arm/virt and spapr machines that
> support numa today. The rest would inherit from plain Machine.
> 
> > And how about add numa.c's globals into MachineState? This may not 
> > change a lot.
> That's another possible way, but it's polluting generic machine with
> numa specifics about which only 3-4 boards care.
> From my point of view it's a bit better then globals but not much.

I suggest following the pattern used for
MachineState::device_memory and MachineState::nvdimms_state
(pointer to optional feature-specific state) intead of
inheritance.

-- 
Eduardo