Re: [PATCH v3 6/8] domain: introduce GADDR based runstate area registration alternative

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.09.2023 17:24, Roger Pau Monné wrote:
> On Wed, May 03, 2023 at 05:57:40PM +0200, Jan Beulich wrote:
>> The registration by virtual/linear address has downsides: At least on
>> x86 the access is expensive for HVM/PVH domains. Furthermore for 64-bit
>> PV domains the area is inaccessible (and hence cannot be updated by Xen)
>> when in guest-user mode.
>>
>> Introduce a new vCPU operation allowing to register the runstate area by
>> guest-physical address.
>>
>> An at least theoretical downside to using physically registered areas is
>> that PV then won't see dirty (and perhaps also accessed) bits set in its
>> respective page table entries.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

> One comment below.
> 
>> --- a/xen/include/public/vcpu.h
>> +++ b/xen/include/public/vcpu.h
>> @@ -221,6 +221,19 @@ struct vcpu_register_time_memory_area {
>>  typedef struct vcpu_register_time_memory_area 
>> vcpu_register_time_memory_area_t;
>>  DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
>>  
>> +/*
>> + * Like the respective VCPUOP_register_*_memory_area, just using the 
>> "addr.p"
>> + * field of the supplied struct as a guest physical address (i.e. in GFN 
>> space).
>> + * The respective area may not cross a page boundary.  Pass ~0 to 
>> unregister an
>> + * area.  Note that as long as an area is registered by physical address, 
>> the
>> + * linear address based area will not be serviced (updated) by the 
>> hypervisor.
>> + *
>> + * Note that the area registered via VCPUOP_register_runstate_memory_area 
>> will
>> + * be updated in the same manner as the one registered via virtual address 
>> PLUS
>> + * VMASST_TYPE_runstate_update_flag engaged by the domain.
>> + */
>> +#define VCPUOP_register_runstate_phys_area      14
> 
> Just to make it more obvious, it might be nice to add a note in the
> comment on VCPUOP_register_runstate_memory_area that `p` can also be
> used with the `VCPUOP_register_runstate_phys_area` hypercall.

I can add a reference there (and then in the later patch also for the
time area), but I don't think it wants or needs to emphasize p. It
merely needs to point to the alternative sub-function imo.

Jan