| Message ID | 20230731162201.271114-5-xiaoyao.li@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | QEMU gmem implemention | expand |
On Mon, Jul 31, 2023 at 12:21:46PM -0400, Xiaoyao Li wrote: > +bool memory_region_can_be_private(MemoryRegion *mr) > +{ > + return mr->ram_block && mr->ram_block->gmem_fd >= 0; > +} This is not really MAP_PRIVATE, am I right? If so, is there still chance we rename it (it seems to be also in the kernel proposal all across..)? I worry it can be very confusing in the future against MAP_PRIVATE / MAP_SHARED otherwise. Thanks,
On Mon, Jul 31, 2023 at 05:23:55PM -0400, Peter Xu wrote: > On Mon, Jul 31, 2023 at 12:21:46PM -0400, Xiaoyao Li wrote: > > +bool memory_region_can_be_private(MemoryRegion *mr) > > +{ > > + return mr->ram_block && mr->ram_block->gmem_fd >= 0; > > +} > > This is not really MAP_PRIVATE, am I right? If so, is there still chance > we rename it (it seems to be also in the kernel proposal all across..)? > > I worry it can be very confusing in the future against MAP_PRIVATE / > MAP_SHARED otherwise. > > Thanks, It is - kernel calls this "map shared" but unfortunately qemu calls this just "shared". Maybe e.g. "protect_shared" would be a better name for qemu?
On Mon, Jul 31, 2023, Peter Xu wrote: > On Mon, Jul 31, 2023 at 12:21:46PM -0400, Xiaoyao Li wrote: > > +bool memory_region_can_be_private(MemoryRegion *mr) > > +{ > > + return mr->ram_block && mr->ram_block->gmem_fd >= 0; > > +} > > This is not really MAP_PRIVATE, am I right? If so, is there still chance > we rename it (it seems to be also in the kernel proposal all across..)? Yes and yes. > I worry it can be very confusing in the future against MAP_PRIVATE / > MAP_SHARED otherwise. Heh, it's already quite confusing at times. I'm definitely open to naming that doesn't collide with MAP_{PRIVATE,SHARED}, especially if someone can come with a naming scheme that includes a succinct way to describe memory that is shared between two or more VMs, but is accessible to _only_ those VMs.
On Mon, Jul 31, 2023 at 02:34:22PM -0700, Sean Christopherson wrote: > On Mon, Jul 31, 2023, Peter Xu wrote: > > On Mon, Jul 31, 2023 at 12:21:46PM -0400, Xiaoyao Li wrote: > > > +bool memory_region_can_be_private(MemoryRegion *mr) > > > +{ > > > + return mr->ram_block && mr->ram_block->gmem_fd >= 0; > > > +} > > > > This is not really MAP_PRIVATE, am I right? If so, is there still chance > > we rename it (it seems to be also in the kernel proposal all across..)? > > Yes and yes. > > > I worry it can be very confusing in the future against MAP_PRIVATE / > > MAP_SHARED otherwise. > > Heh, it's already quite confusing at times. I'm definitely open to naming that > doesn't collide with MAP_{PRIVATE,SHARED}, especially if someone can come with a > naming scheme that includes a succinct way to describe memory that is shared > between two or more VMs, but is accessible to _only_ those VMs. Standard solution is a technology specific prefix. protect_shared, encrypt_shared etc.
On Mon, Jul 31, 2023 at 05:36:37PM -0400, Michael S. Tsirkin wrote: > On Mon, Jul 31, 2023 at 02:34:22PM -0700, Sean Christopherson wrote: > > On Mon, Jul 31, 2023, Peter Xu wrote: > > > On Mon, Jul 31, 2023 at 12:21:46PM -0400, Xiaoyao Li wrote: > > > > +bool memory_region_can_be_private(MemoryRegion *mr) > > > > +{ > > > > + return mr->ram_block && mr->ram_block->gmem_fd >= 0; > > > > +} > > > > > > This is not really MAP_PRIVATE, am I right? If so, is there still chance > > > we rename it (it seems to be also in the kernel proposal all across..)? > > > > Yes and yes. > > > > > I worry it can be very confusing in the future against MAP_PRIVATE / > > > MAP_SHARED otherwise. > > > > Heh, it's already quite confusing at times. I'm definitely open to naming that > > doesn't collide with MAP_{PRIVATE,SHARED}, especially if someone can come with a > > naming scheme that includes a succinct way to describe memory that is shared > > between two or more VMs, but is accessible to _only_ those VMs. > > Standard solution is a technology specific prefix. > protect_shared, encrypt_shared etc. Agreed, a prefix could definitely help (if nothing better comes at last..). If e.g. "encrypted" too long to be applied everywhere in var names and functions, maybe it can also be "enc_{private|shared}". Thanks,
On Mon, Jul 31, 2023, Peter Xu wrote: > On Mon, Jul 31, 2023 at 05:36:37PM -0400, Michael S. Tsirkin wrote: > > On Mon, Jul 31, 2023 at 02:34:22PM -0700, Sean Christopherson wrote: > > > On Mon, Jul 31, 2023, Peter Xu wrote: > > > > On Mon, Jul 31, 2023 at 12:21:46PM -0400, Xiaoyao Li wrote: > > > > > +bool memory_region_can_be_private(MemoryRegion *mr) > > > > > +{ > > > > > + return mr->ram_block && mr->ram_block->gmem_fd >= 0; > > > > > +} > > > > > > > > This is not really MAP_PRIVATE, am I right? If so, is there still chance > > > > we rename it (it seems to be also in the kernel proposal all across..)? > > > > > > Yes and yes. > > > > > > > I worry it can be very confusing in the future against MAP_PRIVATE / > > > > MAP_SHARED otherwise. > > > > > > Heh, it's already quite confusing at times. I'm definitely open to naming that > > > doesn't collide with MAP_{PRIVATE,SHARED}, especially if someone can come with a > > > naming scheme that includes a succinct way to describe memory that is shared > > > between two or more VMs, but is accessible to _only_ those VMs. > > > > Standard solution is a technology specific prefix. > > protect_shared, encrypt_shared etc. > > Agreed, a prefix could definitely help (if nothing better comes at last..). > If e.g. "encrypted" too long to be applied everywhere in var names and > functions, maybe it can also be "enc_{private|shared}". FWIW, I would stay away from "encrypted", there is no requirement that the memory actually be encrypted.
On 7/31/23 18:21, Xiaoyao Li wrote: > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > --- > include/exec/memory.h | 9 +++++++++ > softmmu/memory.c | 5 +++++ > 2 files changed, 14 insertions(+) > > diff --git a/include/exec/memory.h b/include/exec/memory.h > index 61e31c7b9874..e119d3ce1a1d 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -1679,6 +1679,15 @@ static inline bool memory_region_is_romd(MemoryRegion *mr) > */ > bool memory_region_is_protected(MemoryRegion *mr); > > +/** > + * memory_region_can_be_private: check whether a memory region can be private The name of the function is not particularly informative, > + * > + * Returns %true if a memory region's ram_block has valid gmem fd assigned. but in your comment you describe more accurately what it does, why not make it the function name? bool memory_region_has_valid_gmem_fd() > + * > + * @mr: the memory region being queried > + */ > +bool memory_region_can_be_private(MemoryRegion *mr); bool memory_region_has_valid_gmem_fd() Thanks, C > + > /** > * memory_region_get_iommu: check whether a memory region is an iommu > * > diff --git a/softmmu/memory.c b/softmmu/memory.c > index 4f8f8c0a02e6..336c76ede660 100644 > --- a/softmmu/memory.c > +++ b/softmmu/memory.c > @@ -1855,6 +1855,11 @@ bool memory_region_is_protected(MemoryRegion *mr) > return mr->ram && (mr->ram_block->flags & RAM_PROTECTED); > } > > +bool memory_region_can_be_private(MemoryRegion *mr) > +{ > + return mr->ram_block && mr->ram_block->gmem_fd >= 0; > +} > + > uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr) > { > uint8_t mask = mr->dirty_log_mask;
On 8/1/23 18:48, Claudio Fontana wrote: > On 7/31/23 18:21, Xiaoyao Li wrote: >> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> >> --- >> include/exec/memory.h | 9 +++++++++ >> softmmu/memory.c | 5 +++++ >> 2 files changed, 14 insertions(+) >> >> diff --git a/include/exec/memory.h b/include/exec/memory.h >> index 61e31c7b9874..e119d3ce1a1d 100644 >> --- a/include/exec/memory.h >> +++ b/include/exec/memory.h >> @@ -1679,6 +1679,15 @@ static inline bool memory_region_is_romd(MemoryRegion *mr) >> */ >> bool memory_region_is_protected(MemoryRegion *mr); >> >> +/** >> + * memory_region_can_be_private: check whether a memory region can be private > > The name of the function is not particularly informative, > >> + * >> + * Returns %true if a memory region's ram_block has valid gmem fd assigned. > > but in your comment you describe more accurately what it does, why not make it the function name? > > bool memory_region_has_valid_gmem_fd() btw can a memory region have an invalid gmem_fd ? If an invalid gmem_fd is just used to mark whether gmem_fd is present or not, we could make it just: bool memory_region_has_gmem_fd() Thanks, C > >> + * >> + * @mr: the memory region being queried >> + */ >> +bool memory_region_can_be_private(MemoryRegion *mr); > > > bool memory_region_has_valid_gmem_fd() > > > Thanks, > > C > >> + >> /** >> * memory_region_get_iommu: check whether a memory region is an iommu >> * >> diff --git a/softmmu/memory.c b/softmmu/memory.c >> index 4f8f8c0a02e6..336c76ede660 100644 >> --- a/softmmu/memory.c >> +++ b/softmmu/memory.c >> @@ -1855,6 +1855,11 @@ bool memory_region_is_protected(MemoryRegion *mr) >> return mr->ram && (mr->ram_block->flags & RAM_PROTECTED); >> } >> >> +bool memory_region_can_be_private(MemoryRegion *mr) >> +{ >> + return mr->ram_block && mr->ram_block->gmem_fd >= 0; >> +} >> + >> uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr) >> { >> uint8_t mask = mr->dirty_log_mask; >
On 8/2/2023 12:52 AM, Claudio Fontana wrote: > On 8/1/23 18:48, Claudio Fontana wrote: >> On 7/31/23 18:21, Xiaoyao Li wrote: >>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> >>> --- >>> include/exec/memory.h | 9 +++++++++ >>> softmmu/memory.c | 5 +++++ >>> 2 files changed, 14 insertions(+) >>> >>> diff --git a/include/exec/memory.h b/include/exec/memory.h >>> index 61e31c7b9874..e119d3ce1a1d 100644 >>> --- a/include/exec/memory.h >>> +++ b/include/exec/memory.h >>> @@ -1679,6 +1679,15 @@ static inline bool memory_region_is_romd(MemoryRegion *mr) >>> */ >>> bool memory_region_is_protected(MemoryRegion *mr); >>> >>> +/** >>> + * memory_region_can_be_private: check whether a memory region can be private >> >> The name of the function is not particularly informative, >> >>> + * >>> + * Returns %true if a memory region's ram_block has valid gmem fd assigned. >> >> but in your comment you describe more accurately what it does, why not make it the function name? >> >> bool memory_region_has_valid_gmem_fd() > > > btw can a memory region have an invalid gmem_fd ? > > If an invalid gmem_fd is just used to mark whether gmem_fd is present or not, > > we could make it just: > > bool memory_region_has_gmem_fd() yes. It's a good suggestion! I will use it in next version if no objection from others. Thanks, -Xiaoyao
diff --git a/include/exec/memory.h b/include/exec/memory.h index 61e31c7b9874..e119d3ce1a1d 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -1679,6 +1679,15 @@ static inline bool memory_region_is_romd(MemoryRegion *mr) */ bool memory_region_is_protected(MemoryRegion *mr); +/** + * memory_region_can_be_private: check whether a memory region can be private + * + * Returns %true if a memory region's ram_block has valid gmem fd assigned. + * + * @mr: the memory region being queried + */ +bool memory_region_can_be_private(MemoryRegion *mr); + /** * memory_region_get_iommu: check whether a memory region is an iommu * diff --git a/softmmu/memory.c b/softmmu/memory.c index 4f8f8c0a02e6..336c76ede660 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -1855,6 +1855,11 @@ bool memory_region_is_protected(MemoryRegion *mr) return mr->ram && (mr->ram_block->flags & RAM_PROTECTED); } +bool memory_region_can_be_private(MemoryRegion *mr) +{ + return mr->ram_block && mr->ram_block->gmem_fd >= 0; +} + uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr) { uint8_t mask = mr->dirty_log_mask;
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> --- include/exec/memory.h | 9 +++++++++ softmmu/memory.c | 5 +++++ 2 files changed, 14 insertions(+)