Re: [PATCH v3 05/33] vl: Add sgx compound properties to expose SGX EPC sections to guest

[Yang Zhong <yang.zhong@intel.com>]

On Fri, Jul 09, 2021 at 06:07:13PM +0200, Paolo Bonzini wrote:
> On 09/07/21 13:09, Yang Zhong wrote:
> >+    sgx_epc = g_malloc0(sizeof(*sgx_epc));
> >+    pcms->sgx_epc = sgx_epc;
> >+
> 
> No need to malloc this, it's small.
>

  Thanks Paolo, i will use g_new0() to replace this malloc, thanks!
 
  Yang
  
 
> >  }
> >+##
> >+# @SgxEPC:
> >+#
> >+# Sgx EPC cmdline information
> >+#
> >+# @id: device's ID
> >+#
> >+# @memdev: memory backend linked with device
> >+#
> >+# Since: 6.1
> >+##
> >+{ 'struct': 'SgxEPC',
> >+  'data': { 'id': [ 'str' ],
> >+            'memdev': [ 'str' ]
> >+          }
> >+}
> 
> Is the "id" needed at all?  If not, you can make the property just a
> string list.
> 

  The current "id" is only shown in 'info memory-devices" command in the
  monitor.

   qemu) info memory-devices
    Memory device [sgx-epc]: "epc1"
      memaddr: 0x180000000
      size: 29360128
      memdev: /objects/mem1
    Memory device [sgx-epc]: "epc2"
      memaddr: 0x181c00000
      size: 10485760
      memdev: /objects/mem2

 If this "id" is not MUST, i can remove this. thanks!


> If not, you should still make the property a list, and SgxEPC can be
> just the id/memdev pair.
> 

  The SGX EPC will support NUMA function, 

   -object memory-backend-ram,size=2G,host-nodes=0,policy=bind,id=node0 \ 
   -object memory-backend-epc,id=mem0,size=100M \
   -sgx-epc id=epc0,memdev=mem0,node=0 \
   -numa node,nodeid=0,cpus=0-1,memdev=node0 \

  Sorry this is older command style, i will change this to compound 
  property in the NUMA patchset.

  So, the SgxEPC struct still needed even i removed 'id'.


> Also please place the compound property in PCMachineState, not in
> MachineState.  You can call the field something else than sgx_epc to
> avoid conflicts with the SGXEPCState, for example sgx_epc_memdevs or
> sgx_epc_backends.  Later it can be moved to X86MachineState if
> needed, but in any case it should not be in common
> target-independent code.
> 
  
  Yes, i will directly move compound property get/set from MachineState 
  to X86MachineState and change the sgx_epc to sgx_epc_backends to avoid
  conflicts(In fact, i have done this and it works well). Thanks!

  Yang

> Paolo