On Fri, Jul 29, 2022 at 09:38:35 +0200, Michal Prívozník wrote:
> On 7/28/22 14:41, Peter Krempa wrote:
> > On Wed, Jul 27, 2022 at 12:34:57 +0200, Michal Privoznik wrote:
> >> From: Lin Yang <lin.a.yang@xxxxxxxxx>
> >>
> >> With NUMA config:
> >>
> >> <devices>
> >> ...
> >> <memory model='sgx-epc'>
> >> <source>
> >> <nodemask>0-1</nodemask>
> >> </source>
> >> <target>
> >> <size unit='KiB'>512</size>
> >> <node>0</node>
> >> </target>
> >> </memory>
> >> ...
> >> </devices>
> >>
> >> Without NUMA config:
> >>
> >> <devices>
> >> ...
> >> <memory model='sgx-epc'>
> >> <target>
> >> <size unit='KiB'>512</size>
> >> </target>
> >> </memory>
> >> ...
> >> </devices>
> >>
> >> Signed-off-by: Lin Yang <lin.a.yang@xxxxxxxxx>
> >> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> >> ---
> >
> > [...]
> >
> >> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> >> index 1ed969ac3e..bdd0fcea8e 100644
> >> --- a/docs/formatdomain.rst
> >> +++ b/docs/formatdomain.rst
> >> @@ -7940,6 +7940,20 @@ Example: usage of the memory devices
> >> <current unit='KiB'>524288</current>
> >> </target>
> >> </memory>
> >> + <memory model='sgx-epc'>
> >> + <source>
> >> + <nodemask>0-1</nodemask>
> >> + </source>
> >> + <target>
> >> + <size unit='KiB'>16384</size>
> >> + <node>0</node>
> >> + </target>
> >> + </memory>
> >> + <memory model='sgx-epc'>
> >> + <target>
> >> + <size unit='KiB'>16384</size>
> >> + </target>
> >> + </memory>
> >> </devices>
> >> ...
> >>
> >> @@ -7948,7 +7962,9 @@ Example: usage of the memory devices
> >> 1.2.14` Provide ``nvdimm`` model that adds a Non-Volatile DIMM module.
> >> :since:`Since 3.2.0` Provide ``virtio-pmem`` model to add a paravirtualized
> >> persistent memory device. :since:`Since 7.1.0` Provide ``virtio-mem`` model
> >> - to add paravirtualized memory device. :since:`Since 7.9.0`
> >> + to add paravirtualized memory device. :since:`Since 7.9.0` Provide
> >> + ``sgx-epc`` model to add a SGX enclave page cache (EPC) memory to the guest.
> >> + :since:`Since 8.7.0 and QEMU 6.2.0`
> >
> > I don't quite understand from this description whether this is real
> > memory usable by the guest OS or something for internal use by the
> > hypervisor.
> >
> > Specifically which leads me to questioning this is that the example
> > sizes are very tiny compared to real memory sizing.
>
> I admit that I don't know all the details and let somebody from Intel to
> provide them. But basically, SGX works by reserving a chunk of RAM (on
> each NUMA node) which is then encrypted and the processor controls
> access to it. This chunk is referred to as Processor Reserved Memory and
> to my understanding is the maximum size of an enclave. Thus I can
> understand why new <memory/> model was used.
Even this interpretation would not sit well with the semantics of the
<memory> element.
> But I'm not sure how QEMU
> accounts for this memory, whether say 16KiB worth of SGX is added to
> whatever current guest has OR it's taken from an existing memory.
Note that the name expands to "enclave page cache", so even the name
doesn't really seem to be hinting that the memory declared like this is
used by the guest OS.
Preferrably the documentation added in this patch will clarify that,
because if it is not clarified by the documentation, we will be getting
clarification requests and it's very apparent that we can't answer them
since neither of us actually knows what's going on.
>
> >
> > Additionally in qemuDomainDefValidateMemoryHotplug you are changing the
> > code to specifically exclude the sizing of the 'sgx-epc' memory devices
> > from the total size of the memory, but this contrasts with the memory
> > _not_ being excluded from the initial memory calculation in
> > virDomainDefGetMemoryInitial which is used to format the initial memory
> > argument ('-m size=...'). Thus at least one of them is wrong.
> >
> > If this is not guest usable memory, then the <memory> element should not
> > be used to bolt this on, but rather add a new element similarly to what
> > we have when AMD SEV is in use.
> >
>
> Alright, I'm hold off reworking these patches per your review until we
> are clear on this.
>
> Michal
>
