Re: [Qemu-devel] [PATCH qemu v2 2/4] vfio/spapr: Rename local systempagesize variable

[Alexey Kardashevskiy]

On 15/02/2019 14:57, David Gibson wrote:
> On Fri, Feb 15, 2019 at 02:37:55PM +1100, Alexey Kardashevskiy wrote:
>>
>>
>> On 15/02/2019 14:32, David Gibson wrote:
>>> On Thu, Feb 14, 2019 at 04:21:42PM +1100, Alexey Kardashevskiy wrote:
>>>> The "systempagesize" name suggests that it is the host system page size
>>>> while it is the smallest page size of memory backing the guest RAM so
>>>> let's rename it to stop confusion. This should cause no behavioral change.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>>>> ---
>>>>  hw/vfio/spapr.c | 6 +++---
>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
>>>> index 302d6b0..1498fee 100644
>>>> --- a/hw/vfio/spapr.c
>>>> +++ b/hw/vfio/spapr.c
>>>> @@ -148,14 +148,14 @@ int vfio_spapr_create_window(VFIOContainer 
>>>> *container,
>>>>      uint64_t pagesize = memory_region_iommu_get_min_page_size(iommu_mr);
>>>>      unsigned entries, bits_total, bits_per_level, max_levels;
>>>>      struct vfio_iommu_spapr_tce_create create = { .argsz = sizeof(create) 
>>>> };
>>>> -    long systempagesize = qemu_getrampagesize();
>>>> +    long minpagesize = qemu_getrampagesize();
>>>
>>> I'd prefer "rampagesize" or "minrampagesize" here.  "minpagesize"
>>> doesn't make it clear what it's the minimum of, whereas rampagesize at
>>> least makes it clear it's the same thing that qemu_getrampagesize() is
>>> working with.
>>
>> In my mind "ram" in this context is a guest RAM but here is it not (is
>> is backend)
> 
> Well.. it is guest RAM, in that we're only considering the backing
> page size for memory mapped into the guest as RAM.  I guess it is a
> host page size, but then guest page sizes don't really exist until the
> guest starts mapping things (and may not be unique, since it could
> map the same memory in multiple ways).


Well, I'll make rampagesize then.


>> so I just avoided using any specifics in the name (system,
>> ram) to make the reader of this code look around where the value comes
>> from which is the qemu_getrampagesize() helper (more descriptive name).
>>
>>
>>
>>>
>>>>      /*
>>>>       * The host might not support the guest supported IOMMU page size,
>>>>       * so we will use smaller physical IOMMU pages to back them.
>>>>       */
>>>> -    if (pagesize > systempagesize) {
>>>> -        pagesize = systempagesize;
>>>> +    if (pagesize > minpagesize) {
>>>> +        pagesize = minpagesize;
>>>>      }
>>>>      pagesize = 1ULL << (63 - clz64(container->pgsizes &
>>>>                                     (pagesize | (pagesize - 1))));
>>>
>>
> 

-- 
Alexey