Re: [PATCH v2 1/5] x86/HVM: introduce hvm_point_entry()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.11.2023 22:30, Andrew Cooper wrote:
> On 16/11/2023 1:46 pm, Jan Beulich wrote:
>> ... to accompany hvm_read_entry() when actual copying isn't desirable.
>> This allows to remove open-coded stream accesses from hpet_load(),
>> along with using the helper in hvm_load() itself.
>>
>> Since arch_hvm_load()'s declaration would need changing, and since the
>> function is not used from elsewhere, purge the declaration. With that it
>> makes little sense to keep arch_hvm_save()'s around; convert that
>> function to static then at the same time.
>>
>> In hpet_load() simplify the specific case of error return that's in
>> context anyway: There's no need to hold the lock when only updating a
>> local variable.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Thanks.

> I really do hate all of this logic and most of it wants to live in
> userspace, but this an improvement.
> 
> The only thing I'm a little concerned with is the name. 
> hvm_read_entry() clearly consumes an entry, while hvm_point_entry()
> does, but doesn't obviously convey this at a glance.
> 
> Ideally I'd say that hvm_{get,copy}_entry() would be better nomeclature,
> as both have at least a vague implication of unmarshalling and one
> clearly is making a separate copy.

I'm fine renaming the new one to hvm_get_entry(). For the existing one,
"copy" may be marginally better than "read" / "load", but then it doesn't
indicate direction (i.e. somewhat collides with hvm_{write,load}_entry()).
So I'd want to leave those as they are.

Jan