On 5/30/22, 6:09 AM, "Michal Prívozník" <mprivozn@redhat.com> wrote:

> On 5/18/22 09:59, Haibin Huang wrote:

> > From: Lin Yang <lin.a.yang@intel.com>

> >

> > According to the result parsing from xml, add the argument of

> > SGX EPC memory backend into QEMU command line:

> >

> >     #qemu-system-x86_64 \

> >         ...... \

> >         -object memory-backend-epc,id=memepc0,size=64M,prealloc=on \

> >         -object memory-backend-epc,id=memepc1,size=28M \

> >         -machine sgx-epc.0.memdev=memepc0,sgx-epc.1.memdev=memepc1

> >

> > Signed-off-by: Lin Yang <lin.a.yang@intel.com>

> > Signed-off-by: Haibin Huang <haibin.huang@intel.com>

> > ---

> >  src/qemu/qemu_alias.c                         |  3 +-

> >  src/qemu/qemu_command.c                       | 53 +++++++++++++++++--

> >  .../sgx-epc.x86_64-6.2.0.args                 | 37 +++++++++++++

> >  tests/qemuxml2argvtest.c                      |  2 +

> >  4 files changed, 90 insertions(+), 5 deletions(-)

> >  create mode 100644 tests/qemuxml2argvdata/sgx-epc.x86_64-6.2.0.args

> >

> > diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c

> > index e5a946cbed..03c79bcf0e 100644

> > --- a/src/qemu/qemu_alias.c

> > +++ b/src/qemu/qemu_alias.c

> > @@ -467,7 +467,8 @@ qemuDeviceMemoryGetAliasID(virDomainDef *def,

> >       * valid */

> >      if (!oldAlias &&

> >          mem->model != VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM &&

> > -        mem->model != VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM)

> > +        mem->model != VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM &&

> > +        mem->model != VIR_DOMAIN_MEMORY_MODEL_SGX_EPC)

> >          return mem->info.addr.dimm.slot;

> > 

> >      for (i = 0; i < def->nmems; i++) {

> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c

> > index 4807b137b6..9c83f0e168 100644

> > --- a/src/qemu/qemu_command.c

> > +++ b/src/qemu/qemu_command.c

> > @@ -3774,6 +3774,10 @@ qemuBuildMemoryBackendProps(virJSONValue **backendProps,

> >          if (systemMemory)

> >              disableCanonicalPath = true;

> > 

> > +    } else if (mem->model == VIR_DOMAIN_MEMORY_MODEL_SGX_EPC) {

> > +        backendType = "memory-backend-epc";

> > +        if (!priv->memPrealloc)

> > +            prealloc = true;

> >      } else if (useHugepage || mem->nvdimmPath || memAccess ||

> >          def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) {

> > 

> > @@ -3934,6 +3938,11 @@ qemuBuildMemoryBackendProps(virJSONValue **backendProps,

> >                             _("this qemu doesn't support the "

> >                               "memory-backend-memfd object"));

> >              return -1;

> > +        } else if (STREQ(backendType, "memory-backend-epc") &&

> > +                   !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SGX_EPC)) {

> > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",

> > +                           _("this qemu doesn't support the memory-backend-epc object"));

> > +            return -1;

> >          }

> > 

> >          rc = 0;

> > @@ -6979,6 +6988,7 @@ qemuBuildMachineCommandLine(virCommand *cmd,

> >      virCPUDef *cpu = def->cpu;

> >      g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;

> >      size_t i;

> > +    int epcNum = 0;

> > 

> >      virCommandAddArg(cmd, "-machine");

> >      virBufferAdd(&buf, def->os.machine, -1);

> > @@ -7199,6 +7209,25 @@ qemuBuildMachineCommandLine(virCommand *cmd,

> >          virBufferAddLit(&buf, ",graphics=off");

> >      }

> > 

> > +    /* add sgx epc memory to -machine parameter */

> > +    for (i = 0; i < def->nmems; i++) {

> > +        switch ((virDomainMemoryModel) def->mems[i]->model) {

> > +        case VIR_DOMAIN_MEMORY_MODEL_SGX_EPC:

> > +            virBufferAsprintf(&buf, ",sgx-epc.%d.memdev=mem%s", epcNum++,

> > +                              def->mems[i]->info.alias);

>

> So there really isn't any better way to specify sgx-epc than through -M?

> This way libvirt loses capability to set th device @id attribute which

> means we have to jump through hoops (like in your patch 2/6) when the

> @id value is expected.

>

> I vaguely pointing this out earlier (but maybe I just thought about

> pointing it out and never did - it's been a while since I've reviewed

> these patches), but I don't know what the consensus was.

I was not involved in the QEMU SGX patches review process and don’t

know the reason why it use -machine instead of -device. The initial

patch introduce separate QEMU argument -sgx-epc, but finally was

updated to -machine according to the comments.

https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg00644.html

Thanks,

Lin.